Bug 102780

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:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Fixes the bug	none
Fixed the test path	none
Patch for landing	none
Patch for landing	none

Masataka Yakura

Reported 2012-11-20 00:45:46 PST

Per the HTML spec, elements with tabindex value of zero (or greater) must be focusable. http://whatwg.org/html#sequential-focus-navigation-and-the-tabindex-attribute The label element, however, is not focusable. Minimal testcase: data:text/html;charset=utf-8,<!DOCTYPE html><label tabindex=0>label with tabindex=0 set</label>

Attachments
Fixes the bug (10.88 KB, patch) 2016-12-22 22:13 PST, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Fixed the test path (10.88 KB, patch) 2016-12-22 22:16 PST, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Patch for landing (17.23 KB, patch) 2017-01-03 18:28 PST, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Patch for landing (17.26 KB, patch) 2017-01-03 19:16 PST, Ryosuke Niwa	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Rene

Comment 1 2014-12-18 01:41:33 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.

Abhijeet Kandalkar

Comment 2 2015-04-14 23:20:04 PDT

Related Blink issue: https://code.google.com/p/chromium/issues/detail?id=477208

Giovanni Piller Cottrer

Comment 3 2016-12-19 01:00:53 PST

Related Blink issue has been fixed: https://bugs.chromium.org/p/chromium/issues/detail?id=443725 Version 55.0.2883.95 (64-bit)

Giovanni Piller Cottrer

Comment 4 2016-12-19 01:03:06 PST

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.

Radar WebKit Bug Importer

Comment 5 2016-12-22 20:04:47 PST

<rdar://problem/29796608>

Ryosuke Niwa

Comment 6 2016-12-22 22:13:16 PST

Created attachment 297704 [details] Fixes the bug

WebKit Commit Bot

Comment 7 2016-12-22 22:14:21 PST

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.

Ryosuke Niwa

Comment 8 2016-12-22 22:16:29 PST

Created attachment 297705 [details] Fixed the test path

WebKit Commit Bot

Comment 9 2016-12-22 22:19:05 PST

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.

Darin Adler

Comment 10 2016-12-25 09:58:43 PST

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

Ryosuke Niwa

Comment 11 2017-01-03 17:16:15 PST

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.

Ryosuke Niwa

Comment 12 2017-01-03 18:28:15 PST

Created attachment 297980 [details] Patch for landing

WebKit Commit Bot

Comment 13 2017-01-03 19:15:48 PST

The commit-queue encountered the following flaky tests while processing attachment 297980 [details]: The commit-queue is continuing to process your patch.

Ryosuke Niwa

Comment 14 2017-01-03 19:16:02 PST

Created attachment 297984 [details] Patch for landing

Ryosuke Niwa

Comment 15 2017-01-03 19:16:20 PST

Oops, the test was failing on WK1. Fixed that.

Ryosuke Niwa

Comment 16 2017-01-03 19:19:45 PST

*** Bug 166020 has been marked as a duplicate of this bug. ***

WebKit Commit Bot

Comment 17 2017-01-03 19:40:35 PST

Comment on attachment 297984 [details] Patch for landing Clearing flags on attachment: 297984 Committed r210267: <http://trac.webkit.org/changeset/210267>

WebKit Commit Bot

Comment 18 2017-01-03 19:40:41 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.