Focus handling in focus_trap.dart is not correct

[jolleekin]

The following code is from focus_trap.dart

  void _focusFirstInOrder(Iterator<Element> iterator) {
    while (iterator.moveNext()) {
      if (iterator.current.tabIndex == 0 && _visible(iterator.current)) {
        iterator.current.focus();
        return;
      }
    }
    _focusDefault();
  }

  bool _visible(Element element) {
    return (element.offsetWidth != 0 && element.offsetHeight != 0);
  }

The focusability check iterator.current.tabIndex == 0 && _visible(iterator.current) is not correct. An element is focusable if

1. The display CSS property must not be none for the element and all of its ancestors
2. Its computed visibility CSS property must be visible
3. Its tabIndex is >= 0
4. It doesn't have a disabled property or the disabled property is set to true
5. There may be other cases not listed here

The solution could be trying to focus the element and check if it's actually focused.

bool _tryFocus(Element e) {
  // May check if tabIndex >= 0 before calling focus().
  e.focus();
  return e == document.activeElement; // Assume no ShadowDOM.
}

[TedSander]

While not perfect I don't see us changing this, at least not anytime soon.

The solution that you suggest I think is unperformant as it would require draw calls for each call to focus, and then a read for activeElement. We try to wrap focus calls in domService.write calls so that we can schedule them correctly.

We also have a good amount of code that is using this logic at this point. That said we could document it better what is going on.

[jolleekin]

Should a check for disabled be added? Native controls such as button and input have tabIndex == 0 even when the disabled property is true.

Using two bindings, one to set disabled and one to set tabIndex, is not very convenient.

[TedSander]

The problem is that disabled isn't on the Element/HtmlElement interface itself and with dart typing I don't think you could do: (dynamic element).disabled. So instead you would have to check for all the types that you know have a disabled property.

if (element is InputElement) disabled = element.disabled; if (element is ButtonElement) disabled = element.disabled; if (element is OptGroupElement) disabled = element.disabled;

Just a quick look this would be for LinkElement, SelectElement, OptionElement, etc.

Seems like that could be slow and buggy, but we'll think about it.

That isn't including the psuedo-elements you could have yourself.