| Summary: | label element with tabindex >= 0 is not focusable | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Masataka Yakura <myakura.web> | ||||||||||
| Component: | DOM | Assignee: | Ryosuke Niwa <rniwa> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | 7raivis, ap, cdumez, commit-queue, daryn_mitchell, giovanni.piller, jcraig, kandalkar.abhijeet58, mfairchild365, rene, rniwa, webkit-bug-importer | ||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Masataka Yakura
2012-11-20 00:45:46 PST
This is still the case on 537.36 (Chrome 39.0.2171.95): Labels with positive tabindex are ignored in tabbing, unless they are content-editable. See http://codepen.io/rh/pen/gbMRPJ for a simple test case. Related Blink issue: https://code.google.com/p/chromium/issues/detail?id=477208 Related Blink issue has been fixed: https://bugs.chromium.org/p/chromium/issues/detail?id=443725 Version 55.0.2883.95 (64-bit) Please disregard "Version 55.0.2883.95 (64-bit)". Pasted it by mistake. Blink's issue has been marked as "fixed" on Nov 11, 2015. Created attachment 297704 [details]
Fixes the bug
Attachment 297704 [details] did not pass style-queue:
ERROR: LayoutTests/platform/ios-simulator-wk2/TestExpectations:1789: Path does not exist. [test/expectations] [5]
Total errors found: 1 in 9 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 297705 [details]
Fixed the test path
Attachment 297705 [details] did not pass style-queue:
ERROR: LayoutTests/platform/ios-simulator-wk2/TestExpectations:1789: Path does not exist. [test/expectations] [5]
Total errors found: 1 in 9 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 297705 [details] Fixed the test path View in context: https://bugs.webkit.org/attachment.cgi?id=297705&action=review > Source/WebCore/html/HTMLLabelElement.cpp:150 > + // To match other browsers, always restore previous selection Should add a period too, not just a capital letter. This is a strange place for this comment. It seems to be grouped with the paragraph of code just below, but that’s not right. We should add a blank line so the comment is not grouped with the code below. > Source/WebCore/html/HTMLLabelElement.cpp:152 > + if (document().haveStylesheetsLoaded()) { > + document().updateLayoutIgnorePendingStylesheets(); Given that we are checking haveStylesheetsLoaded, I don’t think we need the “ignore pending stylesheets” version of the updateLayout function. > Source/WebCore/html/HTMLLabelElement.cpp:154 > + Element::focus(true, direction); Are you sure this should be "true" rather than passing along the argument we received? Do we have a test case that covers that choice to compare with other browsers’ behavior? > Source/WebCore/html/HTMLLegendElement.cpp:62 > // To match other browsers' behavior, never restore previous selection. This comment contradicts what the code below does, since it passes "true" when calling the base class, although it does pass "false" when not calling the base class. > Source/WebCore/html/HTMLLegendElement.cpp:66 > + Element::focus(true, direction); Are you sure this should be "true" rather than passing along the argument we received? Do we have a test case that covers that choice to compare with other browsers’ behavior? > Source/WebCore/html/HTMLLegendElement.cpp:70 > if (HTMLFormControlElement* control = associatedControl()) Nicer to use auto*. Comment on attachment 297705 [details] Fixed the test path View in context: https://bugs.webkit.org/attachment.cgi?id=297705&action=review Thanks for the review! >> Source/WebCore/html/HTMLLabelElement.cpp:150 >> + // To match other browsers, always restore previous selection > > Should add a period too, not just a capital letter. > > This is a strange place for this comment. It seems to be grouped with the paragraph of code just below, but that’s not right. We should add a blank line so the comment is not grouped with the code below. Fixed. >> Source/WebCore/html/HTMLLabelElement.cpp:154 >> + Element::focus(true, direction); > > Are you sure this should be "true" rather than passing along the argument we received? Do we have a test case that covers that choice to compare with other browsers’ behavior? This behavior was introduced 10 years ago in https://trac.webkit.org/changeset/20072 (with tests). Because restorePreviousSelection is only used by updateFocusAppearance overriden in HTMLAreaElement, HTMLTextAreaElement, and HTMLInputElement, this has no effect in for label & legend element (i.e. the value of this argument has not behavioral impact and thus not testable). I’ve added a comment about it, pass along the argument (it doesn’t matter anyway because the value is unused), and moved the above comment about restoring selection to the below where element->focus is called. >> Source/WebCore/html/HTMLLegendElement.cpp:66 >> + Element::focus(true, direction); > > Are you sure this should be "true" rather than passing along the argument we received? Do we have a test case that covers that choice to compare with other browsers’ behavior? Did the same thing as HTMLLabelElement::focus. >> Source/WebCore/html/HTMLLegendElement.cpp:70 >> if (HTMLFormControlElement* control = associatedControl()) > > Nicer to use auto*. Fixed. Created attachment 297980 [details]
Patch for landing
The commit-queue encountered the following flaky tests while processing attachment 297980 [details]:
The commit-queue is continuing to process your patch.
Created attachment 297984 [details]
Patch for landing
Oops, the test was failing on WK1. Fixed that. *** Bug 166020 has been marked as a duplicate of this bug. *** Comment on attachment 297984 [details] Patch for landing Clearing flags on attachment: 297984 Committed r210267: <http://trac.webkit.org/changeset/210267> All reviewed patches have been landed. Closing bug. |