Bug 199606

<rdar://problem/52811448>

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

Created attachment 376444 [details] WIP

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

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

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

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

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

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

Created attachment 376970 [details] Patch

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

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

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

Created attachment 376981 [details] Updated for trunk

Created attachment 376983 [details] Rebaelined more tests

Created attachment 376984 [details] Rebaelined more tests

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

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

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

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

Ping reviewers.

Ping?

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());

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.

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

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

(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.

(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(); });

(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!

(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 :)

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

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

(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.