add iron-focusables-helper #200

valdrinkoshi · 2016-06-28T14:18:47Z

Fixes #196, fixes #194, fixes #192.
iron-focusables-helper provides getTabbableNodes method, used to compute the tabbable elements within a specific node, searching both in the light and shadow dom, sorting by tabindex.
_focusableNodes has been changed to invoke this method. Tests have been updated to look for the focusable nodes once the overlays are opened (as it is required for a node to be visible in order to be considered tabbable).
Sorting by tabindex is a stable sort (merge sort), since Array.prototype.sort is not guaranteed to be stable (fixes #209).

bicknellr · 2016-06-28T23:32:18Z

iron-focusables-helper.html

+        // If there is at least one element with tabindex > 0, we need to sort
+        // the final array by tabindex.
+        var maxTabIndex = this._findTabbableNodes(node, result);
+        maxTabIndex > 0 && result.sort(this._tabIndexSort);


Array.prototype.sort isn't guaranteed to be stable.

Uhm, in which way/which browsers? I've written unit tests that check for the result of this method & if it returns a correctly sorted array

Practically speaking, I doubt any browsers use an unstable sort, but I figured it might be worth pointing out.

I couldn't experience any issue so far, and the tests ensured it works as expected. Will keep an eye on it if it shows a weird behavior!

@bicknellr you were right, the issue was reported here #209
Will fix this!

bicknellr · 2016-07-08T22:16:05Z

iron-focusables-helper.html

+       * @return {Number}
+       * @private
+       */
+      _findTabbableNodes: function(node, result) {


The signature of this function feels very specific to its use in getTabbableNodes; given the name _findTabbableNodes, I'd expect this to return the list rather than taking a list to modify. You'd be able to take lines 74, 79, 88-90 out of this function too if you computed maxTabIndex in getTabbableNodes.

The reasoning for this layout is to avoid too many searches & achieve recursion, as we need to do a depth first search in order to keep the tabindex order. The fact that we return the maxTabIndex is just to support cases where we might have a tabindex > 0 (which is an anti-pattern, but I guess we have to support it, right?).
I could move the searching of the max tabindex outside, but that would require another for loop over the result :/

Either way, the tab index of each element in the list has to be reviewed. As written now, the check is interleaved with the traversal; splitting it out won't change the time complexity but would let this function do one less thing. If there was some sort of early return behavior that could be exploited here, I'd agree, but I don't think there is.

Yes, the tab index of each element has to be reviewed. Currently, I do that on the same loop where i collect the tabbables. This allows me to save another loop over the tabbables after they get collected.

// o(node-depth) search var tabbables = []; var maxTabIndex = this._findTabbables(node, tabbables); // o(node-depth) + o(tabbables) search var tabbables = this._findTabbables(node); var maxTabIndex = this._findMaxTabIndex(tabbables);

The second option is more readable but less performant. How to proceed on this one?

All the work done in _findMaxTabIndex still happens in the original _findTabbables. I think the second case can even potentially be more performant: considering only work done to find the max tab index, the original _findTabbables has best case O(n) because line 105 occurs for every node traversed, even when the nodes in question are not in the tab order. However, splitting this out into a separate loop gives a best case of O(1) because the list, which will be some subset of the traversed elements, can potentially be empty. (Both still have worst case O(n).)

Just to recap:

we don't want to sort if all elements have tabindex = 0 -> look if there is one element with tabIndex > 0

we have to traverse all the children to find the tabbables -> minimum is O(n)

to filter the tabbable elements, we must look at their tabindex to be >= 0

Let's compare the two options in some scenarios for n nodes of which m are tabbable, and i is the index of the tabbable with tabindex > 0:

// option 1 var tabbables = []; var maxTabIndex = this._findTabbables(node, tabbables); // option 2 var tabbables = this._findTabbables(node); var maxTabIndex = this._findMaxTabIndex(tabbables);

scenario 1 (worst case scenario)

m = n (all are tabbable), i = -1 (no tabbable with tabindex > 0)

option 1: O(n)

option 2: O(n) + O(n) (needs to search until the end of tabbables)

scenario 2

m = n/10 (only 10% tabbable), i = -1 (no tabbable with tabindex > 0)

option 1: O(n)

option 2: O(n) + O(n/10) (needs to search until the end of tabbables)

scenario 3

m = n/10 (only 10% tabbable), i = m/2 (the tabbable on the middle has tabindex > 0)

option 1: O(n)

option 2: O(n) + O(n/20) (can stop as soon as it finds the tabbable with tabindex > 0)

scenario 4 (best case scenario)

m = 0 (no tabbables)

option 1: O(n)

option 2: O(n)

If we average of worst and best scenarios, we see that on average option 2 is 50% slower than option 1.
In practice probably this doesn't make a huge difference, I just would like to understand if you see a better, more performant way to tackle this. Please correct me if I wrote something wrong :)

I'm pretty sure all of those are O(n) since "O(n) + O(n)" = O(n) but, expanding out the two pieces of work (gather and max), option 1 would be "O(n) + O(n)" (maybe more clearly written as something like "n * 2O(1)" in this case) for all of those situations, even though it uses a single loop.

function sumAndConcat1(list) { var sum = 0; var string = ""; for (var i = 0; i < list.length; i++) { sum += list[i]; string += list[i]; } return {sum, string}; } function sumAndConcat2(list) { var sum = 0; var string = ""; for (var i = 0; i < list.length; i++) { sum += list[i]; } for (var i = 0; i < list.length; i++) { string += list[i]; } return {sum, string}; }

I don't think the use of a single for loop in sumAndConcat1 shouldn't give it any performance advantage over sumAndConcat2. And, if the second loop in sumAndConcat2 could perform fewer than list.length iterations (assuming the function was doing something else), wouldn't that be an advantage for sumAndConcat2?

However, I suppose the end to my argument wouldn't have much benefit anyway given the operation being optimized out (max) has such a small cost. :P I still think the function's signature is weird but, yeah, the rest of my argument is more pedantic than practical.

Since we only need to know if the maxTabIndex is greater than zero, and not its actual value, I could remove the Math.max part and make it return a boolean. I'll check also a better/clearer signature for this method 👍

kornalius · 2016-07-22T18:58:08Z

Got a bug in iron-focusables-helper in _isVisible.

if (node.style.visibility !== 'hidden' && node.style.display !== 'none') {
"Cannot read property 'visibility' of undefined"

node.style does not exist on #comment nodes.

valdrinkoshi · 2016-07-22T19:54:55Z

Good catch @kornalius, fixed in the last commit!

valdrinkoshi · 2016-08-05T01:34:13Z

Did some tests with heavy.html (see below), and on average:

_onCaptureTab when no wrapping is ~ 0.02ms
_onCaptureTab when wrapping is ~1ms

Querying for tabbable nodes is the part that takes the most of the time:

implementation on master (doesn't sort correctly with this example) takes ~0.8ms
the implementation of this PR does sort correctly but takes ~5ms (most likely because now we explore deeply the node)

heavy.html

<button onclick="plain.open()">overlay</button>
<simple-overlay with-backdrop id="plain">
  <a href="#">click</a>
  <a href="#">click</a>
  <a href="#">click</a>
  <input>
  <paper-input></paper-input>
  <paper-input></paper-input>
  <paper-input></paper-input>
  <paper-input></paper-input>
  <paper-input></paper-input>
  <div contenteditable>contenteditable</div>
  <div tabindex="0">tabindex 0</div>

  <a href="#">click</a>


  <paper-dropdown-menu>
    <ul class="dropdown-content">
      <button>one</button>
      <button>two</button>
      <li><a href="#">click</a></li>
      <li><a href="#">click</a></li>
      <li><a href="#">click</a></li>
      <li><a href="#">click</a></li>
      <li><a href="#">click</a></li>
    </ul>
  </paper-dropdown-menu>
  <paper-dropdown-menu>
    <ul class="dropdown-content">
      <button>one</button>
      <button>two</button>
      <li><a href="#">click</a></li>
      <li><a href="#">click</a></li>
      <li><a href="#">click</a></li>
      <li><a href="#">click</a></li>
      <li><a href="#">click</a></li>
    </ul>
  </paper-dropdown-menu>
  <paper-dropdown-menu>
    <ul class="dropdown-content">
      <button>one</button>
      <button>two</button>
      <li><a href="#">click</a></li>
      <li><a href="#">click</a></li>
      <li><a href="#">click</a></li>
      <li><a href="#">click</a></li>
      <li><a href="#">click</a></li>
    </ul>
  </paper-dropdown-menu>
  <paper-dropdown-menu>
    <ul class="dropdown-content">
      <button>one</button>
      <button>two</button>
      <li><a href="#">click</a></li>
      <li><a href="#">click</a></li>
      <li><a href="#">click</a></li>
      <li><a href="#">click</a></li>
      <li><a href="#">click</a></li>
    </ul>
  </paper-dropdown-menu>
  <paper-dropdown-menu>
    <ul class="dropdown-content">
      <button>one</button>
      <button>two</button>
      <li><a href="#">click</a></li>
      <li><a href="#">click</a></li>
      <li><a href="#">click</a></li>
      <li><a href="#">click</a></li>
      <li><a href="#">click</a></li>
    </ul>
  </paper-dropdown-menu>
  <paper-dropdown-menu>
    <ul class="dropdown-content">
      <button>one</button>
      <button>two</button>
      <li><a href="#">click</a></li>
      <li><a href="#">click</a></li>
      <li><a href="#">click</a></li>
      <li><a href="#">click</a></li>
      <li><a href="#">click</a></li>
    </ul>
  </paper-dropdown-menu>
  <paper-dropdown-menu>
    <ul class="dropdown-content">
      <button>one</button>
      <button>two</button>
      <li><a href="#">click</a></li>
      <li><a href="#">click</a></li>
      <li><a href="#">click</a></li>
      <li><a href="#">click</a></li>
      <li><a href="#">click</a></li>
    </ul>
  </paper-dropdown-menu>
  <button onclick="plain.close()">Close</button>
</simple-overlay>

valdrinkoshi · 2016-08-05T01:36:34Z

@cdata FYI ^

notwaldorf · 2016-08-08T23:56:13Z

going to unassign myself from this since I don't seem needed 😂

bicknellr · 2016-08-11T00:06:56Z

iron-focusables-helper.html

+       */
+      _mergeSortByTabIndex: function(left, right) {
+        var result = [];
+        while ((left.length > 0) && (right.length > 0)) {


I was totally about to leave comments about how everything's going to break if either is empty and the concat with 'empty' arrays until I noticed that this is && and not ||!

mercmobily · 2016-09-04T13:21:40Z

Thanks for your work on this @valdrinkoshi . This is the kind of contribution I feel I will never be able to make. It would be great to get a proper solution to PolymerElements/paper-dialog#125 !

romancmk · 2016-10-24T20:46:40Z

@valdrinkoshi - Is there any update on this PR? I'm interested in having this PR merged in. Please let me know if there's anything I can do to help. Thanks!

…ble to discard disabled elements that have tabindex

bicknellr · 2016-10-26T18:32:20Z

iron-focusables-helper.html

+        //   <input id="A" slot="a">
+        //   <input id="B" slot="b" tabindex="1">
+        //  </div>
+        // TODO(valdrin) implement ShadowDOM v1 when upgrading to Polymer v2.0.


bicknellr

Ok, minor comment and LGTM.

bicknellr · 2016-10-26T18:38:10Z

iron-focusables-helper.html

+      },
+
+      /**
+       * Returns if an element has lower tab order compared to another element


Which is a and which is b?

valdrinkoshi · 2016-10-26T18:58:09Z

@bicknellr good catch, clarified the jsdoc of _hasLowerTabOrder 👌

Files App's quick view is suffered from the bug PolymerElements/iron-overlay-behavior#200, and focus doesn't move inside files-quick-view element if a button inside it is hidden. Updating iron-overlay-behavior fixes the issue. This CL was created by the following steps. 1. Change the version in third_party/polymer/v1_0/bower.json 2. Run third_party/polymer/v1_0/reproduce.sh 3. tools/polymer/polymer_grdp_to_txt.py ui/webui/resources/polymer_resources.grdp > polymer.txt vim polymer.txt # add iron-overlay-behavior/iron-focusables-helper{-extracted.js,.html} tools/polymer/txt_to_polymer_grdp.py polymer.txt > ui/webui/resources/polymer_resources.grdp BUG=663636 TEST=git cl try. third_party/closure_compiler/run_compiler. The issue mentioned above is fixed. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2633003003 Cr-Commit-Position: refs/heads/master@{#444313}

valdrinkoshi assigned cdata and notwaldorf Jun 28, 2016

googlebot added the cla: yes label Jun 28, 2016

valdrinkoshi assigned bicknellr Jun 28, 2016

bicknellr reviewed Jun 28, 2016
View reviewed changes

valdrinkoshi mentioned this pull request Jul 6, 2016

exclude hidden or no-display elements from focusables #198

Closed

bicknellr reviewed Jul 8, 2016
View reviewed changes

valdrinkoshi force-pushed the iron-focusables-helper branch 3 times, most recently from 27b4e7d to 284fdda Compare July 12, 2016 19:56

This was referenced Jul 12, 2016

iron-focus-wrap-behavior & iron-overlay-base-behavior #178

Closed

focus issue specific to the versions used #204

Closed

valdrinkoshi force-pushed the iron-focusables-helper branch 3 times, most recently from 980b33a to 6067107 Compare July 21, 2016 00:25

valdrinkoshi force-pushed the iron-focusables-helper branch from c9ae30b to fc214ea Compare July 22, 2016 19:54

valdrinkoshi force-pushed the iron-focusables-helper branch from fc214ea to ce768cf Compare August 5, 2016 01:11

notwaldorf removed their assignment Aug 8, 2016

bicknellr reviewed Aug 11, 2016
View reviewed changes

valdrinkoshi mentioned this pull request Aug 12, 2016

document.blockingElements polyfill with inert PolymerLabs/blocking-elements#1

Merged

valdrinkoshi mentioned this pull request Aug 30, 2016

Absurd problem with tabs when containing an element with <paper-input> PolymerElements/paper-dialog#125

Open

2 tasks

justinribeiro mentioned this pull request Sep 14, 2016

Keyboard navigation for UI (mainly for forms) GoogleDeveloperExperts/experts-app-web#93

Closed

valdrinkoshi added 8 commits October 26, 2016 10:55

add iron-focusables-helper

8a84257

comments for isFocusable. Remove _isStyleVisible method. Fix isFocusa…

8d4fc0e

…ble to discard disabled elements that have tabindex

check for tabindex attribute instead of tabIndex property

8a5dfe6

_normalizedTabIndex instead of isTabbable

3a370ed

return needsSortByTabIndex boolean

7e2c362

implement a stable sort (merge sort)

a9b2158

filter out node types that are not an element. Updated jsdocs

7451cd3

use matches instead of looking at the localName

8b7f616

valdrinkoshi force-pushed the iron-focusables-helper branch from ce768cf to e5fa398 Compare October 26, 2016 18:31

bicknellr reviewed Oct 26, 2016

View reviewed changes

bicknellr approved these changes Oct 26, 2016

View reviewed changes

valdrinkoshi force-pushed the iron-focusables-helper branch from e5fa398 to 1f4558a Compare October 26, 2016 18:54

fix test, add comment for supporting ShadowDom v1

1f4558a

valdrinkoshi merged commit 7ed18e8 into master Oct 26, 2016

valdrinkoshi deleted the iron-focusables-helper branch April 17, 2017 23:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add iron-focusables-helper #200

add iron-focusables-helper #200

valdrinkoshi commented Jun 28, 2016 •

edited

Loading

bicknellr Jun 28, 2016

valdrinkoshi Jun 28, 2016

bicknellr Jun 28, 2016

valdrinkoshi Jun 29, 2016

valdrinkoshi Jul 20, 2016

bicknellr Jul 8, 2016 •

edited

Loading

valdrinkoshi Jul 8, 2016

bicknellr Jul 8, 2016

valdrinkoshi Jul 8, 2016

bicknellr Jul 9, 2016

valdrinkoshi Jul 9, 2016

valdrinkoshi Jul 9, 2016

bicknellr Jul 11, 2016

valdrinkoshi Jul 12, 2016

kornalius commented Jul 22, 2016

valdrinkoshi commented Jul 22, 2016

valdrinkoshi commented Aug 5, 2016 •

edited

Loading

valdrinkoshi commented Aug 5, 2016

notwaldorf commented Aug 8, 2016

bicknellr Aug 11, 2016

mercmobily commented Sep 4, 2016

romancmk commented Oct 24, 2016

bicknellr Oct 26, 2016

bicknellr left a comment

bicknellr Oct 26, 2016

valdrinkoshi commented Oct 26, 2016

add iron-focusables-helper #200

add iron-focusables-helper #200

Conversation

valdrinkoshi commented Jun 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bicknellr Jul 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scenario 1 (worst case scenario)

scenario 2

scenario 3

scenario 4 (best case scenario)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kornalius commented Jul 22, 2016

valdrinkoshi commented Jul 22, 2016

valdrinkoshi commented Aug 5, 2016 • edited Loading

valdrinkoshi commented Aug 5, 2016

notwaldorf commented Aug 8, 2016

Choose a reason for hiding this comment

mercmobily commented Sep 4, 2016

romancmk commented Oct 24, 2016

Choose a reason for hiding this comment

bicknellr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valdrinkoshi commented Oct 26, 2016

valdrinkoshi commented Jun 28, 2016 •

edited

Loading

bicknellr Jul 8, 2016 •

edited

Loading

valdrinkoshi commented Aug 5, 2016 •

edited

Loading