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.
Created attachment 269565 [details] Patch
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
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
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
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
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
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
Don't forget to investigate these failing tests.
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 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.
(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).
I misunderstood showsOverflowControls, I'll update the patch to actually check for overlay scrollbars.
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?
(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.
(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
(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.
Comment on attachment 269565 [details] Patch New patch is coming, right?
(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.
Created attachment 269864 [details] Updated patch
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.
Committed r195660: <http://trac.webkit.org/changeset/195660>
This (or possibly r195661) has introduced several layout test crashes, see bug #153695 and bug #153936.
(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
(In reply to comment #23) > Not this one, it was r195661 Wasn’t that a pure rename? Did some other change creep in?
(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.