Bug 199606 – Make tabIndex IDL attribute reflect its content attribute

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ryosuke Niwa]

See https://github.com/whatwg/html/issues/1786#issuecomment-504925534

PR: https://github.com/whatwg/html/pull/4754

[Radar WebKit Bug Importer]

<rdar://problem/52811448>

[Domenic Denicola]

The spec PR was merged, but perhaps a bit prematurely. A follow up is being done at https://github.com/whatwg/html/pull/4759.

According to the latest test results at https://github.com/web-platform-tests/wpt/pull/17657 WebKit is already very compliant in terms of observable behavior. The only failing tests as of this time are around summary elements that are not first-summary-children of details elements, and how they aren't respecting explicitly-set tabindex="" attribute values.

That said, there may be opportunities for code simplification here, similar to in Blink. https://bugs.chromium.org/p/chromium/issues/detail?id=982350 is the counterpart Blink bug

[Ryosuke Niwa]

Created attachment 376444 [details]
WIP

[EWS Watchlist]

Comment on attachment 376444 [details]
WIP

Attachment 376444 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/12920931

New failing tests:
imported/w3c/web-platform-tests/html/dom/reflection-misc.html
svg/custom/tabindex-order.html

[EWS Watchlist]

Created attachment 376456 [details]
Archive of layout-test-results from ews100 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 376444 [details]
WIP

Attachment 376444 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12921299

New failing tests:
svg/custom/tabindex-order.html

[EWS Watchlist]

Created attachment 376466 [details]
Archive of layout-test-results from ews117 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 376444 [details]
WIP

Attachment 376444 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12921617

New failing tests:
svg/custom/tabindex-order.html

[EWS Watchlist]

Created attachment 376469 [details]
Archive of layout-test-results from ews213 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews213  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Ryosuke Niwa]

Created attachment 376970 [details]
Patch

[EWS Watchlist]

Comment on attachment 376970 [details]
Patch

Attachment 376970 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12955051

New failing tests:
fast/dom/tabindex-defaults.html
svg/custom/tabindex-order.html

[EWS Watchlist]

Created attachment 376980 [details]
Archive of layout-test-results from ews212 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews212  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Ryosuke Niwa]

Hm... maybe Windows EWS didn't have https://trac.webkit.org/changeset/248983/webkit?

[Ryosuke Niwa]

Created attachment 376981 [details]
Updated for trunk

[Ryosuke Niwa]

Created attachment 376983 [details]
Rebaelined more tests

[Ryosuke Niwa]

Created attachment 376984 [details]
Rebaelined more tests

[EWS Watchlist]

Comment on attachment 376984 [details]
Rebaelined more tests

Attachment 376984 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12955418

New failing tests:
svg/custom/tabindex-order.html

[EWS Watchlist]

Created attachment 376992 [details]
Archive of layout-test-results from ews213 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews213  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Ryosuke Niwa]

Hm... it looks like I'd have to tweak the test further.

[Ryosuke Niwa]

Created attachment 377071 [details]
Fix svg/custom/tabindex-order.html on Windows

[Ryosuke Niwa]

Ping reviewers.

[Ryosuke Niwa]

Ping?

[Chris Dumez]

Comment on attachment 377071 [details]
Fix svg/custom/tabindex-order.html on Windows

View in context: https://bugs.webkit.org/attachment.cgi?id=377071&action=review

r=me with nit.

> Source/WebCore/dom/Element.cpp:270
> +    auto tabIndex = tabIndexSetExplicitly();

nit: Could be:
return tabIndexSetExplicitly().valueOr(defaultTabIndex());

[Ryosuke Niwa]

Comment on attachment 377071 [details]
Fix svg/custom/tabindex-order.html on Windows

View in context: https://bugs.webkit.org/attachment.cgi?id=377071&action=review

>> Source/WebCore/dom/Element.cpp:270
>> +    auto tabIndex = tabIndexSetExplicitly();
> 
> nit: Could be:
> return tabIndexSetExplicitly().valueOr(defaultTabIndex());

Oh, I specifically didn't do that to avoid calling defaultTabIndex(), which is a virtual function.

[Ryosuke Niwa]

Maybe we should add a comment next to it explaining the reason we don't use valueOr(~)?

[Chris Dumez]

Comment on attachment 377071 [details]
Fix svg/custom/tabindex-order.html on Windows

View in context: https://bugs.webkit.org/attachment.cgi?id=377071&action=review

>>> Source/WebCore/dom/Element.cpp:270
>>> +    auto tabIndex = tabIndexSetExplicitly();
>> 
>> nit: Could be:
>> return tabIndexSetExplicitly().valueOr(defaultTabIndex());
> 
> Oh, I specifically didn't do that to avoid calling defaultTabIndex(), which is a virtual function.

return tabIndexSetExplicitly().valueOrCompute([] { return defaultTabIndex(); });

:D

[Chris Dumez]

(In reply to Ryosuke Niwa from comment #25)
> Maybe we should add a comment next to it explaining the reason we don't use
> valueOr(~)?

We added valueOrCompute() specifically for this already, let's use it.

[Chris Dumez]

(In reply to Chris Dumez from comment #26)
> Comment on attachment 377071 [details]
> Fix svg/custom/tabindex-order.html on Windows
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=377071&action=review
> 
> >>> Source/WebCore/dom/Element.cpp:270
> >>> +    auto tabIndex = tabIndexSetExplicitly();
> >> 
> >> nit: Could be:
> >> return tabIndexSetExplicitly().valueOr(defaultTabIndex());
> > 
> > Oh, I specifically didn't do that to avoid calling defaultTabIndex(), which is a virtual function.
> 
> return tabIndexSetExplicitly().valueOrCompute([] { return defaultTabIndex();
> });
> 
> :D

Actually, it looks like the syntax is like so:
return valueOrCompute(tabIndexSetExplicitly(), [] { return defaultTabIndex(); });

[Ryosuke Niwa]

(In reply to Chris Dumez from comment #28)
> (In reply to Chris Dumez from comment #26)
> > Comment on attachment 377071 [details]
> > Fix svg/custom/tabindex-order.html on Windows
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=377071&action=review
> > 
> > >>> Source/WebCore/dom/Element.cpp:270
> > >>> +    auto tabIndex = tabIndexSetExplicitly();
> > >> 
> > >> nit: Could be:
> > >> return tabIndexSetExplicitly().valueOr(defaultTabIndex());
> > > 
> > > Oh, I specifically didn't do that to avoid calling defaultTabIndex(), which is a virtual function.
> > 
> > return tabIndexSetExplicitly().valueOrCompute([] { return defaultTabIndex();
> > });
> > 
> > :D
> 
> Actually, it looks like the syntax is like so:
> return valueOrCompute(tabIndexSetExplicitly(), [] { return
> defaultTabIndex(); });

Oh neat!

[Chris Dumez]

(In reply to Ryosuke Niwa from comment #29)
> (In reply to Chris Dumez from comment #28)
> > (In reply to Chris Dumez from comment #26)
> > > Comment on attachment 377071 [details]
> > > Fix svg/custom/tabindex-order.html on Windows
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=377071&action=review
> > > 
> > > >>> Source/WebCore/dom/Element.cpp:270
> > > >>> +    auto tabIndex = tabIndexSetExplicitly();
> > > >> 
> > > >> nit: Could be:
> > > >> return tabIndexSetExplicitly().valueOr(defaultTabIndex());
> > > > 
> > > > Oh, I specifically didn't do that to avoid calling defaultTabIndex(), which is a virtual function.
> > > 
> > > return tabIndexSetExplicitly().valueOrCompute([] { return defaultTabIndex();
> > > });
> > > 
> > > :D
> > 
> > Actually, it looks like the syntax is like so:
> > return valueOrCompute(tabIndexSetExplicitly(), [] { return
> > defaultTabIndex(); });
> 
> Oh neat!

Good catch on the virtual function call, I failed to notice :)

[Ryosuke Niwa]

Committed r249237: <https://trac.webkit.org/changeset/249237>

[Alexey Proskuryakov]

This change made svg/custom/tabindex-order.html very flaky, slowing down EWS and commit queue.

[Ryosuke Niwa]

(In reply to Alexey Proskuryakov from comment #32)
> This change made svg/custom/tabindex-order.html very flaky, slowing down EWS
> and commit queue.

Hm... weird. Let me look into that.