Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
507533002:
Fix to keep the selection of the text field in input element after setSelectionRange is called by d… (Closed)
|
Created:
6 years, 3 months ago by Miyoung Shin(g) Modified:
6 years, 3 months ago CC:
blink-reviews, blink-reviews-events_chromium.org, dglazkov+blink, eae+blinkwatch, kenjibaheux, ppi, jam Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionFix to keep the selection of the text field in input element after setSelectionRange is called by dom events related to mouse
The problem is that the selection is cleared when handling mouse release.
The selection should be kept if the selection is set by setSelectionRange during handling mouse event.
BUG=32865
R=yosin@chromium.org
R=rbyers@chromium.org
TEST=LayoutTests/fast/forms/setrangetext-within-events.html
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181490
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181959
Patch Set 1 #Patch Set 2 : fix the indentation of HTML document #
Total comments: 12
Patch Set 3 : #
Total comments: 1
Patch Set 4 : #
Total comments: 14
Patch Set 5 : #Patch Set 6 : #
Total comments: 1
Patch Set 7 : #Patch Set 8 : rebase #
Messages
Total messages: 38 (5 generated)
yosin@chromium.org changed reviewers: + yoichio@chromium.org, yosin@chromium.org
yoichio@, does this patch fix issue http://crbug.com/404969, Blue selection highlight for autofill data doesn't go away on mouse click. https://codereview.chromium.org/507533002/diff/20001/LayoutTests/fast/forms/s... File LayoutTests/fast/forms/setrangetext-within-events.html (right): https://codereview.chromium.org/507533002/diff/20001/LayoutTests/fast/forms/s... LayoutTests/fast/forms/setrangetext-within-events.html:1: <html> nit: Please <!DOCTYPE html>. We prefer to use strict mode. https://codereview.chromium.org/507533002/diff/20001/LayoutTests/fast/forms/s... LayoutTests/fast/forms/setrangetext-within-events.html:1: <html> nit: We don't need to have HTML and HEAD. https://codereview.chromium.org/507533002/diff/20001/LayoutTests/fast/forms/s... LayoutTests/fast/forms/setrangetext-within-events.html:13: function doSetSelectionRange (event) { nit: No need to have a space between function name and parameter list. https://codereview.chromium.org/507533002/diff/20001/LayoutTests/fast/forms/s... LayoutTests/fast/forms/setrangetext-within-events.html:13: function doSetSelectionRange (event) { nit: Please insert a blank line to separate function definitions. https://codereview.chromium.org/507533002/diff/20001/LayoutTests/fast/forms/s... LayoutTests/fast/forms/setrangetext-within-events.html:14: var textfield = document.getElementById('textfield'); nit: Please use one type of quote in script, L7-L8 use double quote. We prefer single quote. https://codereview.chromium.org/507533002/diff/20001/LayoutTests/fast/forms/s... LayoutTests/fast/forms/setrangetext-within-events.html:21: alert('start: ' + textfield.selectionStart + ' end: ' + textfield.selectionEnd + ' [' + event + ']'); Could you use "resources/js-test.js" framework? https://codereview.chromium.org/507533002/diff/20001/LayoutTests/fast/forms/s... LayoutTests/fast/forms/setrangetext-within-events.html:23: eventSender.mouseMoveTo(tx, ty+100); nit: Please have spaces around |+|. https://codereview.chromium.org/507533002/diff/20001/LayoutTests/fast/forms/s... LayoutTests/fast/forms/setrangetext-within-events.html:26: textfield.removeEventListener(event, setSelectionRange, false); nit: You don't need to have third argument |false|. Default is |false|. https://codereview.chromium.org/507533002/diff/20001/LayoutTests/fast/forms/s... LayoutTests/fast/forms/setrangetext-within-events.html:28: function setSelectionRange(e) { We can omit parameter |e|. Since, it isn't used. https://codereview.chromium.org/507533002/diff/20001/LayoutTests/fast/forms/s... LayoutTests/fast/forms/setrangetext-within-events.html:34: <body onload="test()"> nit: You can use window.onload = function() { ... }; https://codereview.chromium.org/507533002/diff/20001/LayoutTests/fast/forms/s... LayoutTests/fast/forms/setrangetext-within-events.html:38: nit: Please remove an extra blank line. https://codereview.chromium.org/507533002/diff/20001/Source/core/editing/Fram... File Source/core/editing/FrameSelection.h (right): https://codereview.chromium.org/507533002/diff/20001/Source/core/editing/Fram... Source/core/editing/FrameSelection.h:248: void notifyEventHandlerrForSelectionChange(); nit: Extra "r" in Handlerr notifyEventHandlerrForSelectionChange => notifyEventHandlerForSelectionChange
On 2014/08/26 03:55:08, Yosi_UTC9 wrote: > yoichio@, does this patch fix issue http://crbug.com/404969, Blue selection > highlight for autofill data doesn't go away on mouse click. I think that it's a different issue because this issue is not related to autofill(autocomplete). And this problem is not fixed even if I applied yoichio@'s patch on the latest blink. I will upload the patch again regarding the others comment
LGTM https://codereview.chromium.org/507533002/diff/40001/LayoutTests/fast/forms/s... File LayoutTests/fast/forms/setrangetext-within-events.html (right): https://codereview.chromium.org/507533002/diff/40001/LayoutTests/fast/forms/s... LayoutTests/fast/forms/setrangetext-within-events.html:26: } nit: Please align "}" and "else" like: "} else {"
myid.o.shin@gmail.com changed reviewers: + hayato@chromium.org, rbyers@chromium.org
myid.o.shin@gmail.com changed reviewers: - hayato@chromium.org
myid.o.shin@gmail.com changed reviewers: - philipj@opera.com
kenjibaheux@ is the expert on text selection. But this seems fairly straight forward and (assuming it matches the other browsers) low risk. So if Kenji doesn't raise any objections I'm OK with it modulo some potential simplification. https://codereview.chromium.org/507533002/diff/60001/LayoutTests/fast/forms/s... File LayoutTests/fast/forms/setrangetext-within-events.html (right): https://codereview.chromium.org/507533002/diff/60001/LayoutTests/fast/forms/s... LayoutTests/fast/forms/setrangetext-within-events.html:4: window.onload = function() { please add a description() call https://codereview.chromium.org/507533002/diff/60001/LayoutTests/fast/forms/s... LayoutTests/fast/forms/setrangetext-within-events.html:21: if (event == 'mousedown') { is this behavior of clearing for changes in mousedown but not in the other 3 events consistent with other browsers? That seems a little odd to me - perhaps selection changes during mousedown also shouldn't be overwritten? https://codereview.chromium.org/507533002/diff/60001/LayoutTests/fast/forms/s... LayoutTests/fast/forms/setrangetext-within-events.html:28: eventSender.mouseUp(); The first three lines here are duplicated with above - make only the last shouldBe conditional. https://codereview.chromium.org/507533002/diff/60001/LayoutTests/fast/forms/s... LayoutTests/fast/forms/setrangetext-within-events.html:32: eventSender.mouseMoveTo(tx, ty + 100); Is this click here to clear the selection? If so please verify that it actually got cleared. https://codereview.chromium.org/507533002/diff/60001/LayoutTests/fast/forms/s... LayoutTests/fast/forms/setrangetext-within-events.html:39: var textfield = document.getElementById('textfield'); perhaps this function should output a log entry too, just to confirm that the event handler actually got invoked at all (and we're not just keeping the selection from the last time around)? https://codereview.chromium.org/507533002/diff/60001/Source/core/page/EventHa... File Source/core/page/EventHandler.cpp (right): https://codereview.chromium.org/507533002/diff/60001/Source/core/page/EventHa... Source/core/page/EventHandler.cpp:2963: m_selectionChanged = true; It looks to me like m_selectionInitiationState is designed to track something like this: has text selection been modified (in that case by mouse input) during mouse event handling. Perhaps we can just set m_selectionInitiationState = ExtendedSelection here instead of introducing a new field we need to track the state of?
https://codereview.chromium.org/507533002/diff/60001/LayoutTests/fast/forms/s... File LayoutTests/fast/forms/setrangetext-within-events.html (right): https://codereview.chromium.org/507533002/diff/60001/LayoutTests/fast/forms/s... LayoutTests/fast/forms/setrangetext-within-events.html:4: window.onload = function() { On 2014/08/28 18:16:41, Rick Byers wrote: > please add a description() call OK, I will https://codereview.chromium.org/507533002/diff/60001/LayoutTests/fast/forms/s... LayoutTests/fast/forms/setrangetext-within-events.html:21: if (event == 'mousedown') { On 2014/08/28 18:16:41, Rick Byers wrote: > is this behavior of clearing for changes in mousedown but not in the other 3 > events consistent with other browsers? That seems a little odd to me - perhaps > selection changes during mousedown also shouldn't be overwritten? This is intended to work like it. I referred totally to FF's behavior. FF doesn't keep the selection by setSelectionRange() in case of mousedown event. And I agree with their behavior because we can't handle the selection by user if we keep the selection in case of mousedown. https://codereview.chromium.org/507533002/diff/60001/LayoutTests/fast/forms/s... LayoutTests/fast/forms/setrangetext-within-events.html:28: eventSender.mouseUp(); On 2014/08/28 18:16:41, Rick Byers wrote: > The first three lines here are duplicated with above - make only the last > shouldBe conditional. OK, I will https://codereview.chromium.org/507533002/diff/60001/LayoutTests/fast/forms/s... LayoutTests/fast/forms/setrangetext-within-events.html:32: eventSender.mouseMoveTo(tx, ty + 100); On 2014/08/28 18:16:41, Rick Byers wrote: > Is this click here to clear the selection? If so please verify that it actually > got cleared. OK, I will https://codereview.chromium.org/507533002/diff/60001/LayoutTests/fast/forms/s... LayoutTests/fast/forms/setrangetext-within-events.html:39: var textfield = document.getElementById('textfield'); On 2014/08/28 18:16:41, Rick Byers wrote: > perhaps this function should output a log entry too, just to confirm that the > event handler actually got invoked at all (and we're not just keeping the > selection from the last time around)? If I try to output here, we may lost focus of input element, then the selection is cleared. For preventing flaky issue, I don't want to add here any log https://codereview.chromium.org/507533002/diff/60001/Source/core/page/EventHa... File Source/core/page/EventHandler.cpp (right): https://codereview.chromium.org/507533002/diff/60001/Source/core/page/EventHa... Source/core/page/EventHandler.cpp:2963: m_selectionChanged = true; On 2014/08/28 18:16:41, Rick Byers wrote: > It looks to me like m_selectionInitiationState is designed to track something > like this: has text selection been modified (in that case by mouse input) during > mouse event handling. Perhaps we can just set m_selectionInitiationState = > ExtendedSelection here instead of introducing a new field we need to track the > state of? Ok, I think I can use m_selectionInitiationState
myid.o.shin@gmail.com changed reviewers: + kenjibaheux@chromium.org
Patchset #5 (id:80001) has been deleted
Patchset #5 (id:100001) has been deleted
https://codereview.chromium.org/507533002/diff/60001/LayoutTests/fast/forms/s... File LayoutTests/fast/forms/setrangetext-within-events.html (right): https://codereview.chromium.org/507533002/diff/60001/LayoutTests/fast/forms/s... LayoutTests/fast/forms/setrangetext-within-events.html:21: if (event == 'mousedown') { On 2014/08/29 14:18:52, Miyoung Shin(g) wrote: > On 2014/08/28 18:16:41, Rick Byers wrote: > > is this behavior of clearing for changes in mousedown but not in the other 3 > > events consistent with other browsers? That seems a little odd to me - > perhaps > > selection changes during mousedown also shouldn't be overwritten? > This is intended to work like it. > I referred totally to FF's behavior. > FF doesn't keep the selection by setSelectionRange() in case of mousedown event. > And I agree with their behavior because we can't handle the selection by user if > we keep the selection in case of mousedown. Sounds reasonable, thanks. https://codereview.chromium.org/507533002/diff/60001/LayoutTests/fast/forms/s... LayoutTests/fast/forms/setrangetext-within-events.html:39: var textfield = document.getElementById('textfield'); On 2014/08/29 14:18:52, Miyoung Shin(g) wrote: > On 2014/08/28 18:16:41, Rick Byers wrote: > > perhaps this function should output a log entry too, just to confirm that the > > event handler actually got invoked at all (and we're not just keeping the > > selection from the last time around)? > > If I try to output here, we may lost focus of input element, then the selection > is cleared. > For preventing flaky issue, I don't want to add here any log Ok, even better then - how about resetting the selection state at the start of each test case - eg. textfield.setSelectionRange(0,1) so that we can tell from the output that the selection was actually changed from it's dummy initial state?
> https://codereview.chromium.org/507533002/diff/60001/LayoutTests/fast/forms/s... > LayoutTests/fast/forms/setrangetext-within-events.html:39: var textfield = > document.getElementById('textfield'); > On 2014/08/29 14:18:52, Miyoung Shin(g) wrote: > > On 2014/08/28 18:16:41, Rick Byers wrote: > > > perhaps this function should output a log entry too, just to confirm that > the > > > event handler actually got invoked at all (and we're not just keeping the > > > selection from the last time around)? > > > > If I try to output here, we may lost focus of input element, then the > selection > > is cleared. > > For preventing flaky issue, I don't want to add here any log > > Ok, even better then - how about resetting the selection state at the start of > each test case - eg. textfield.setSelectionRange(0,1) so that we can tell from > the output that the selection was actually changed from it's dummy initial > state? OK, I will
This CL might change the mouse dragging event behavior, right? Thus, I suggest to run "git cl try" to confirm layout tests.
On 2014/09/01 01:17:18, yoichio wrote: > This CL might change the mouse dragging event behavior, right? > Thus, I suggest to run "git cl try" to confirm layout tests. Sorry, I couldn't. Because I don't have a permission to run "git cl try". However, I run manually layout tests about fast/event/* & fast/form/* in LayoutTests. There isn't any regression issue.
On 2014/09/01 03:50:46, Miyoung Shin(g) wrote: > On 2014/09/01 01:17:18, yoichio wrote: > > This CL might change the mouse dragging event behavior, right? > > Thus, I suggest to run "git cl try" to confirm layout tests. > > Sorry, I couldn't. Because I don't have a permission to run "git cl try". > However, I run manually layout tests about fast/event/* & fast/form/* in > LayoutTests. > There isn't any regression issue. I've requested try jobs. This seems reasonable to me. Kenji can you (or someone else on your blink-editing team) please comment? If you're happy then I'm happy to stamp this.
https://codereview.chromium.org/507533002/diff/140001/Source/core/page/EventH... File Source/core/page/EventHandler.cpp (right): https://codereview.chromium.org/507533002/diff/140001/Source/core/page/EventH... Source/core/page/EventHandler.cpp:1287: m_selectionInitiationState = HaveNotStartedSelection; Please add a comment here saying why we don't consider selection to start until after the mousedown event (and that it's more compatible with other browsers).
On 2014/09/02 15:39:20, Rick Byers wrote: > https://codereview.chromium.org/507533002/diff/140001/Source/core/page/EventH... > File Source/core/page/EventHandler.cpp (right): > > https://codereview.chromium.org/507533002/diff/140001/Source/core/page/EventH... > Source/core/page/EventHandler.cpp:1287: m_selectionInitiationState = > HaveNotStartedSelection; > Please add a comment here saying why we don't consider selection to start until > after the mousedown event (and that it's more compatible with other browsers). I've uploaded the patch to add the comment.
On 2014/09/03 07:59:44, Miyoung Shin(g) wrote: > On 2014/09/02 15:39:20, Rick Byers wrote: > > > https://codereview.chromium.org/507533002/diff/140001/Source/core/page/EventH... > > File Source/core/page/EventHandler.cpp (right): > > > > > https://codereview.chromium.org/507533002/diff/140001/Source/core/page/EventH... > > Source/core/page/EventHandler.cpp:1287: m_selectionInitiationState = > > HaveNotStartedSelection; > > Please add a comment here saying why we don't consider selection to start > until > > after the mousedown event (and that it's more compatible with other browsers). > > I've uploaded the patch to add the comment. I was hoping to get some input from Kenji, but let's not block any longer. I think the idea is reasonable - we can always revert if it uncovers an issue we didn't anticipate. LGTM.
The CQ bit was checked by rbyers@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/myid.o.shin@gmail.com/507533002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded time limit waiting for builds to trigger.
The CQ bit was checked by rbyers@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/myid.o.shin@gmail.com/507533002/160001
Message was sent while issue was closed.
Committed patchset #7 (id:160001) as 181490
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:160001) has been created in https://codereview.chromium.org/548413002/ by ppi@chromium.org. The reason for reverting is: Suspected cause of flaky failures of org.chromium.content.browser.input.ImeTest on Android bots - see http://crbug.com/411756.
On 2014/09/08 15:56:08, ppi wrote: > A revert of this CL (patchset #7 id:160001) has been created in > https://codereview.chromium.org/548413002/ by mailto:ppi@chromium.org. > > The reason for reverting is: Suspected cause of flaky failures of > org.chromium.content.browser.input.ImeTest on Android bots - see > http://crbug.com/411756. I will check it after the environment setup of android
ppi@, jam@ When I tested org.chromium.content.browser.input.ImeTest on Android, there is still the flaky failures regardless this patch even now. And, I saw that "that cl you reverted landed at Sep 6 12:45:56 2014 UTC. We have been seeing flakes from Sept 2 or 3." (#15 comment at http://crbug.com/411756) Do you think this patch has still effect to that test? Actually, I think it has nothing with that flack issue. if you agree with me, I will re-land this patch, PTAL.
ppi@chromium.org changed reviewers: + fdegans@chromium.org
+Fabrice Go ahead, but please verify that the test doesn't regress after relanding. Thanks!
On 2014/09/13 13:35:42, ppi wrote: > +Fabrice > > Go ahead, but please verify that the test doesn't regress after relanding. > Thanks! LGTM Looks like the test was disabled in the mean time. Your patch needs a rebase, just have it go through android_dbg_tests_recipe to be sure. Thanks!
On 2014/09/13 14:19:40, Fabrice de Gans wrote: > On 2014/09/13 13:35:42, ppi wrote: > > +Fabrice > > > > Go ahead, but please verify that the test doesn't regress after relanding. > > Thanks! > > LGTM > Looks like the test was disabled in the mean time. Your patch needs a rebase, > just have it go through android_dbg_tests_recipe to be sure. > Thanks! Fabrice@, I re-uploaded the patch after rebase. But I don't have a permission to run trybot. could you run them instead of me for android_dbg_tests_recipe? Thanks!
The CQ bit was checked by fdegans@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/507533002/180001
Message was sent while issue was closed.
Committed patchset #8 (id:180001) as 181959 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
