Bug 56493 – Drag-scrolling overlay scrollbars thumb in overflow regions does not work

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Beth Dakin]

If you try try grab onto the scroll thumb of an overlay scrollbar in an overflow region, it will not work. I have a fix. 

<rdar://problem/9112688>

[Beth Dakin]

Created attachment 85985 [details]
Patch

[Darin Adler]

Comment on attachment 85985 [details]
Patch

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

> Source/WebCore/rendering/RenderBlock.cpp:3860
> +    bool checkChildren = !useClip || (hasControlClip() ? controlClipRect(tx, ty).intersects(hitTestArea) : overflowClipRect(tx, ty, true).intersects(hitTestArea));

If we’re going to be passing constants to this, then it should be an enum instead of a boolean.

[Beth Dakin]

Thanks, Darin! I switch over to an enum.

Fixed with revision 81304.

[WebKit Review Bot]

http://trac.webkit.org/changeset/81304 might have broken GTK Linux 32-bit Release

[Beth Dakin]

Re-opening since this is still broken for transformed or positioned overflow areas.

[Beth Dakin]

Created attachment 88847 [details]
Patch for transformed and/or positioned overflow areas

Okay, this approach might be a little crazy. To get the ClipRects right (necessary for positioned objects to hit test properly), this patch re-computes ClipRects whenever we paint to hit test so that painting does not include the scrollbar size and hit testing does. This might be completely crazy. Maybe it would be better to have two instances of the ClipRects? One could be the painting ClipRects and one could be the hit-testing ClipRects. Anyway, I thought I would put this out there and gauge opinions on the matter.

[Beth Dakin]

Comment on attachment 85985 [details]
Patch

Clearing the flag since this first patch was committed.

[WebKit Review Bot]

Attachment 88847 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/rendering/RenderLayer.h:371:  The parameter name "relevancy" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/rendering/RenderLayer.h:374:  The parameter name "relevancy" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/rendering/RenderLayer.h:377:  The parameter name "relevancy" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/rendering/RenderLayer.h:567:  The parameter name "relevancy" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/rendering/RenderLayer.h:568:  The parameter name "relevancy" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 5 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 88847 [details]
Patch for transformed and/or positioned overflow areas

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

> Source/WebCore/rendering/RenderLayer.cpp:2350
> +    // The clip rects need to be recomputed for painting not to include scrollbar size for overlay scrollbars.
> +    if (ScrollbarTheme::nativeTheme()->usesOverlayScrollbars())
> +        clearClipRectsIncludingDescendants();

Why can't you just set the flag to use temporary clip rects? We really don't want to kill the clip rects cache altogether.

> Source/WebCore/rendering/RenderLayer.cpp:3217
> +void RenderLayer::updateClipRects(const RenderLayer* rootLayer, OverlayScrollbarSizeRelevancy relevancy)

I don't get the "relevancy" terminology. Can't this just be an 'IncludeScrollbars enum?

[Simon Fraser (smfr)]

RenderLayer::hitTestLayer already has:

    useTemporaryClipRects = compositor()->inCompositingMode();

Why can't you just also set this when you have overlay scrollbars?

[Dave Hyatt]

I think you should just cache two sets of clip rects based off the relevancy.

[Dave Hyatt]

And yeah what Simon said.  I think relevancy is not the best term to use.  It seems like it could just be a boolean.  Everyone who calls it is going to parameterize with a variable name, so I don't see an enum as being required.  You only need to use an enum to avoid passing raw true/false values, but I don't think you'll be doing that ever, so a boolean should be fine.

But yeah I'd just compute two sets of clip rects and only branch to the alternate set if the overflowClipRect returned actually changes.  That way overflow:hidden (far and away the most common clip) doesn't have to recompute two sets or have to be cleared just because you used overlay scrollbars.

[Dave Hyatt]

If you don't want to cache two sets of clip rects you could just set useTemporaryClipRects during hit testing.  You should only need to set this though if there is an actual overlay scrollbar present.

Remember that 99% of clipping on the Web is done using overflow:hidden, which doesn't care about scrollbars.  Let's not penalize that case.

[Beth Dakin]

Thanks for the feedback, guys!

First,  the temporary clip rects concept does not seem to work for this. I confess I am not entirely sure why, but I know that reverting to the original clip rect at any point before the end of hit testing results in the wrong behavior.

In terms of the relevancy name and using a bool versus and enum, the original patch added the OverlayScrollbarSizeRelevancy enumeration. My original patch just used a bool, but Darin suggested switching to an enum in his comment in this very bug (see comment 2). I am open the changing the enum or switching it back to a bool, but I would rather do that in a follow-up patch since that would touch a lot of code that is not necessary for getting positioned and transformed hit-testing to work properly. Also, I agree that OverlayScrollbarSizeRelevancy is not the greatest name, but I think that the values of the enum -- IgnoreOverlayScrollbarSize and IncludeOverlayScrollbar size -- are clear and informative.

I like the plan of caching two sets of clip rects. It should also be easy to do this only in the case when it's necessary…I will add logic for that too.

[Beth Dakin]

(In reply to comment #14)
> Thanks for the feedback, guys!
> 
> First,  the temporary clip rects concept does not seem to work for this. I confess I am not entirely sure why, but I know that reverting to the original clip rect at any point before the end of hit testing results in the wrong behavior.
> 
>

Since I couldn't remember why this didn't work, it started gnawing at me, and I tried it again…I was able to make it work this time! I think I know exactly what silly thing I missed that made me think this didn't work. Anyway. Now I think the temporary clip rects is the simplest approach, and I will post a new patch in a bit.

[Beth Dakin]

Created attachment 88941 [details]
Patch that uses temporary clip rects

Here is a much better patch that uses temporary clip rects.

A few things about this patch:

1. Since this patch turned out to be so minimal, I am definitely open to changing the name of the OverlayScrollbarSizeRelevancy enum in the same patch if anyone has a better suggestion. Maybe it should just be OverlayScrollbarSize? I still think the values of this enum have good names -- as a reminder, the values are IgnoreOverlayScrollbarSize and IncludeOverlayScrollbarSize. I do think they add meaning over a bool, but I agree that the type name of the enum is not the best. Anyone have a suggestion?

2. The concept of m_overlayScrollbarsRequireTemporaryClipRects that I added with this patch is admittedly not the smartest mechanism, in that it never gets set to false once it is set to true. So if a positioned overflow area with overlay scrollbars is resized so that it doesn't have scrollbars anymore, then m_overlayScrollbarsRequireTemporaryClipRects is not set to false as it should be. That being said, you don't lose much by having it always be true in that case -- you just do some extra work, but you don't sacrifice functionality at all. Anyway, I think it's okay and it avoid the extra work in the vast majority of cases. If you disagree, please chime in. Also the name is a bit cumbersomely-long, so if you have a better suggestion, let me know

[WebKit Review Bot]

Attachment 88941 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/rendering/RenderLayer.cpp:2868:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/rendering/RenderLayer.cpp:2868:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 88941 [details]
Patch that uses temporary clip rects

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

r- because I think this is still not quite right.

I wish we could have some layout tests for this.

> Source/WebCore/rendering/RenderLayer.cpp:2188
> +        if (renderer()->isPositioned() || renderer()->isRelPositioned() || renderer()->hasClip())
> +            rootLayer->setOverlayScrollbarsRequireTemporaryClipRects(true);

Why hasClip(), rather than hasOverflowClip()?

I don't really see the need to cache this value on the rootLayer (which, BTW, is just the painting root, so it changes with transforms and compositing). So I think it's wrong to stuff it on rootLayer.

>>> Source/WebCore/rendering/RenderLayer.cpp:2868
>>> +    useTemporaryClipRects = rootLayer->overlayScrollbarsRequireTemporaryClipRects();//ScrollbarTheme::nativeTheme()->usesOverlayScrollbars();
>> 
>> One space before end of line comments  [whitespace/comments] [5]
> 
> Should have a space between // and comment  [whitespace/comments] [4]

This needs to be an |=, otherwise you're clobbering the compositor()->inCompositingMode() setting.

> Source/WebCore/rendering/RenderLayer.cpp:3205
> +void RenderLayer::updateClipRects(const RenderLayer* rootLayer, OverlayScrollbarSizeRelevancy relevancy)

Maybe just call the enum ScrollBarSizeIncluded?

[Beth Dakin]

(In reply to comment #18)
> (From update of attachment 88941 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88941&action=review
> 
> r- because I think this is still not quite right.
> 
> I wish we could have some layout tests for this.
> 

I agree. We definitely need to turn on overlay scrollbars in DRT soon. 

> > Source/WebCore/rendering/RenderLayer.cpp:2188
> > +        if (renderer()->isPositioned() || renderer()->isRelPositioned() || renderer()->hasClip())
> > +            rootLayer->setOverlayScrollbarsRequireTemporaryClipRects(true);
> 
> Why hasClip(), rather than hasOverflowClip()?
> 

This code was added to RenderLayer::paintOverflow controls, so hasOverflowClip() is always true (there is an early return if it is not). hasClip() is there because RenderLayer::calculateClipRects() does special assignments that involve scrollbars when hasClip() is true. 

> I don't really see the need to cache this value on the rootLayer (which, BTW, is just the painting root, so it changes with transforms and compositing). So I think it's wrong to stuff it on rootLayer.
> 

Do you have another suggestion? This is what we do for painting scrollbars, and it works well in the transform cases that I have tested. 

> >>> Source/WebCore/rendering/RenderLayer.cpp:2868
> >>> +    useTemporaryClipRects = rootLayer->overlayScrollbarsRequireTemporaryClipRects();//ScrollbarTheme::nativeTheme()->usesOverlayScrollbars();
> >> 
> >> One space before end of line comments  [whitespace/comments] [5]
> > 
> > Should have a space between // and comment  [whitespace/comments] [4]
> 
> This needs to be an |=, otherwise you're clobbering the compositor()->inCompositingMode() setting.
> 

Eek, good catch. Will fix.

> > Source/WebCore/rendering/RenderLayer.cpp:3205
> > +void RenderLayer::updateClipRects(const RenderLayer* rootLayer, OverlayScrollbarSizeRelevancy relevancy)
> 
> Maybe just call the enum ScrollBarSizeIncluded?

I considered a similar name when I first created this enum, and I don't care for it because I feel like it is naming the enumeration after one of the two values in the enumeration, which seems odd. Also, I feel that it must still include the work "overlay" to describe the scrollbars since non-overlay scrollbars are always included.

[Beth Dakin]

Created attachment 89509 [details]
New patch

New patch! This patch fixes the |= bug that Simon pointed out above. It also uses ScrollView to track whether there are any overlay scrollbars painted in the view currently rather than trying to do something similar with the root layer.

I started renaming OverlayScrollbarSizeRelevancy to OverlayScrollbarSizeIncluded despite my protestations above. But in the end, I reverted the change. The most obvious variable name for something of type OverlayScrollbarSizeIncluded would be scrollbarSizeIncluded, which just makes it sound like the scrollbar size IS included. We need a name that conveys that the variable might mean that the size is included, or it might mean the size is ignored. So yeah. I went back to the original protestations.

[Simon Fraser (smfr)]

Comment on attachment 89509 [details]
New patch

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

> Source/WebCore/platform/ScrollView.cpp:927
> +    m_containsScrollableAreaWithOverlayScrollbars = false;

Init this in the constructor too?

> Source/WebCore/rendering/RenderLayer.cpp:2184
> +        renderView->layer()->setContainsDirtyOverlayScrollbars(true);

I'm not sure what dirtyOverflowScrollbars are?

[Beth Dakin]

(In reply to comment #21)
> (From update of attachment 89509 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=89509&action=review
> 
> > Source/WebCore/platform/ScrollView.cpp:927
> > +    m_containsScrollableAreaWithOverlayScrollbars = false;
> 
> Init this in the constructor too?

Good catch, will do.

> 
> > Source/WebCore/rendering/RenderLayer.cpp:2184
> > +        renderView->layer()->setContainsDirtyOverlayScrollbars(true);
> 
> I'm not sure what dirtyOverflowScrollbars are?

This is a line of code that was there already and is only touched by this patch because I stores the RenderView in a local variable to avoid fetching it twice. Anyway, in case you still want to know what it is, painting does a second pass through the layer tree to paint overlay scrollbars if and only if dirtyOverflowScrollbars is true. (See RenderLayer::paintOverlayScrollbars().) We can't use the same mechanism for hit testing simply because this variable is set back to false immediately once the overlay scrollbars have been painted.

[Beth Dakin]

Committed patch with revision 83899.