Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2484563003:
Determine the layoutobject of middleClickAutoscroll by the scroll direction. (Closed)
|
Created:
4 years, 1 month ago by sunyunjia Modified:
4 years, 1 month ago Reviewers:
bokan CC:
chromium-reviews, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1, blink-reviews-layout_chromium.org, sof, eae+blinkwatch, leviw+renderwatch, blink-reviews-dom_chromium.org, dglazkov+blink, jchaffraix+rendering, blink-reviews, rwlbuis Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDetermine the layoutobject of middleClickAutoscroll by the scroll direction.
Previously, when middleClickAutoscroll is triggered, the layoutObject to be
scrolled is only determined by the element clicked. However, if the
layoutObject can be scrolled but not in the direction we want, the autoscroll
could not be triggered. In this patch, we determine the layoutObject not only
by the initial click position, but also the initial scroll direction. We also
make sure that when the initial click position is in an iframe, the event will
correctly propagate to its owner element.
BUG=623366
Committed: https://crrev.com/e4af1f04e6a998799e2dab6633518470dc95d826
Cr-Commit-Position: refs/heads/master@{#432534}
Patch Set 1 #Patch Set 2 : Add test. #Patch Set 3 : Rename #
Total comments: 5
Patch Set 4 : Move the logic to AutoscrollController.cpp #
Total comments: 2
Patch Set 5 : Change the variable name #Patch Set 6 : Better tests. #
Messages
Total messages: 41 (29 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: This issue passed the CQ dry run.
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...
Description was changed from ========== Pass the panscrolling to parent node when the current node is not scrollable in that direction. BUG=623366 ========== to ========== Determine the layoutobject of middleClickAutoscroll by the scroll direction. Previously, when middleClickAutoscroll is triggered, the layoutObject to be scrolled is only determined by the element clicked. However, if the layoutObject can be scrolled but not in the direction we want, the autoscroll could not be triggered. In this patch, we determine the layoutObject not only by the initial click position, but also the initial scroll direction. We also make sure that when the initial click position is in an iframe, the event will correctly propagate to its owner element. BUG=623366 ==========
The CQ bit was unchecked by sunyunjia@chromium.org
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...
sunyunjia@chromium.org changed reviewers: + bokan@chromium.org
PTAL, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2484563003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/middleClickAutoscroll-layoutObject.html (right): https://codereview.chromium.org/2484563003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/middleClickAutoscroll-layoutObject.html:21: var startx = 100; It's hard to tell where you're clicking from the raw numbers. Please use getBoundingElement coordinates from an element so it's clearer, or barring that, at least provide comments/name to make it more clear. https://codereview.chromium.org/2484563003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/middleClickAutoscroll-layoutObject.html:28: eventSender.mouseMoveTo(endx, endy) I would also add a followup horizontal scroll to this test to make sure we're still targeting the window. https://codereview.chromium.org/2484563003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/middleClickAutoscroll-layoutObject.html:70: eventSender.mouseUp(1); Is this Down/Up needed? https://codereview.chromium.org/2484563003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/2484563003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBox.cpp:1091: if (!delta.isZero() && !result.didScroll()) { We should put all this logic inside AutoscrollController https://codereview.chromium.org/2484563003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBox.cpp:1102: if (layoutObject->node() && layoutObject->node()->isDocumentNode()) { This will just pick the next scroller up in the hierarchy but that doesn't necessarily mean it's scrollable in the direction. E.g. if we want to scroll vertically but the vertical scroller has two nested horizontal scrollers. I think you need to call scroll() from this loop.
Patchset #4 (id:60001) has been deleted
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...
PTAL, thanks!
Is there some kind of threshold you have to cross before we start autoscrolling in a direction? As in, you have to move x pixels beyond the anchor vertically before we start scrolling vertically? If not, I suspect we may frequently latch to the wrong scroller as most scrolls will include both components. Anyway, we can play with it and fix it later if it's a problem. https://codereview.chromium.org/2484563003/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/middleClickAutoscroll-layoutObject.html (right): https://codereview.chromium.org/2484563003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/middleClickAutoscroll-layoutObject.html:45: if (animationFrameCount == 30) { Why do these constants have to be so high? Shouldn't the animation start in 1 rAF? It could be that it takes 100ms+ to get the next frame, but the meaning of rAF is that it should be called the next time we prepare a frame which is when we tick animations. https://codereview.chromium.org/2484563003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/AutoscrollController.cpp (right): https://codereview.chromium.org/2484563003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/AutoscrollController.cpp:357: m_middleClickAutoscrolled = true; This variable would be more descriptive as m_didLatchForMiddleClickAutoscroll
By the way, for future reference, when you do a rebase/merge it's helpful for reviewers if you first upload the previous patch rebased, then apply your changes and upload that. It's then easier to see the difference between your last patch and the new one. As it is now, a reviewer can see your changes *and* everything else that's changed in a file in the mean time - or your entire patch all over again. (e.g. see my patch here: https://codereview.chromium.org/2480903002/) It's not a problem for this patch since it's not too big (and I sometimes forget to do it myself), but it's helpful and ensures better reviews :) Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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...
On 2016/11/11 19:48:46, bokan wrote: > Is there some kind of threshold you have to cross before we start autoscrolling > in a direction? As in, you have to move x pixels beyond the anchor vertically > before we start scrolling vertically? If not, I suspect we may frequently latch > to the wrong scroller as most scrolls will include both components. Anyway, we > can play with it and fix it later if it's a problem. Yes there is a constant noMiddleClickAutoscrollRadius. > https://codereview.chromium.org/2484563003/diff/80001/third_party/WebKit/Layo... > File > third_party/WebKit/LayoutTests/fast/events/middleClickAutoscroll-layoutObject.html > (right): > > https://codereview.chromium.org/2484563003/diff/80001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/fast/events/middleClickAutoscroll-layoutObject.html:45: > if (animationFrameCount == 30) { > Why do these constants have to be so high? Shouldn't the animation start in 1 > rAF? It could be that it takes 100ms+ to get the next frame, but the meaning of > rAF is that it should be called the next time we prepare a frame which is when > we tick animations. I haven't investigated this why it needs more than 1 frame. The animationFrameCount may not be as high as exactly 30, but lowering it down can make the test fail.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/11/13 23:22:55, sunyunjia wrote: > On 2016/11/11 19:48:46, bokan wrote: > > Is there some kind of threshold you have to cross before we start > autoscrolling > > in a direction? As in, you have to move x pixels beyond the anchor vertically > > before we start scrolling vertically? If not, I suspect we may frequently > latch > > to the wrong scroller as most scrolls will include both components. Anyway, we > > can play with it and fix it later if it's a problem. > > Yes there is a constant noMiddleClickAutoscrollRadius. Got it, thanks. > https://codereview.chromium.org/2484563003/diff/80001/third_party/WebKit/Layo... > > File > > > third_party/WebKit/LayoutTests/fast/events/middleClickAutoscroll-layoutObject.html > > (right): > > > > > https://codereview.chromium.org/2484563003/diff/80001/third_party/WebKit/Layo... > > > third_party/WebKit/LayoutTests/fast/events/middleClickAutoscroll-layoutObject.html:45: > > if (animationFrameCount == 30) { > > Why do these constants have to be so high? Shouldn't the animation start in 1 > > rAF? It could be that it takes 100ms+ to get the next frame, but the meaning > of > > rAF is that it should be called the next time we prepare a frame which is when > > we tick animations. > > I haven't investigated this why it needs more than 1 frame. The > animationFrameCount may not be as high as exactly 30, but lowering it down can > make the test fail. Could you spend a short time investigating how this works? If it fails when you lower the numbers, that means it might fail even with these (bots like MSAN and ASAN run really slow compared to normal bots) and will cause flakes.
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.
On 2016/11/14 14:22:45, bokan wrote: > On 2016/11/13 23:22:55, sunyunjia wrote: > > On 2016/11/11 19:48:46, bokan wrote: > > > Is there some kind of threshold you have to cross before we start > > autoscrolling > > > in a direction? As in, you have to move x pixels beyond the anchor > vertically > > > before we start scrolling vertically? If not, I suspect we may frequently > > latch > > > to the wrong scroller as most scrolls will include both components. Anyway, > we > > > can play with it and fix it later if it's a problem. > > > > Yes there is a constant noMiddleClickAutoscrollRadius. > > Got it, thanks. > > > > https://codereview.chromium.org/2484563003/diff/80001/third_party/WebKit/Layo... > > > File > > > > > > third_party/WebKit/LayoutTests/fast/events/middleClickAutoscroll-layoutObject.html > > > (right): > > > > > > > > > https://codereview.chromium.org/2484563003/diff/80001/third_party/WebKit/Layo... > > > > > > third_party/WebKit/LayoutTests/fast/events/middleClickAutoscroll-layoutObject.html:45: > > > if (animationFrameCount == 30) { > > > Why do these constants have to be so high? Shouldn't the animation start in > 1 > > > rAF? It could be that it takes 100ms+ to get the next frame, but the meaning > > of > > > rAF is that it should be called the next time we prepare a frame which is > when > > > we tick animations. > > > > I haven't investigated this why it needs more than 1 frame. The > > animationFrameCount may not be as high as exactly 30, but lowering it down can > > make the test fail. > > Could you spend a short time investigating how this works? If it fails when you > lower the numbers, that means it might fail even with these (bots like MSAN and > ASAN run really slow compared to normal bots) and will cause flakes. Used .onscroll() instead. PTAL
On 2016/11/16 17:10:49, sunyunjia wrote: > On 2016/11/14 14:22:45, bokan wrote: > > On 2016/11/13 23:22:55, sunyunjia wrote: > > > On 2016/11/11 19:48:46, bokan wrote: > > > > Is there some kind of threshold you have to cross before we start > > > autoscrolling > > > > in a direction? As in, you have to move x pixels beyond the anchor > > vertically > > > > before we start scrolling vertically? If not, I suspect we may frequently > > > latch > > > > to the wrong scroller as most scrolls will include both components. > Anyway, > > we > > > > can play with it and fix it later if it's a problem. > > > > > > Yes there is a constant noMiddleClickAutoscrollRadius. > > > > Got it, thanks. > > > > > > > > https://codereview.chromium.org/2484563003/diff/80001/third_party/WebKit/Layo... > > > > File > > > > > > > > > > third_party/WebKit/LayoutTests/fast/events/middleClickAutoscroll-layoutObject.html > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2484563003/diff/80001/third_party/WebKit/Layo... > > > > > > > > > > third_party/WebKit/LayoutTests/fast/events/middleClickAutoscroll-layoutObject.html:45: > > > > if (animationFrameCount == 30) { > > > > Why do these constants have to be so high? Shouldn't the animation start > in > > 1 > > > > rAF? It could be that it takes 100ms+ to get the next frame, but the > meaning > > > of > > > > rAF is that it should be called the next time we prepare a frame which is > > when > > > > we tick animations. > > > > > > I haven't investigated this why it needs more than 1 frame. The > > > animationFrameCount may not be as high as exactly 30, but lowering it down > can > > > make the test fail. > > > > Could you spend a short time investigating how this works? If it fails when > you > > lower the numbers, that means it might fail even with these (bots like MSAN > and > > ASAN run really slow compared to normal bots) and will cause flakes. > > Used .onscroll() instead. PTAL Thanks, lgtm. Out of curiosity, do you know why waiting an animation frame doesn't work?
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...
Message was sent while issue was closed.
Description was changed from ========== Determine the layoutobject of middleClickAutoscroll by the scroll direction. Previously, when middleClickAutoscroll is triggered, the layoutObject to be scrolled is only determined by the element clicked. However, if the layoutObject can be scrolled but not in the direction we want, the autoscroll could not be triggered. In this patch, we determine the layoutObject not only by the initial click position, but also the initial scroll direction. We also make sure that when the initial click position is in an iframe, the event will correctly propagate to its owner element. BUG=623366 ========== to ========== Determine the layoutobject of middleClickAutoscroll by the scroll direction. Previously, when middleClickAutoscroll is triggered, the layoutObject to be scrolled is only determined by the element clicked. However, if the layoutObject can be scrolled but not in the direction we want, the autoscroll could not be triggered. In this patch, we determine the layoutObject not only by the initial click position, but also the initial scroll direction. We also make sure that when the initial click position is in an iframe, the event will correctly propagate to its owner element. BUG=623366 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Determine the layoutobject of middleClickAutoscroll by the scroll direction. Previously, when middleClickAutoscroll is triggered, the layoutObject to be scrolled is only determined by the element clicked. However, if the layoutObject can be scrolled but not in the direction we want, the autoscroll could not be triggered. In this patch, we determine the layoutObject not only by the initial click position, but also the initial scroll direction. We also make sure that when the initial click position is in an iframe, the event will correctly propagate to its owner element. BUG=623366 ========== to ========== Determine the layoutobject of middleClickAutoscroll by the scroll direction. Previously, when middleClickAutoscroll is triggered, the layoutObject to be scrolled is only determined by the element clicked. However, if the layoutObject can be scrolled but not in the direction we want, the autoscroll could not be triggered. In this patch, we determine the layoutObject not only by the initial click position, but also the initial scroll direction. We also make sure that when the initial click position is in an iframe, the event will correctly propagate to its owner element. BUG=623366 Committed: https://crrev.com/e4af1f04e6a998799e2dab6633518470dc95d826 Cr-Commit-Position: refs/heads/master@{#432534} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/e4af1f04e6a998799e2dab6633518470dc95d826 Cr-Commit-Position: refs/heads/master@{#432534} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
