Bug 153304 - Main frame scrollbars not updated on hovering when using overlay scrollbars
Summary: Main frame scrollbars not updated on hovering when using overlay scrollbars
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 153405
  Show dependency treegraph
 
Reported: 2016-01-21 03:44 PST by Carlos Garcia Campos
Modified: 2016-01-26 09:00 PST (History)
8 users (show)

See Also:


Attachments
Patch (3.57 KB, patch)
2016-01-21 03:47 PST, Carlos Garcia Campos
mcatanzaro: review+
Details | Formatted Diff | Diff
Patch for landing (3.37 KB, patch)
2016-01-25 23:51 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2016-01-21 03:44:38 PST
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.
Comment 1 Carlos Garcia Campos 2016-01-21 03:47:04 PST
Created attachment 269444 [details]
Patch
Comment 2 Beth Dakin 2016-01-21 11:00:55 PST
(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?
Comment 3 Carlos Garcia Campos 2016-01-21 23:36:21 PST
(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.
Comment 4 Michael Catanzaro 2016-01-22 07:51:07 PST
I'm curious, do OS X scrollbars not change in appearance when hovered?
Comment 5 Carlos Garcia Campos 2016-01-24 01:40:51 PST
(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.
Comment 6 Michael Catanzaro 2016-01-24 06:35:22 PST
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
Comment 7 Beth Dakin 2016-01-24 22:34:09 PST
(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?
Comment 8 Beth Dakin 2016-01-24 22:39:43 PST
(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.)
Comment 9 Carlos Garcia Campos 2016-01-25 00:27:09 PST
(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();
Comment 10 Carlos Garcia Campos 2016-01-25 00:31:19 PST
(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.
Comment 11 Michael Catanzaro 2016-01-25 07:03:36 PST
(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
Comment 12 Beth Dakin 2016-01-25 11:19:20 PST
(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!
Comment 13 Carlos Garcia Campos 2016-01-25 23:51:18 PST
Created attachment 269859 [details]
Patch for landing
Comment 14 Carlos Garcia Campos 2016-01-26 09:00:35 PST
Committed r195591: <http://trac.webkit.org/changeset/195591>