Bug 60602

Summary: <output>, <meter> and <progress> elements with display:block can be focused if you try to tab to it
Product: WebKit Reporter: Kenichi Ishibashi <bashi>
Component: UI EventsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: cfleizach, commit-queue, hayato, jcraig, morrita, tkent, yael
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
URL: http://crbug.com/81107
Attachments:
Description Flags
Patch V0
none
Tentative patch V1
none
Patch V2
none
Patch V3
none
Archive of layout-test-results from eseidel-cq-sf
none
Retry commit
none
Archive of layout-test-results from eseidel-cq-sf
none
Patch V4 none

Description Kenichi Ishibashi 2011-05-10 20:02:21 PDT
Originally reported at http://crbug.com/81107.

Testcase: http://software.hixie.ch/utilities/js/live-dom-viewer/saved/970

An <output> element should not be focused.  <progress> and <meter> elements can also be focused with display:block for now, but I think they should not be focused as well.
Comment 1 Kenichi Ishibashi 2011-05-10 22:50:03 PDT
Created attachment 93075 [details]
Patch V0
Comment 2 Kent Tamura 2011-05-10 23:03:10 PDT
Comment on attachment 93075 [details]
Patch V0

View in context: https://bugs.webkit.org/attachment.cgi?id=93075&action=review

> LayoutTests/ChangeLog:5
> +        An <output> element with display:block can be focused if you try to tab to it

The summary doesn't match the code change.  We should mention <meter> and <progress>.

> LayoutTests/fast/forms/focus-with-display-block-expected.txt:9
> + Output
> +
> +INPUT has focus.
> +INPUT has focus.
> +INPUT has focus.
> +INPUT has focus.
> +PASS

The test result readability is not good.
 - "Output" makes no sense.
 - The test result doesn't show what is succeeded.  We had better have lines such as "METER should not have focus."

> LayoutTests/fast/forms/focus-with-display-block.html:52
> +function test() {
> +    if (window.layoutTestController)
> +        layoutTestController.dumpAsText();
> +
> +    var results = document.getElementById('results');
> +    if (doTest())
> +        results.innerHTML += 'PASS';
> +    else
> +        results.innerHTML += 'FAIL';
> +}

We had better use functions in fast/js/resources/js-test-pre.js.
Comment 3 Ryosuke Niwa 2011-05-10 23:07:35 PDT
Comment on attachment 93075 [details]
Patch V0

View in context: https://bugs.webkit.org/attachment.cgi?id=93075&action=review

> Source/WebCore/ChangeLog:8
> +        Adds overrides of isFocusable() to some form associated elements to return false.

This is wordy.  Just say "override isFocusable() to return false in meter, output, and progress elements."

You should also explain what caused the bug and refer to wherever place this behavior is specified.
Comment 4 Kenichi Ishibashi 2011-05-11 01:18:29 PDT
Hi,

Thank you for review.

(In reply to comment #3)
> You should also explain what caused the bug and refer to wherever place this behavior is specified.

I've addressed your comments except for pointing out the specification of this behavior.  In this patch, I follow the manner what the HTMLLabelElement does.  However, according to http://dev.w3.org/html5/spec/editing.html#focus, it seems that any elements might be focusable when the element has the tabindex attribute or content editable.  Should we change the behavior to follow the specification?  If so, it may affect almost all elements because it should be implemented in more generic place such as the Node class.
Comment 5 Kenichi Ishibashi 2011-05-11 01:27:32 PDT
Created attachment 93088 [details]
Tentative patch V1
Comment 6 Kent Tamura 2011-05-11 02:01:20 PDT
(In reply to comment #4)
> it seems that any elements might be focusable when the element has the tabindex attribute or content editable.  Should we change the behavior to follow the specification?  If so, it may affect almost all elements because it should be implemented in more generic place such as the Node class.

It's already implemented in Node::supportsFocus().
Output, progress, and meter should override supportsFocus() like
    return Node:supportsFocus() && !disabled().

<label> is special because it delegates focus operation to its target node.
Comment 7 Kenichi Ishibashi 2011-05-11 02:46:26 PDT
Created attachment 93094 [details]
Patch V2
Comment 8 Kenichi Ishibashi 2011-05-11 02:48:48 PDT
Hi Kent-san,

(In reply to comment #6)
> It's already implemented in Node::supportsFocus().
> Output, progress, and meter should override supportsFocus() like
>     return Node:supportsFocus() && !disabled().

Oh, I didn't notice that. Thank you for letting know that. I've updated the patch.

Regards,
Comment 9 Kent Tamura 2011-05-11 16:58:53 PDT
Comment on attachment 93094 [details]
Patch V2

View in context: https://bugs.webkit.org/attachment.cgi?id=93094&action=review

ok.

BTW, I don't know why we have Node::isFocusable() and Node::supprotsFocus()...

> LayoutTests/fast/forms/script-tests/focus-with-display-block.js:1
> +description('This test ensures that &lt;output&gt;, &lt;meter&gt; and &lt;progress&gt; are not focused.');

nit: We don't need to have separated js file. We can merge this into focus-with-display-block.html.
Comment 10 Kenichi Ishibashi 2011-05-11 17:44:41 PDT
Created attachment 93224 [details]
Patch V3
Comment 11 Kenichi Ishibashi 2011-05-11 17:47:19 PDT
Comment on attachment 93224 [details]
Patch V3

Hi Kent-san,

Thank you for review.  I've merged the test into one html file.

Regards,
Comment 12 WebKit Commit Bot 2011-05-12 00:02:43 PDT
Comment on attachment 93224 [details]
Patch V3

Rejecting attachment 93224 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sf', 'bu..." exit_code: 2

Last 500 characters of output:
tests/xmlhttprequest ................................................................................................................................................................................
http/tests/xmlhttprequest/web-apps ...............
http/tests/xmlhttprequest/workers ...........
http/tests/xmlviewer .
http/tests/xmlviewer/dumpAsText ...........
764.12s total testing time

23503 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
17 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/8688615
Comment 13 WebKit Commit Bot 2011-05-12 00:02:45 PDT
Created attachment 93259 [details]
Archive of layout-test-results from eseidel-cq-sf

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: eseidel-cq-sf  Port: Mac  Platform: Mac OS X 10.6.7
Comment 14 Kenichi Ishibashi 2011-05-12 18:17:41 PDT
Created attachment 93378 [details]
Retry commit
Comment 15 Kent Tamura 2011-05-12 18:23:57 PDT
(In reply to comment #14)
> Created an attachment (id=93378) [details]
> Retry commit

What's the difference from the previous one?
Comment 16 Kenichi Ishibashi 2011-05-12 19:08:46 PDT
(In reply to comment #15)
> What's the difference from the previous one?

There is no difference.  Just rebased.  I'm not sure the cause of the error, but I ran layout tests on mac and linux and I didn't face with any errors on my environment.
Comment 17 WebKit Commit Bot 2011-05-12 21:44:01 PDT
Comment on attachment 93378 [details]
Retry commit

Rejecting attachment 93378 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sf', 'bu..." exit_code: 2

Last 500 characters of output:
tests/xmlhttprequest ................................................................................................................................................................................
http/tests/xmlhttprequest/web-apps ...............
http/tests/xmlhttprequest/workers ...........
http/tests/xmlviewer .
http/tests/xmlviewer/dumpAsText ...........
764.77s total testing time

23518 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
17 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/8694010
Comment 18 WebKit Commit Bot 2011-05-12 21:44:04 PDT
Created attachment 93398 [details]
Archive of layout-test-results from eseidel-cq-sf

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: eseidel-cq-sf  Port: Mac  Platform: Mac OS X 10.6.7
Comment 19 Kent Tamura 2011-05-12 22:05:16 PDT
Chris, Yael,
We have a question about LayoutTests/platform/mac/accessicbility/progressbar.html.
The patch in this bug will make <progress> not focusable by default, and progressbar.html assumes <progress> is focusable.  Should we make the progress elements in the test focusable by tabindex, or shouldn't we make <progress> not focusable by default?
Comment 20 Yael 2011-05-13 05:56:51 PDT
(In reply to comment #19)
> Chris, Yael,
> We have a question about LayoutTests/platform/mac/accessicbility/progressbar.html.
> The patch in this bug will make <progress> not focusable by default, and progressbar.html assumes <progress> is focusable.  Should we make the progress elements in the test focusable by tabindex, or shouldn't we make <progress> not focusable by default?

My understanding of http://dev.w3.org/html5/spec/editing.html#focus is also that <progress> should be focused only if it has the tabindex attribute. I would suggest to add that attribute to the test progressbar.html.
Comment 21 James Craig 2011-05-13 14:47:22 PDT
(In reply to comment #20)
> (In reply to comment #19)
> > Chris, Yael,
> > We have a question about LayoutTests/platform/mac/accessicbility/progressbar.html.
> > The patch in this bug will make <progress> not focusable by default, and progressbar.html assumes <progress> is focusable.  Should we make the progress elements in the test focusable by tabindex, or shouldn't we make <progress> not focusable by default?
> 
> My understanding of http://dev.w3.org/html5/spec/editing.html#focus is also that <progress> should be focused only if it has the tabindex attribute. I would suggest to add that attribute to the test progressbar.html.

(Chris asked me to respond to this one.)

I agree with your assessment. The progress element should only be user-focusable if tabindex >= 0 is specified, or script-focusable (el.focus()) if tabindex >= -1 is specified.

James
Comment 22 Kent Tamura 2011-05-15 17:30:43 PDT
Yael, James, thank you!
It's reasonable.
Comment 23 Kenichi Ishibashi 2011-05-15 20:19:54 PDT
Created attachment 93600 [details]
Patch V4
Comment 24 Kent Tamura 2011-05-15 20:23:56 PDT
Comment on attachment 93600 [details]
Patch V4

ok
Comment 25 WebKit Commit Bot 2011-05-15 22:59:59 PDT
Comment on attachment 93600 [details]
Patch V4

Clearing flags on attachment: 93600

Committed r86526: <http://trac.webkit.org/changeset/86526>
Comment 26 WebKit Commit Bot 2011-05-15 23:00:07 PDT
All reviewed patches have been landed.  Closing bug.