Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

focus() on a non-focusable legend delegates focus in WebKit/Chromium #3950

Closed
zcorpan opened this issue Aug 21, 2018 · 18 comments
Closed

focus() on a non-focusable legend delegates focus in WebKit/Chromium #3950

zcorpan opened this issue Aug 21, 2018 · 18 comments
Labels
interop Implementations are not interoperable with each other topic: fieldset topic: focus

Comments

@zcorpan
Copy link
Member

zcorpan commented Aug 21, 2018

See https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/fast/forms/fieldset/legend-focus.html?q=lang:html+fieldset&sq=package:chromium&dr=C

Demo http://software.hixie.ch/utilities/js/live-dom-viewer/saved/6129

In WebKit and Chromium, the first input in the fieldset is focused. In EdgeHTML and Gecko, focus is not delegated.

cc @whatwg/fieldset
@zcorpan zcorpan added interop Implementations are not interoperable with each other topic: fieldset labels Aug 21, 2018
@zcorpan
Copy link
Member Author

zcorpan commented Aug 21, 2018

Similar test with accesskey https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/fast/forms/fieldset/legend-access-key.html?sq=package:chromium&dr=C&g=0

@MatsPalmgren
Copy link

MatsPalmgren commented Aug 21, 2018 •
edited

FYI, the tabbing order is also incompatible. Firefox treats a rendered <legend> as its first child in terms of tabbing order. Example: http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=6132
It appears Chrome is using DOM order. I tend to think that Firefox' tabbing order makes more sense for a user.

@zcorpan
Copy link
Member Author

zcorpan commented Aug 22, 2018

@MatsPalmgren given the resolution of https://bugzilla.mozilla.org/show_bug.cgi?id=812687 I'm inclined to think that DOM order is "correct".

@MatsPalmgren
Copy link

I'm inclined to think that DOM order is "correct"

Yeah, I agree. I filed a bug.

@MatsPalmgren
Copy link

Regarding the original issue, I think Edge/Firefox is "correct". I don't really see a reason for magically delegating focus like that.

@zcorpan
Copy link
Member Author

zcorpan commented Aug 22, 2018

@mcking65 do you have an opinion here?

@mcking65
Copy link

mcking65 commented Aug 22, 2018 •
edited

100% agree that there is no reason to delegate focus; doing so is definitely a bug.

@zcorpan
Copy link
Member Author

zcorpan commented Aug 27, 2018

@tkent-google you made a change in https://bugs.chromium.org/p/chromium/issues/detail?id=450511 , WDYT about removing this magic altogether?

@rniwa
Copy link
Collaborator

rniwa commented Aug 27, 2018

An important question is whether if any website relies on this WebKit/Chromium behavior or not.

@zcorpan
Copy link
Member Author

zcorpan commented Aug 27, 2018

@tkent-google or @foolip would it be possible to add a use counter?

@tkent-google
Copy link
Collaborator

@tkent-google or @foolip would it be possible to add a use counter?

Done: http://crrev.com/586606
Chrome 70 will have the counter.

aarongable pushed a commit to chromium/chromium that referenced this issue Aug 28, 2018
@tkent-google
Forms: Add a UseCounter for a LEGEND quirk behavior
37d1500 
Bug: 878220
Bug: whatwg/html#3950
Change-Id: Ia0d53411b9dbeb15555ed66bf7539d3fd3fdd509
Reviewed-on: https://chromium-review.googlesource.com/1192367
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Commit-Queue: Kent Tamura <tkent@chromium.org>
Cr-Commit-Position: refs/heads/master@{#586606}
@zcorpan
Copy link
Member Author

zcorpan commented Aug 29, 2018

So apparently Firefox delegates focus when using an accesskey, but not for focus().

https://bugzilla.mozilla.org/show_bug.cgi?id=81481

Edge also behaves the same as Firefox, except if the legend is itself focusable (with tabindex=0), Edge still delegates focus but Firefox does not.

@zcorpan
Copy link
Member Author

zcorpan commented Aug 29, 2018

The spec has this

https://html.spec.whatwg.org/multipage/interactive-elements.html#using-the-accesskey-attribute-on-a-legend-element-to-define-a-command

It doesn't check if the legend is itself focusable. It also says to skip descendants of the legend, but browsers don't do that AFAICT.

@MatsPalmgren
Copy link

Should §4.11.3.6 use the term rendered legend now instead of legend element?

It also says to skip descendants of the legend, but browsers don't do that AFAICT.

Fwiw, I'd prefer to update the spec to follow what UAs do in this case. I think we should traverse the rendered legend and its descendants first, then other fieldset descendants when searching for an element that defines a command. If the rendered legend itself is focusable then it doesn't make sense to delegate the accesskey IMO.

@zcorpan
Copy link
Member Author

zcorpan commented Aug 30, 2018

The behavior and semantic aspects typically use the DOM tree rather than how it's rendered. I think accesskey on legend should work the same even if you style it with e.g. float: left.

zcorpan added a commit that referenced this issue Sep 3, 2018
@zcorpan
Tweak how accesskey on legend works
b240785 
Fixes #3950.

Tests: TODO
@zcorpan zcorpan mentioned this issue Sep 3, 2018
Merged
zcorpan added a commit to web-platform-tests/wpt that referenced this issue Sep 3, 2018
@zcorpan
HTML: test that focus() on legend does not delegate focus
08b1e01 
See whatwg/html#3950
@zcorpan zcorpan mentioned this issue Sep 3, 2018
Merged
@zcorpan
Copy link
Member Author

zcorpan commented Sep 3, 2018

Bugs about focus():
https://bugs.webkit.org/show_bug.cgi?id=189242
https://bugs.chromium.org/p/chromium/issues/detail?id=880026

zcorpan added a commit that referenced this issue Sep 6, 2018
@zcorpan
Tweak how accesskey on legend works
aa374be 
Fixes #3950.

Tests: web-platform-tests/wpt#12800

* Don't special-case focusable legend, like Edge/Chrome/Safari

* Fix grammar

* Fix wording

* Use lists to make it less ambiguous

* and

* Address domenic's comments

* Add an example

* Source formatting nits
zcorpan added a commit to web-platform-tests/wpt that referenced this issue Sep 7, 2018
@zcorpan
HTML: test that focus() on legend does not delegate focus
910a8da 
See whatwg/html#3950
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Sep 12, 2018
@zcorpan @moz-wptsync-bot
Bug 1488179 [wpt PR 12801] - HTML: test that focus() on legend does n…
2730637 
…ot delegate focus, a=testonly

Automatic update from web-platform-tests HTML: test that focus() on legend does not delegate focus

See whatwg/html#3950
--

wpt-commits: 910a8da21bb508cc774ae1acf9ead70f119ebb97
wpt-pr: 12801


--HG--
rename : testing/web-platform/tests/html/editing/focus/composed.window.js => testing/web-platform/tests/html/interaction/focus/composed.window.js
rename : testing/web-platform/tests/html/editing/focus/document-level-focus-apis/document-level-apis.html => testing/web-platform/tests/html/interaction/focus/document-level-focus-apis/document-level-apis.html
rename : testing/web-platform/tests/html/editing/focus/document-level-focus-apis/test.html => testing/web-platform/tests/html/interaction/focus/document-level-focus-apis/test.html
rename : testing/web-platform/tests/html/editing/focus/focus-01.html => testing/web-platform/tests/html/interaction/focus/focus-01.html
rename : testing/web-platform/tests/html/editing/focus/focus-02.html => testing/web-platform/tests/html/interaction/focus/focus-02.html
rename : testing/web-platform/tests/html/editing/focus/focus-management/focus-event-targets-simple.html => testing/web-platform/tests/html/interaction/focus/focus-management/focus-event-targets-simple.html
rename : testing/web-platform/tests/html/editing/focus/focus-management/focus-events.html => testing/web-platform/tests/html/interaction/focus/focus-management/focus-events.html
rename : testing/web-platform/tests/html/editing/focus/processing-model/focus-fixup-rule-one-no-dialogs.html => testing/web-platform/tests/html/interaction/focus/processing-model/focus-fixup-rule-one-no-dialogs.html
rename : testing/web-platform/tests/html/editing/focus/processing-model/preventScroll.html => testing/web-platform/tests/html/interaction/focus/processing-model/preventScroll.html
rename : testing/web-platform/tests/html/editing/focus/processing-model/support/preventScroll-helper.html => testing/web-platform/tests/html/interaction/focus/processing-model/support/preventScroll-helper.html
rename : testing/web-platform/tests/html/editing/focus/sequential-focus-navigation-and-the-tabindex-attribute/focus-tabindex-default-value.html => testing/web-platform/tests/html/interaction/focus/sequential-focus-navigation-and-the-tabindex-attribute/focus-tabindex-default-value.html
rename : testing/web-platform/tests/html/editing/focus/sequential-focus-navigation-and-the-tabindex-attribute/focus-tabindex-negative.html => testing/web-platform/tests/html/interaction/focus/sequential-focus-navigation-and-the-tabindex-attribute/focus-tabindex-negative.html
rename : testing/web-platform/tests/html/editing/focus/sequential-focus-navigation-and-the-tabindex-attribute/focus-tabindex-order.html => testing/web-platform/tests/html/interaction/focus/sequential-focus-navigation-and-the-tabindex-attribute/focus-tabindex-order.html
rename : testing/web-platform/tests/html/editing/focus/sequential-focus-navigation-and-the-tabindex-attribute/focus-tabindex-positive.html => testing/web-platform/tests/html/interaction/focus/sequential-focus-navigation-and-the-tabindex-attribute/focus-tabindex-positive.html
rename : testing/web-platform/tests/html/editing/focus/sequential-focus-navigation-and-the-tabindex-attribute/focus-tabindex-zero.html => testing/web-platform/tests/html/interaction/focus/sequential-focus-navigation-and-the-tabindex-attribute/focus-tabindex-zero.html
rename : testing/web-platform/tests/html/editing/focus/tabindex-focus-flag.html => testing/web-platform/tests/html/interaction/focus/tabindex-focus-flag.html
jankeromnes pushed a commit to jankeromnes/gecko that referenced this issue Sep 12, 2018
@zcorpan @moz-wptsync-bot
Bug 1488179 [wpt PR 12801] - HTML: test that focus() on legend does n…
b030b96 
…ot delegate focus, a=testonly

Automatic update from web-platform-tests HTML: test that focus() on legend does not delegate focus

See whatwg/html#3950
--

wpt-commits: 910a8da21bb508cc774ae1acf9ead70f119ebb97
wpt-pr: 12801
@zcorpan
Copy link
Member Author

zcorpan commented Oct 26, 2018

https://www.chromestatus.com/metrics/feature/timeline/popularity/2535

@tkent-google usage seems pretty low?

@tkent-google
Copy link
Collaborator

usage seems pretty low?

Indeed. Chrome will remove this behavior.

aarongable pushed a commit to chromium/chromium that referenced this issue Oct 30, 2018
@amoseui
Forms: Remove delegation of focus on legend
2010bba 
legend.focus() should not delegate focus.

Bug: whatwg/html#3950
Bug: 880026
Change-Id: I35e046e45bbc45628f148383eaa77f12245e9e98
Reviewed-on: https://chromium-review.googlesource.com/c/1304180
Reviewed-by: Kent Tamura <tkent@chromium.org>
Commit-Queue: Amos Lim <eui-sang.lim@samsung.com>
Cr-Commit-Position: refs/heads/master@{#603783}
mustaqahmed pushed a commit to mustaqahmed/html that referenced this issue Feb 15, 2019
@zcorpan @mustaqahmed
Tweak how accesskey on legend works
15612e6 
Fixes whatwg#3950.

Tests: web-platform-tests/wpt#12800

* Don't special-case focusable legend, like Edge/Chrome/Safari

* Fix grammar

* Fix wording

* Use lists to make it less ambiguous

* and

* Address domenic's comments

* Add an example

* Source formatting nits
mustaqahmed pushed a commit to mustaqahmed/html that referenced this issue Feb 15, 2019
@zcorpan @mustaqahmed
Tweak how accesskey on legend works
f32fe75 
Fixes whatwg#3950.

Tests: web-platform-tests/wpt#12800

* Don't special-case focusable legend, like Edge/Chrome/Safari

* Fix grammar

* Fix wording

* Use lists to make it less ambiguous

* and

* Address domenic's comments

* Add an example

* Source formatting nits
robin-raymond pushed a commit to webrtc-uwp/chromium-tools that referenced this issue May 8, 2019
@tkent-google
Forms: Add a UseCounter for a LEGEND quirk behavior
48d6c40 
Bug: 878220
Bug: whatwg/html#3950
Change-Id: Ia0d53411b9dbeb15555ed66bf7539d3fd3fdd509
Reviewed-on: https://chromium-review.googlesource.com/1192367
Reviewed-by: Yoshifumi Inoue <yosin@chromium.org>
Commit-Queue: Kent Tamura <tkent@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#586606}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: 37d150002f3144ae2181316a11d4c76b92beaab5
SergioOpenPeer pushed a commit to webrtc-uwp/chromium-tools that referenced this issue May 10, 2019
@amoseui
Forms: Remove delegation of focus on legend
c7b7159 
legend.focus() should not delegate focus.

Bug: whatwg/html#3950
Bug: 880026
Change-Id: I35e046e45bbc45628f148383eaa77f12245e9e98
Reviewed-on: https://chromium-review.googlesource.com/c/1304180
Reviewed-by: Kent Tamura <tkent@chromium.org>
Commit-Queue: Amos Lim <eui-sang.lim@samsung.com>
Cr-Original-Commit-Position: refs/heads/master@{#603783}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: 2010bba41812ac1f3ea92a286c3894a14535766e
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 3, 2019
@marco-c
Bug 1488179 [wpt PR 12801] - HTML: test that focus() on legend does n…
b2d85ed 
…ot delegate focus, a=testonly

Automatic update from web-platform-tests HTML: test that focus() on legend does not delegate focus

See whatwg/html#3950
--

wpt-commits: 910a8da21bb508cc774ae1acf9ead70f119ebb97
wpt-pr: 12801

UltraBlame original commit: de87d57a04c11fff1aee7f2805ce0600086f1692
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 3, 2019
@marco-c
Bug 1488179 [wpt PR 12801] - HTML: test that focus() on legend does n…
8f8921c 
…ot delegate focus, a=testonly

Automatic update from web-platform-tests HTML: test that focus() on legend does not delegate focus

See whatwg/html#3950
--

wpt-commits: 910a8da21bb508cc774ae1acf9ead70f119ebb97
wpt-pr: 12801

UltraBlame original commit: de87d57a04c11fff1aee7f2805ce0600086f1692
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 3, 2019
@marco-c
Bug 1488179 [wpt PR 12801] - HTML: test that focus() on legend does n…
816c37e 
…ot delegate focus, a=testonly

Automatic update from web-platform-tests HTML: test that focus() on legend does not delegate focus

See whatwg/html#3950
--

wpt-commits: 910a8da21bb508cc774ae1acf9ead70f119ebb97
wpt-pr: 12801

UltraBlame original commit: de87d57a04c11fff1aee7f2805ce0600086f1692
@Ahmad-S792 Ahmad-S792 mentioned this issue Dec 18, 2022
Merged
webkit-commit-queue pushed a commit to Ahmad-S792/WebKit that referenced this issue Dec 19, 2022
@Ahmad-S792
legend.focus() should not delegate focus
05f693e 
legend.focus() should not delegate focus
https://bugs.webkit.org/show_bug.cgi?id=189242

Reviewed by Ryosuke Niwa.

This patch is to align Webkit with Blink / Chromium, Gecko / Firefox and Web-Specification.

Web-Spec Discussion Issue - whatwg/html#3950

Merge - https://chromium.googlesource.com/chromium/src.git/+/2010bba41812ac1f3ea92a286c3894a14535766e

This patch removes focus delegation and accesskey from the legend element to align with Web-Specification.

* Source/WebCore/html/HTMLLegendElement.h:
(1) Remove "HTMLFormControlElement" class
(2) Remove "associatedControl", "accessKeyActions" and "focus" function
* Source/WebCore/html/HTMLLegendElement.cpp:
(HTMLLegendElement::associatedControl): Deleted
(HTMLLegendElement::focus): Ditto
(HTMLLegendElement::accessKeyAction): Ditto
* LayoutTests/fast/form/legend-access-key.html: Deleted
* LayoutTests/fast/form/legend-access-key-expected.txt: Deleted
* LayoutTests/fast/form/focus.html: Updated
* LayoutTests/fast/form/focus-expected.txt: Ditto
* LayoutTests/fast/forms/focus-selection-textarea-expected.txt: Ditto
* LayoutTests/fast/forms/focus-selection-input.html: Ditto
* LayoutTests/fast/forms/focus-selection-input-expected.txt: Ditto
* LayoutTests/fast/form/access-key-expected.txt: Ditto
* LayoutTests/imported/w3c/web-platform-tests/html/interaction/focus/processing-model/legend-expected.html: Rebaselined
* LayoutTests/platform/ios/TestExpectations: Remove 'deleted' testcase expectation
* LayoutTests/platform/win/TestExpectations: Remove 'deleted' testcase expectation

Canonical link: https://commits.webkit.org/258074@main
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
interop Implementations are not interoperable with each other topic: fieldset topic: focus
Milestone
No milestone
Development

No branches or pull requests
6 participants
@zcorpan @rniwa @annevk @MatsPalmgren @mcking65 @tkent-google