Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2536083002:
SelectionAutoscroll should not happen in user-select:none. (Closed)
DescriptionSelectionAutoscroll should not happen in user-select:none.
AutoscrollForSelection is to help users select content outside the current
viewport. However, the current implementation starts the autoscroll whenever
the mouse-drag event reaches the edge of the viewport, regardless of whether
the user is selecting or not. It makes no sense to start the autoscroll when
the element was not selectable at all. This patch allows the
AutoscrollForSelection fire only when the mouse is dragged across a selectable
element.
BUG=588448
Committed: https://crrev.com/94c73aebe631326d3d09aed291a6c335040b5447
Cr-Commit-Position: refs/heads/master@{#436317}
Patch Set 1 #Patch Set 2 : Clean code #Patch Set 3 : m_mouseDownMayStartAutoscroll #Patch Set 4 : Work for the tests #Patch Set 5 : Add test. #
Total comments: 2
Patch Set 6 : To pass the tests. #
Total comments: 4
Messages
Total messages: 42 (31 generated)
The CQ bit was checked by sunyunjia@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...
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_...)
The CQ bit was checked by sunyunjia@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sunyunjia@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Change && -> || BUG=588448 ========== to ========== SelectionAutoscroll should not happen in user-select:none. AutoscrollForSelection is to help users select content outside the current viewport. However, the current implementation starts the autoscroll whenever the mouse-drag event reaches the edge of the viewport, regardless of whether the user is selecting or not. It makes no sense to start the autoscroll when the element was not selectable at all. This patch allows the AutoscrollForSelection fire only when the mouse is dragged across a selectable element. BUG=588448 ==========
sunyunjia@chromium.org changed reviewers: + bokan@chromium.org
The CQ bit was checked by sunyunjia@chromium.org to run a CQ dry run
PTAL, thanks!
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: This issue passed the CQ dry run.
https://codereview.chromium.org/2536083002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/MouseEventManager.cpp (right): https://codereview.chromium.org/2536083002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/MouseEventManager.cpp:737: layoutObject->isSelectable()) { Selection is created/added by the call below to handleMouseDraggedEvent right? I'm wondering, would it work if we moved this scrolling to happen after that and then just check whether or not we have a selection? I'm worried that this check is too simplistic and there might be all sorts of other cases where we can't select even though the layout object might be selectable. It would be better if we can just rely on the selection system to worry about all the edge cases and use its output to decide whether we should scroll.
The CQ bit was checked by sunyunjia@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sunyunjia@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #6 (id:100001) has been deleted
PTAL, thanks! https://codereview.chromium.org/2536083002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/MouseEventManager.cpp (right): https://codereview.chromium.org/2536083002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/MouseEventManager.cpp:737: layoutObject->isSelectable()) { On 2016/12/02 17:32:38, bokan wrote: > Selection is created/added by the call below to handleMouseDraggedEvent right? > I'm wondering, would it work if we moved this scrolling to happen after that and > then just check whether or not we have a selection? I'm worried that this check > is too simplistic and there might be all sorts of other cases where we can't > select even though the layout object might be selectable. It would be better if > we can just rely on the selection system to worry about all the edge cases and > use its output to decide whether we should scroll. Done.
Question about change to test but otherwise lgtm. https://codereview.chromium.org/2536083002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/forms/text/input-readonly-autoscroll.html (right): https://codereview.chromium.org/2536083002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/forms/text/input-readonly-autoscroll.html:20: eventSender.mouseMoveTo(40, h); Why was this change necessary?
https://codereview.chromium.org/2536083002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/forms/text/input-readonly-autoscroll.html (right): https://codereview.chromium.org/2536083002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/forms/text/input-readonly-autoscroll.html:20: eventSender.mouseMoveTo(40, h); On 2016/12/05 15:39:36, bokan wrote: > Why was this change necessary? Autoscroll can only be triggered AFTER some text has been selected. Directly moving the mouse out of the border can't start autoscroll since no text has been selected at this moment. We need to select something at first.
https://codereview.chromium.org/2536083002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/forms/text/input-readonly-autoscroll.html (right): https://codereview.chromium.org/2536083002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/forms/text/input-readonly-autoscroll.html:20: eventSender.mouseMoveTo(40, h); On 2016/12/05 15:44:39, sunyunjia wrote: > On 2016/12/05 15:39:36, bokan wrote: > > Why was this change necessary? > > Autoscroll can only be triggered AFTER some text has been selected. Directly > moving the mouse out of the border can't start autoscroll since no text has been > selected at this moment. We need to select something at first. Yah, but wouldn't the mouseMove below cause selectioN?
https://codereview.chromium.org/2536083002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/forms/text/input-readonly-autoscroll.html (right): https://codereview.chromium.org/2536083002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/forms/text/input-readonly-autoscroll.html:20: eventSender.mouseMoveTo(40, h); On 2016/12/05 15:45:54, bokan wrote: > On 2016/12/05 15:44:39, sunyunjia wrote: > > On 2016/12/05 15:39:36, bokan wrote: > > > Why was this change necessary? > > > > Autoscroll can only be triggered AFTER some text has been selected. Directly > > moving the mouse out of the border can't start autoscroll since no text has > been > > selected at this moment. We need to select something at first. > > Yah, but wouldn't the mouseMove below cause selectioN? I did some more investigation. The autoscroll is triggered on the node that contains the last known mouse position, if it hasn't been triggered before. In the original test, when things are selected, we've already moved the mouse out of the text field, so the autoscroll is triggered on <body> instead.
On 2016/12/05 16:25:53, sunyunjia wrote: > https://codereview.chromium.org/2536083002/diff/120001/third_party/WebKit/Lay... > File > third_party/WebKit/LayoutTests/fast/forms/text/input-readonly-autoscroll.html > (right): > > https://codereview.chromium.org/2536083002/diff/120001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/fast/forms/text/input-readonly-autoscroll.html:20: > eventSender.mouseMoveTo(40, h); > On 2016/12/05 15:45:54, bokan wrote: > > On 2016/12/05 15:44:39, sunyunjia wrote: > > > On 2016/12/05 15:39:36, bokan wrote: > > > > Why was this change necessary? > > > > > > Autoscroll can only be triggered AFTER some text has been selected. Directly > > > moving the mouse out of the border can't start autoscroll since no text has > > been > > > selected at this moment. We need to select something at first. > > > > Yah, but wouldn't the mouseMove below cause selectioN? > > I did some more investigation. The autoscroll is triggered on the node that > contains the last known mouse position, if it hasn't been triggered before. > In the original test, when things are selected, we've already moved the mouse > out of the text field, so the autoscroll is triggered on <body> instead. Ok, sounds good, thanks for looking into it.
The CQ bit was checked by sunyunjia@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": 120001, "attempt_start_ts": 1480955310366070,
"parent_rev": "b1eaafb4b3f1b19ed042cca19698131ab24cb44a", "commit_rev":
"f1fc90d41020dce11c9e02ce16961f48721ef712"}
Message was sent while issue was closed.
Description was changed from ========== SelectionAutoscroll should not happen in user-select:none. AutoscrollForSelection is to help users select content outside the current viewport. However, the current implementation starts the autoscroll whenever the mouse-drag event reaches the edge of the viewport, regardless of whether the user is selecting or not. It makes no sense to start the autoscroll when the element was not selectable at all. This patch allows the AutoscrollForSelection fire only when the mouse is dragged across a selectable element. BUG=588448 ========== to ========== SelectionAutoscroll should not happen in user-select:none. AutoscrollForSelection is to help users select content outside the current viewport. However, the current implementation starts the autoscroll whenever the mouse-drag event reaches the edge of the viewport, regardless of whether the user is selecting or not. It makes no sense to start the autoscroll when the element was not selectable at all. This patch allows the AutoscrollForSelection fire only when the mouse is dragged across a selectable element. BUG=588448 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== SelectionAutoscroll should not happen in user-select:none. AutoscrollForSelection is to help users select content outside the current viewport. However, the current implementation starts the autoscroll whenever the mouse-drag event reaches the edge of the viewport, regardless of whether the user is selecting or not. It makes no sense to start the autoscroll when the element was not selectable at all. This patch allows the AutoscrollForSelection fire only when the mouse is dragged across a selectable element. BUG=588448 ========== to ========== SelectionAutoscroll should not happen in user-select:none. AutoscrollForSelection is to help users select content outside the current viewport. However, the current implementation starts the autoscroll whenever the mouse-drag event reaches the edge of the viewport, regardless of whether the user is selecting or not. It makes no sense to start the autoscroll when the element was not selectable at all. This patch allows the AutoscrollForSelection fire only when the mouse is dragged across a selectable element. BUG=588448 Committed: https://crrev.com/94c73aebe631326d3d09aed291a6c335040b5447 Cr-Commit-Position: refs/heads/master@{#436317} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/94c73aebe631326d3d09aed291a6c335040b5447 Cr-Commit-Position: refs/heads/master@{#436317} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
