Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
546753002:
Fix to Handle downkey event on last and first radio button of a radio button list (Closed)
|
Created:
6 years, 3 months ago by Paritosh Kumar Modified:
6 years, 2 months ago CC:
tkent Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionFix to Handle downkey event on last and first radio button of a radio button list
After pressing last radio button in the list, on pressing downArrow it will select back first radio button, similarly after pressing last radio button in the list, on pressing upArrow it will select last radio button.
For this change I have added one test case : radio-checked-LastorFirst-then-DownorUp-keyDown.html. This HTML contains a list of radio buttons and after selecting last radio button downArrow keyDown event raised. Similarly done for first radio button also.
R=habib.virji@samsung.com
BUG=408512
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183661
Patch Set 1 #Patch Set 2 : #
Total comments: 19
Patch Set 3 : #
Total comments: 1
Patch Set 4 : #
Total comments: 7
Patch Set 5 : After Adding New Method HTMLElement* findNextFocusableRadioButton() #
Total comments: 8
Patch Set 6 : #
Total comments: 11
Patch Set 7 : After Changing the test case #
Total comments: 2
Patch Set 8 : #Patch Set 9 : #Patch Set 10 : #Patch Set 11 : After Updating branch #
Messages
Total messages: 60 (23 generated)
paritosh.in@samsung.com changed reviewers: - habib.virji@samsung.com
On 2014/09/05 11:36:30, paritosh.in wrote:
Few things regarding description:
1. Title should be part of the description, as text under description is the one
that gets committed.
2. In description, you should not describe problem but the details about the
changes.
3. Bug is repeated twice in the description.
4. Since change is new, it should have new test. You should be able to test
using below test.
<!DOCTYPE html>
<body>
<input type="radio" value="1" name="option" id="radio1"/>
<input type="radio" value="2" name="option" id="radio2"/>
<input type="radio" value="3" name="option" id="radio3"/>
<input type="radio" value="4" name="option" id="radio4"/>
<script src="../resources/common.js"></script>
<script src="../../../resources/js-test.js"></script>
<script>
description('Tests after pressing last radio button in the list, on pressing
downkey it selects back first radio button');
clickElement(document.getElementById('radio4')); // Allows clicking on the
element
shouldBeTrue('document.getElementById("radio4").checked'); // Click element 4,
so should be checked.
eventSender.keyDown('downArrow'); // simulates down arrow key
shouldBeTrue('document.getElementById("radio1").checked'); // As per my
understanding this should be true with your change.
</script>
</body>
Tests after pressing last radio button in the list, on pressing downkey it
selects back first radio button
On success, you will see a series of "PASS" messages, followed by "TEST
COMPLETE".
PASS document.getElementById("radio4").checked is true
FAIL document.getElementById("radio1").checked should be true. Was false.
PASS successfullyParsed is true
TEST COMPLETE
Few things are not clear to me: 1. Did you test your code with by running all tests in fast/forms. Would suggest to do that as radio tests are not just located in fast/forms/radio. 2. Did you test code by having multiple input elements? what happens when you try move from radio button group to next input element such as select. Does it move to select or gets stuck with radio? I will need to try the code, somehow solution looks bit tricky to me.
Thanks Mr. Habib. I changed the description. I will add this test case.
On 2014/09/08 12:10:57, Paritosh Kumar wrote: Paritosh sorry I could not complete in today..will update first thing tomorrow.
habib.virji@samsung.com changed reviewers: + habib.virji@samsung.com
As mentioned, I am still not comfortable with the logic. Because if it is not HTMLFormElement, the below code will falter. Also issue is you are traversing all node in backward direction. If you press right, after it reaches last element. Should it go back to first element or previous element? Also forgot to mention, you need to add your name in AUTHORS file as part of submission. https://codereview.chromium.org/546753002/diff/20001/LayoutTests/fast/forms/r... File LayoutTests/fast/forms/radio/radio-checked-LastorFirst-then-DownorUp-keyDown.html (right): https://codereview.chromium.org/546753002/diff/20001/LayoutTests/fast/forms/r... LayoutTests/fast/forms/radio/radio-checked-LastorFirst-then-DownorUp-keyDown.html:23: clickElement(document.getElementById('radio4')); // Allows clicking on the element Comments can be probably removed, wrote them to give you some understanding about them. https://codereview.chromium.org/546753002/diff/20001/LayoutTests/fast/forms/r... LayoutTests/fast/forms/radio/radio-checked-LastorFirst-then-DownorUp-keyDown.html:24: shouldBeTrue('document.getElementById("radio4").checked'); // Click element 4, so should be checked. ditto https://codereview.chromium.org/546753002/diff/20001/LayoutTests/fast/forms/r... LayoutTests/fast/forms/radio/radio-checked-LastorFirst-then-DownorUp-keyDown.html:25: eventSender.keyDown('downArrow'); // simulates down arrow key ditto https://codereview.chromium.org/546753002/diff/20001/LayoutTests/fast/forms/r... LayoutTests/fast/forms/radio/radio-checked-LastorFirst-then-DownorUp-keyDown.html:27: shouldBeTrue('document.getElementById("radio1").checked'); // As per change this should be true. Ditto https://codereview.chromium.org/546753002/diff/20001/LayoutTests/fast/forms/r... LayoutTests/fast/forms/radio/radio-checked-LastorFirst-then-DownorUp-keyDown.html:29: clickElement(document.getElementById('radio1')); // Allows clicking on the element ditto https://codereview.chromium.org/546753002/diff/20001/LayoutTests/fast/forms/r... LayoutTests/fast/forms/radio/radio-checked-LastorFirst-then-DownorUp-keyDown.html:30: shouldBeTrue('document.getElementById("radio1").checked'); // Click element 1, so should be checked. ditto https://codereview.chromium.org/546753002/diff/20001/LayoutTests/fast/forms/r... LayoutTests/fast/forms/radio/radio-checked-LastorFirst-then-DownorUp-keyDown.html:31: eventSender.keyDown('upArrow'); // simulates down arrow key ditto https://codereview.chromium.org/546753002/diff/20001/LayoutTests/fast/forms/r... LayoutTests/fast/forms/radio/radio-checked-LastorFirst-then-DownorUp-keyDown.html:33: shouldBeTrue('document.getElementById("radio4").checked'); // As per change this should be true. ditto https://codereview.chromium.org/546753002/diff/20001/Source/core/html/forms/R... File Source/core/html/forms/RadioInputType.cpp (right): https://codereview.chromium.org/546753002/diff/20001/Source/core/html/forms/R... Source/core/html/forms/RadioInputType.cpp:98: if (!htmlElement || (isHTMLInputElement(*htmlElement) && toHTMLInputElement(htmlElement)->form() != element().form()) || isHTMLFormElement(*htmlElement)) { If htmlElement is null, this line will result in crash. Just try without radio group and you will see the crash https://codereview.chromium.org/546753002/diff/20001/Source/core/html/forms/R... Source/core/html/forms/RadioInputType.cpp:104: while (!isHTMLFormElement(*nextHtmlElement) && nextHtmlElement) { is this loop necessary and should not nextHTMLElement be first condition that you check, because if it is null it will avoid isHTMLFormElement https://codereview.chromium.org/546753002/diff/20001/Source/core/html/forms/R... Source/core/html/forms/RadioInputType.cpp:107: if (inputElement->form() == element().form() && inputElement->isRadioButton() && inputElement->name() == element().name() && inputElement->isFocusable()) Should not there be a break, if we found the radio element. Based on your logic, we are traversing back, when we press right or down key, how do we make sure we land after option 4 to option 1.
Updated code as per suggestion. https://codereview.chromium.org/546753002/diff/20001/LayoutTests/fast/forms/r... File LayoutTests/fast/forms/radio/radio-checked-LastorFirst-then-DownorUp-keyDown.html (right): https://codereview.chromium.org/546753002/diff/20001/LayoutTests/fast/forms/r... LayoutTests/fast/forms/radio/radio-checked-LastorFirst-then-DownorUp-keyDown.html:23: clickElement(document.getElementById('radio4')); // Allows clicking on the element On 2014/09/10 09:31:52, Habib Virji wrote: > Comments can be probably removed, wrote them to give you some understanding > about them. Done. https://codereview.chromium.org/546753002/diff/20001/LayoutTests/fast/forms/r... LayoutTests/fast/forms/radio/radio-checked-LastorFirst-then-DownorUp-keyDown.html:24: shouldBeTrue('document.getElementById("radio4").checked'); // Click element 4, so should be checked. On 2014/09/10 09:31:52, Habib Virji wrote: > ditto Done. https://codereview.chromium.org/546753002/diff/20001/LayoutTests/fast/forms/r... LayoutTests/fast/forms/radio/radio-checked-LastorFirst-then-DownorUp-keyDown.html:25: eventSender.keyDown('downArrow'); // simulates down arrow key On 2014/09/10 09:31:52, Habib Virji wrote: > ditto Done. https://codereview.chromium.org/546753002/diff/20001/LayoutTests/fast/forms/r... LayoutTests/fast/forms/radio/radio-checked-LastorFirst-then-DownorUp-keyDown.html:27: shouldBeTrue('document.getElementById("radio1").checked'); // As per change this should be true. On 2014/09/10 09:31:52, Habib Virji wrote: > Ditto Done. https://codereview.chromium.org/546753002/diff/20001/LayoutTests/fast/forms/r... LayoutTests/fast/forms/radio/radio-checked-LastorFirst-then-DownorUp-keyDown.html:29: clickElement(document.getElementById('radio1')); // Allows clicking on the element On 2014/09/10 09:31:52, Habib Virji wrote: > ditto Done. https://codereview.chromium.org/546753002/diff/20001/LayoutTests/fast/forms/r... LayoutTests/fast/forms/radio/radio-checked-LastorFirst-then-DownorUp-keyDown.html:30: shouldBeTrue('document.getElementById("radio1").checked'); // Click element 1, so should be checked. On 2014/09/10 09:31:52, Habib Virji wrote: > ditto Done. https://codereview.chromium.org/546753002/diff/20001/LayoutTests/fast/forms/r... LayoutTests/fast/forms/radio/radio-checked-LastorFirst-then-DownorUp-keyDown.html:31: eventSender.keyDown('upArrow'); // simulates down arrow key On 2014/09/10 09:31:52, Habib Virji wrote: > ditto Done. https://codereview.chromium.org/546753002/diff/20001/LayoutTests/fast/forms/r... LayoutTests/fast/forms/radio/radio-checked-LastorFirst-then-DownorUp-keyDown.html:33: shouldBeTrue('document.getElementById("radio4").checked'); // As per change this should be true. On 2014/09/10 09:31:52, Habib Virji wrote: > ditto Done.
https://codereview.chromium.org/546753002/diff/40001/Source/core/html/forms/R... File Source/core/html/forms/RadioInputType.cpp (right): https://codereview.chromium.org/546753002/diff/40001/Source/core/html/forms/R... Source/core/html/forms/RadioInputType.cpp:105: reachedAtEnd = true; How about we move this code outside this for loop? --> check nextElement(element(), forward), is null and your other conditions, and set forward = false. Will that work. Keeping code to the min, will be good.
Updated. PTAL.
On 2014/09/22 11:42:52, Paritosh Kumar wrote: > Updated. PTAL. Looks good. Can you remove WIP and go ahead to get it reviewed by keishi and tkent.
On 2014/09/23 13:10:54, Habib Virji wrote: > On 2014/09/22 11:42:52, Paritosh Kumar wrote: > > Updated. PTAL. > > Looks good. Can you remove WIP and go ahead to get it reviewed by keishi and > tkent. Thanks.
paritosh.in@samsung.com changed reviewers: + keishi@chromium.org, tkent@chromium.org
PTAL.
https://codereview.chromium.org/546753002/diff/60001/LayoutTests/fast/forms/r... File LayoutTests/fast/forms/radio/radio-checked-LastorFirst-then-DownorUp-keyDown.html (right): https://codereview.chromium.org/546753002/diff/60001/LayoutTests/fast/forms/r... LayoutTests/fast/forms/radio/radio-checked-LastorFirst-then-DownorUp-keyDown.html:8: <script src="../../repaint/resources/text-based-repaint.js"></script> Do we need this? https://codereview.chromium.org/546753002/diff/60001/LayoutTests/fast/forms/r... LayoutTests/fast/forms/radio/radio-checked-LastorFirst-then-DownorUp-keyDown.html:16: <input type="radio" value="4" name="option" id="radio4"/> It would be great to add tests for the complicated cases. Like, mixing buttons from different groups and other elements <input type=radio name=foo> <input type=radio name=bar> <div><input type=radio name=foo></div> And attaching form elements to form elements <form name=alpha> <input type=radio name=foo> <input type=radio name=foo> </form> <form name=beta> <input type=radio name=foo> </form> <script> document.alpha.appendChild(document.beta); </script> https://codereview.chromium.org/546753002/diff/60001/Source/core/html/forms/R... File Source/core/html/forms/RadioInputType.cpp (right): https://codereview.chromium.org/546753002/diff/60001/Source/core/html/forms/R... Source/core/html/forms/RadioInputType.cpp:97: bool reachedAtEnd = false; nit: I think reachedEnd is better. https://codereview.chromium.org/546753002/diff/60001/Source/core/html/forms/R... Source/core/html/forms/RadioInputType.cpp:105: if ((isHTMLInputElement(*htmlElement) && toHTMLInputElement(htmlElement)->form() != element().form())) { Why do we end the search? I think we should just ignore radio buttons with different associated forms. https://codereview.chromium.org/546753002/diff/60001/Source/core/html/forms/R... Source/core/html/forms/RadioInputType.cpp:109: if (isHTMLFormElement(*htmlElement)) { I know this was here from before but this seems wrong. The associated form element of this control may have a form element as one of its descendants. I think we should be using the stayWithin argument for Traversal::next to traverse within the associated form element. https://codereview.chromium.org/546753002/diff/60001/Source/core/html/forms/R... Source/core/html/forms/RadioInputType.cpp:132: if (inputElement->form() == element().form() && inputElement->isRadioButton() && inputElement->name() == element().name() && inputElement->isFocusable()) This code is mostly same from the for loop above. Could you share the code by adding a method like: HTMLElement* findNextFocusableRadioButton(bool forward) https://codereview.chromium.org/546753002/diff/60001/Source/core/html/forms/R... Source/core/html/forms/RadioInputType.cpp:139: if (inputElement->isRadioButton() && inputElement->name() == element().name() && inputElement->isFocusable()) { Isn't this checking the same thing twice? We already checked this in the for loop and while loop.
Patchset #5 (id:80001) has been deleted
Thanks Keishi. Sorry for too much delay in update. I updated the test case with some comlication as you suggested and added a new method as you suggested. PTAL.
https://codereview.chromium.org/546753002/diff/100001/LayoutTests/fast/forms/... File LayoutTests/fast/forms/radio/radio-checked-LastorFirst-then-DownorUp-keyDown.html (right): https://codereview.chromium.org/546753002/diff/100001/LayoutTests/fast/forms/... LayoutTests/fast/forms/radio/radio-checked-LastorFirst-then-DownorUp-keyDown.html:21: <script> Please incorporate the test cases from http://jsbin.com/tumeculiheku/7 https://codereview.chromium.org/546753002/diff/100001/Source/core/html/forms/... File Source/core/html/forms/RadioInputType.cpp (right): https://codereview.chromium.org/546753002/diff/100001/Source/core/html/forms/... Source/core/html/forms/RadioInputType.cpp:74: HTMLElement* RadioInputType::findNextFocusableRadioButton(HTMLInputElement* currentHtmlInputElement, bool forward) I'm sorry. I know I wrote HTMLElement* in my previous comment, but this always returns an input element so returning a HTMLInputElement* is probably better. https://codereview.chromium.org/546753002/diff/100001/Source/core/html/forms/... Source/core/html/forms/RadioInputType.cpp:79: if (isHTMLFormElement(*htmlElement)) { No curly braces for single line if statements. https://codereview.chromium.org/546753002/diff/100001/Source/core/html/forms/... Source/core/html/forms/RadioInputType.cpp:79: if (isHTMLFormElement(*htmlElement)) { As I said in my previous comment, this isn't right. We might have other form elements as a descendent of the associated form and those should just be ignored. The set of elements to cycle should match the RadioNodeList you get from the form element. Here is the test case I used. http://jsbin.com/tumeculiheku/7 Traversal::next can take a node to stay inside as its second argument. You can pass element() in there. Something like this: HTMLElement* nextElement(const HTMLElement& element, HTMLElement* stayWithin, bool forward) { return forward ? Traversal<HTMLElement>::next(element, stayWithin) : Traversal<HTMLElement>::previous(element, stayWithin); } for (htmlElement = nextElement(*currentHtmlInputElement, element().form(), forward); htmlElement; htmlElement = nextElement(*htmlElement, element().form(), forward)) {...} https://codereview.chromium.org/546753002/diff/100001/Source/core/html/forms/... Source/core/html/forms/RadioInputType.cpp:83: if (!isHTMLInputElement(*htmlElement)) { Ditto. https://codereview.chromium.org/546753002/diff/100001/Source/core/html/forms/... Source/core/html/forms/RadioInputType.cpp:87: if (inputElement->form() != currentHtmlInputElement->form()) { Ditto. https://codereview.chromium.org/546753002/diff/100001/Source/core/html/forms/... Source/core/html/forms/RadioInputType.cpp:87: if (inputElement->form() != currentHtmlInputElement->form()) { We should be checking if form equals element().form(), not the form for currentHtmlInputElement. https://codereview.chromium.org/546753002/diff/100001/Source/core/html/forms/... Source/core/html/forms/RadioInputType.cpp:90: if (inputElement->isRadioButton() && inputElement->name() == currentHtmlInputElement->name() && inputElement->isFocusable()) { Ditto.
Thanks Keishi. Apologize, I am on leave, can I update on 6th Oct?
Patchset #6 (id:120001) has been deleted
Apologize, I was on leave so not updating. Updated as you suggested, but unable to call element() method in findNextFocusableRadioButton() beacause element() is a protected method of InputTypeView class. PTAL. https://codereview.chromium.org/546753002/diff/140001/Source/core/html/forms/... File Source/core/html/forms/RadioInputType.cpp (right): https://codereview.chromium.org/546753002/diff/140001/Source/core/html/forms/... Source/core/html/forms/RadioInputType.cpp:81: if (currentHtmlInputElement->form() == inputElement->form() && inputElement->isRadioButton() && inputElement->name() == currentHtmlInputElement->name() && inputElement->isFocusable()) Unable to call element() method of InputTypeView within this method.
https://codereview.chromium.org/546753002/diff/140001/LayoutTests/fast/forms/... File LayoutTests/fast/forms/radio/radio-checked-LastorFirst-then-DownorUp-keyDown.html (right): https://codereview.chromium.org/546753002/diff/140001/LayoutTests/fast/forms/... LayoutTests/fast/forms/radio/radio-checked-LastorFirst-then-DownorUp-keyDown.html:2: radio-checked-LastorFirst-then-DownorUp-keyDown.html is quite confusing because it mixes CamelCaps. I think we only do that when we want to write DOM interface names. How about something like radio-group-keyboard-cycle-first-last.html or radio-group-arrow-cycle-edge.html https://codereview.chromium.org/546753002/diff/140001/LayoutTests/fast/forms/... LayoutTests/fast/forms/radio/radio-checked-LastorFirst-then-DownorUp-keyDown.html:20: document.alpha.insertBefore(document.beta, document.alpha.fruit[2]); You probably forgot to remove this line. https://codereview.chromium.org/546753002/diff/140001/LayoutTests/fast/forms/... LayoutTests/fast/forms/radio/radio-checked-LastorFirst-then-DownorUp-keyDown.html:23: clickElement(document.getElementById('cherry')); nit: You could use the $(id) defined in common.js if you want to. https://codereview.chromium.org/546753002/diff/140001/LayoutTests/fast/forms/... LayoutTests/fast/forms/radio/radio-checked-LastorFirst-then-DownorUp-keyDown.html:40: shouldBeTrue('document.getElementById("cucumber").checked'); nit: Could you add a comment here explaining that tomato and cucumber cycle because both have null owner forms. https://codereview.chromium.org/546753002/diff/140001/Source/core/html/forms/... File Source/core/html/forms/RadioInputType.cpp (right): https://codereview.chromium.org/546753002/diff/140001/Source/core/html/forms/... Source/core/html/forms/RadioInputType.cpp:73: HTMLInputElement* RadioInputType::findNextFocusableRadioButton(HTMLInputElement* currentHtmlInputElement, bool forward) nit: currentHtmlInputElement is a pretty long variable name. You could choose a shorter one. https://codereview.chromium.org/546753002/diff/140001/Source/core/html/forms/... Source/core/html/forms/RadioInputType.cpp:81: if (currentHtmlInputElement->form() == inputElement->form() && inputElement->isRadioButton() && inputElement->name() == currentHtmlInputElement->name() && inputElement->isFocusable()) On 2014/10/06 08:32:24, Paritosh Kumar wrote: > Unable to call element() method of InputTypeView within this method. I don't understand this, element() is being called on lines 76 and 77. But I now realized that for your code, element().form() is always the same as currentHtmlInputElement->form(). If you want to keep the code this way, we should have an ASSERT checking this so that we won't pass inputs with different forms by mistake.
Patchset #7 (id:160001) has been deleted
https://codereview.chromium.org/546753002/diff/140001/LayoutTests/fast/forms/... File LayoutTests/fast/forms/radio/radio-checked-LastorFirst-then-DownorUp-keyDown.html (right): https://codereview.chromium.org/546753002/diff/140001/LayoutTests/fast/forms/... LayoutTests/fast/forms/radio/radio-checked-LastorFirst-then-DownorUp-keyDown.html:40: shouldBeTrue('document.getElementById("cucumber").checked'); On 2014/10/06 10:52:32, keishi wrote: > nit: Could you add a comment here explaining that tomato and cucumber cycle > because both have null owner forms. Actually for both radio button, form() returns 0 and both button has the same name, it makes the condition for second "if" statement in findNextFocusableradioButton, "true". This results in cycle here. And the same behavior exist in FF and IE for null owner.
Thanks Keishi. Changed the name of test case file with simple and resonable name. And updated the code with use of element().form(). Is there any need of ASSERT now? Please have a look on this. https://codereview.chromium.org/546753002/diff/140001/LayoutTests/fast/forms/... File LayoutTests/fast/forms/radio/radio-checked-LastorFirst-then-DownorUp-keyDown.html (right): https://codereview.chromium.org/546753002/diff/140001/LayoutTests/fast/forms/... LayoutTests/fast/forms/radio/radio-checked-LastorFirst-then-DownorUp-keyDown.html:2: On 2014/10/06 10:52:32, keishi wrote: > radio-checked-LastorFirst-then-DownorUp-keyDown.html is quite confusing because > it mixes CamelCaps. I think we only do that when we want to write DOM interface > names. > How about something like radio-group-keyboard-cycle-first-last.html or > radio-group-arrow-cycle-edge.html Thanks. Done. https://codereview.chromium.org/546753002/diff/140001/LayoutTests/fast/forms/... LayoutTests/fast/forms/radio/radio-checked-LastorFirst-then-DownorUp-keyDown.html:20: document.alpha.insertBefore(document.beta, document.alpha.fruit[2]); On 2014/10/06 10:52:32, keishi wrote: > You probably forgot to remove this line. Apologize. Done. Thanks. https://codereview.chromium.org/546753002/diff/140001/Source/core/html/forms/... File Source/core/html/forms/RadioInputType.cpp (right): https://codereview.chromium.org/546753002/diff/140001/Source/core/html/forms/... Source/core/html/forms/RadioInputType.cpp:81: if (currentHtmlInputElement->form() == inputElement->form() && inputElement->isRadioButton() && inputElement->name() == currentHtmlInputElement->name() && inputElement->isFocusable()) Apologize. Yes it's working. Done. Thanks.
PTAL.
lgtm https://codereview.chromium.org/546753002/diff/180001/Source/core/html/forms/... File Source/core/html/forms/RadioInputType.cpp (right): https://codereview.chromium.org/546753002/diff/180001/Source/core/html/forms/... Source/core/html/forms/RadioInputType.cpp:73: HTMLInputElement* RadioInputType::findNextFocusableRadioButton(HTMLInputElement* currentElement, bool forward) nit: Maybe findNextFocusableRadioButtonInGroup will be a more accurate name. https://codereview.chromium.org/546753002/diff/180001/Source/core/html/forms/... Source/core/html/forms/RadioInputType.cpp:77: // Look for more radio buttons. nit: I think you could remove this. I don't think this comment adds information.
The CQ bit was checked by paritosh.in@samsung.com
The CQ bit was unchecked by paritosh.in@samsung.com
Thanks Keishi. Updated as suggested. Done.
paritosh.in@samsung.com changed reviewers: - habib.virji@samsung.com, keishi@chromium.org
Please have a look.
The CQ bit was checked by habib.virji@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/546753002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for Source/core/html/forms/RadioInputType.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file Source/core/html/forms/RadioInputType.cpp
Hunk #3 FAILED at 105.
1 out of 3 hunks FAILED -- saving rejects to file
Source/core/html/forms/RadioInputType.cpp.rej
Patch: Source/core/html/forms/RadioInputType.cpp
Index: Source/core/html/forms/RadioInputType.cpp
diff --git a/Source/core/html/forms/RadioInputType.cpp
b/Source/core/html/forms/RadioInputType.cpp
index
4e992f650156e2874b2a8c1e9af574347c232a5a..c661962c05338f3e0b2a44c031386091214e6267
100644
--- a/Source/core/html/forms/RadioInputType.cpp
+++ b/Source/core/html/forms/RadioInputType.cpp
@@ -37,11 +37,10 @@ namespace blink {
namespace {
-HTMLElement* nextElement(const HTMLElement& element, bool forward)
+HTMLElement* nextElement(const HTMLElement& element, HTMLFormElement*
stayWithin, bool forward)
{
- return forward ? Traversal<HTMLElement>::next(element) :
Traversal<HTMLElement>::previous(element);
+ return forward ? Traversal<HTMLElement>::next(element, (Node* )stayWithin)
: Traversal<HTMLElement>::previous(element, (Node* )stayWithin);
}
-
} // namespace
using namespace HTMLNames;
@@ -71,6 +70,19 @@ void RadioInputType::handleClickEvent(MouseEvent* event)
event->setDefaultHandled();
}
+HTMLInputElement*
RadioInputType::findNextFocusableRadioButtonInGroup(HTMLInputElement*
currentElement, bool forward)
+{
+ HTMLElement* htmlElement;
+ for (htmlElement = nextElement(*currentElement, element().form(), forward);
htmlElement; htmlElement = nextElement(*htmlElement, element().form(), forward))
{
+ if (!isHTMLInputElement(*htmlElement))
+ continue;
+ HTMLInputElement* inputElement = toHTMLInputElement(htmlElement);
+ if (element().form() == inputElement->form() &&
inputElement->isRadioButton() && inputElement->name() == element().name() &&
inputElement->isFocusable())
+ return inputElement;
+ }
+ return nullptr;
+}
+
void RadioInputType::handleKeydownEvent(KeyboardEvent* event)
{
BaseCheckableInputType::handleKeydownEvent(event);
@@ -93,24 +105,23 @@ void RadioInputType::handleKeydownEvent(KeyboardEvent*
event)
// We can only stay within the form's children if the form hasn't been
demoted to a leaf because
// of malformed HTML.
- for (HTMLElement* htmlElement = nextElement(element(), forward);
htmlElement; htmlElement = nextElement(*htmlElement, forward)) {
- // Once we encounter a form element, we know we're through.
- if (isHTMLFormElement(*htmlElement))
- break;
- // Look for more radio buttons.
- if (!isHTMLInputElement(*htmlElement))
- continue;
- HTMLInputElement* inputElement = toHTMLInputElement(htmlElement);
- if (inputElement->form() != element().form())
- break;
- if (inputElement->isRadioButton() && inputElement->name() ==
element().name() && inputElement->isFocusable()) {
- RefPtrWillBeRawPtr<HTMLInputElement> protector(inputElement);
- document.setFocusedElement(inputElement);
- inputElement->dispatchSimulatedClick(event, SendNoEvents);
- event->setDefaultHandled();
- return;
+ HTMLInputElement* inputElement =
findNextFocusableRadioButtonInGroup(toHTMLInputElement(&element()), forward);
+ if (!inputElement) {
+ // Traverse in reverse direction till last or first radio button
+ forward = !(forward);
+ HTMLInputElement* nextInputElement =
findNextFocusableRadioButtonInGroup(toHTMLInputElement(&element()), forward);
+ while (nextInputElement) {
+ inputElement = nextInputElement;
+ nextInputElement =
findNextFocusableRadioButtonInGroup(nextInputElement, forward);
}
}
+ if (inputElement) {
+ RefPtrWillBeRawPtr<HTMLInputElement> protector(inputElement);
+ document.setFocusedElement(inputElement);
+ inputElement->dispatchSimulatedClick(event, SendNoEvents);
+ event->setDefaultHandled();
+ return;
+ }
}
void RadioInputType::handleKeyupEvent(KeyboardEvent* event)
The CQ bit was unchecked by habib.virji@chromium.org
@paritosh: Please have a look. it is probably not updated for longtime to latest master. Please use git rebase-update and it will update your branch. Resolve changes and then push in new changes.
The CQ bit was checked by paritosh.in@samsung.com
The CQ bit was unchecked by paritosh.in@samsung.com
On 2014/10/07 12:42:42, habib.virji wrote: > @paritosh: Please have a look. it is probably not updated for longtime to latest > master. > > Please use git rebase-update and it will update your branch. Resolve changes and > then push in new changes. Thanks. Done. Please have a look.
PTAL.
The CQ bit was checked by paritosh.in@samsung.com
The CQ bit was unchecked by paritosh.in@samsung.com
The CQ bit was checked by paritosh.in@samsung.com
The CQ bit was unchecked by paritosh.in@samsung.com
The CQ bit was checked by paritosh.in@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/546753002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for Source/core/html/forms/RadioInputType.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file Source/core/html/forms/RadioInputType.cpp
Hunk #3 FAILED at 106.
1 out of 3 hunks FAILED -- saving rejects to file
Source/core/html/forms/RadioInputType.cpp.rej
Patch: Source/core/html/forms/RadioInputType.cpp
Index: Source/core/html/forms/RadioInputType.cpp
diff --git a/Source/core/html/forms/RadioInputType.cpp
b/Source/core/html/forms/RadioInputType.cpp
index
4e992f650156e2874b2a8c1e9af574347c232a5a..250870d3d9c783b29450924c79b31eea0084e015
100644
--- a/Source/core/html/forms/RadioInputType.cpp
+++ b/Source/core/html/forms/RadioInputType.cpp
@@ -37,9 +37,9 @@ namespace blink {
namespace {
-HTMLElement* nextElement(const HTMLElement& element, bool forward)
+HTMLElement* nextElement(const HTMLElement& element, HTMLFormElement*
stayWithin, bool forward)
{
- return forward ? Traversal<HTMLElement>::next(element) :
Traversal<HTMLElement>::previous(element);
+ return forward ? Traversal<HTMLElement>::next(element, (Node* )stayWithin)
: Traversal<HTMLElement>::previous(element, (Node* )stayWithin);
}
} // namespace
@@ -71,6 +71,19 @@ void RadioInputType::handleClickEvent(MouseEvent* event)
event->setDefaultHandled();
}
+HTMLInputElement*
RadioInputType::findNextFocusableRadioButtonInGroup(HTMLInputElement*
currentElement, bool forward)
+{
+ HTMLElement* htmlElement;
+ for (htmlElement = nextElement(*currentElement, element().form(), forward);
htmlElement; htmlElement = nextElement(*htmlElement, element().form(), forward))
{
+ if (!isHTMLInputElement(*htmlElement))
+ continue;
+ HTMLInputElement* inputElement = toHTMLInputElement(htmlElement);
+ if (element().form() == inputElement->form() &&
inputElement->isRadioButton() && inputElement->name() == element().name() &&
inputElement->isFocusable())
+ return inputElement;
+ }
+ return nullptr;
+}
+
void RadioInputType::handleKeydownEvent(KeyboardEvent* event)
{
BaseCheckableInputType::handleKeydownEvent(event);
@@ -93,24 +106,23 @@ void RadioInputType::handleKeydownEvent(KeyboardEvent*
event)
// We can only stay within the form's children if the form hasn't been
demoted to a leaf because
// of malformed HTML.
- for (HTMLElement* htmlElement = nextElement(element(), forward);
htmlElement; htmlElement = nextElement(*htmlElement, forward)) {
- // Once we encounter a form element, we know we're through.
- if (isHTMLFormElement(*htmlElement))
- break;
- // Look for more radio buttons.
- if (!isHTMLInputElement(*htmlElement))
- continue;
- HTMLInputElement* inputElement = toHTMLInputElement(htmlElement);
- if (inputElement->form() != element().form())
- break;
- if (inputElement->isRadioButton() && inputElement->name() ==
element().name() && inputElement->isFocusable()) {
- RefPtrWillBeRawPtr<HTMLInputElement> protector(inputElement);
- document.setFocusedElement(inputElement);
- inputElement->dispatchSimulatedClick(event, SendNoEvents);
- event->setDefaultHandled();
- return;
+ HTMLInputElement* inputElement =
findNextFocusableRadioButtonInGroup(toHTMLInputElement(&element()), forward);
+ if (!inputElement) {
+ // Traverse in reverse direction till last or first radio button
+ forward = !(forward);
+ HTMLInputElement* nextInputElement =
findNextFocusableRadioButtonInGroup(toHTMLInputElement(&element()), forward);
+ while (nextInputElement) {
+ inputElement = nextInputElement;
+ nextInputElement =
findNextFocusableRadioButtonInGroup(nextInputElement, forward);
}
}
+ if (inputElement) {
+ RefPtrWillBeRawPtr<HTMLInputElement> protector(inputElement);
+ document.setFocusedElement(inputElement);
+ inputElement->dispatchSimulatedClick(event, SendNoEvents);
+ event->setDefaultHandled();
+ return;
+ }
}
void RadioInputType::handleKeyupEvent(KeyboardEvent* event)
The CQ bit was checked by paritosh.in@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/546753002/310001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/android_chromium_gn_comp...) mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/26869)
The CQ bit was checked by paritosh.in@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/546753002/460001
Message was sent while issue was closed.
Committed patchset #11 (id:460001) as 183661 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
