Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
296003011:
Fix event passing to overlay scrollbars when over a plugin (Closed)
|
Created:
6 years, 7 months ago by raymes Modified:
6 years, 6 months ago CC:
blink-reviews, blink-reviews-events_chromium.org, blink-reviews-rendering, bokan, chrome-apps-syd-reviews_chromium.org, dglazkov+blink, eae+blinkwatch, jamesr, jchaffraix+rendering, leviw+renderwatch, pdr., piman, rune+blink, weiliangc, zoltan1 Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionFix event passing to overlay scrollbars when over a plugin
There are two fixes here:
1) Previously frame scrollbars never had a chance to handle an event if an element or event handler swallowed the event. So, for example, if a mousedown event listener was added to the window and the handler called preventDefault(), then the frame scrollbar would never receive mouse clicks. This change always gives frame scrollbars an opportunity to handle the event. This would also affect elements (such as plugins) that lie underneath overlay scrollbars and swallowed events.
2) Previously if an overlay scrollbar was above a plugin, clicking on the scrollbar could trigger mouse capture on the plugin, which would interfere with the scrollbar receiving events. This change prevents mouse capture on a plugin from starting if the event was initiated above a scrollbar.
BUG=369898, 358248
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175857
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 2
Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #
Total comments: 18
Patch Set 7 : #Patch Set 8 : #Patch Set 9 : #
Total comments: 1
Patch Set 10 : #
Total comments: 9
Patch Set 11 : #Patch Set 12 : #Patch Set 13 : #Messages
Total messages: 32 (0 generated)
Starting with japhet@ - please help me find the right reviewer (jamesr@ is OOO). I know nothing of this code, so guidance is appreciated!
Rick, this looks like your area of expertise, yes?
I don't have a ton of context on scrollbars, but I can review. /cc bokan who may have some scrollbar specific context. It feel to me like this fix is hacking around a hit testing bug. Shouldn't we have one reliable mechanism for asking what's under the mouse cursor right now? Can you fix the hit testing instead? Also you need to add tests. https://codereview.chromium.org/296003011/diff/40001/Source/core/page/EventHa... File Source/core/page/EventHandler.cpp (right): https://codereview.chromium.org/296003011/diff/40001/Source/core/page/EventHa... Source/core/page/EventHandler.cpp:1275: Scrollbar* scrollbar = view ? view->scrollbarAtPoint(mouseEvent.position()) : 0; Shouldn't this already be in mev.scrollbar()? Seems wrong to hit test this special case again when we already have a HitTestResults here. https://codereview.chromium.org/296003011/diff/40001/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/296003011/diff/40001/Source/web/WebViewImpl.c... Source/web/WebViewImpl.cpp:495: if (event.button == WebMouseEvent::ButtonLeft && !m_page->mainFrame()->view()->scrollbarAtPoint(point)) { Again, why isn't the problem here really that the hit test below is broken? If the hit test properly hit the scrollbar then I think we wouldn't need this hack.
I agree that it seems like the hit test should probably return the frame scrollbar, though I know nothing about hit testing. However, I don't think this is particularly worse than the current code. The hack is already there, down at line 1302 in the original source: FrameView* view = m_frame->view(); Scrollbar* scrollbar = view ? view->scrollbarAtPoint(mouseEvent.position ()) : 0; Those lines are the only reason that clicking on overlay scrollbars work at all on mac. So all that I did was to generalize the existing hack. Note that currently, hit testing does not seem to return frame scrollbars on any platform. My fear would be that changing this would break something else. But if someone who knows more about HitTesting can verify that it is the right thing to do then I can simply add a check like "view->scrollbarAtPoint(position)" in the HitTesting code and be done! :) A pointer to where to add a test for this would be great as well. Thanks! On Thu, May 22, 2014 at 11:53 PM, <rbyers@chromium.org> wrote: > I don't have a ton of context on scrollbars, but I can review. /cc bokan > who > may have some scrollbar specific context. > > It feel to me like this fix is hacking around a hit testing bug. Shouldn't > we > have one reliable mechanism for asking what's under the mouse cursor right > now? > Can you fix the hit testing instead? > > Also you need to add tests. > > > https://codereview.chromium.org/296003011/diff/40001/Source/core/page/EventHa... > File Source/core/page/EventHandler.cpp (right): > > https://codereview.chromium.org/296003011/diff/40001/Source/core/page/EventHa... > Source/core/page/EventHandler.cpp:1275: Scrollbar* scrollbar = view ? > view->scrollbarAtPoint(mouseEvent.position()) : 0; > Shouldn't this already be in mev.scrollbar()? Seems wrong to hit test > this special case again when we already have a HitTestResults here. > > https://codereview.chromium.org/296003011/diff/40001/Source/web/WebViewImpl.cpp > File Source/web/WebViewImpl.cpp (right): > > https://codereview.chromium.org/296003011/diff/40001/Source/web/WebViewImpl.c... > Source/web/WebViewImpl.cpp:495: if (event.button == > > WebMouseEvent::ButtonLeft && > !m_page->mainFrame()->view()->scrollbarAtPoint(point)) { > Again, why isn't the problem here really that the hit test below is > broken? If the hit test properly hit the scrollbar then I think we > wouldn't need this hack. > > https://codereview.chromium.org/296003011/ To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On 2014/05/23 00:18:36, raymes wrote:
> I agree that it seems like the hit test should probably return the
> frame scrollbar, though I know nothing about hit testing. However, I
> don't think this is particularly worse than the current code. The hack
> is already there, down at line 1302 in the original source:
>
> FrameView* view = m_frame->view();
> Scrollbar* scrollbar = view ?
> view->scrollbarAtPoint(mouseEvent.position ()) : 0;
>
> Those lines are the only reason that clicking on overlay scrollbars
> work at all on mac. So all that I did was to generalize the existing
> hack.
>
> Note that currently, hit testing does not seem to return frame
> scrollbars on any platform. My fear would be that changing this would
> break something else. But if someone who knows more about HitTesting
> can verify that it is the right thing to do then I can simply add a
> check like "view->scrollbarAtPoint(position)" in the HitTesting code
> and be done! :)
OK, I agree generalizing this to do it in both the swallow and not-swallow cases
is strictly an improvement. But duplicating the hack in
WebViewImpl::handleMouseDown seems unfortunate. I checked and this code is all
very old (even in WebKit it dates back many years) with no substantial changes.
So I'm OK being conservative in trying to fix it since we probably don't have
anyone with expertise here.
There's another issue though - it looks like you're changing web-exposed
behavior, might this break some websites? In particular previously when a
mousedown hit a scrollbar the page would still see the mousedown/mouseup events,
it just wouldn't see any mousemove events from dragging the scrollbar (at least
that's what my tests on ChromeOS show). Now it looks like the page will never
see the events at all. Did you intend to make this change? If so, how do other
browsers behave in this regard? Have you checked to see if the behavior here is
covered by the DOM events spec?
The plugin case for the above is trickier - seems like we want to capture all
mouse events to plugins, so we probably can't send to the scrollbar and the
plugin. In that case sending to only the scrollbar seems not too likely to
break things (and seems reasonable to be better than the alternative of sending
only to the plugin).
> A pointer to where to add a test for this would be great as well. Thanks!
The EventHandler case should be testable in a LayoutTest - see
LayoutTest/fast/events/. Eg. mousedown_in_scrollbar.html,
mousedown-in-subframe-scrollbar.html look like they test some small pieces of
this. From a quick search I don't see obvious tests that cover the main
behavior here. I assume nothing failed with your patch here? Sigh...
Looks like you'll probably want to use the following (from
LayoutTests/fast/scrolling/overlay-scrollbars.html) to ensure you're testing the
overlay scrollbar path:
internals.settings.setOverlayScrollbarsEnabled(true);
internals.settings.setMockScrollbarsEnabled(true);
Hopefully the plugin case will be similar - just a special case of the above.
See LayoutTests/plugin for existing plugin-centric tests (eg. hopefully there's
a dummy plugin you can just use).
Note that weiliangc@ is bringing overlay scrollbars to all Aura platforms, so
this'll soon affect platforms beyond Mac. She may have pointers to people with
more context on overlay scrollbar behavior in particular (my expertise is more
in event handling and hit testing).
> On Thu, May 22, 2014 at 11:53 PM, <mailto:rbyers@chromium.org> wrote:
> > I don't have a ton of context on scrollbars, but I can review. /cc bokan
> > who
> > may have some scrollbar specific context.
> >
> > It feel to me like this fix is hacking around a hit testing bug. Shouldn't
> > we
> > have one reliable mechanism for asking what's under the mouse cursor right
> > now?
> > Can you fix the hit testing instead?
> >
> > Also you need to add tests.
> >
> >
> >
>
https://codereview.chromium.org/296003011/diff/40001/Source/core/page/EventHa...
> > File Source/core/page/EventHandler.cpp (right):
> >
> >
>
https://codereview.chromium.org/296003011/diff/40001/Source/core/page/EventHa...
> > Source/core/page/EventHandler.cpp:1275: Scrollbar* scrollbar = view ?
> > view->scrollbarAtPoint(mouseEvent.position()) : 0;
> > Shouldn't this already be in mev.scrollbar()? Seems wrong to hit test
> > this special case again when we already have a HitTestResults here.
> >
> >
>
https://codereview.chromium.org/296003011/diff/40001/Source/web/WebViewImpl.cpp
> > File Source/web/WebViewImpl.cpp (right):
> >
> >
>
https://codereview.chromium.org/296003011/diff/40001/Source/web/WebViewImpl.c...
> > Source/web/WebViewImpl.cpp:495: if (event.button ==
> >
> > WebMouseEvent::ButtonLeft &&
> > !m_page->mainFrame()->view()->scrollbarAtPoint(point)) {
> > Again, why isn't the problem here really that the hit test below is
> > broken? If the hit test properly hit the scrollbar then I think we
> > wouldn't need this hack.
> >
> > https://codereview.chromium.org/296003011/
>
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:blink-reviews+unsubscribe@chromium.org.
On Tue, May 27, 2014 at 3:48 PM, <rbyers@chromium.org> wrote: > On 2014/05/23 00:18:36, raymes wrote: > >> I agree that it seems like the hit test should probably return the >> frame scrollbar, though I know nothing about hit testing. However, I >> don't think this is particularly worse than the current code. The hack >> is already there, down at line 1302 in the original source: >> > > FrameView* view = m_frame->view(); >> Scrollbar* scrollbar = view ? >> view->scrollbarAtPoint(mouseEvent.position ()) : 0; >> > > Those lines are the only reason that clicking on overlay scrollbars >> work at all on mac. So all that I did was to generalize the existing >> hack. >> > > Note that currently, hit testing does not seem to return frame >> scrollbars on any platform. My fear would be that changing this would >> break something else. But if someone who knows more about HitTesting >> can verify that it is the right thing to do then I can simply add a >> check like "view->scrollbarAtPoint(position)" in the HitTesting code >> and be done! :) >> > > OK, I agree generalizing this to do it in both the swallow and not-swallow > cases > is strictly an improvement. But duplicating the hack in > WebViewImpl::handleMouseDown seems unfortunate. I checked and this code > is all > very old (even in WebKit it dates back many years) with no substantial > changes. > So I'm OK being conservative in trying to fix it since we probably don't > have > anyone with expertise here. > > There's another issue though - it looks like you're changing web-exposed > behavior, might this break some websites? In particular previously when a > mousedown hit a scrollbar the page would still see the mousedown/mouseup > events, > it just wouldn't see any mousemove events from dragging the scrollbar (at > least > that's what my tests on ChromeOS show). Now it looks like the page will > never > see the events at all. Did you intend to make this change? If so, how do > other > browsers behave in this regard? Have you checked to see if the behavior > here is > covered by the DOM events spec? > > The plugin case for the above is trickier - seems like we want to capture > all > mouse events to plugins, so we probably can't send to the scrollbar and the > plugin. In that case sending to only the scrollbar seems not too likely to > break things (and seems reasonable to be better than the alternative of > sending > only to the plugin). > > A pointer to where to add a test for this would be great as well. Thanks! >> > > The EventHandler case should be testable in a LayoutTest - see > LayoutTest/fast/events/. Eg. mousedown_in_scrollbar.html, > mousedown-in-subframe-scrollbar.html look like they test some small > pieces of > this. From a quick search I don't see obvious tests that cover the main > behavior here. I assume nothing failed with your patch here? Sigh... > > Looks like you'll probably want to use the following (from > LayoutTests/fast/scrolling/overlay-scrollbars.html) to ensure you're > testing the > overlay scrollbar path: > > internals.settings.setOverlayScrollbarsEnabled(true); > internals.settings.setMockScrollbarsEnabled(true); > > Hopefully the plugin case will be similar - just a special case of the > above. > See LayoutTests/plugin for existing plugin-centric tests (eg. hopefully > there's > a dummy plugin you can just use). > > Note that weiliangc@ is bringing overlay scrollbars to all Aura > platforms, so > this'll soon affect platforms beyond Mac. She may have pointers to people > with > more context on overlay scrollbar behavior in particular (my expertise is > more > in event handling and hit testing). > I haven't looked too closely at event handling and hit testing for scrollbars. +DaveMoore and see whether he has more info on this. For aura platforms, there is already overlay scrollbar implementation in places, which can be turned on by command line argument --enable-overlay-scrollbar. Those are compositor rendered scrollbars, and might behave differently regards hit testing. Future Aura overlay scrollbars will be Blink rendered and sounds like they are more like Mac case. Sorry don't have much insight into this. > > On Thu, May 22, 2014 at 11:53 PM, <mailto:rbyers@chromium.org> wrote: >> > I don't have a ton of context on scrollbars, but I can review. /cc >> bokan >> > who >> > may have some scrollbar specific context. >> > >> > It feel to me like this fix is hacking around a hit testing bug. >> Shouldn't >> > we >> > have one reliable mechanism for asking what's under the mouse cursor >> right >> > now? >> > Can you fix the hit testing instead? >> > >> > Also you need to add tests. >> > >> > >> > >> > > https://codereview.chromium.org/296003011/diff/40001/ > Source/core/page/EventHandler.cpp > >> > File Source/core/page/EventHandler.cpp (right): >> > >> > >> > > https://codereview.chromium.org/296003011/diff/40001/ > Source/core/page/EventHandler.cpp#newcode1275 > >> > Source/core/page/EventHandler.cpp:1275: Scrollbar* scrollbar = view ? >> > view->scrollbarAtPoint(mouseEvent.position()) : 0; >> > Shouldn't this already be in mev.scrollbar()? Seems wrong to hit test >> > this special case again when we already have a HitTestResults here. >> > >> > >> > > https://codereview.chromium.org/296003011/diff/40001/ > Source/web/WebViewImpl.cpp > >> > File Source/web/WebViewImpl.cpp (right): >> > >> > >> > > https://codereview.chromium.org/296003011/diff/40001/ > Source/web/WebViewImpl.cpp#newcode495 > >> > Source/web/WebViewImpl.cpp:495: if (event.button == >> > >> > WebMouseEvent::ButtonLeft && >> > !m_page->mainFrame()->view()->scrollbarAtPoint(point)) { >> > Again, why isn't the problem here really that the hit test below is >> > broken? If the hit test properly hit the scrollbar then I think we >> > wouldn't need this hack. >> > >> > https://codereview.chromium.org/296003011/ >> > > To unsubscribe from this group and stop receiving emails from it, send an >> > email > >> to mailto:blink-reviews+unsubscribe@chromium.org. >> > > > https://codereview.chromium.org/296003011/ > To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Rick, thanks for the detailed explanation and advice, very helpful! Writing the tests helped to verify the expected behavior here and hopefully simplify the fix. I've really split this up into 2 fixes with 2 corresponding tests. 1) EventHandler.cpp: just as element-scrollbars (e.g. div scrollbars) always get a chance to handle events, even if they are swallowed by a node, so too should frame scrollbars. The impact is wider than just the plugin, for example you used to be able to set an event handler on the window which would block scrolling on the frame. 2)WebViewImpl.cpp: Don't start event capture on the plugin if the event might go to the scrollbar. This avoids the scrollbar missing events that go to the plugin. Thanks, Raymes
Raymes, this is great! Seems quite rational now, and I love the tests and little cleanup - thank you! Just a few small issues with the tests... https://codereview.chromium.org/296003011/diff/100001/LayoutTests/fast/scroll... File LayoutTests/fast/scrolling/scrollbar-prevent-default.html (right): https://codereview.chromium.org/296003011/diff/100001/LayoutTests/fast/scroll... LayoutTests/fast/scrolling/scrollbar-prevent-default.html:9: testRunner.waitUntilDone(); I don't think this test needs to be async (eventSender operates synchronously) - you should be able to just remove this and the notifyDone call, and maybe move the JS to the end of the file (outside a 'load' handler) after the html and css. https://codereview.chromium.org/296003011/diff/100001/LayoutTests/fast/scroll... LayoutTests/fast/scrolling/scrollbar-prevent-default.html:23: window.receivedClickEvent = true; nit: this name is misleading, how about receivedMousedownEvent? https://codereview.chromium.org/296003011/diff/100001/LayoutTests/fast/scroll... LayoutTests/fast/scrolling/scrollbar-prevent-default.html:28: eventSender.mouseMoveTo(window.innerWidth - 10, 1); Can you add a second case (or second file) for a div scrollbar too (since there are separate code paths)? Since the code paths are the same it should be OK to test just layout scrollbars (or just overlay scrollbars) and not both, right? Or I guess you'll get overlay by default on Mac, right? So that's even better if it works in both configs. https://codereview.chromium.org/296003011/diff/100001/LayoutTests/fast/scroll... LayoutTests/fast/scrolling/scrollbar-prevent-default.html:38: width: 200vw; is this div actually necessary for the test? In the CL description you say it's impossible to have an element under the frame scrollbar but the description above suggests otherwise. https://codereview.chromium.org/296003011/diff/100001/LayoutTests/fast/scroll... LayoutTests/fast/scrolling/scrollbar-prevent-default.html:41: /* Use customized scrollbar to avoid platform differences. */ Can't you use internals.settings.setMockScrollbarsEnabled(true) to achieve the equivalent benefit? Oh and this is commented out :-) https://codereview.chromium.org/296003011/diff/100001/LayoutTests/plugins/ove... File LayoutTests/plugins/overlay-scrollbar-mouse-capture.html (right): https://codereview.chromium.org/296003011/diff/100001/LayoutTests/plugins/ove... LayoutTests/plugins/overlay-scrollbar-mouse-capture.html:19: window.testRunner.notifyDone(); again this could probably be a little simpler as a sync test instead of async https://codereview.chromium.org/296003011/diff/100001/LayoutTests/plugins/ove... LayoutTests/plugins/overlay-scrollbar-mouse-capture.html:37: mouseupFired = true; looks like this is unused, remove it https://codereview.chromium.org/296003011/diff/100001/LayoutTests/plugins/ove... LayoutTests/plugins/overlay-scrollbar-mouse-capture.html:55: // because there shouldn't be any mouse capture. isn't capture released as soon as there's a mouseup? Even if the mouse went down inside the plugin I wouldn't expect it to get movement events from outside AFTER the mouse has gone back up (should be easy to verify by tweaking this test). I think you want to do the move before the mouseup. Do you know if there's a test anywhere that verifies this capture behavior (i.e. if you just disable it does anything fail)? If not it would be nice (and pretty easy) to add that base case here (i.e. run the test twice - once in the middle of the plugin and once over the scrollbar). https://codereview.chromium.org/296003011/diff/100001/LayoutTests/plugins/ove... LayoutTests/plugins/overlay-scrollbar-mouse-capture.html:60: eventSender.mouseUp(); oh, was the above mouseUp a typo? You shouldn't have two mouseUps with out an intervening mousedown...
https://codereview.chromium.org/296003011/diff/100001/LayoutTests/fast/scroll... File LayoutTests/fast/scrolling/scrollbar-prevent-default.html (right): https://codereview.chromium.org/296003011/diff/100001/LayoutTests/fast/scroll... LayoutTests/fast/scrolling/scrollbar-prevent-default.html:9: testRunner.waitUntilDone(); On 2014/05/31 04:32:33, Rick Byers wrote: > I don't think this test needs to be async (eventSender operates synchronously) - > you should be able to just remove this and the notifyDone call, and maybe move > the JS to the end of the file (outside a 'load' handler) after the html and css. Done. https://codereview.chromium.org/296003011/diff/100001/LayoutTests/fast/scroll... LayoutTests/fast/scrolling/scrollbar-prevent-default.html:23: window.receivedClickEvent = true; On 2014/05/31 04:32:33, Rick Byers wrote: > nit: this name is misleading, how about receivedMousedownEvent? Done. https://codereview.chromium.org/296003011/diff/100001/LayoutTests/fast/scroll... LayoutTests/fast/scrolling/scrollbar-prevent-default.html:28: eventSender.mouseMoveTo(window.innerWidth - 10, 1); Done. Right, the codepath for layout and non-overlay scrollbars should be the same. https://codereview.chromium.org/296003011/diff/100001/LayoutTests/fast/scroll... LayoutTests/fast/scrolling/scrollbar-prevent-default.html:38: width: 200vw; This is only here in order to force there to be scrollbars on the page :) I just moved the css to the body though. https://codereview.chromium.org/296003011/diff/100001/LayoutTests/fast/scroll... LayoutTests/fast/scrolling/scrollbar-prevent-default.html:41: /* Use customized scrollbar to avoid platform differences. */ I'm not sure exactly what setMockScrollbarsEnabled does, but it seems to work as you suggested. This wasn't actually commented out in the css, only the HTML. Not exactly sure why (maybe cross-browser compatibility? =\) I stole it from another test. But I've removed it now anyway. https://codereview.chromium.org/296003011/diff/100001/LayoutTests/plugins/ove... File LayoutTests/plugins/overlay-scrollbar-mouse-capture.html (right): https://codereview.chromium.org/296003011/diff/100001/LayoutTests/plugins/ove... LayoutTests/plugins/overlay-scrollbar-mouse-capture.html:19: window.testRunner.notifyDone(); On 2014/05/31 04:32:33, Rick Byers wrote: > again this could probably be a little simpler as a sync test instead of async Done. https://codereview.chromium.org/296003011/diff/100001/LayoutTests/plugins/ove... LayoutTests/plugins/overlay-scrollbar-mouse-capture.html:37: mouseupFired = true; On 2014/05/31 04:32:33, Rick Byers wrote: > looks like this is unused, remove it Done. https://codereview.chromium.org/296003011/diff/100001/LayoutTests/plugins/ove... LayoutTests/plugins/overlay-scrollbar-mouse-capture.html:55: // because there shouldn't be any mouse capture. Sorry - yes this is a spurious mouseUp that I threw in while testing. It works the same with this line removed. mouse-capture-inside-shadow should test the mouse capture functionality (which is what I based this off). https://codereview.chromium.org/296003011/diff/100001/LayoutTests/plugins/ove... LayoutTests/plugins/overlay-scrollbar-mouse-capture.html:60: eventSender.mouseUp(); On 2014/05/31 04:32:33, Rick Byers wrote: > oh, was the above mouseUp a typo? You shouldn't have two mouseUps with out an > intervening mousedown... Done.
Looks great, LGTM! Please update your CL description for the current design, eg. ".. to handle the event _prior_ to elements" is not true (but scrollbars get a chance to handle them regardless of whether the element handles them). You'll need Ojan for OWNERS in Source/web.
On 2014/06/03 14:22:46, Rick Byers wrote: > Looks great, LGTM! > > Please update your CL description for the current design, eg. ".. to handle the > event _prior_ to elements" is not true (but scrollbars get a chance to handle > them regardless of whether the element handles them). > > You'll need Ojan for OWNERS in Source/web. Oh and see also http://crbug.com/358248 - I believe you'll fix that case as well (and so should reference that bug in your CL too). There's a separate simpler CL up just for that here: https://codereview.chromium.org/309793003/
On 2014/06/03 14:23:42, Rick Byers wrote: > On 2014/06/03 14:22:46, Rick Byers wrote: > > Looks great, LGTM! > > > > Please update your CL description for the current design, eg. ".. to handle > the > > event _prior_ to elements" is not true (but scrollbars get a chance to handle > > them regardless of whether the element handles them). > > > > You'll need Ojan for OWNERS in Source/web. > > Oh and see also http://crbug.com/358248 - I believe you'll fix that case as well > (and so should reference that bug in your CL too). There's a separate simpler > CL up just for that here: https://codereview.chromium.org/309793003/ Thanks Rick! Yes this should fix that as well. I had to add an entry to TestExpectations because one of the tests is failing on Mac due to an unrelated bug (https://code.google.com/p/chromium/issues/detail?id=267206). Overlay scrollbars on mac don't receive mousedown events sent by the EventSender. Since this issue reproduces with overlay scrollbars on other platforms and is tested correctly by the test, I'd like to go ahead and still land this patch, if that is ok with you. Also, I tested the behavior manually on mac and it seems to be working as intended. Thanks! Raymes
Thanks for doing this. You could add the layout test from my CL https://codereview.chromium.org/309793003/ which particularly tests the main frame scrollbars http://crbug.com/358248.
This is already tested in LayoutTests/fast/scrolling/scrollbar-prevent-default.html
On 2014/06/04 05:20:23, raymes1 wrote: > This is already tested in > LayoutTests/fast/scrolling/scrollbar-prevent-default.html Oh. I missed it :)
On 2014/06/04 02:42:31, raymes wrote: > On 2014/06/03 14:23:42, Rick Byers wrote: > > On 2014/06/03 14:22:46, Rick Byers wrote: > > > Looks great, LGTM! > > > > > > Please update your CL description for the current design, eg. ".. to handle > > the > > > event _prior_ to elements" is not true (but scrollbars get a chance to > handle > > > them regardless of whether the element handles them). > > > > > > You'll need Ojan for OWNERS in Source/web. > > > > Oh and see also http://crbug.com/358248 - I believe you'll fix that case as > well > > (and so should reference that bug in your CL too). There's a separate simpler > > CL up just for that here: https://codereview.chromium.org/309793003/ > > Thanks Rick! Yes this should fix that as well. > > I had to add an entry to TestExpectations because one of the tests is failing on > Mac due to an unrelated bug > (https://code.google.com/p/chromium/issues/detail?id=267206). Overlay scrollbars > on mac don't receive mousedown events sent by the EventSender. Since this issue > reproduces with overlay scrollbars on other platforms and is tested correctly by > the test, I'd like to go ahead and still land this patch, if that is ok with > you. Also, I tested the behavior manually on mac and it seems to be working as > intended. Ok. You're sure this is really the same issue? Eg. that bug claims the test started failing after some extensions-related change to Chrome. Does that make any sense to you? Please add your analysis to the bug for whoever ultimately tries to tackle this. I can believe there's some challenges here around getting MacOS to draw the scrollbars while EventSender generates only synthetic events for blink (not real MacOS events). > > Thanks! > Raymes
https://codereview.chromium.org/296003011/diff/160001/LayoutTests/TestExpecta... File LayoutTests/TestExpectations (right): https://codereview.chromium.org/296003011/diff/160001/LayoutTests/TestExpecta... LayoutTests/TestExpectations:1078: crbug.com/267206 [ Mavericks Lion MountainLion Retina ] plugins/overlay-scrollbar-mouse-capture.html [ Failure ] might as well move this up to group it with the other failure under this bug.
Thanks, I'll move it up. The CL that broke this was https://src.chromium.org/viewvc/blink?view=revision&revision=155279 (Enable composited scrollbars on Mac) so it seems probable that it's related. Also I looked through a bunch of other tests. fast/scrolling/scrollbar-tickmarks-hittest.html was the only other test I could find which attempts to send mouse events to overlay scrollbars on mac and it is disabled :( On Fri, Jun 6, 2014 at 6:55 AM, <rbyers@chromium.org> wrote: > > https://codereview.chromium.org/296003011/diff/160001/LayoutTests/TestExpecta... > File LayoutTests/TestExpectations (right): > > https://codereview.chromium.org/296003011/diff/160001/LayoutTests/TestExpecta... > LayoutTests/TestExpectations:1078: crbug.com/267206 [ Mavericks Lion > MountainLion Retina ] plugins/overlay-scrollbar-mouse-capture.html [ > Failure ] > might as well move this up to group it with the other failure under this > bug. > > https://codereview.chromium.org/296003011/ To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On 2014/06/06 00:00:08, raymes wrote: > Thanks, I'll move it up. > > The CL that broke this was > https://src.chromium.org/viewvc/blink?view=revision&revision=155279 > (Enable composited scrollbars on Mac) so it seems probable that it's > related. > > Also I looked through a bunch of other tests. > fast/scrolling/scrollbar-tickmarks-hittest.html was the only other > test I could find which attempts to send mouse events to overlay > scrollbars on mac and it is disabled :( Ok, makes sense - thanks for the details! > On Fri, Jun 6, 2014 at 6:55 AM, <mailto:rbyers@chromium.org> wrote: > > > > > https://codereview.chromium.org/296003011/diff/160001/LayoutTests/TestExpecta... > > File LayoutTests/TestExpectations (right): > > > > > https://codereview.chromium.org/296003011/diff/160001/LayoutTests/TestExpecta... > > LayoutTests/TestExpectations:1078: crbug.com/267206 [ Mavericks Lion > > MountainLion Retina ] plugins/overlay-scrollbar-mouse-capture.html [ > > Failure ] > > might as well move this up to group it with the other failure under this > > bug. > > > > https://codereview.chromium.org/296003011/ > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:blink-reviews+unsubscribe@chromium.org.
Where is the code for doing this behavior for elements? It obviously makes sense for elements and the main frame to have the same behavior. I just want to understand it. Also, did you test other browsers? I didn't see you answer rbyers's question regarding that. It'd be good to know that we're changing in a way that makes us compatible with other browsers. https://codereview.chromium.org/296003011/diff/180001/LayoutTests/TestExpecta... File LayoutTests/TestExpectations (right): https://codereview.chromium.org/296003011/diff/180001/LayoutTests/TestExpecta... LayoutTests/TestExpectations:489: crbug.com/267206 [ Mavericks Lion MountainLion Retina ] plugins/overlay-scrollbar-mouse-capture.html [ Failure ] This is not right. This should be NeedsRebaseline. See http://www.chromium.org/developers/testing/webkit-layout-tests/testexpectations. https://codereview.chromium.org/296003011/diff/180001/LayoutTests/fast/scroll... File LayoutTests/fast/scrolling/scrollbar-prevent-default.html (right): https://codereview.chromium.org/296003011/diff/180001/LayoutTests/fast/scroll... LayoutTests/fast/scrolling/scrollbar-prevent-default.html:2: <html> Nit: normally we leave out the html/head/body elements unless the test requires it. The one requires body of course, but not the others. https://codereview.chromium.org/296003011/diff/180001/LayoutTests/plugins/ove... File LayoutTests/plugins/overlay-scrollbar-mouse-capture.html (right): https://codereview.chromium.org/296003011/diff/180001/LayoutTests/plugins/ove... LayoutTests/plugins/overlay-scrollbar-mouse-capture.html:2: <html> ditto https://codereview.chromium.org/296003011/diff/180001/LayoutTests/plugins/ove... LayoutTests/plugins/overlay-scrollbar-mouse-capture.html:2: <html> ditto https://codereview.chromium.org/296003011/diff/180001/LayoutTests/plugins/ove... LayoutTests/plugins/overlay-scrollbar-mouse-capture.html:28: window.startLogging = false; Here and elsewhere: We don't usually prefix globals with window in tests unless there's a need to disambiguate. It's just clutter otherwise.
On 2014/06/06 02:02:56, ojan wrote: > Where is the code for doing this behavior for elements? It obviously makes sense > for elements and the main frame to have the same behavior. I just want to > understand it. > > Also, did you test other browsers? I didn't see you answer rbyers's question > regarding that. It'd be good to know that we're changing in a way that makes us > compatible with other browsers. Note that I was asking this back when the patch made some substantial web-visible behavior changes. Now the patch is just consistently applying our primary behavior (mousedown/mouseup go to elements under scroll bars but can also manipulate scroll bars). It would still be nice to know how other browsers behave here, but I think our mainline behavior is staying the same with just some fixes to edge cases. > https://codereview.chromium.org/296003011/diff/180001/LayoutTests/TestExpecta... > File LayoutTests/TestExpectations (right): > > https://codereview.chromium.org/296003011/diff/180001/LayoutTests/TestExpecta... > LayoutTests/TestExpectations:489: crbug.com/267206 [ Mavericks Lion MountainLion > Retina ] plugins/overlay-scrollbar-mouse-capture.html [ Failure ] > This is not right. This should be NeedsRebaseline. See > http://www.chromium.org/developers/testing/webkit-layout-tests/testexpectations. > > https://codereview.chromium.org/296003011/diff/180001/LayoutTests/fast/scroll... > File LayoutTests/fast/scrolling/scrollbar-prevent-default.html (right): > > https://codereview.chromium.org/296003011/diff/180001/LayoutTests/fast/scroll... > LayoutTests/fast/scrolling/scrollbar-prevent-default.html:2: <html> > Nit: normally we leave out the html/head/body elements unless the test requires > it. The one requires body of course, but not the others. > > https://codereview.chromium.org/296003011/diff/180001/LayoutTests/plugins/ove... > File LayoutTests/plugins/overlay-scrollbar-mouse-capture.html (right): > > https://codereview.chromium.org/296003011/diff/180001/LayoutTests/plugins/ove... > LayoutTests/plugins/overlay-scrollbar-mouse-capture.html:2: <html> > ditto > > https://codereview.chromium.org/296003011/diff/180001/LayoutTests/plugins/ove... > LayoutTests/plugins/overlay-scrollbar-mouse-capture.html:2: <html> > ditto > > https://codereview.chromium.org/296003011/diff/180001/LayoutTests/plugins/ove... > LayoutTests/plugins/overlay-scrollbar-mouse-capture.html:28: window.startLogging > = false; > Here and elsewhere: We don't usually prefix globals with window in tests unless > there's a need to disambiguate. It's just clutter otherwise.
On 2014/06/06 02:08:41, Rick Byers wrote: > On 2014/06/06 02:02:56, ojan wrote: > > Where is the code for doing this behavior for elements? It obviously makes > sense > > for elements and the main frame to have the same behavior. I just want to > > understand it. > > > > Also, did you test other browsers? I didn't see you answer rbyers's question > > regarding that. It'd be good to know that we're changing in a way that makes > us > > compatible with other browsers. > > Note that I was asking this back when the patch made some substantial > web-visible behavior changes. Now the patch is just consistently applying our > primary behavior (mousedown/mouseup go to elements under scroll bars but can > also manipulate scroll bars). It would still be nice to know how other browsers > behave here, but I think our mainline behavior is staying the same with just > some fixes to edge cases. > > > > https://codereview.chromium.org/296003011/diff/180001/LayoutTests/TestExpecta... > > File LayoutTests/TestExpectations (right): > > > > > https://codereview.chromium.org/296003011/diff/180001/LayoutTests/TestExpecta... > > LayoutTests/TestExpectations:489: crbug.com/267206 [ Mavericks Lion > MountainLion > > Retina ] plugins/overlay-scrollbar-mouse-capture.html [ Failure ] > > This is not right. This should be NeedsRebaseline. See > > > http://www.chromium.org/developers/testing/webkit-layout-tests/testexpectations. > > > > > https://codereview.chromium.org/296003011/diff/180001/LayoutTests/fast/scroll... > > File LayoutTests/fast/scrolling/scrollbar-prevent-default.html (right): > > > > > https://codereview.chromium.org/296003011/diff/180001/LayoutTests/fast/scroll... > > LayoutTests/fast/scrolling/scrollbar-prevent-default.html:2: <html> > > Nit: normally we leave out the html/head/body elements unless the test > requires > > it. The one requires body of course, but not the others. I've seen you mention this guideline a number of times, but I think most of our existing LayoutTests don't follow it. Do we have anywhere to write down LayoutTest guidelines like this? If not I can add a section to http://www.chromium.org/developers/testing/webkit-layout-tests. > > > > > https://codereview.chromium.org/296003011/diff/180001/LayoutTests/plugins/ove... > > File LayoutTests/plugins/overlay-scrollbar-mouse-capture.html (right): > > > > > https://codereview.chromium.org/296003011/diff/180001/LayoutTests/plugins/ove... > > LayoutTests/plugins/overlay-scrollbar-mouse-capture.html:2: <html> > > ditto > > > > > https://codereview.chromium.org/296003011/diff/180001/LayoutTests/plugins/ove... > > LayoutTests/plugins/overlay-scrollbar-mouse-capture.html:2: <html> > > ditto > > > > > https://codereview.chromium.org/296003011/diff/180001/LayoutTests/plugins/ove... > > LayoutTests/plugins/overlay-scrollbar-mouse-capture.html:28: > window.startLogging > > = false; > > Here and elsewhere: We don't usually prefix globals with window in tests > unless > > there's a need to disambiguate. It's just clutter otherwise. This is the first I've heard this, another good thing to add to a LayoutTest style guideline somewhere (if there's not already some page I'm not aware of / can't find).
> Where is the code for doing this behavior for elements? It obviously makes sense > for elements and the main frame to have the same behavior. I just want to > understand it. Have a look in EventHandler::passMousePressEventToScrollbar. mev.scrollbar() is the scrollbar from the hit test. If the click was over a plugin that is attached to an element, it will be returned in the hit test result. However frame scrollbars are not returned in hit test results. > Also, did you test other browsers? I didn't see you answer rbyers's question > regarding that. It'd be good to know that we're changing in a way that makes us > compatible with other browsers. Firefox and Internet Explorer both have the same behavior as with this CL applied. That is, frame scrollbars still get events even when they are swallowed be an element. Safari is broken in the same way Chrome was before this CL. https://codereview.chromium.org/296003011/diff/180001/LayoutTests/TestExpecta... File LayoutTests/TestExpectations (right): https://codereview.chromium.org/296003011/diff/180001/LayoutTests/TestExpecta... LayoutTests/TestExpectations:489: crbug.com/267206 [ Mavericks Lion MountainLion Retina ] plugins/overlay-scrollbar-mouse-capture.html [ Failure ] On 2014/06/06 02:02:55, ojan wrote: > This is not right. This should be NeedsRebaseline. See > http://www.chromium.org/developers/testing/webkit-layout-tests/testexpectations. Done. https://codereview.chromium.org/296003011/diff/180001/LayoutTests/fast/scroll... File LayoutTests/fast/scrolling/scrollbar-prevent-default.html (right): https://codereview.chromium.org/296003011/diff/180001/LayoutTests/fast/scroll... LayoutTests/fast/scrolling/scrollbar-prevent-default.html:2: <html> I've kept the <head> tags for <style> is that ok? https://codereview.chromium.org/296003011/diff/180001/LayoutTests/plugins/ove... File LayoutTests/plugins/overlay-scrollbar-mouse-capture.html (right): https://codereview.chromium.org/296003011/diff/180001/LayoutTests/plugins/ove... LayoutTests/plugins/overlay-scrollbar-mouse-capture.html:2: <html> On 2014/06/06 02:02:55, ojan wrote: > ditto Done. https://codereview.chromium.org/296003011/diff/180001/LayoutTests/plugins/ove... LayoutTests/plugins/overlay-scrollbar-mouse-capture.html:28: window.startLogging = false; These don't even need to be globals actually.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/raymes@chromium.org/296003011/220001
Matching FF and IE and our element handling is compelling to me.
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/10527)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/10527)
The CQ bit was checked by raymes@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/raymes@chromium.org/296003011/240001
Message was sent while issue was closed.
Change committed as 175857 |
