Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1804063002:
Fix wheel scrolling over scrollbar not scrolling page. (Closed)
DescriptionFix wheel scrolling over scrollbar not scrolling page.
I introduced this bug in my refactoring in r380403. The fix is to simply use the
document node as the element to scroll if the hit test doesn't return a node but
does hit a scrollbar.
BUG=594074
Committed: https://crrev.com/2931e932cacf7c7e7ca1629971c169b5b52c18c0
Cr-Commit-Position: refs/heads/master@{#381533}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Updates from jbroman@ #Patch Set 3 : Made test async #
Messages
Total messages: 25 (8 generated)
Description was changed from ========== Fix wheel scrolling over scrollbar not scrolling page. I introduced this bug in my refactoring in r380403. The fix is to simply use the document node as the element to scroll if the hit test doesn't return a node but does hit a scrollbar. BUG=594074 ========== to ========== Fix wheel scrolling over scrollbar not scrolling page. I introduced this bug in my refactoring in r380403. The fix is to simply use the document node as the element to scroll if the hit test doesn't return a node but does hit a scrollbar. BUG=594074 ==========
bokan@chromium.org changed reviewers: + tdresser@chromium.org
Tim, small fix from my earlier refactorings.
LGTM
dtapuska@chromium.org changed reviewers: + dtapuska@chromium.org
https://codereview.chromium.org/1804063002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/events/wheel-in-scrollbar.html (right): https://codereview.chromium.org/1804063002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/events/wheel-in-scrollbar.html:25: shouldBe('window.scrollY', '30'); This test assumes that scrolling occurs on mouse scrolls. Which isn't true for wheel gestures. A setTimeout(function() {....}, 0); makes it pass for wheel gestures (although it is currently broken but I've fixed it locally)
On 2016/03/15 18:57:18, dtapuska wrote: > https://codereview.chromium.org/1804063002/diff/1/third_party/WebKit/LayoutTe... > File third_party/WebKit/LayoutTests/fast/events/wheel-in-scrollbar.html (right): > > https://codereview.chromium.org/1804063002/diff/1/third_party/WebKit/LayoutTe... > third_party/WebKit/LayoutTests/fast/events/wheel-in-scrollbar.html:25: > shouldBe('window.scrollY', '30'); > This test assumes that scrolling occurs on mouse scrolls. Which isn't true for > wheel gestures. > > A setTimeout(function() {....}, 0); > > makes it pass for wheel gestures (although it is currently broken but I've fixed > it locally) err; nevermind.. event_sender always generates wheel events that scroll even though wheel gestures is enabled on my build.
On 2016/03/15 19:13:24, dtapuska wrote: > On 2016/03/15 18:57:18, dtapuska wrote: > > > https://codereview.chromium.org/1804063002/diff/1/third_party/WebKit/LayoutTe... > > File third_party/WebKit/LayoutTests/fast/events/wheel-in-scrollbar.html > (right): > > > > > https://codereview.chromium.org/1804063002/diff/1/third_party/WebKit/LayoutTe... > > third_party/WebKit/LayoutTests/fast/events/wheel-in-scrollbar.html:25: > > shouldBe('window.scrollY', '30'); > > This test assumes that scrolling occurs on mouse scrolls. Which isn't true for > > wheel gestures. > > > > A setTimeout(function() {....}, 0); > > > > makes it pass for wheel gestures (although it is currently broken but I've > fixed > > it locally) > > err; nevermind.. event_sender always generates wheel events that scroll even > though wheel gestures is enabled on my build. Hmmm, we should probably figure out a good way of testing the wheel gesture case with these existing layout tests.
On 2016/03/15 20:58:40, tdresser wrote: > On 2016/03/15 19:13:24, dtapuska wrote: > > On 2016/03/15 18:57:18, dtapuska wrote: > > > > > > https://codereview.chromium.org/1804063002/diff/1/third_party/WebKit/LayoutTe... > > > File third_party/WebKit/LayoutTests/fast/events/wheel-in-scrollbar.html > > (right): > > > > > > > > > https://codereview.chromium.org/1804063002/diff/1/third_party/WebKit/LayoutTe... > > > third_party/WebKit/LayoutTests/fast/events/wheel-in-scrollbar.html:25: > > > shouldBe('window.scrollY', '30'); > > > This test assumes that scrolling occurs on mouse scrolls. Which isn't true > for > > > wheel gestures. > > > > > > A setTimeout(function() {....}, 0); > > > > > > makes it pass for wheel gestures (although it is currently broken but I've > > fixed > > > it locally) > > > > err; nevermind.. event_sender always generates wheel events that scroll even > > though wheel gestures is enabled on my build. > > Hmmm, we should probably figure out a good way of testing the wheel gesture case > with these existing layout tests. I wrote an explicit test for this in https://codereview.chromium.org/1805063003/
On 2016/03/15 21:00:39, dtapuska wrote: > On 2016/03/15 20:58:40, tdresser wrote: > > On 2016/03/15 19:13:24, dtapuska wrote: > > > On 2016/03/15 18:57:18, dtapuska wrote: > > > > > > > > > > https://codereview.chromium.org/1804063002/diff/1/third_party/WebKit/LayoutTe... > > > > File third_party/WebKit/LayoutTests/fast/events/wheel-in-scrollbar.html > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1804063002/diff/1/third_party/WebKit/LayoutTe... > > > > third_party/WebKit/LayoutTests/fast/events/wheel-in-scrollbar.html:25: > > > > shouldBe('window.scrollY', '30'); > > > > This test assumes that scrolling occurs on mouse scrolls. Which isn't true > > for > > > > wheel gestures. > > > > > > > > A setTimeout(function() {....}, 0); > > > > > > > > makes it pass for wheel gestures (although it is currently broken but I've > > > fixed > > > > it locally) > > > > > > err; nevermind.. event_sender always generates wheel events that scroll even > > > though wheel gestures is enabled on my build. > > > > Hmmm, we should probably figure out a good way of testing the wheel gesture > case > > with these existing layout tests. > > I wrote an explicit test for this in https://codereview.chromium.org/1805063003/ There are a bunch of other existing tests that we aren't currently running with wheel gestures, right? At some point we should come up with a strategy to run them with wheel gestures, which will likely require modifying EventSender.
On 2016/03/16 13:36:30, tdresser wrote: > On 2016/03/15 21:00:39, dtapuska wrote: > > On 2016/03/15 20:58:40, tdresser wrote: > > > On 2016/03/15 19:13:24, dtapuska wrote: > > > > On 2016/03/15 18:57:18, dtapuska wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/1804063002/diff/1/third_party/WebKit/LayoutTe... > > > > > File third_party/WebKit/LayoutTests/fast/events/wheel-in-scrollbar.html > > > > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1804063002/diff/1/third_party/WebKit/LayoutTe... > > > > > third_party/WebKit/LayoutTests/fast/events/wheel-in-scrollbar.html:25: > > > > > shouldBe('window.scrollY', '30'); > > > > > This test assumes that scrolling occurs on mouse scrolls. Which isn't > true > > > for > > > > > wheel gestures. > > > > > > > > > > A setTimeout(function() {....}, 0); > > > > > > > > > > makes it pass for wheel gestures (although it is currently broken but > I've > > > > fixed > > > > > it locally) > > > > > > > > err; nevermind.. event_sender always generates wheel events that scroll > even > > > > though wheel gestures is enabled on my build. > > > > > > Hmmm, we should probably figure out a good way of testing the wheel gesture > > case > > > with these existing layout tests. > > > > I wrote an explicit test for this in > https://codereview.chromium.org/1805063003/ > > There are a bunch of other existing tests that we aren't currently running with > wheel gestures, right? > At some point we should come up with a strategy to run them with wheel gestures, > which will likely require modifying EventSender. +1, no sense in rewriting tests when we already have them.
bokan@chromium.org changed reviewers: + jbroman@chromium.org
+jbroman@ for OWNER
On 2016/03/16 13:36:30, tdresser wrote: > On 2016/03/15 21:00:39, dtapuska wrote: > > On 2016/03/15 20:58:40, tdresser wrote: > > > On 2016/03/15 19:13:24, dtapuska wrote: > > > > On 2016/03/15 18:57:18, dtapuska wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/1804063002/diff/1/third_party/WebKit/LayoutTe... > > > > > File third_party/WebKit/LayoutTests/fast/events/wheel-in-scrollbar.html > > > > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1804063002/diff/1/third_party/WebKit/LayoutTe... > > > > > third_party/WebKit/LayoutTests/fast/events/wheel-in-scrollbar.html:25: > > > > > shouldBe('window.scrollY', '30'); > > > > > This test assumes that scrolling occurs on mouse scrolls. Which isn't > true > > > for > > > > > wheel gestures. > > > > > > > > > > A setTimeout(function() {....}, 0); > > > > > > > > > > makes it pass for wheel gestures (although it is currently broken but > I've > > > > fixed > > > > > it locally) > > > > > > > > err; nevermind.. event_sender always generates wheel events that scroll > even > > > > though wheel gestures is enabled on my build. > > > > > > Hmmm, we should probably figure out a good way of testing the wheel gesture > > case > > > with these existing layout tests. > > > > I wrote an explicit test for this in > https://codereview.chromium.org/1805063003/ > > There are a bunch of other existing tests that we aren't currently running with > wheel gestures, right? > At some point we should come up with a strategy to run them with wheel gestures, > which will likely require modifying EventSender. I took this approach initially; I'll resurrect what I was doing.
https://codereview.chromium.org/1804063002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/events/wheel-in-scrollbar.html (right): https://codereview.chromium.org/1804063002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/events/wheel-in-scrollbar.html:27: finishJSTest(); "finishJSTest()" without "jsTestIsAsync = true;" is surprising; is this intended? https://codereview.chromium.org/1804063002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/1804063002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/EventHandler.cpp:1801: // If we're over a scrollbar, scroll the document. Just the (root?) frame scrollbars, right? Presumably hitting an overflow scrollbar shouldn't scroll the document. (I'm not familiar with what HitTestResult::innerNode will be in such cases, so I don't know if this is just a question about the comment, or also the code.)
https://codereview.chromium.org/1804063002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/events/wheel-in-scrollbar.html (right): https://codereview.chromium.org/1804063002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/events/wheel-in-scrollbar.html:27: finishJSTest(); On 2016/03/16 15:41:00, jbroman wrote: > "finishJSTest()" without "jsTestIsAsync = true;" is surprising; is this > intended? Nope, it was cargo-culted. I've removed it since it's not needed. https://codereview.chromium.org/1804063002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/1804063002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/EventHandler.cpp:1801: // If we're over a scrollbar, scroll the document. On 2016/03/16 15:41:00, jbroman wrote: > Just the (root?) frame scrollbars, right? Presumably hitting an overflow > scrollbar shouldn't scroll the document. > > (I'm not familiar with what HitTestResult::innerNode will be in such cases, so I > don't know if this is just a question about the comment, or also the code.) Yah, if it's over an overflow div innerNode will have the associated Node. I've updated the comment.
lgtm with one remaining comment https://codereview.chromium.org/1804063002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/events/wheel-in-scrollbar.html (right): https://codereview.chromium.org/1804063002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/events/wheel-in-scrollbar.html:27: finishJSTest(); On 2016/03/16 at 18:01:45, bokan wrote: > On 2016/03/16 15:41:00, jbroman wrote: > > "finishJSTest()" without "jsTestIsAsync = true;" is surprising; is this > > intended? > > Nope, it was cargo-culted. I've removed it since it's not needed. Actually, I think you do want jsTestIsAsync and finishJSTest. You'll note that in the previous patchset, you had two "TEST COMPLETE" lines, and in this one you only have one, but it's in the middle. It probably isn't flaky in this case, but the test-complete code should probably only run when the test is actually complete.
https://codereview.chromium.org/1804063002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/events/wheel-in-scrollbar.html (right): https://codereview.chromium.org/1804063002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/events/wheel-in-scrollbar.html:27: finishJSTest(); On 2016/03/16 18:05:52, jbroman wrote: > On 2016/03/16 at 18:01:45, bokan wrote: > > On 2016/03/16 15:41:00, jbroman wrote: > > > "finishJSTest()" without "jsTestIsAsync = true;" is surprising; is this > > > intended? > > > > Nope, it was cargo-culted. I've removed it since it's not needed. > > Actually, I think you do want jsTestIsAsync and finishJSTest. You'll note that > in the previous patchset, you had two "TEST COMPLETE" lines, and in this one you > only have one, but it's in the middle. > > It probably isn't flaky in this case, but the test-complete code should probably > only run when the test is actually complete. Yah, I think you're right. I thought the layout test is "finished" after the load event but I guess that gets run before my handler.
The CQ bit was checked by bokan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdresser@chromium.org, jbroman@chromium.org Link to the patchset: https://codereview.chromium.org/1804063002/#ps40001 (title: "Made test async")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1804063002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1804063002/40001
Message was sent while issue was closed.
Description was changed from ========== Fix wheel scrolling over scrollbar not scrolling page. I introduced this bug in my refactoring in r380403. The fix is to simply use the document node as the element to scroll if the hit test doesn't return a node but does hit a scrollbar. BUG=594074 ========== to ========== Fix wheel scrolling over scrollbar not scrolling page. I introduced this bug in my refactoring in r380403. The fix is to simply use the document node as the element to scroll if the hit test doesn't return a node but does hit a scrollbar. BUG=594074 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Fix wheel scrolling over scrollbar not scrolling page. I introduced this bug in my refactoring in r380403. The fix is to simply use the document node as the element to scroll if the hit test doesn't return a node but does hit a scrollbar. BUG=594074 ========== to ========== Fix wheel scrolling over scrollbar not scrolling page. I introduced this bug in my refactoring in r380403. The fix is to simply use the document node as the element to scroll if the hit test doesn't return a node but does hit a scrollbar. BUG=594074 Committed: https://crrev.com/2931e932cacf7c7e7ca1629971c169b5b52c18c0 Cr-Commit-Position: refs/heads/master@{#381533} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/2931e932cacf7c7e7ca1629971c169b5b52c18c0 Cr-Commit-Position: refs/heads/master@{#381533} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
