Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
484383002:
Disable the focus moves to an element which doesn't satisfy the spec. (Closed)
DescriptionDisable the focus moves to an element which doesn't satisfy the spec.
The current behavior, if there are some elements that are set ‘ tabindex="negative integer" ', and then, when one of them is focused on, the next focus moves to the others.
however, it does not match the spec.
This patch disallow them.
The spec:
http://www.w3.org/TR/html5/editing.html#sequential-focus-navigation-and-the-tabindex-attribute
BUG=395761
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182040
Patch Set 1 #Patch Set 2 : add test #
Total comments: 2
Patch Set 3 : Ver.2 #
Total comments: 3
Patch Set 4 : Ver.2+ #Patch Set 5 : Ver.2 Patch Test. #Patch Set 6 : Test for Ver.2 Patch #
Total comments: 4
Patch Set 7 : Two Tests for Ver.2 Patch. #
Total comments: 2
Patch Set 8 : use document.activeElement #
Total comments: 6
Patch Set 9 : no png #Patch Set 10 : remove png #Patch Set 11 : Fix Test #Patch Set 12 : Fix Test #
Total comments: 12
Patch Set 13 : Use shouldBe and so on #
Total comments: 14
Patch Set 14 : Fix indent and add space after ', #
Messages
Total messages: 37 (5 generated)
Hello. Could you please review this my patch.
Thank you for the first contribution! You might want to update the description of this patch. It seems duplicated. I'd recommend you to do `git cl try`. See http://dev.chromium.org/developers/testing/try-server-usage That will tell you whether your patch will break something or not. Basically, you shouldn't commit a patch unless you can verify your patch break nothing. Though, I'm afraid that you don't have a permission to use try server. Could you use yanagawa@chromium.org account instead of yanagawa@google.com? I guess you can use Try Server with yanagawa@chromium.org account. You've received the email which explains how to set up your @chromium.org account See http://www.chromium.org/developers/testing/webkit-layout-tests for more info about Layout tests. You can also run LayoutTests on your local machine. One more thing. You should add a layout test somehow for this patch. Otherwise, you should explain why we can't write a layout test for this. Keep going! ＼(^o^)／ Let's iterate this review until your patch is ready to commit.
We added test code for this patch.
Thank you for adding the test. Looks your patch includes #tabindex-focus-chain-expected.txt#, which might be a autosave file by Emacs. You should remove that from the patch. https://codereview.chromium.org/484383002/diff/20001/Source/core/page/FocusCo... File Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/484383002/diff/20001/Source/core/page/FocusCo... Source/core/page/FocusController.cpp:582: return 0; I am afraid we couldn't return 0 here. Suppose we are given the following HTML: <input id=A tabindex="1" type="text" /> <input id=B tabindex="-1" type="text" /> <input id=C tabindex="-1" type="text" /> <input id=D tabindex="-1" type="text" /> If users press 'Tab' key when the element 'B' is currently focused, I think the element 'A' should be focused in this case. If we return 0 here, the element A wouldn't get focused. https://codereview.chromium.org/484383002/diff/20001/Source/core/page/FocusCo... Source/core/page/FocusController.cpp:630: return 0; Ditto
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
I fixed error. Could you please review this.
Thank you for fixing that. The fix looks promising. I think you might want to update description of this patch so that it reflects your latest patch. The current description looks a little confusing. For example, it is unclear what 'the value' means. https://codereview.chromium.org/484383002/diff/100001/LayoutTests/fast/events... File LayoutTests/fast/events/tabindex-no-focusable.html (right): https://codereview.chromium.org/484383002/diff/100001/LayoutTests/fast/events... LayoutTests/fast/events/tabindex-no-focusable.html:1: <script> Could you update and/or add a test too? At least, you should add a test which covers the case I pointed out in comment #4. https://codereview.chromium.org/484383002/diff/100001/Source/core/page/FocusC... File Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/484383002/diff/100001/Source/core/page/FocusC... Source/core/page/FocusController.cpp:585: if (tabIndex >= 0) { You can use `else` here rather than repeating `if`. if (tabIndex < 0) { ... } else { ... } would be better than if (tabIndex < 0) { ... } if (tabIndex >= 0) { ... } https://codereview.chromium.org/484383002/diff/100001/Source/core/page/FocusC... Source/core/page/FocusController.cpp:632: if (startingTabIndex >= 0) { Ditto.
I fixed ver.2 patch's Test. Could you see it.
Thank you for updating the test. It seems you've added a test for the scenario which I pointed out in comment #4. However, you've removed a test scenario which your patch set #2 tested, haven't you? I think you should test *both* cases. The layout test contains some style issues. See style guide: https://sites.google.com/a/chromium.org/dev/blink/coding-style/layout-test-st... Could you polish the description of this patch? The current description needs to be improved. For example, `the value` is ambiguous. You might want to rewrite the description so that it makes sense. Please see the description of other patches in chromium for a better reference. https://codereview.chromium.org/484383002/diff/160001/LayoutTests/fast/events... File LayoutTests/fast/events/tabindex-no-focusable.html (right): https://codereview.chromium.org/484383002/diff/160001/LayoutTests/fast/events... LayoutTests/fast/events/tabindex-no-focusable.html:16: var current_focus=(document.activeElement || window.getSelection().focusNode); document.activeElement isn't enough? Is there a strong reason to use "document.activeElement || window.getSelection().focusNode"? https://codereview.chromium.org/484383002/diff/160001/LayoutTests/fast/events... LayoutTests/fast/events/tabindex-no-focusable.html:18: if (!key && elem_movetome==current_focus) { // first test passed, continue with second test You need spaces around oprator, such as '=='. https://codereview.chromium.org/484383002/diff/160001/LayoutTests/fast/events... LayoutTests/fast/events/tabindex-no-focusable.html:18: if (!key && elem_movetome==current_focus) { // first test passed, continue with second test You don't need `key` variable anymore because you are testing activeElement explicitly here. https://codereview.chromium.org/484383002/diff/160001/LayoutTests/fast/events... LayoutTests/fast/events/tabindex-no-focusable.html:24: if (!key && elem_movetome==current_focus) { // second test passed Ditto.
Patchset #7 (id:180001) has been deleted
Thanks for your advice. I fixed my Test.
Thank you for updating the test. Please update the description of this patch also. You can update the description of this patch by clicking 'Edit Issue' on this review page. https://codereview.chromium.org/484383002/diff/200001/LayoutTests/fast/events... File LayoutTests/fast/events/tabindex-no-focusable.html (right): https://codereview.chromium.org/484383002/diff/200001/LayoutTests/fast/events... LayoutTests/fast/events/tabindex-no-focusable.html:19: if (!key) { // second test passed I don't think it is good to use the value of the `key`. It looks a very fragile test. Why don't you use document.activeElement or something similar to check the focused element explicitly? You don't have to follow the bad part of the existing test.
Sorry, I overlooked your advice. I use the function.
Patchset #8 (id:220001) has been deleted
I upload this patch again. Could you look this. https://codereview.chromium.org/484383002/diff/200001/LayoutTests/fast/events... File LayoutTests/fast/events/tabindex-no-focusable.html (right): https://codereview.chromium.org/484383002/diff/200001/LayoutTests/fast/events... LayoutTests/fast/events/tabindex-no-focusable.html:19: if (!key) { // second test passed On 2014/09/05 07:08:11, hayato wrote: > I don't think it is good to use the value of the `key`. It looks a very fragile > test. > Why don't you use document.activeElement or something similar to check the > focused element explicitly? > > You don't have to follow the bad part of the existing test. Done.
Thank you for uploading the test. As general tips, I recommend you to run your test *without* your fix, in addition to *with* your fix. I think the test you added should *fail* without your patch. If the test passes without your fix, something is wrong with your test. https://codereview.chromium.org/484383002/diff/240001/LayoutTests/fast/events... File LayoutTests/fast/events/tabindex-no-focusable-all-negative.html (right): https://codereview.chromium.org/484383002/diff/240001/LayoutTests/fast/events... LayoutTests/fast/events/tabindex-no-focusable-all-negative.html:5: var key = 0; Ditto. You don't have to use `key`. https://codereview.chromium.org/484383002/diff/240001/LayoutTests/fast/events... File LayoutTests/fast/events/tabindex-no-focusable.html (right): https://codereview.chromium.org/484383002/diff/240001/LayoutTests/fast/events... LayoutTests/fast/events/tabindex-no-focusable.html:15: if (current_focus = elem_movetome) { // first test passed, continue with second test This should be '==' instead of '=', shouldn't this? https://codereview.chromium.org/484383002/diff/240001/LayoutTests/fast/events... LayoutTests/fast/events/tabindex-no-focusable.html:19: if (current_focus = elem_movetome) { // second test passed Because the focused element has changed by sending [shift+tab], current_focus is no longer equal to document.activeElement. You should use document.activeElement again. You might want to use console.log(current_focus) to see what is assigned to current_focus.
Could you remove LayoutTests/platform/linux/fast/events/tabindex-no-focusable-expected.png? I guess it's accidentally added.
On 2014/09/10 06:00:11, hayato wrote: > Could you remove > LayoutTests/platform/linux/fast/events/tabindex-no-focusable-expected.png? > I guess it's accidentally added. Done.
I remove abindex-no-focusable-expected.png
On 2014/09/10 06:57:50, yanagawa wrote: > I remove abindex-no-focusable-expected.png Thank you. I guess you missed my comment #18. :) Could you address that?
I think I fixed all problem. Could you look it? https://codereview.chromium.org/484383002/diff/240001/LayoutTests/fast/events... File LayoutTests/fast/events/tabindex-no-focusable-all-negative.html (right): https://codereview.chromium.org/484383002/diff/240001/LayoutTests/fast/events... LayoutTests/fast/events/tabindex-no-focusable-all-negative.html:5: var key = 0; On 2014/09/10 05:56:40, hayato wrote: > Ditto. You don't have to use `key`. Done. https://codereview.chromium.org/484383002/diff/240001/LayoutTests/fast/events... File LayoutTests/fast/events/tabindex-no-focusable.html (right): https://codereview.chromium.org/484383002/diff/240001/LayoutTests/fast/events... LayoutTests/fast/events/tabindex-no-focusable.html:15: if (current_focus = elem_movetome) { // first test passed, continue with second test On 2014/09/10 05:56:40, hayato wrote: > This should be '==' instead of '=', shouldn't this? Done. https://codereview.chromium.org/484383002/diff/240001/LayoutTests/fast/events... LayoutTests/fast/events/tabindex-no-focusable.html:19: if (current_focus = elem_movetome) { // second test passed On 2014/09/10 05:56:40, hayato wrote: > Because the focused element has changed by sending [shift+tab], current_focus is > no longer equal to document.activeElement. You should use document.activeElement > again. > > You might want to use console.log(current_focus) to see what is assigned to > current_focus. Done.
Thanks. I think this patch is very close to be ready to land. https://codereview.chromium.org/484383002/diff/320001/LayoutTests/fast/events... File LayoutTests/fast/events/tabindex-no-focusable-all-negative.html (right): https://codereview.chromium.org/484383002/diff/320001/LayoutTests/fast/events... LayoutTests/fast/events/tabindex-no-focusable-all-negative.html:13: var current_focus = document.activeElement; You might want to get rid of current_focus variable and use document.activeElement directly, replacing the usage of current_focus. https://codereview.chromium.org/484383002/diff/320001/LayoutTests/fast/events... LayoutTests/fast/events/tabindex-no-focusable-all-negative.html:15: if (current_focus == elem_focusme) { // first test passed, continue with second test This comment doesn't give us much value. You can remove this comment. https://codereview.chromium.org/484383002/diff/320001/LayoutTests/fast/events... LayoutTests/fast/events/tabindex-no-focusable-all-negative.html:20: if (current_focus == elem_focusme) { // second test passed Ditto. https://codereview.chromium.org/484383002/diff/320001/LayoutTests/fast/events... LayoutTests/fast/events/tabindex-no-focusable-all-negative.html:21: document.write("PASSED"); I am not fan of using document,write() in testing. You might want to use `shouldBe(xxx, yyy)` or something similar provided by js-test.js, instead of using document.write("PASS"). That would increase the readability of this test. You can eliminate this nested `if` statements. https://codereview.chromium.org/484383002/diff/320001/LayoutTests/fast/events... LayoutTests/fast/events/tabindex-no-focusable-all-negative.html:34: <div id="results"></div> It seems this #result div is not used. https://codereview.chromium.org/484383002/diff/320001/LayoutTests/fast/events... File LayoutTests/fast/events/tabindex-no-focusable.html (right): https://codereview.chromium.org/484383002/diff/320001/LayoutTests/fast/events... LayoutTests/fast/events/tabindex-no-focusable.html:1: <script> Ditto. You might update this test also as I commented for the other test.
I done all. I didn't use shuldBe,But use shouldBeEqualToString. It is okay? https://codereview.chromium.org/484383002/diff/320001/LayoutTests/fast/events... File LayoutTests/fast/events/tabindex-no-focusable-all-negative.html (right): https://codereview.chromium.org/484383002/diff/320001/LayoutTests/fast/events... LayoutTests/fast/events/tabindex-no-focusable-all-negative.html:13: var current_focus = document.activeElement; On 2014/09/12 04:37:12, hayato wrote: > You might want to get rid of current_focus variable and use > document.activeElement directly, replacing the usage of current_focus. Done. https://codereview.chromium.org/484383002/diff/320001/LayoutTests/fast/events... LayoutTests/fast/events/tabindex-no-focusable-all-negative.html:15: if (current_focus == elem_focusme) { // first test passed, continue with second test On 2014/09/12 04:37:12, hayato wrote: > This comment doesn't give us much value. You can remove this comment. Done. https://codereview.chromium.org/484383002/diff/320001/LayoutTests/fast/events... LayoutTests/fast/events/tabindex-no-focusable-all-negative.html:20: if (current_focus == elem_focusme) { // second test passed On 2014/09/12 04:37:12, hayato wrote: > Ditto. Done. https://codereview.chromium.org/484383002/diff/320001/LayoutTests/fast/events... LayoutTests/fast/events/tabindex-no-focusable-all-negative.html:21: document.write("PASSED"); On 2014/09/12 04:37:12, hayato wrote: > I am not fan of using document,write() in testing. > > You might want to use `shouldBe(xxx, yyy)` or something similar provided by > js-test.js, instead of using document.write("PASS"). That would increase the > readability of this test. You can eliminate this nested `if` statements. Done. https://codereview.chromium.org/484383002/diff/320001/LayoutTests/fast/events... LayoutTests/fast/events/tabindex-no-focusable-all-negative.html:34: <div id="results"></div> On 2014/09/12 04:37:12, hayato wrote: > It seems this #result div is not used. Done. https://codereview.chromium.org/484383002/diff/320001/LayoutTests/fast/events... File LayoutTests/fast/events/tabindex-no-focusable.html (right): https://codereview.chromium.org/484383002/diff/320001/LayoutTests/fast/events... LayoutTests/fast/events/tabindex-no-focusable.html:1: <script> On 2014/09/12 04:37:12, hayato wrote: > Ditto. You might update this test also as I commented for the other test. Done.
> I done all. > I didn't use shuldBe,But use shouldBeEqualToString. > It is okay? Yes. `shouldBeEqualToString` looks better to me. You have to update the description of this CL so that everyone can understand the purpose of this CL easily. You can edit the description by clicking 'Edit Issue' link in this review page. > "Before this patch, if the value is a negative integer, ..." I am afraid that you should improve the description so that the meaning becomes more clear to every one. Could you try to rewrite the description? https://codereview.chromium.org/484383002/diff/340001/LayoutTests/fast/events... File LayoutTests/fast/events/tabindex-no-focusable-all-negative.html (right): https://codereview.chromium.org/484383002/diff/340001/LayoutTests/fast/events... LayoutTests/fast/events/tabindex-no-focusable-all-negative.html:14: shouldBeEqualToString('document.activeElement.id','focusMe'); Ditto https://codereview.chromium.org/484383002/diff/340001/LayoutTests/fast/events... LayoutTests/fast/events/tabindex-no-focusable-all-negative.html:16: eventSender.keyDown("\t",["shiftKey"]); Ditto https://codereview.chromium.org/484383002/diff/340001/LayoutTests/fast/events... LayoutTests/fast/events/tabindex-no-focusable-all-negative.html:17: shouldBeEqualToString('document.activeElement.id','focusMe'); Ditto https://codereview.chromium.org/484383002/diff/340001/LayoutTests/fast/events... File LayoutTests/fast/events/tabindex-no-focusable.html (right): https://codereview.chromium.org/484383002/diff/340001/LayoutTests/fast/events... LayoutTests/fast/events/tabindex-no-focusable.html:15: shouldBeEqualToString('document.activeElement.id','MoveToMe'); You need a space after `,`: shouldBeEqualToString('document.activeElement.id', 'MoveToMe'); https://codereview.chromium.org/484383002/diff/340001/LayoutTests/fast/events... LayoutTests/fast/events/tabindex-no-focusable.html:17: eventSender.keyDown("\t",["shiftKey"]); Ditto https://codereview.chromium.org/484383002/diff/340001/LayoutTests/fast/events... LayoutTests/fast/events/tabindex-no-focusable.html:18: shouldBeEqualToString('document.activeElement.id','MoveToMe'); Ditto https://codereview.chromium.org/484383002/diff/340001/Source/core/page/FocusC... File Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/484383002/diff/340001/Source/core/page/FocusC... Source/core/page/FocusController.cpp:583: // First try to find a node with the same tabindex as start that comes after start in the scope. Please fix the indent.
I fixed it. Please confirm. https://codereview.chromium.org/484383002/diff/340001/LayoutTests/fast/events... File LayoutTests/fast/events/tabindex-no-focusable-all-negative.html (right): https://codereview.chromium.org/484383002/diff/340001/LayoutTests/fast/events... LayoutTests/fast/events/tabindex-no-focusable-all-negative.html:14: shouldBeEqualToString('document.activeElement.id','focusMe'); On 2014/09/12 06:24:13, hayato wrote: > Ditto Done. https://codereview.chromium.org/484383002/diff/340001/LayoutTests/fast/events... LayoutTests/fast/events/tabindex-no-focusable-all-negative.html:16: eventSender.keyDown("\t",["shiftKey"]); On 2014/09/12 06:24:13, hayato wrote: > Ditto Done. https://codereview.chromium.org/484383002/diff/340001/LayoutTests/fast/events... LayoutTests/fast/events/tabindex-no-focusable-all-negative.html:17: shouldBeEqualToString('document.activeElement.id','focusMe'); On 2014/09/12 06:24:13, hayato wrote: > Ditto Done. https://codereview.chromium.org/484383002/diff/340001/LayoutTests/fast/events... File LayoutTests/fast/events/tabindex-no-focusable.html (right): https://codereview.chromium.org/484383002/diff/340001/LayoutTests/fast/events... LayoutTests/fast/events/tabindex-no-focusable.html:15: shouldBeEqualToString('document.activeElement.id','MoveToMe'); On 2014/09/12 06:24:13, hayato wrote: > You need a space after `,`: > > shouldBeEqualToString('document.activeElement.id', 'MoveToMe'); Done. https://codereview.chromium.org/484383002/diff/340001/LayoutTests/fast/events... LayoutTests/fast/events/tabindex-no-focusable.html:17: eventSender.keyDown("\t",["shiftKey"]); On 2014/09/12 06:24:13, hayato wrote: > Ditto Done. https://codereview.chromium.org/484383002/diff/340001/LayoutTests/fast/events... LayoutTests/fast/events/tabindex-no-focusable.html:18: shouldBeEqualToString('document.activeElement.id','MoveToMe'); On 2014/09/12 06:24:13, hayato wrote: > Ditto Done. https://codereview.chromium.org/484383002/diff/340001/Source/core/page/FocusC... File Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/484383002/diff/340001/Source/core/page/FocusC... Source/core/page/FocusController.cpp:583: // First try to find a node with the same tabindex as start that comes after start in the scope. On 2014/09/12 06:24:13, hayato wrote: > Please fix the indent. Done.
Thank you. You are close to goal. I recommend you to follow the change list description structured elements. See http://www.chromium.org/developers/contributing-code#TOC-Change-List-Descript... As explained, the first line should be the summary of the change. Usually the summary starts with the (past tense) verb. You can see a lot of examples by 'git log'. Please update the subject of this review also. It is not necessary, but it is recommended. The subject is usually equals to the first line of the description, the summary of the change.
I rewrite description. Is this okay?
The description still needs to be improved. Please remember that the description of the patch is very important for us. > In this patch,When the all element's tabindexs are negative value, any element are not focus. I recommend you to rewrite this sentence so that it starts with (past tense) verb as I explained in comment #28 . The first line of the commit message usually starts with verb. You might want to fix some grammatical errors too. > In present program, the focus moves from the currently-focused input to the next input, if the all element's tabindex value are negative integer. I am afraid that this might lead to misunderstanding because this patch is not specific to <input> elements. If your patch fixed the wrong behavior which is not only specific to <input> elements, but also other focusable elements, you should explain it clearly.
Yurina helps me. I think it got better dive than my statement. This starts with verb. This is not specific to <input> elements.
LGTM Congratulations and thank you for your first patch. > Disable the focus moves to an element which is not satisfy the spec. IMHO, this should be: Disable the focus moves to an element which doesn't satisfy the spec. :)
The CQ bit was checked by yanagawa@google.com
The CQ bit was unchecked by yanagawa@google.com
The CQ bit was checked by yanagawa@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/484383002/360001
Message was sent while issue was closed.
Committed patchset #14 (id:360001) as 182040 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
