Skip to content
Snippets Groups Projects

Grid-style field type keyboard navigation

Grid-style field type keyboard navigation
issue/drupal-3390914:3390914-grid-style-field-type into 11.x
10 open threads
Open
Konstantin Shupenkorequested to merge
issue/drupal-3390914:3390914-grid-style-field-type into 11.x
10 open threads

Closes #3390914

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
core/modules/field_ui/src/Form/FieldStorageAddForm.php
281 281 $form['add'] = [
282 282 '#type' => 'container',
283 283 '#attributes' => [
284 'class' => 'add-field-container',
284 'class' => [
285 'js-click-to-select-container',
  • Théodore Biadala
    Théodore Biadala @nod_
    Maintainer

    this should be a data attribute of some kind, js- classes were supposed to be an intermediary step 8 years ago before data- attributes were well supported everywhere

  • Please register or sign in to reply
  • Théodore Biadala
    Théodore Biadala @nod_ started a thread on the diff
    • core/modules/field_ui/js/field_ui.js
    509 (clickToSelectEl) => {
    510 const input = clickToSelectEl.querySelector('input');
    511 if (input) {
    512 Drupal.behaviors.clickToSelect.clickHandler(clickToSelectEl, input);
    508 once(
    509 'field-click-to-select',
    510 '.js-click-to-select-container',
    511 context,
    512 ).forEach((container) => {
    513 const itemClassName = 'js-click-to-select';
    514 const items = container.querySelectorAll(`.${itemClassName}`);
    515 items.forEach((item) => {
    516 const input = item.querySelector('input');
    517 if (!input) {
    518 return;
    519 }
    • Comment on lines +514 to +519
      Théodore Biadala
      Théodore Biadala @nod_
      Maintainer

      We can avoid the awkard check with css selectors

      Suggested change
      514 const items = container.querySelectorAll(`.${itemClassName}`);
      515 items.forEach((item) => {
      516 const input = item.querySelector('input');
      517 if (!input) {
      518 return;
      519 }
      514 const items = container.querySelectorAll(`.${itemClassName}:has(input)`);
      515 items.forEach((item) => {
      516 const input = item.querySelector('input');
    • Please register or sign in to reply
  • Théodore Biadala
    Théodore Biadala @nod_ started a thread on the diff
    • core/modules/field_ui/js/field_ui.js
    517 587 if (input.checked) {
    518 this.selectHandler(clickToSelectEl, input);
    588 this.selectHandler(items, item, input);
    519 589 }
    520 },
    521 );
    590 });
    591 });
    522 592 },
    523 593 // Adds click event listener to the field card.
    524 clickHandler(clickToSelectEl, input) {
    525 $(clickToSelectEl).on('click', (event) => {
    526 const clickToSelect = event.target.closest('.js-click-to-select');
    527 this.selectHandler(clickToSelect, input);
    528 $(input).trigger('updateOptions');
    594 itemClickHandler(items, item, input) {
    • Théodore Biadala
      Théodore Biadala @nod_
      Maintainer

      behaviors object are not supposed to have more than attach/detach. If there is logic to store, let's create a new thing in Drupal.fieldUIsomething in a similar way as fieldUIOverview, and fieldUIDisplayOverview

    • Please register or sign in to reply
  • Théodore Biadala
    Théodore Biadala @nod_ started a thread on the diff
    • core/modules/field_ui/js/field_ui.js
    597 // by keyboard navigation.
    598 if (
    599 event.type === 'click' &&
    600 event.clientX !== 0 &&
    601 event.clientY !== 0
    602 ) {
    603 this.selectHandler(items, item, input);
    604 input.dispatchEvent(new Event('updateOptions'));
    605 }
    529 606 });
    530 607 },
    531 608 // Handles adding and removing classes for the different states.
    532 selectHandler(clickToSelect, input) {
    533 $(input).on('focus', () => clickToSelect.classList.add('focus'));
    534 $(input).on('blur', () => clickToSelect.classList.remove('focus'));
    609 selectHandler(items, item, input) {
    • Théodore Biadala
      Théodore Biadala @nod_
      Maintainer

      behaviors object are not supposed to have more than attach/detach. If there is logic to store, let's create a new thing in Drupal.fieldUIsomething in a similar way as fieldUIOverview, and fieldUIDisplayOverview

    • Please register or sign in to reply
  • Théodore Biadala
    Théodore Biadala @nod_ started a thread on the diff
    • core/modules/field_ui/js/field_ui.js
    601 event.clientY !== 0
    602 ) {
    603 this.selectHandler(items, item, input);
    604 input.dispatchEvent(new Event('updateOptions'));
    605 }
    529 606 });
    530 607 },
    531 608 // Handles adding and removing classes for the different states.
    532 selectHandler(clickToSelect, input) {
    533 $(input).on('focus', () => clickToSelect.classList.add('focus'));
    534 $(input).on('blur', () => clickToSelect.classList.remove('focus'));
    609 selectHandler(items, item, input) {
    610 input.addEventListener('focus', () => {
    611 item.classList.add('focus');
    612 });
    613 input.addEventListener('blur', () => item.classList.remove('focus'));
    • Comment on lines +610 to +613
      Théodore Biadala
      Théodore Biadala @nod_
      Maintainer

      This is adding a new event listener every time a field is selected, it you select the item 10 times there will be 10 focus/blur event listeners.

      This should be delegated (my preference) or added once when initializing the grid navigation.

    • Please register or sign in to reply
  • Théodore Biadala
    Théodore Biadala @nod_ started a thread on the diff
    • core/modules/field_ui/js/field_ui.js
    533 $(input).on('focus', () => clickToSelect.classList.add('focus'));
    534 $(input).on('blur', () => clickToSelect.classList.remove('focus'));
    609 selectHandler(items, item, input) {
    610 input.addEventListener('focus', () => {
    611 item.classList.add('focus');
    612 });
    613 input.addEventListener('blur', () => item.classList.remove('focus'));
    535 614 input.checked = true;
    536 document
    537 .querySelectorAll('.js-click-to-select.selected')
    538 .forEach((item) => {
    539 item.classList.remove('selected');
    615 [...items]
    616 .filter((selected) => selected.classList.contains('selected'))
    617 .forEach((selected) => {
    618 selected.classList.remove('selected');
    • Comment on lines +615 to +618
      Théodore Biadala
      Théodore Biadala @nod_
      Maintainer

      There should only be one selected right? Dom if fast enough, we can do

      const selected = container.querySelector(`.${itemClassName}.selected`);
    • Please register or sign in to reply
  • Théodore Biadala
    Théodore Biadala @nod_ started a thread on the diff
    • core/modules/field_ui/js/field_ui.js
    524 clickHandler(clickToSelectEl, input) {
    525 $(clickToSelectEl).on('click', (event) => {
    526 const clickToSelect = event.target.closest('.js-click-to-select');
    527 this.selectHandler(clickToSelect, input);
    528 $(input).trigger('updateOptions');
    594 itemClickHandler(items, item, input) {
    595 item.addEventListener('click', (event) => {
    596 // Checking if it's a real click and event is not triggered
    597 // by keyboard navigation.
    598 if (
    599 event.type === 'click' &&
    600 event.clientX !== 0 &&
    601 event.clientY !== 0
    602 ) {
    603 this.selectHandler(items, item, input);
    604 input.dispatchEvent(new Event('updateOptions'));
  • Théodore Biadala
    Théodore Biadala @nod_ started a thread on the diff
    • core/modules/field_ui/js/field_ui.js
    520
    521 this.itemClickHandler(items, item, input);
    522
    523 // Adding grid-style keyboard navigation.
    524 // For the reference https://www.w3.org/WAI/ARIA/apg/patterns/grid/
    525 input.addEventListener('keydown', (event) => {
    526 const columns = window
    527 .getComputedStyle(container)
    528 .getPropertyValue('grid-template-columns')
    529 .split(' ').length;
    530 if (columns > 1) {
    531 const currentIndex = Array.from(items).indexOf(
    532 document.activeElement.closest(`.${itemClassName}`),
    533 );
    534 let newIndex;
    535 let isReachedEdge = false;
  • Théodore Biadala
    Théodore Biadala @nod_ started a thread on the diff
    • core/modules/field_ui/js/field_ui.js
    516 const input = item.querySelector('input');
    517 if (!input) {
    518 return;
    519 }
    520
    521 this.itemClickHandler(items, item, input);
    522
    523 // Adding grid-style keyboard navigation.
    524 // For the reference https://www.w3.org/WAI/ARIA/apg/patterns/grid/
    525 input.addEventListener('keydown', (event) => {
    526 const columns = window
    527 .getComputedStyle(container)
    528 .getPropertyValue('grid-template-columns')
    529 .split(' ').length;
    530 if (columns > 1) {
    531 const currentIndex = Array.from(items).indexOf(
  • Théodore Biadala
    Théodore Biadala @nod_ started a thread on the diff
    • core/modules/field_ui/js/field_ui.js
    542 }
    543 break;
    544 case 'ArrowDown':
    545 newIndex = currentIndex + columns;
    546 if (newIndex >= items.length) {
    547 isReachedEdge = true;
    548 }
    549 break;
    550 case 'ArrowLeft':
    551 newIndex = currentIndex - 1;
    552 if ([columns - 1, -1].includes(newIndex % columns)) {
    553 isReachedEdge = true;
    513 554 }
    555 break;
    556 case 'ArrowRight':
    557 newIndex = currentIndex + 1;
    Please register or sign in to reply