Bug 181516

Summary:

Elements of zero width or height is not focusable

Product:

WebKit

Reporter:

Ryosuke Niwa <rniwa>

Component:

UI Events

Assignee:

Ryosuke Niwa <rniwa>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cdumez, dbates, hi, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Fixes the bug	cdumez: review+

Ryosuke Niwa

Reported 2018-01-10 20:20:44 PST

Right now, input elements, SVG anchor elements, and MathML elements are not focusable while their width or height is zero. This is an ancient behavior introduced in https://trac.webkit.org/changeset/5327 and https://trac.webkit.org/changeset/5598 but does not match other browsers' behaviors. Remove this restriction so that any element can be focused regardless of its CSS box size.

Attachments
Fixes the bug (8.90 KB, patch) 2018-01-11 13:12 PST, Ryosuke Niwa	cdumez: review+	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

Ryosuke Niwa

Comment 1 2018-01-11 13:12:26 PST

Created attachment 331102 [details] Fixes the bug

Chris Dumez

Comment 2 2018-01-11 14:30:44 PST

Comment on attachment 331102 [details] Fixes the bug r=me

Ryosuke Niwa

Comment 3 2018-01-11 16:57:01 PST

Committed r226823: <https://trac.webkit.org/changeset/226823>

Radar WebKit Bug Importer

Comment 4 2018-01-11 16:58:44 PST

<rdar://problem/36454514>

Daniel Bates

Comment 5 2018-01-12 10:29:13 PST

Comment on attachment 331102 [details] Fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=331102&action=review > LayoutTests/fast/events/focus-zero-size-element.html:6 > +<body> > +<style> > +input { padding: 0; margin: 0; border: none; } > +</style> This document is not well-formed per the HTML living standard. In particular, this section of the document is not well-formed. Although WebKit is lenient with regards to <style> as a child of <body>, for the purposes of this test <style> should only appear as a child of <head>. See "Contexts in which this element can be used" of section style element of the HTML living standard: <https://html.spec.whatwg.org/multipage/semantics.html#the-style-element> (12 January 2018).

Daniel Bates

Comment 6 2018-01-12 10:52:02 PST

Comment on attachment 331102 [details] Fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=331102&action=review > LayoutTests/fast/events/focus-zero-size-element.html:22 > +shouldBe('div = insertElement("div", {tabindex: 0}); div.focus(); document.activeElement', 'div'); I find this hard to read because we are abusing the evaluation mechanics of shouldBe() to perform more than once thing: create the test element, focus it, and check a result. Although each one of this actions is simple and it is unlikely that we will regress this functionality, the abuse of shouldBe() makes debugging a failure of this subtest more difficult because a person cannot use the Web Inspector to easily step through each expression when the first argument to shouldBe() is evaluated and will need to manually perform the steps. I suggest that we separate the test setup logic (creating the element and focusing it) from the logic to test correction (the call to shouldBe()). This has the benefit that is makes the test code straightforward to read and straightforward to debug. On another note, is it necessary to programmatically create the HTML div element? It may also help readability to declaratively create this element. > LayoutTests/fast/events/focus-zero-size-element.html:23 > +shouldBe('zeroWidthInput = insertElement("input", {style: "width: 0px"}); zeroWidthInput.focus(); document.activeElement', 'zeroWidthInput'); Ditto. > LayoutTests/fast/events/focus-zero-size-element.html:24 > +evalAndLog('div.innerHTML = "<svg><a tabindex=0 x=10 y=20><text x=10 y=20></text></a></svg>".replace(/</g, "<"); svgA = div.querySelector("a");'); Is it necessary to programmatically create this element? Is it necessary to programmatically create the SVG hierarchy using innerHTML? Programmatic creation, especially via an innerHTML for non-trivial markup, makes the test hard to read because we need to HTML entity encode characters among other things. Moreover since the test creates these elements in the same Using declarative markup, putting as much style information in <style>, and then focus()ing the elements defined by this markup would likely improve the readability of this test. Improving the readability of this test can help make it more straightforward to understand and debug this test should the functionality exercised by the test regress.

Daniel Bates

Comment 7 2018-01-12 11:03:54 PST

(In reply to Daniel Bates from comment #6) > Comment on attachment 331102 [details] > Fixes the bug > > View in context: > https://bugs.webkit.org/attachment.cgi?id=331102&action=review > > > LayoutTests/fast/events/focus-zero-size-element.html:22 > > +shouldBe('div = insertElement("div", {tabindex: 0}); div.focus(); document.activeElement', 'div'); > > I find this hard to read because we are abusing the evaluation mechanics of > shouldBe() to perform more than once thing: create the test element, focus > it, and check a result. Although each one of this actions is simple and it > is unlikely that we will regress this functionality, the abuse of shouldBe() > makes debugging a failure of this subtest more difficult because a person > cannot use the Web Inspector to easily step through each expression when the > first argument to shouldBe() is evaluated and will need to manually perform > the steps. I suggest that we separate the test setup logic (creating the > element and focusing it) from the logic to test correction (the call to > shouldBe()). This has the benefit that is makes the test code > straightforward to read and straightforward to debug. > This should read: I find this hard to read because we are abusing the evaluation mechanics of shouldBe() to perform more than one thing: create the test element, focus it, and check a result. Although each one of these actions is simple and is unlikely to regress (or will be noticeable from the failure of many other tests), the abuse of shouldBe() makes debugging a failure of this subtest more difficult because a person cannot use Web Inspector to easily step through each subexpression when the first argument to shouldBe() is evaluated (since shouldBe() calls eval() on its arguments). A person will need to extract the string-encoded subexpressions inside the first argument passed to shouldBe() into actual JavaScript code to debug a regression. I suggest that we do this separation in the test itself. That is, separate the test setup logic (creating the element and focusing it) from the logic to test correction (the call to shouldBe()). This has the benefit that is makes the test code straightforward to read and straightforward to debug > > LayoutTests/fast/events/focus-zero-size-element.html:24 > > +evalAndLog('div.innerHTML = "<svg><a tabindex=0 x=10 y=20><text x=10 y=20></text></a></svg>".replace(/</g, "<"); svgA = div.querySelector("a");'); > > Is it necessary to programmatically create this element? Is it necessary to > programmatically create the SVG hierarchy using innerHTML? ... > Moreover since the test creates these elements in the same This should read: Moreover, we need to perform a dance with quoting since we need to string-encode all this logic so that it can be eval()ed.

Ryosuke Niwa

Comment 8 2018-01-12 15:03:16 PST

I always prefer creating elements inside shouldBe because it would document everything in the expected result. If shouldBe only contained reference to an element, I'd have to open the test file to understand what kind of element it is.

Ryosuke Niwa

Comment 9 2018-02-19 21:37:21 PST

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

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