Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2646963002:
Stop dismissing selection handles when selection is kept (Closed)
|
Created:
3 years, 11 months ago by Changwan Ryu Modified:
3 years, 10 months ago CC:
aelias_OOO_until_Jul13, blink-reviews, chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionStop dismissing selection handles when selection is kept
When we finish composing text and keep selection, selection handles get
dismissed because of intermediate selection changes, even though the final
selection will not change.
BUG=630137
Review-Url: https://codereview.chromium.org/2646963002
Cr-Commit-Position: refs/heads/master@{#448875}
Committed: https://chromium.googlesource.com/chromium/src/+/29810350d256c855e8d1af4c715b28da3a992336
Patch Set 1 #
Total comments: 12
Patch Set 2 : avoid control flow in IMC and rebase #
Total comments: 14
Patch Set 3 : rebased on amaralp@'s CLs #
Messages
Total messages: 30 (16 generated)
The CQ bit was checked by changwan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
yosin@chromium.org changed reviewers: + yosin@chromium.org
Thanks quick work! It is nearly what I imagine, see comments for direction. https://codereview.chromium.org/2646963002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/2646963002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/FrameSelection.cpp:195: (options & HandleVisible || s.isHandleVisible()) We don't need to have |SelectionOption::HandleVisible| anymore. Selection handle visibility is the first class citizen, |newSelection|:SelectionTemplate<DOMTree> carries selection handle visibility. https://codereview.chromium.org/2646963002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2646963002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/InputMethodController.cpp:270: if (isHandleVisible) This line should be include in L268. One of benefit to make SelectionTemplate to have handle visibility is avoiding control flow. const SelectionInDOMTree& selection = SelectionInDOMTree::Builder() .setBaseAndExtent(oldSelectionRange) .setIsHandVisbile(isHandleVisible) .build(); This is equivalent to your change. https://codereview.chromium.org/2646963002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/InputMethodController.cpp:711: FrameSelection::SetSelectionOptions options) { Please avoid to use FrameSelection::SetSelectionOptions. It is ugly FrameSelection::setSelection() parameter which we should get rid of. https://codereview.chromium.org/2646963002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/VisibleSelection.h (right): https://codereview.chromium.org/2646963002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/VisibleSelection.h:125: bool isHandleVisible() const { return m_isHandleVisible; } We don't need to make |VisibleSelection| to have |isHandleVisible()|. It is only for |SelectionTemplate| and |FrameSelection|. [1] will get rid of |m_isHandleVisible| from |FrameSelection|, Since |SelectionEditor| hold |m_seleciton|:SelectionTemplate<DOMTree> with |m_isHandleVisible|. [1] 2637013002: WIP Make FrameSelection to hold non-canonicalized positions
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
amaralp@chromium.org changed reviewers: + amaralp@chromium.org
https://codereview.chromium.org/2646963002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/VisibleSelection.h (right): https://codereview.chromium.org/2646963002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/VisibleSelection.h:125: bool isHandleVisible() const { return m_isHandleVisible; } On 2017/01/20 at 07:54:07, Yosi_UTC9 wrote: > We don't need to make |VisibleSelection| to have |isHandleVisible()|. > It is only for |SelectionTemplate| and |FrameSelection|. How will |FrameSelection::setSelectionAlgorithm()| know if the handle is visible if |VisibleSelection| doesn't have |isHandleVisible()|?
On 2017/01/24 at 03:38:29, amaralp wrote: > https://codereview.chromium.org/2646963002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/editing/VisibleSelection.h (right): > > https://codereview.chromium.org/2646963002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/editing/VisibleSelection.h:125: bool isHandleVisible() const { return m_isHandleVisible; } > On 2017/01/20 at 07:54:07, Yosi_UTC9 wrote: > > We don't need to make |VisibleSelection| to have |isHandleVisible()|. > > It is only for |SelectionTemplate| and |FrameSelection|. > How will |FrameSelection::setSelectionAlgorithm()| know if the handle is visible if > |VisibleSelection| doesn't have |isHandleVisible()|? We keep FrameSelection::m_isHandleVisible. So, FrameSelection can use it. Note: FrameSelection, actually SelectionEditor, will hold SelectionInDOMTree instead of VisibleSelection. VisibleSelection will be deprecated and it is used only for document.execCommand.
The CQ bit was checked by changwan@chromium.org to run a CQ dry run
https://codereview.chromium.org/2646963002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/2646963002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/FrameSelection.cpp:195: (options & HandleVisible || s.isHandleVisible()) On 2017/01/20 07:54:07, Yosi_UTC9 wrote: > We don't need to have |SelectionOption::HandleVisible| anymore. Selection handle > visibility is the first class citizen, |newSelection|:SelectionTemplate<DOMTree> > carries selection handle visibility. I just wanted to minimize the scope of this CL to avoid conflict with amaralp@’s future patch. amaralp@, please upload a new patch if you have one. I’ll try to make my patch depend on it if possible. Or I could wait for your patch to land first. https://codereview.chromium.org/2646963002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2646963002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/InputMethodController.cpp:270: if (isHandleVisible) On 2017/01/20 07:54:07, Yosi_UTC9 wrote: > This line should be include in L268. One of benefit to make SelectionTemplate to > have handle visibility is avoiding control flow. > > const SelectionInDOMTree& selection = > SelectionInDOMTree::Builder() > .setBaseAndExtent(oldSelectionRange) > .setIsHandVisbile(isHandleVisible) > .build(); > > > This is equivalent to your change. > > > Done. https://codereview.chromium.org/2646963002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/InputMethodController.cpp:711: FrameSelection::SetSelectionOptions options) { On 2017/01/20 07:54:07, Yosi_UTC9 wrote: > Please avoid to use FrameSelection::SetSelectionOptions. It is ugly > FrameSelection::setSelection() parameter which we should get rid of. Hmm... Most cases passing CloseTyping is necessary, but we should not do that in the middle of setComposition, according to https://codereview.chromium.org/2206923002/ .,We still need this parameter for setComposition. https://codereview.chromium.org/2646963002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/VisibleSelection.h (right): https://codereview.chromium.org/2646963002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/VisibleSelection.h:125: bool isHandleVisible() const { return m_isHandleVisible; } On 2017/01/24 03:38:29, amaralp wrote: > On 2017/01/20 at 07:54:07, Yosi_UTC9 wrote: > > We don't need to make |VisibleSelection| to have |isHandleVisible()|. > > It is only for |SelectionTemplate| and |FrameSelection|. > How will |FrameSelection::setSelectionAlgorithm()| know if the handle is visible > if > |VisibleSelection| doesn't have |isHandleVisible()|? Hmm… Although we want to deprecate VisibleSelection in the future, the current setSelectionAlgorithm function takes VisibleSelection as a parameter. FrameSelection holds on to the previous isHandleVisible value while VisibleSelection parameter keeps the new isHandleVisible value. It is hard to imagine how to pass the new value to setSelectionAlgorithm other than through VisibleSelection with the current structure. Please advise.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2646963002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2646963002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/InputMethodController.cpp:711: FrameSelection::SetSelectionOptions options) { On 2017/01/24 at 06:38:45, Changwan Ryu wrote: > On 2017/01/20 07:54:07, Yosi_UTC9 wrote: > > Please avoid to use FrameSelection::SetSelectionOptions. It is ugly > > FrameSelection::setSelection() parameter which we should get rid of. > > Hmm... Most cases passing CloseTyping is necessary, but we should not do that in the middle of setComposition, according to https://codereview.chromium.org/2206923002/ .,We still need this parameter for setComposition. I see. https://codereview.chromium.org/2646963002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/VisibleSelection.h (right): https://codereview.chromium.org/2646963002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/VisibleSelection.h:125: bool isHandleVisible() const { return m_isHandleVisible; } On 2017/01/24 at 06:38:45, Changwan Ryu wrote: > On 2017/01/24 03:38:29, amaralp wrote: > > On 2017/01/20 at 07:54:07, Yosi_UTC9 wrote: > > > We don't need to make |VisibleSelection| to have |isHandleVisible()|. > > > It is only for |SelectionTemplate| and |FrameSelection|. > > How will |FrameSelection::setSelectionAlgorithm()| know if the handle is visible > > if > > |VisibleSelection| doesn't have |isHandleVisible()|? > > Hmm… Although we want to deprecate VisibleSelection in the future, the current setSelectionAlgorithm function takes VisibleSelection as a parameter. FrameSelection holds on to the previous isHandleVisible value while VisibleSelection parameter keeps the new isHandleVisible value. It is hard to imagine how to pass the new value to setSelectionAlgorithm other than through VisibleSelection with the current structure. Please advise. Just pass |isHandleVisbile| as a parameter of setSeeltionAlgorithm<T>, like: void FrameSelection::setSelectionAlgorithm( const VisibleSelectionTemplate<Strategy>& newSelection, bool isHandleVisbile, SetSelectionOptions options, CursorAlignOnScroll align, TextGranularity granularity) { ... }
https://codereview.chromium.org/2646963002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/2646963002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/FrameSelection.cpp:195: (options & HandleVisible || s.isHandleVisible()) On 2017/01/24 at 06:38:45, Changwan Ryu wrote: > On 2017/01/20 07:54:07, Yosi_UTC9 wrote: > > We don't need to have |SelectionOption::HandleVisible| anymore. Selection handle > > visibility is the first class citizen, |newSelection|:SelectionTemplate<DOMTree> > > carries selection handle visibility. > > I just wanted to minimize the scope of this CL to avoid conflict with amaralp@’s future patch. > amaralp@, please upload a new patch if you have one. I’ll try to make my patch depend on it if possible. Or I could wait for your patch to land first. I uploaded a patch crrev.com/2647283006.
https://codereview.chromium.org/2646963002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2646963002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:253: bool isHandleVisible = frame().selection().isHandleVisible(); s/bool/const bool/ https://codereview.chromium.org/2646963002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:254: EphemeralRange oldSelectionRange = s/EphemeralRange/const EphemeralRange&/ https://codereview.chromium.org/2646963002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:273: frame().selection().setSelection(selection, FrameSelection::CloseTyping); We'll get rid of |CloseTyping| option[1] [1] http://crbug.com/684364 ⚐ We should get rid of SetSelecitonOption CloseTyping and ClearTypingStyle in FrameSelection https://codereview.chromium.org/2646963002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:708: bool InputMethodController::setSelectionOffsets( It is not you due, but we miss comment for |bool| return value. Could you add comment about |bool| return value. Since you change signature of this. Thanks! https://codereview.chromium.org/2646963002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/SelectionTemplate.cpp (right): https://codereview.chromium.org/2646963002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/SelectionTemplate.cpp:20: m_isHandleVisible(other.m_isHandleVisible) Please move this change in another patch and test cases in "SelectionTemplate.cpp" https://codereview.chromium.org/2646963002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/VisibleSelection.cpp (right): https://codereview.chromium.org/2646963002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleSelection.cpp:51: m_isHandleVisible(false) {} Please don't add |m_isHandleVisible| to |VisibleSelection|. We hold |m_isHandleVisibile| in |FrameSelection|. https://codereview.chromium.org/2646963002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/VisibleSelection.h (right): https://codereview.chromium.org/2646963002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleSelection.h:199: bool m_isHandleVisible : 1; // True if handle should be visible. Please don't add |m_isHandleVisible| to |VisibleSelection|. We hold |m_isHandleVisibile| in |FrameSelection|.
Let me upload a new patch after https://codereview.chromium.org/2647283006 and https://codereview.chromium.org/2664443002 land first.
On 2017/02/02 at 06:42:51, changwan wrote: > Let me upload a new patch after > https://codereview.chromium.org/2647283006 and > https://codereview.chromium.org/2664443002 > land first. The two CL's have landed. You can continue with your patch.
The CQ bit was checked by changwan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL amaralp@: Thanks for fixing them! https://codereview.chromium.org/2646963002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/2646963002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:253: bool isHandleVisible = frame().selection().isHandleVisible(); On 2017/01/25 03:49:08, yosin_BlinkOn_slow wrote: > s/bool/const bool/ Done. https://codereview.chromium.org/2646963002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:254: EphemeralRange oldSelectionRange = On 2017/01/25 03:49:08, yosin_BlinkOn_slow wrote: > s/EphemeralRange/const EphemeralRange&/ Done. https://codereview.chromium.org/2646963002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:273: frame().selection().setSelection(selection, FrameSelection::CloseTyping); On 2017/01/25 03:49:08, yosin_BlinkOn_slow wrote: > We'll get rid of |CloseTyping| option[1] > > [1] http://crbug.com/684364 ⚐ We should get rid of SetSelecitonOption > CloseTyping and ClearTypingStyle in FrameSelection Acknowledged. https://codereview.chromium.org/2646963002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:708: bool InputMethodController::setSelectionOffsets( On 2017/01/25 03:49:08, yosin_BlinkOn_slow wrote: > It is not you due, but we miss comment for |bool| return value. > Could you add comment about |bool| return value. Since you change signature of > this. > Thanks! Done. https://codereview.chromium.org/2646963002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/SelectionTemplate.cpp (right): https://codereview.chromium.org/2646963002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/SelectionTemplate.cpp:20: m_isHandleVisible(other.m_isHandleVisible) On 2017/01/25 03:49:08, yosin_BlinkOn_slow wrote: > Please move this change in another patch and test cases in > "SelectionTemplate.cpp" This is fixed in amaralp@'s patch. https://codereview.chromium.org/2646963002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/VisibleSelection.cpp (right): https://codereview.chromium.org/2646963002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleSelection.cpp:51: m_isHandleVisible(false) {} On 2017/01/25 03:49:08, yosin_BlinkOn_slow wrote: > Please don't add |m_isHandleVisible| to |VisibleSelection|. > We hold |m_isHandleVisibile| in |FrameSelection|. Done. https://codereview.chromium.org/2646963002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/VisibleSelection.h (right): https://codereview.chromium.org/2646963002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/VisibleSelection.h:199: bool m_isHandleVisible : 1; // True if handle should be visible. On 2017/01/25 03:49:08, yosin_BlinkOn_slow wrote: > Please don't add |m_isHandleVisible| to |VisibleSelection|. > We hold |m_isHandleVisibile| in |FrameSelection|. Done.
lgtm
The CQ bit was unchecked by changwan@chromium.org
The CQ bit was checked by changwan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1486518852771730,
"parent_rev": "16f3d2401ca5d2e68d956507a86d57fb3b60d350", "commit_rev":
"29810350d256c855e8d1af4c715b28da3a992336"}
Message was sent while issue was closed.
Description was changed from ========== Stop dismissing selection handles when selection is kept When we finish composing text and keep selection, selection handles get dismissed because of intermediate selection changes, even though the final selection will not change. BUG=630137 ========== to ========== Stop dismissing selection handles when selection is kept When we finish composing text and keep selection, selection handles get dismissed because of intermediate selection changes, even though the final selection will not change. BUG=630137 Review-Url: https://codereview.chromium.org/2646963002 Cr-Commit-Position: refs/heads/master@{#448875} Committed: https://chromium.googlesource.com/chromium/src/+/29810350d256c855e8d1af4c715b... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/29810350d256c855e8d1af4c715b... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
