Bug 60602 – <output>, <meter> and <progress> elements with display:block can be focused if you try to tab to it

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Kenichi Ishibashi]

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.

[Kenichi Ishibashi]

Created attachment 93075 [details]
Patch V0

[Kent Tamura]

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.

[Ryosuke Niwa]

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.

[Kenichi Ishibashi]

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.

[Kenichi Ishibashi]

Created attachment 93088 [details]
Tentative patch V1

[Kent Tamura]

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

[Kenichi Ishibashi]

Created attachment 93094 [details]
Patch V2

[Kenichi Ishibashi]

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,

[Kent Tamura]

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 <output>, <meter> and <progress> are not focused.');

nit: We don't need to have separated js file. We can merge this into focus-with-display-block.html.

[Kenichi Ishibashi]

Created attachment 93224 [details]
Patch V3

[Kenichi Ishibashi]

Comment on attachment 93224 [details]
Patch V3

Hi Kent-san,

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

Regards,

[WebKit Commit Bot]

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

[WebKit Commit Bot]

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

[Kenichi Ishibashi]

Created attachment 93378 [details]
Retry commit

[Kent Tamura]

(In reply to comment #14)
> Created an attachment (id=93378) [details]
> Retry commit

What's the difference from the previous one?

[Kenichi Ishibashi]

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

[WebKit Commit Bot]

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

[WebKit Commit Bot]

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

[Kent Tamura]

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?

[Yael]

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

[James Craig]

(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

[Kent Tamura]

Yael, James, thank you!
It's reasonable.

[Kenichi Ishibashi]

Created attachment 93600 [details]
Patch V4

[Kent Tamura]

Comment on attachment 93600 [details]
Patch V4

ok

[WebKit Commit Bot]

Comment on attachment 93600 [details]
Patch V4

Clearing flags on attachment: 93600

Committed r86526: <http://trac.webkit.org/changeset/86526>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.