luke-chang / js-spatial-navigation

A javascript-based implementation of Spatial Navigation.
Mozilla Public License 2.0
370 stars 117 forks source link

DefaultElement is not working properly with selectors #48

Closed erwanlfrt closed 2 years ago

erwanlfrt commented 2 years ago

ISSUE : defaultElement is not working properly with selectors

A - The issue

The defaultElement is not working properly.

According to the documentation about defaultElement :

the first element matching defaultElement within this section will be chosen first.

However in the source code, into _spatialnavigation.js -> function getSectionDefaultElement, l. 561 :

if (typeof defaultElement === 'string') {
      defaultElement = parseSelector(defaultElement)[0]; 
}

The default element is indeed the first element matching the selector but the first element is not necessarily within the targeted section.

Example with this sample :

<div id="section1" class="focusable">
      <button class="target" >Button 1</button> 
      <button>Button 2</button>
      <button>Button 3</button>
</div>
<div id="section2" class="focusable">
    <button class="target">Button 4</button>
    <button>Button 5</button>
    <button>Button 6</button>
</div>

And for section2 the following configuration :

var conf = {
  enterTo: 'default-element',
  defaultElement: '.target'
}

In this case, parseSelector(defaultElement)[0] will return the <button class="target" >Button 1</button> element because the selector is .target. But this element is not within section2 so it will not be focused but the source code does not check if other elements returned by parseSelector(defaultElement) is within the targeted section.

B - Solutions to fix it...

1 - Checking each returned element

Edit - 04/01/2022 A simple way to fix it :

function getSectionDefaultElement(sectionId) {
  var defaultElement = _sections[sectionId].defaultElement;
  if (!defaultElement) {
    return null;
  }
  if (typeof defaultElement === 'string') {
    // defaultElement = parseSelector(defaultElement)[0]; // bug !
    var elements = parseSelector(defaultElement);
    // check each element to see if it's navigable and stop when one has been found
    for (var element of elements) {
      if (isNavigable(element, sectionId, true)) {
        return element;
      }
    }
    return null;
  } else if ($ && defaultElement instanceof $) {
    defaultElement = defaultElement.get(0);
    if (isNavigable(defaultElement, sectionId, true)) {
      return defaultElement;
    }
  }
  return null;
}

2 - Changing the documentation

If it is the normal behavior then change the documentation about defaultElement by something like that:

When a section is specified to be the next focused target, e.g. focus('some-section-id') is called, the first element matching defaultElement within this section will be chosen first (warning : the given selector must concern elements exclusively within this section).

luke-chang commented 2 years ago

That's indeed an issue, and your first solution is what it's supposed to be. Let me see if I can patch it in the coming days. Thanks for spotting it.

martinscola-hwm commented 2 years ago

That's indeed an issue, and your first solution is what it's supposed to be. Let me see if I can patch it in the coming days. Thanks for spotting it.

Any idea if you're going to fix this in the near future?

luke-chang commented 2 years ago

Fixed in 1a59d0bda1cbae89cb44babbefb8bafa2438a1bf. Thank @erwanlfrt again for pointing this out.