Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1049233003:
Keep the selection of the text field when changed by JS. (Closed)
|
Created:
5 years, 8 months ago by Miyoung Shin(g) Modified:
5 years, 5 months ago CC:
blink-reviews, blink-reviews-events_chromium.org, dglazkov+blink, eae+blinkwatch, Donn Denman, yoichio Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionKeep the selection of the text field when changed by JS.
Fixed to keep the selection of the text field in the input
element after setSelectionRange is called by dom events
when triggering the mouse event or the tap event.
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 the
mouse/tap event.
BUG=32865, 429967, 438102, 440086, 457147
R=yosin@chromium.org, rbyers@chromium.org
TEST=
fast/events/touch/gesture/gesture-tap-input-after-composition.html
fast/events/touch/gesture/gesture-tap-reset-selection-range.html
fast/events/touch/gesture/gesture-tap-setrangetext-with-events.html
fast/forms/setrangetext-out-of-range.html
fast/forms/setrangetext-within-events.html
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=198752
Patch Set 1 #
Total comments: 7
Patch Set 2 : #Patch Set 3 : #
Total comments: 15
Patch Set 4 : #
Total comments: 13
Patch Set 5 : #Patch Set 6 : #Messages
Total messages: 25 (5 generated)
PTAL. The patch was reverted at http://crrev.com/965203003. I've uploaded the new patch to fix the problem that the editor is not activated when tapping on the second input box if the compostion exists in first input box. We can reproduce the problem on only Android. Because other implemenations of Aura finishes the composition before conveying the mouse/tap event to blink, but Android not. I agree with the reviewer's opinion at http://crrev.com/1011523002/. I think Blink should handle the selection well even if the composition lives. Finally, I found that my previous patch did not proper to keep the selection's status. The selection can be changed many times when updating the focus to new one. And the latest status of the selection should be kept. This patch added 4 line from the previous patch. + else if (m_frame->selection().isCaret()) + m_selectionInitiationState = PlacedCaret; + else + m_selectionInitiationState = HaveNotStartedSelection; , added TC fast/events/touch/gesture/gesture-tap-input-after-composition.html
rbyers@chromium.org changed reviewers: - donnd@chromium.org
There's clearly (given the 4 different regressions caused by the first attempts to land this) some subtle text selection state management issues here. I don't have enough context in text selection to review this properly myself. The experts are yosin@ and yoichio@. yosin@ can you please take a look, or suggest an alternate way to fix the bug? Improving interoperability with other browsers here certainly seems valuable to me. Is this behavior spec'd anywhere? I'm happy to provide an OWNERS review once a text selection expert is happy with the change.
On 2015/04/09 19:39:46, Rick Byers wrote: > Is this behavior spec'd anywhere? I've never seen it. Selection for INPUT/TEXTAREA and others are isolated concept in the spec and implementation except for Blink/WebKit. In Blink/WebKit, we use one selection implementation for both INPUT/TEXTAREA and content editable thanks by shadow DOM. It seems managing selection state in both EventHandler and FrameSeleciton makes us confusing. I also feel |m_selecitonInitationState| is now having initial state, current state or something. It is burned to update |m_selectionInitationState| in EventHandler from FrameSeleciton. Can we make FrameSelection class as event listener and managing user action? In other words, decoupling EventHandler and FrameSeleciton.
My suggestion is make EventHandler simpler and selection updates in different place, e.g. SelectionController or another better name. Previously, I suggest to move FrameSeleciton, but FrameSelection class is huge too, so we should have another place. https://codereview.chromium.org/1049233003/diff/1/LayoutTests/fast/events/tou... File LayoutTests/fast/events/touch/gesture/gesture-tap-reset-selection-range.html (right): https://codereview.chromium.org/1049233003/diff/1/LayoutTests/fast/events/tou... LayoutTests/fast/events/touch/gesture/gesture-tap-reset-selection-range.html:10: shouldBeGreaterThan('window.getSelection().rangeCount', '0'); nit: Please check equality with 1. Seleciton.rangeCount is either zero or one in Blink/WebKit. https://codereview.chromium.org/1049233003/diff/1/Source/core/page/EventHandl... File Source/core/page/EventHandler.cpp (right): https://codereview.chromium.org/1049233003/diff/1/Source/core/page/EventHandl... Source/core/page/EventHandler.cpp:3133: void EventHandler::notifySelectionChanged() It is better to move selection handling code in EventHandler to FrameSeleciton, or new class SelectionController. I feel strange that FrameSelection needs to notify about selection change to EventHandler. https://codereview.chromium.org/1049233003/diff/1/Source/core/page/EventHandl... Source/core/page/EventHandler.cpp:3136: m_selectionInitiationState = ExtendedSelection; |m_selectionInitiationState| is no longer represent initiation state. It seems it represent current state of selection, but enum fields aren't reflected it.
https://codereview.chromium.org/1049233003/diff/1/Source/core/page/EventHandl... File Source/core/page/EventHandler.cpp (right): https://codereview.chromium.org/1049233003/diff/1/Source/core/page/EventHandl... Source/core/page/EventHandler.cpp:3133: void EventHandler::notifySelectionChanged() On 2015/04/10 02:06:09, Yosi_UTC9 wrote: > It is better to move selection handling code in EventHandler to FrameSeleciton, > or new class SelectionController. I feel strange that FrameSelection needs to > notify about selection change to EventHandler. I've tried to move the code related to the selection from EventHandler to SelectionHandler. I'm looking forward to getting your feedback. https://codereview.chromium.org/1049233003/diff/1/Source/core/page/EventHandl... Source/core/page/EventHandler.cpp:3136: m_selectionInitiationState = ExtendedSelection; On 2015/04/10 02:06:09, Yosi_UTC9 wrote: > |m_selectionInitiationState| is no longer represent initiation state. It seems > it represent current state of selection, but enum fields aren't reflected it. I've changed |m_selectionInitiationState| to |m_selectionState| to be explicit. Could you explain "enum fields aren't reflected it" that you mentioned in more detail ?
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/1049233003/diff/1/Source/core/page/EventHandl... File Source/core/page/EventHandler.cpp (right): https://codereview.chromium.org/1049233003/diff/1/Source/core/page/EventHandl... Source/core/page/EventHandler.cpp:3133: void EventHandler::notifySelectionChanged() On 2015/04/22 15:32:37, Miyoung Shin(g) wrote: > On 2015/04/10 02:06:09, Yosi_UTC9 wrote: > > It is better to move selection handling code in EventHandler to > FrameSeleciton, > > or new class SelectionController. I feel strange that FrameSelection needs to > > notify about selection change to EventHandler. > > I've tried to move the code related to the selection from EventHandler to > SelectionHandler. > I'm looking forward to getting your feedback. Yes, I'm thinking to have a SelectionHandler class. Although, we need to use better name other than SelectionHandler, or SelectionController. https://codereview.chromium.org/1049233003/diff/1/Source/core/page/EventHandl... Source/core/page/EventHandler.cpp:3136: m_selectionInitiationState = ExtendedSelection; On 2015/04/22 15:32:37, Miyoung Shin(g) wrote: > On 2015/04/10 02:06:09, Yosi_UTC9 wrote: > > |m_selectionInitiationState| is no longer represent initiation state. It seems > > it represent current state of selection, but enum fields aren't reflected it. > > I've changed |m_selectionInitiationState| to |m_selectionState| to be explicit. > Could you explain "enum fields aren't reflected it" that you mentioned in more > detail ? I mean |PlacedCaret| and |ExtendSelection| are state after modification rather than initial state. If we name to |m_selectionState|, I'm OK.
On 2015/04/28 01:59:54, Yosi_UTC9 wrote: > https://codereview.chromium.org/1049233003/diff/1/Source/core/page/EventHandl... > File Source/core/page/EventHandler.cpp (right): > > https://codereview.chromium.org/1049233003/diff/1/Source/core/page/EventHandl... > Source/core/page/EventHandler.cpp:3133: void > EventHandler::notifySelectionChanged() > On 2015/04/22 15:32:37, Miyoung Shin(g) wrote: > > On 2015/04/10 02:06:09, Yosi_UTC9 wrote: > > > It is better to move selection handling code in EventHandler to > > FrameSeleciton, > > > or new class SelectionController. I feel strange that FrameSelection needs > to > > > notify about selection change to EventHandler. > > > > I've tried to move the code related to the selection from EventHandler to > > SelectionHandler. > > I'm looking forward to getting your feedback. > > Yes, I'm thinking to have a SelectionHandler class. Although, we need to use > better name other than SelectionHandler, or SelectionController. > > https://codereview.chromium.org/1049233003/diff/1/Source/core/page/EventHandl... > Source/core/page/EventHandler.cpp:3136: m_selectionInitiationState = > ExtendedSelection; > On 2015/04/22 15:32:37, Miyoung Shin(g) wrote: > > On 2015/04/10 02:06:09, Yosi_UTC9 wrote: > > > |m_selectionInitiationState| is no longer represent initiation state. It > seems > > > it represent current state of selection, but enum fields aren't reflected > it. > > > > I've changed |m_selectionInitiationState| to |m_selectionState| to be > explicit. > > Could you explain "enum fields aren't reflected it" that you mentioned in more > > detail ? > > I mean |PlacedCaret| and |ExtendSelection| are state after modification rather > than initial state. If we name to |m_selectionState|, I'm OK. Rick@, what do you think about separating the class from EventHandler for the selection ? PTAL.
On 2015/04/29 11:58:09, Miyoung Shin(g) wrote: > On 2015/04/28 01:59:54, Yosi_UTC9 wrote: > > > https://codereview.chromium.org/1049233003/diff/1/Source/core/page/EventHandl... > > File Source/core/page/EventHandler.cpp (right): > > > > > https://codereview.chromium.org/1049233003/diff/1/Source/core/page/EventHandl... > > Source/core/page/EventHandler.cpp:3133: void > > EventHandler::notifySelectionChanged() > > On 2015/04/22 15:32:37, Miyoung Shin(g) wrote: > > > On 2015/04/10 02:06:09, Yosi_UTC9 wrote: > > > > It is better to move selection handling code in EventHandler to > > > FrameSeleciton, > > > > or new class SelectionController. I feel strange that FrameSelection needs > > to > > > > notify about selection change to EventHandler. > > > > > > I've tried to move the code related to the selection from EventHandler to > > > SelectionHandler. > > > I'm looking forward to getting your feedback. > > > > Yes, I'm thinking to have a SelectionHandler class. Although, we need to use > > better name other than SelectionHandler, or SelectionController. > > > > > https://codereview.chromium.org/1049233003/diff/1/Source/core/page/EventHandl... > > Source/core/page/EventHandler.cpp:3136: m_selectionInitiationState = > > ExtendedSelection; > > On 2015/04/22 15:32:37, Miyoung Shin(g) wrote: > > > On 2015/04/10 02:06:09, Yosi_UTC9 wrote: > > > > |m_selectionInitiationState| is no longer represent initiation state. It > > seems > > > > it represent current state of selection, but enum fields aren't reflected > > it. > > > > > > I've changed |m_selectionInitiationState| to |m_selectionState| to be > > explicit. > > > Could you explain "enum fields aren't reflected it" that you mentioned in > more > > > detail ? > > > > I mean |PlacedCaret| and |ExtendSelection| are state after modification rather > > than initial state. If we name to |m_selectionState|, I'm OK. > > Rick@, > what do you think about separating the class from EventHandler for the selection > ? > PTAL. The idea seems good to me, but I'd prefer we refactorings in a separate CL (first if necessary) which doesn't itself change behavior (as much as that makes sense). As you know behavior changes here tend to be brittle - it'll be easier on all of us if we can land/revert/reland without trying to reapply big refactorings. Also, does it make sense for this new file to live in Source/core/editing? yosin@ is an OWNER there now and is really the right person to review.
On 2015/04/29 18:19:20, Rick Byers wrote: > On 2015/04/29 11:58:09, Miyoung Shin(g) wrote: > > On 2015/04/28 01:59:54, Yosi_UTC9 wrote: > > > > > > https://codereview.chromium.org/1049233003/diff/1/Source/core/page/EventHandl... > > > File Source/core/page/EventHandler.cpp (right): > > > > > > > > > https://codereview.chromium.org/1049233003/diff/1/Source/core/page/EventHandl... > > > Source/core/page/EventHandler.cpp:3133: void > > > EventHandler::notifySelectionChanged() > > > On 2015/04/22 15:32:37, Miyoung Shin(g) wrote: > > > > On 2015/04/10 02:06:09, Yosi_UTC9 wrote: > > > > > It is better to move selection handling code in EventHandler to > > > > FrameSeleciton, > > > > > or new class SelectionController. I feel strange that FrameSelection > needs > > > to > > > > > notify about selection change to EventHandler. > > > > > > > > I've tried to move the code related to the selection from EventHandler to > > > > SelectionHandler. > > > > I'm looking forward to getting your feedback. > > > > > > Yes, I'm thinking to have a SelectionHandler class. Although, we need to use > > > better name other than SelectionHandler, or SelectionController. > > > > > > > > > https://codereview.chromium.org/1049233003/diff/1/Source/core/page/EventHandl... > > > Source/core/page/EventHandler.cpp:3136: m_selectionInitiationState = > > > ExtendedSelection; > > > On 2015/04/22 15:32:37, Miyoung Shin(g) wrote: > > > > On 2015/04/10 02:06:09, Yosi_UTC9 wrote: > > > > > |m_selectionInitiationState| is no longer represent initiation state. It > > > seems > > > > > it represent current state of selection, but enum fields aren't > reflected > > > it. > > > > > > > > I've changed |m_selectionInitiationState| to |m_selectionState| to be > > > explicit. > > > > Could you explain "enum fields aren't reflected it" that you mentioned in > > more > > > > detail ? > > > > > > I mean |PlacedCaret| and |ExtendSelection| are state after modification > rather > > > than initial state. If we name to |m_selectionState|, I'm OK. > > > > Rick@, > > what do you think about separating the class from EventHandler for the > selection > > ? > > PTAL. > > The idea seems good to me, but I'd prefer we refactorings in a separate CL > (first if necessary) which doesn't itself change behavior (as much as that makes > sense). As you know behavior changes here tend to be brittle - it'll be easier > on all of us if we can land/revert/reland without trying to reapply big > refactorings. > > Also, does it make sense for this new file to live in Source/core/editing? > yosin@ is an OWNER there now and is really the right person to review. Yes, we should do re-factoring first without behavior changes, then we fix this bug. Here is road map: 1. Introducing |SelectionController|(TBD) in core/editing/, which is split from |EventHandler|. 2. Change |HTMLTextFormControll| to work with |SelectionController| 3. Change |FrameSelection| to work with |SelectionController|, need to change dispatching "selectionchange" event. Fixing of this bug may be occurred at 2.5 or 3.5. Either way we do re-factoring without behavior change first.
On 2015/04/30 02:08:11, Yosi_UTC9 wrote: > On 2015/04/29 18:19:20, Rick Byers wrote: > > On 2015/04/29 11:58:09, Miyoung Shin(g) wrote: > > > On 2015/04/28 01:59:54, Yosi_UTC9 wrote: > > > > > > > > > > https://codereview.chromium.org/1049233003/diff/1/Source/core/page/EventHandl... > > > > File Source/core/page/EventHandler.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1049233003/diff/1/Source/core/page/EventHandl... > > > > Source/core/page/EventHandler.cpp:3133: void > > > > EventHandler::notifySelectionChanged() > > > > On 2015/04/22 15:32:37, Miyoung Shin(g) wrote: > > > > > On 2015/04/10 02:06:09, Yosi_UTC9 wrote: > > > > > > It is better to move selection handling code in EventHandler to > > > > > FrameSeleciton, > > > > > > or new class SelectionController. I feel strange that FrameSelection > > needs > > > > to > > > > > > notify about selection change to EventHandler. > > > > > > > > > > I've tried to move the code related to the selection from EventHandler > to > > > > > SelectionHandler. > > > > > I'm looking forward to getting your feedback. > > > > > > > > Yes, I'm thinking to have a SelectionHandler class. Although, we need to > use > > > > better name other than SelectionHandler, or SelectionController. > > > > > > > > > > > > > > https://codereview.chromium.org/1049233003/diff/1/Source/core/page/EventHandl... > > > > Source/core/page/EventHandler.cpp:3136: m_selectionInitiationState = > > > > ExtendedSelection; > > > > On 2015/04/22 15:32:37, Miyoung Shin(g) wrote: > > > > > On 2015/04/10 02:06:09, Yosi_UTC9 wrote: > > > > > > |m_selectionInitiationState| is no longer represent initiation state. > It > > > > seems > > > > > > it represent current state of selection, but enum fields aren't > > reflected > > > > it. > > > > > > > > > > I've changed |m_selectionInitiationState| to |m_selectionState| to be > > > > explicit. > > > > > Could you explain "enum fields aren't reflected it" that you mentioned > in > > > more > > > > > detail ? > > > > > > > > I mean |PlacedCaret| and |ExtendSelection| are state after modification > > rather > > > > than initial state. If we name to |m_selectionState|, I'm OK. > > > > > > Rick@, > > > what do you think about separating the class from EventHandler for the > > selection > > > ? > > > PTAL. > > > > The idea seems good to me, but I'd prefer we refactorings in a separate CL > > (first if necessary) which doesn't itself change behavior (as much as that > makes > > sense). As you know behavior changes here tend to be brittle - it'll be > easier > > on all of us if we can land/revert/reland without trying to reapply big > > refactorings. > > > > Also, does it make sense for this new file to live in Source/core/editing? > > yosin@ is an OWNER there now and is really the right person to review. > > Yes, we should do re-factoring first without behavior changes, then we fix this > bug. > Here is road map: > 1. Introducing |SelectionController|(TBD) in core/editing/, which is split from > |EventHandler|. > 2. Change |HTMLTextFormControll| to work with |SelectionController| > 3. Change |FrameSelection| to work with |SelectionController|, need to change > dispatching "selectionchange" event. > > > Fixing of this bug may be occurred at 2.5 or 3.5. Either way we do re-factoring > without behavior change first. yoshin@ May I go ahead with step 1 in a separate CL ? If you are ok with that, then I will upload the patch for step 1 first. After landing the patch for step 1, I will upload the new patch in this CL for fixing the issue.
yosin@, PTAL.
https://codereview.chromium.org/1049233003/diff/60001/LayoutTests/fast/events... File LayoutTests/fast/events/touch/gesture/gesture-tap-reset-selection-range.html (right): https://codereview.chromium.org/1049233003/diff/60001/LayoutTests/fast/events... LayoutTests/fast/events/touch/gesture/gesture-tap-reset-selection-range.html:10: shouldBeGreaterThan('window.getSelection().rangeCount', '0'); We don't need to check this. Other test checks whether |Selection.selectAllChildren()| works or not. https://codereview.chromium.org/1049233003/diff/60001/LayoutTests/fast/events... File LayoutTests/fast/events/touch/gesture/gesture-tap-setrangetext-with-events.html (right): https://codereview.chromium.org/1049233003/diff/60001/LayoutTests/fast/events... LayoutTests/fast/events/touch/gesture/gesture-tap-setrangetext-with-events.html:2: <script src="../../../../resources/js-test.js"></script> This is optional. Do you want to try new framework, W3C test harness? Good thing is we don't need to have expected text. Example: https://chromium.googlesource.com/chromium/blink/+/master/LayoutTests/editing... Just call |test(closure, testName)|, |testName| will be print when failed. Functions: assert_equals(), assert_false(), assert_true(). https://codereview.chromium.org/1049233003/diff/60001/LayoutTests/fast/events... LayoutTests/fast/events/touch/gesture/gesture-tap-setrangetext-with-events.html:14: function doSetSelectionRange(event) { nit: It is better to use |eventType| or |eventName| rather than |event|. It is confusing |event| containing |Event| object. https://codereview.chromium.org/1049233003/diff/60001/LayoutTests/fast/events... LayoutTests/fast/events/touch/gesture/gesture-tap-setrangetext-with-events.html:25: if (event == 'mousedown') { nit: s/==/===/ https://codereview.chromium.org/1049233003/diff/60001/LayoutTests/fast/events... LayoutTests/fast/events/touch/gesture/gesture-tap-setrangetext-with-events.html:26: shouldBe('textfield.selectionStart', '0'); |shouldBeZero()| https://codereview.chromium.org/1049233003/diff/60001/LayoutTests/fast/events... LayoutTests/fast/events/touch/gesture/gesture-tap-setrangetext-with-events.html:27: shouldBe('textfield.selectionEnd', '0'); |shouldBeZero()| https://codereview.chromium.org/1049233003/diff/60001/LayoutTests/fast/events... LayoutTests/fast/events/touch/gesture/gesture-tap-setrangetext-with-events.html:29: shouldBe('textfield.selectionStart', '0'); |shouldBeZero()| https://codereview.chromium.org/1049233003/diff/60001/Source/core/editing/Sel... File Source/core/editing/SelectionController.cpp (right): https://codereview.chromium.org/1049233003/diff/60001/Source/core/editing/Sel... Source/core/editing/SelectionController.cpp:622: { It is better to have a function which maps |SelectionType| to |SelectionState|. Or, use |SelectionType| instead of |SelectionState|. BTW, what difference between |m_selectionState| and |m_frame->selection().selection().selectionType()|? Do we need to keep tracking selection type in both |FrameSelection| and |SelectionController|? https://codereview.chromium.org/1049233003/diff/60001/Source/core/input/Event... File Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/1049233003/diff/60001/Source/core/input/Event... Source/core/input/EventHandler.cpp:952: // m_selectionInitiationState is initialized after dispatching mousedown event in order not to keep the selection by DOM APIs nit: Could you try to keep comments in 80 chars? It makes some tools happy, e.g. codereview.chromium.org, git log and so on.
https://codereview.chromium.org/1049233003/diff/60001/LayoutTests/fast/events... File LayoutTests/fast/events/touch/gesture/gesture-tap-reset-selection-range.html (right): https://codereview.chromium.org/1049233003/diff/60001/LayoutTests/fast/events... LayoutTests/fast/events/touch/gesture/gesture-tap-reset-selection-range.html:10: shouldBeGreaterThan('window.getSelection().rangeCount', '0'); On 2015/07/06 01:57:29, Yosi_UTC9 wrote: > We don't need to check this. Other test checks whether > |Selection.selectAllChildren()| works or not. Done. https://codereview.chromium.org/1049233003/diff/60001/LayoutTests/fast/events... File LayoutTests/fast/events/touch/gesture/gesture-tap-setrangetext-with-events.html (right): https://codereview.chromium.org/1049233003/diff/60001/LayoutTests/fast/events... LayoutTests/fast/events/touch/gesture/gesture-tap-setrangetext-with-events.html:2: <script src="../../../../resources/js-test.js"></script> On 2015/07/06 01:57:30, Yosi_UTC9 wrote: > This is optional. > > Do you want to try new framework, W3C test harness? Good thing is we don't need > to have expected text. > > Example: > https://chromium.googlesource.com/chromium/blink/+/master/LayoutTests/editing... > > Just call |test(closure, testName)|, |testName| will be print when failed. > > Functions: assert_equals(), assert_false(), assert_true(). > I've changed all test cases of this patch to W3C test harness. Thanks for the good information. https://codereview.chromium.org/1049233003/diff/60001/LayoutTests/fast/events... LayoutTests/fast/events/touch/gesture/gesture-tap-setrangetext-with-events.html:14: function doSetSelectionRange(event) { On 2015/07/06 01:57:29, Yosi_UTC9 wrote: > nit: It is better to use |eventType| or |eventName| rather than |event|. > It is confusing |event| containing |Event| object. Done. https://codereview.chromium.org/1049233003/diff/60001/LayoutTests/fast/events... LayoutTests/fast/events/touch/gesture/gesture-tap-setrangetext-with-events.html:25: if (event == 'mousedown') { On 2015/07/06 01:57:29, Yosi_UTC9 wrote: > nit: s/==/===/ Done. I've changed it to === operator. https://codereview.chromium.org/1049233003/diff/60001/Source/core/editing/Sel... File Source/core/editing/SelectionController.cpp (right): https://codereview.chromium.org/1049233003/diff/60001/Source/core/editing/Sel... Source/core/editing/SelectionController.cpp:622: { On 2015/07/06 01:57:30, Yosi_UTC9 wrote: > It is better to have a function which maps |SelectionType| to |SelectionState|. > Or, use |SelectionType| instead of |SelectionState|. > Done. > BTW, what difference between |m_selectionState| and > |m_frame->selection().selection().selectionType()|? Do we need to keep tracking > selection type in both |FrameSelection| and |SelectionController|? > IIUC, |m_selectionState| works together for events so it can be changed to the range status as |m_frame->selection().selection().selectionType()| is not the range yet. eg. when starting dragging the mouse on text, |m_selectionState| is set to |SelectionState::ExtendedSelection| first in |SelectionController::updateSelectionForMouseDragAlgorithm|. At that time, |selectionType()| is still |SelectionType::CaretSelection|. And then, when handling the next mouse event we can keep the selection range. Also, even if |selectionType()| is |SelectionType::RangeSelection| when handling the mouse release event, we can clear the selection through |m_selectionState|. https://codereview.chromium.org/1049233003/diff/60001/Source/core/input/Event... File Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/1049233003/diff/60001/Source/core/input/Event... Source/core/input/EventHandler.cpp:952: // m_selectionInitiationState is initialized after dispatching mousedown event in order not to keep the selection by DOM APIs On 2015/07/06 01:57:30, Yosi_UTC9 wrote: > nit: Could you try to keep comments in 80 chars? It makes some tools happy, e.g. > http://codereview.chromium.org, git log and so on. Done.
https://codereview.chromium.org/1049233003/diff/80001/LayoutTests/fast/events... File LayoutTests/fast/events/touch/gesture/gesture-tap-input-after-composition.html (right): https://codereview.chromium.org/1049233003/diff/80001/LayoutTests/fast/events... LayoutTests/fast/events/touch/gesture/gesture-tap-input-after-composition.html:10: if (window.eventSender) { nit: Could you use early return style? e.g. if (!window.eventSender) return; https://codereview.chromium.org/1049233003/diff/80001/LayoutTests/fast/events... File LayoutTests/fast/events/touch/gesture/gesture-tap-reset-selection-range.html (right): https://codereview.chromium.org/1049233003/diff/80001/LayoutTests/fast/events... LayoutTests/fast/events/touch/gesture/gesture-tap-reset-selection-range.html:7: if (window.eventSender) { nit: You can move |if (window.eventSender) ...| to the closure to pass |test()|. https://codereview.chromium.org/1049233003/diff/80001/LayoutTests/fast/events... LayoutTests/fast/events/touch/gesture/gesture-tap-reset-selection-range.html:19: if (window.testRunner) nit: I think we don't need to hide HTML with w3c test harness. https://codereview.chromium.org/1049233003/diff/80001/LayoutTests/fast/forms/... File LayoutTests/fast/forms/setrangetext-out-of-range.html (right): https://codereview.chromium.org/1049233003/diff/80001/LayoutTests/fast/forms/... LayoutTests/fast/forms/setrangetext-out-of-range.html:11: if (window.eventSender) { nit: Could you use early return pattern? https://codereview.chromium.org/1049233003/diff/80001/LayoutTests/fast/forms/... File LayoutTests/fast/forms/setrangetext-within-events.html (right): https://codereview.chromium.org/1049233003/diff/80001/LayoutTests/fast/forms/... LayoutTests/fast/forms/setrangetext-within-events.html:7: if (window.eventSender) { nit: Could you use early return pattern?
https://codereview.chromium.org/1049233003/diff/80001/LayoutTests/fast/events... File LayoutTests/fast/events/touch/gesture/gesture-tap-input-after-composition.html (right): https://codereview.chromium.org/1049233003/diff/80001/LayoutTests/fast/events... LayoutTests/fast/events/touch/gesture/gesture-tap-input-after-composition.html:10: if (window.eventSender) { On 2015/07/13 03:47:22, Yosi_UTC9 wrote: > nit: Could you use early return style? > e.g. > if (!window.eventSender) > return; Done. https://codereview.chromium.org/1049233003/diff/80001/LayoutTests/fast/events... File LayoutTests/fast/events/touch/gesture/gesture-tap-reset-selection-range.html (right): https://codereview.chromium.org/1049233003/diff/80001/LayoutTests/fast/events... LayoutTests/fast/events/touch/gesture/gesture-tap-reset-selection-range.html:7: if (window.eventSender) { On 2015/07/13 03:47:22, Yosi_UTC9 wrote: > nit: You can move |if (window.eventSender) ...| to the closure to pass |test()|. Done. https://codereview.chromium.org/1049233003/diff/80001/LayoutTests/fast/events... LayoutTests/fast/events/touch/gesture/gesture-tap-reset-selection-range.html:19: if (window.testRunner) On 2015/07/13 03:47:22, Yosi_UTC9 wrote: > nit: I think we don't need to hide HTML with w3c test harness. If we remove this, we would need the expected result because there is the text, "Make Selection Here", in TC. If I'm wrong, could you correct me? https://codereview.chromium.org/1049233003/diff/80001/LayoutTests/fast/forms/... File LayoutTests/fast/forms/setrangetext-out-of-range.html (right): https://codereview.chromium.org/1049233003/diff/80001/LayoutTests/fast/forms/... LayoutTests/fast/forms/setrangetext-out-of-range.html:11: if (window.eventSender) { On 2015/07/13 03:47:22, Yosi_UTC9 wrote: > nit: Could you use early return pattern? Done. https://codereview.chromium.org/1049233003/diff/80001/LayoutTests/fast/forms/... File LayoutTests/fast/forms/setrangetext-within-events.html (right): https://codereview.chromium.org/1049233003/diff/80001/LayoutTests/fast/forms/... LayoutTests/fast/forms/setrangetext-within-events.html:7: if (window.eventSender) { On 2015/07/13 03:47:22, Yosi_UTC9 wrote: > nit: Could you use early return pattern? Done.
yosin@chromium.org changed reviewers: + tkent@chromium.org
lgtm for core/editing/ and tests +tkent@ for EventHandler.cpp changes. https://codereview.chromium.org/1049233003/diff/80001/LayoutTests/fast/events... File LayoutTests/fast/events/touch/gesture/gesture-tap-reset-selection-range.html (right): https://codereview.chromium.org/1049233003/diff/80001/LayoutTests/fast/events... LayoutTests/fast/events/touch/gesture/gesture-tap-reset-selection-range.html:19: if (window.testRunner) On 2015/07/13 04:23:49, Miyoung Shin(g) wrote: > On 2015/07/13 03:47:22, Yosi_UTC9 wrote: > > nit: I think we don't need to hide HTML with w3c test harness. > > If we remove this, we would need the expected result because there is the text, > "Make Selection Here", in TC. > If I'm wrong, could you correct me? I don't think so. Other test files also have rendered HTML fragments.
EventHandler lgtm https://codereview.chromium.org/1049233003/diff/80001/LayoutTests/fast/events... File LayoutTests/fast/events/touch/gesture/gesture-tap-reset-selection-range.html (right): https://codereview.chromium.org/1049233003/diff/80001/LayoutTests/fast/events... LayoutTests/fast/events/touch/gesture/gesture-tap-reset-selection-range.html:19: if (window.testRunner) On 2015/07/13 04:39:56, Yosi_UTC9 wrote: > On 2015/07/13 04:23:49, Miyoung Shin(g) wrote: > > On 2015/07/13 03:47:22, Yosi_UTC9 wrote: > > > nit: I think we don't need to hide HTML with w3c test harness. > > > > If we remove this, we would need the expected result because there is the > text, > > "Make Selection Here", in TC. > > If I'm wrong, could you correct me? > > I don't think so. Other test files also have rendered HTML fragments. We need to add <div id=log></div> to hide the non-result text correctly. See LayoutTests/resources/testharnessreport.js.
https://codereview.chromium.org/1049233003/diff/80001/LayoutTests/fast/events... File LayoutTests/fast/events/touch/gesture/gesture-tap-reset-selection-range.html (right): https://codereview.chromium.org/1049233003/diff/80001/LayoutTests/fast/events... LayoutTests/fast/events/touch/gesture/gesture-tap-reset-selection-range.html:19: if (window.testRunner) On 2015/07/13 04:51:10, tkent wrote: > On 2015/07/13 04:39:56, Yosi_UTC9 wrote: > > On 2015/07/13 04:23:49, Miyoung Shin(g) wrote: > > > On 2015/07/13 03:47:22, Yosi_UTC9 wrote: > > > > nit: I think we don't need to hide HTML with w3c test harness. > > > > > > If we remove this, we would need the expected result because there is the > > text, > > > "Make Selection Here", in TC. > > > If I'm wrong, could you correct me? > > > > I don't think so. Other test files also have rendered HTML fragments. > > We need to add <div id=log></div> to hide the non-result text correctly. See > LayoutTests/resources/testharnessreport.js. yosin@, tkent@, Thanks, I also found the document of testharness at http://testthewebforward.org/docs/testharness-library.html. It would be helpful if I need to add the new TC. :)
The CQ bit was checked by myid.o.shin@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org, yosin@chromium.org Link to the patchset: https://codereview.chromium.org/1049233003/#ps120001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1049233003/120001
Message was sent while issue was closed.
Committed patchset #6 (id:120001) as https://src.chromium.org/viewvc/blink?view=rev&revision=198752 |
