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.
Created attachment 93075 [details] Patch V0
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 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.
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.
Created attachment 93088 [details] Tentative patch V1
(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.
Created attachment 93094 [details] Patch V2
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 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.
Created attachment 93224 [details] Patch V3
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 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
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
Created attachment 93378 [details] Retry commit
(In reply to comment #14) > Created an attachment (id=93378) [details] > Retry commit What's the difference from the previous one?
(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 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
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
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?
(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.
(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
Yael, James, thank you! It's reasonable.
Created attachment 93600 [details] Patch V4
Comment on attachment 93600 [details] Patch V4 ok
Comment on attachment 93600 [details] Patch V4 Clearing flags on attachment: 93600 Committed r86526: <http://trac.webkit.org/changeset/86526>
All reviewed patches have been landed. Closing bug.