Main frame scrollbars not updated on hovering when using overlay scrollbars

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

Legacy scrollbars were fixed in r194155, but overlay scrollbars are not notified when they are hovered. This is because the layer hit test in RenderView::hitTest always returns true when using overlay scrollbars and we are returning early in such case ignoring the HitTestRequest::AllowFrameScrollbars flag. So, in case of using overlay scrollbars we still need to check the RenderView scrollbars even when the layer hist test succeeded unless it already contains a scrollbar.

[Carlos Garcia Campos]

Created attachment 269444 [details]
Patch

[Beth Dakin]

(In reply to comment #0)
> Legacy scrollbars were fixed in r194155, but overlay scrollbars are not
> notified when they are hovered. This is because the layer hit test in
> RenderView::hitTest always returns true when using overlay scrollbars and we
> are returning early in such case ignoring the
> HitTestRequest::AllowFrameScrollbars flag. So, in case of using overlay
> scrollbars we still need to check the RenderView scrollbars even when the
> layer hist test succeeded unless it already contains a scrollbar.

Can you write up a set of steps that reproduce a bug that you are fixing here?

[Carlos Garcia Campos]

(In reply to comment #2)
> (In reply to comment #0)
> > Legacy scrollbars were fixed in r194155, but overlay scrollbars are not
> > notified when they are hovered. This is because the layer hit test in
> > RenderView::hitTest always returns true when using overlay scrollbars and we
> > are returning early in such case ignoring the
> > HitTestRequest::AllowFrameScrollbars flag. So, in case of using overlay
> > scrollbars we still need to check the RenderView scrollbars even when the
> > layer hist test succeeded unless it already contains a scrollbar.
> 
> Can you write up a set of steps that reproduce a bug that you are fixing
> here?

Sure.

1. Enable overlay scrollbars
2. Open MiniBrowser and visit any page long enough to show a scrollbar
3. Hover the scrollbar

There's no change in appearance on hover. That's from the users point of view. From the WebKit point of view, the scrollbar is not invalidated and WebKit doesn't know the mouse is over a scrollbar until you press the mouse button, because in EventHandler::handleMousePressEvent() we are manually getting the scrollbar again and only using the first hit test scrollbar if we don't get a scrollbar manually, see:

FrameView* view = m_frame.view();
Scrollbar* scrollbar = view ? view->scrollbarAtPoint(platformMouseEvent.position()) : 0;
if (!scrollbar)
    scrollbar = mouseEvent.scrollbar();

updateLastScrollbarUnderMouse(scrollbar, true);

And this is why even when the appearance is not updated on hover, you can still click and drag the scrollbar and it works. EventHandler::handleMousePressEvent() needs to do this because it doesn't use the AllowFrameScrollbars flag when doing the hit test.

[Michael Catanzaro]

I'm curious, do OS X scrollbars not change in appearance when hovered?

[Carlos Garcia Campos]

(In reply to comment #4)
> I'm curious, do OS X scrollbars not change in appearance when hovered?

Legacy scrollbars do, but overlay scrollbars don't.

[Michael Catanzaro]

Comment on attachment 269444 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=269444&action=review

> Source/WebCore/ChangeLog:14
> +        RenderView scrollbars even when the layer hist test succeeded

hist -> hit

[Beth Dakin]

(In reply to comment #5)
> (In reply to comment #4)
> > I'm curious, do OS X scrollbars not change in appearance when hovered?
> 
> Legacy scrollbars do, but overlay scrollbars don't.

This is false. 

On Mac, overlay scrollbars become fatter and get a visible track when hovered, and that works correctly in tip of tree WebKit for me. Does it fail for you on some version of Mac OS? If so, what Mac OS version are you testing on?

Which platform does that patch affect?

[Beth Dakin]

(In reply to comment #7)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > I'm curious, do OS X scrollbars not change in appearance when hovered?
> > 
> > Legacy scrollbars do, but overlay scrollbars don't.
> 
> This is false. 
> 
> On Mac, overlay scrollbars become fatter and get a visible track when
> hovered, and that works correctly in tip of tree WebKit for me. Does it fail
> for you on some version of Mac OS? If so, what Mac OS version are you
> testing on?
> 
> Which platform does that patch affect?

Of course I assume GTK, but I am asking for specifics of the platform because I am curious about the exact platform behaviors and making sure we are as consistent as possible in this code since a lot of it was written thinking about Mac behavior. I want to make sure that other platforms using it understand the Mac behavior and are truly using it for similar purposes. (In other words, I want to make sure we all have the same definition of what an overlay vs. legacy scrollbar is.)

[Carlos Garcia Campos]

(In reply to comment #6)
> Comment on attachment 269444 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=269444&action=review
> 
> > Source/WebCore/ChangeLog:14
> > +        RenderView scrollbars even when the layer hist test succeeded
> 
> hist -> hit

Oops. I'm also considering that main frame scrollbars should take precedence over a possible scrollbar returned by the layer hit test result. That would be consistent with what EventHandler::handleMousePressEvent does, see:

FrameView* view = m_frame.view();
Scrollbar* scrollbar = view ? view->scrollbarAtPoint(platformMouseEvent.position()) : 0;
if (!scrollbar)
    scrollbar = mouseEvent.scrollbar();

[Carlos Garcia Campos]

(In reply to comment #7)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > I'm curious, do OS X scrollbars not change in appearance when hovered?
> > 
> > Legacy scrollbars do, but overlay scrollbars don't.
> 
> This is false. 

Ah, I guess it's now fixed then.

> On Mac, overlay scrollbars become fatter and get a visible track when
> hovered, and that works correctly in tip of tree WebKit for me. Does it fail
> for you on some version of Mac OS? If so, what Mac OS version are you
> testing on?

Yes, that didn't happen for me with Safari in Mac OSX El Capitan (probably not the latest update, I'll try to upgrade).

> Which platform does that patch affect?

It should affect all platforms, I expected this to fix Mac as well, but maybe Mac is already updating the scrollbars from the scroll animator when mouseEnteredScrollbar is called, that would be hiding this bug in Mac, but I'm just guessing.

[Michael Catanzaro]

(In reply to comment #8)
> Of course I assume GTK, but I am asking for specifics of the platform
> because I am curious about the exact platform behaviors and making sure we
> are as consistent as possible in this code since a lot of it was written
> thinking about Mac behavior. I want to make sure that other platforms using
> it understand the Mac behavior and are truly using it for similar purposes.
> (In other words, I want to make sure we all have the same definition of what
> an overlay vs. legacy scrollbar is.)

Here's a video I found that demonstrates how GTK+ scrollbars work nowadays; this is what we're trying to match. (Warning: music.)

https://www.youtube.com/watch?v=8DktM-XLaf0

[Beth Dakin]

(In reply to comment #11)
> (In reply to comment #8)
> > Of course I assume GTK, but I am asking for specifics of the platform
> > because I am curious about the exact platform behaviors and making sure we
> > are as consistent as possible in this code since a lot of it was written
> > thinking about Mac behavior. I want to make sure that other platforms using
> > it understand the Mac behavior and are truly using it for similar purposes.
> > (In other words, I want to make sure we all have the same definition of what
> > an overlay vs. legacy scrollbar is.)
> 
> Here's a video I found that demonstrates how GTK+ scrollbars work nowadays;
> this is what we're trying to match. (Warning: music.)
> 
> https://www.youtube.com/watch?v=8DktM-XLaf0

OH, very interesting. Thank you for this.

This makes me realize that my previous statement was perhaps misleading (or wrong depending on exactly what the original question was asking :-P ).

It seems like in GTK, the scrollbars appear ANY TIME you hover over the scrollbar areas. On Mac, the scrollbars only do anything special on hover if they are ALREADY showing for any other reason. So, for example, if you scroll a little and then quickly move the cursor over to the scrollbar area, then you will see the hover effect. 

Another difference seems to be the the GTK scrollbars show up whenever you are moving the cursor around inside a scrollable box.

Very interesting!

[Carlos Garcia Campos]

Created attachment 269859 [details]
Patch for landing

[Carlos Garcia Campos]

Committed r195591: <http://trac.webkit.org/changeset/195591>