Bug 153352 – Overlay scrollbars should always use the whole contents

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

They are currently respecting the scroll corner. In the case of main frame scrollbars we are indeed returning an empty rectangle from ScrollView::scrollCornerRect() when using overlay scrollbars, but when calculating the size of the scrollbars we are using the actual width/height instead of the occupied with/height. For other scrollbars RenderLayer::scrollCornerRect() is not checking whether scrollbars are overlay or not and we are always returning a scroll corner rectangle when scrollbars are present.

[Carlos Garcia Campos]

Created attachment 269565 [details]
Patch

[Build Bot]

Comment on attachment 269565 [details]
Patch

Attachment 269565 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/725018

New failing tests:
compositing/overflow/composited-scrolling-creates-a-stacking-container.html
scrollbars/scrollbar-parts-opacity.html

[Build Bot]

Created attachment 269567 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 269565 [details]
Patch

Attachment 269565 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/725045

New failing tests:
compositing/overflow/composited-scrolling-creates-a-stacking-container.html
scrollbars/scrollbar-parts-opacity.html

[Build Bot]

Created attachment 269568 [details]
Archive of layout-test-results from ews100 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 269565 [details]
Patch

Attachment 269565 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/725188

New failing tests:
compositing/overflow/composited-scrolling-creates-a-stacking-container.html
scrollbars/scrollbar-parts-opacity.html

[Build Bot]

Created attachment 269569 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Michael Catanzaro]

Don't forget to investigate these failing tests.

[Beth Dakin]

Can you describe the bug that this is fixing from an end-user's perspective? Or the before/after expectations? The description of the bug is all about code details, and I want to understand how this affects behavior.

[Carlos Garcia Campos]

(In reply to comment #8)
> Don't forget to investigate these failing tests.

Yes, it's in my TODO, I assumed layout tests don't use overlay scrollbars at all.

[Carlos Garcia Campos]

(In reply to comment #9)
> Can you describe the bug that this is fixing from an end-user's perspective?
> Or the before/after expectations? The description of the bug is all about
> code details, and I want to understand how this affects behavior.

In case of having both horizontal and vertical scrollbars, the scrollbars respect the scroll corner. That looks good for legacy scrollbars that show the track, but  with the overlay indicators it looks weird that the indicator stops so early before the end of the contents, giving the impression that there's something else to scroll. This happens because the scroll corner is transparent, so it's not obvious that's the scroll corner. It also happens with the text areas having a resizer. Legacy scrollbars take into account the resizer, which is good, but I expect overlay scrollbars to be rendered also over the resizer. The resizer takes precedence so you can still click and drag to resize the text area. If this behavior is not aceptable for mac, we can find the ways to make this dependent of the port (Probably adding a method to ScrollbarTheme, like we do for other similar things).

[Carlos Garcia Campos]

I misunderstood showsOverflowControls, I'll update the patch to actually check for overlay scrollbars.

[Michael Catanzaro]

Comment on attachment 269565 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        They are currently respecting the scroll corner. In the case of

Nit: it would be good if you paste your Bugzilla explanation into the changelog, to make it easier to understand why you're making this change.

> Source/WebCore/rendering/RenderLayer.cpp:2824
> +    if (showsOverflowControls())

Can you add a comment here to indicate that this is to avoid the weird blank space between the two scrollbars?

[Michael Catanzaro]

(In reply to comment #11)
> If this behavior is not
> aceptable for mac, we can find the ways to make this dependent of the port
> (Probably adding a method to ScrollbarTheme, like we do for other similar
> things).

Since you have a Mac, can't you fairly easily check what the correct behavior is for Mac? I opened WebKit/ChangeLog in a GTK+ text editor and scrolled to the bottom and to the right; this makes it easy to see that GTK+ does not have the space between the two overlay scrollbars.

[Carlos Garcia Campos]

(In reply to comment #13)
> Comment on attachment 269565 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=269565&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        They are currently respecting the scroll corner. In the case of
> 
> Nit: it would be good if you paste your Bugzilla explanation into the
> changelog, to make it easier to understand why you're making this change.
> 
> > Source/WebCore/rendering/RenderLayer.cpp:2824
> > +    if (showsOverflowControls())
> 
> Can you add a comment here to indicate that this is to avoid the weird blank
> space between the two scrollbars?

This check is wrong, I should be checking that current scrollbars are overlay scrollbars. I'll add the comment when I update the patch

[Carlos Garcia Campos]

(In reply to comment #14)
> (In reply to comment #11)
> > If this behavior is not
> > aceptable for mac, we can find the ways to make this dependent of the port
> > (Probably adding a method to ScrollbarTheme, like we do for other similar
> > things).
> 
> Since you have a Mac, can't you fairly easily check what the correct
> behavior is for Mac? I opened WebKit/ChangeLog in a GTK+ text editor and
> scrolled to the bottom and to the right; this makes it easy to see that GTK+
> does not have the space between the two overlay scrollbars.

I've tried opening a PDF in preview and scrollbars leave a scroll corner when they are always visible, but they don't when using overlay scrollbars. So this patch matches the behavior of other MAC OSX apps, at least preview.

[Simon Fraser (smfr)]

Comment on attachment 269565 [details]
Patch

New patch is coming, right?

[Beth Dakin]

(In reply to comment #11)
> (In reply to comment #9)
> > Can you describe the bug that this is fixing from an end-user's perspective?
> > Or the before/after expectations? The description of the bug is all about
> > code details, and I want to understand how this affects behavior.
> 
> In case of having both horizontal and vertical scrollbars, the scrollbars
> respect the scroll corner. That looks good for legacy scrollbars that show
> the track, but  with the overlay indicators it looks weird that the
> indicator stops so early before the end of the contents, giving the
> impression that there's something else to scroll. This happens because the
> scroll corner is transparent, so it's not obvious that's the scroll corner.
> It also happens with the text areas having a resizer. Legacy scrollbars take
> into account the resizer, which is good, but I expect overlay scrollbars to
> be rendered also over the resizer. The resizer takes precedence so you can
> still click and drag to resize the text area. If this behavior is not
> aceptable for mac, we can find the ways to make this dependent of the port
> (Probably adding a method to ScrollbarTheme, like we do for other similar
> things).

Thank you for this! I do think you should add this to the Changelog.

This would probably fix a bug we have on Mac where we do not match the AppKit behavior re: scroll corner. SO! If you do check out Mac behavior, it's worthwhile to look at both WebKit and something like Preview or TextEdit to see AppKit's behavior. Our goal in WebKit is to match AppKit, so they always win the "which is right" question on Mac.

[Carlos Garcia Campos]

Created attachment 269864 [details]
Updated patch

[Michael Catanzaro]

Comment on attachment 269864 [details]
Updated patch

If you get scroll animator tests to work, it would be awesome to add a test for this in particular.

[Carlos Garcia Campos]

Committed r195660: <http://trac.webkit.org/changeset/195660>

[Michael Catanzaro]

This (or possibly r195661) has introduced several layout test crashes, see bug #153695 and bug #153936.

[Carlos Garcia Campos]

(In reply to comment #22)
> This (or possibly r195661) has introduced several layout test crashes, see
> bug #153695 and bug #153936.

Not this one, it was r195661

[Darin Adler]

(In reply to comment #23)
> Not this one, it was r195661

Wasn’t that a pure rename? Did some other change creep in?

[Carlos Garcia Campos]

(In reply to comment #24)
> (In reply to comment #23)
> > Not this one, it was r195661
> 
> Wasn’t that a pure rename? Did some other change creep in?

Not exactly:

"this patch also moves the
smooth scrolling animation implementation to its own class
ScrollAnimationSmooth that impements an interface ScrollAnimation
that could be used to implement other animations. This will allow
the GTK+ port to add its own scroll animator class and still
support smooth scrolling sharing the code with the
ScrollAnimationSmooth."

But I just moved code to a new class.