Bug 160359 – Scrolling leaves artifacts when frame-level RTL vertical scrollbar is present

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

[Antonio Gomes]

Now that frame-level vertical scrollbar can be placed on the left side (either in accordance to the associated system preference or to the web content direction), the rect "copied on scroll" should respect.

[Antonio Gomes]

This is reproducible on the Gtk port (minibrowser), but it seems not to be Gtk specific.

[Antonio Gomes]

Created attachment 284906 [details]
patch v1

Similar patches (touching ::rectToCopyOnScroll) did not come accompanied by a test. I am open to ideas.

[Antonio Gomes]

Created attachment 284908 [details]
patch v1.1 - with a changelog entry.

[Build Bot]

Comment on attachment 284908 [details]
patch v1.1 - with a changelog entry.

Attachment 284908 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1775832

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 284915 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.5

[Antonio Gomes]

(In reply to comment #4)
> Comment on attachment 284908 [details]
> patch v1.1 - with a changelog entry.
> 
> Attachment 284908 [details] did not pass ios-sim-ews (ios-simulator-wk2):
> Output: http://webkit-queues.webkit.org/results/1775832
> 
> Number of test failures exceeded the failure limit.

Interesting. If the ios-sim failing tests do not set dir=rtl to a frame level, they should not be affected.

[Darin Adler]

Comment on attachment 284908 [details]
patch v1.1 - with a changelog entry.

Can we create a regression test? Normally the WebKit project requires a regression test for any bug fix or an explanation of why the author of the patch couldn’t figure out how to write one.

[Michael Catanzaro]

(In reply to comment #7)
> Comment on attachment 284908 [details]
> patch v1.1 - with a changelog entry.
> 
> Can we create a regression test? Normally the WebKit project requires a
> regression test for any bug fix or an explanation of why the author of the
> patch couldn’t figure out how to write one.

Antonio, do you have availability to pick this up again?

[Brady Eidson]

Comment on attachment 284908 [details]
patch v1.1 - with a changelog entry.

r-, as this has been pending review for over a year now. It is near-impossible that this patch still applies to trunk and unlikely to still be relevant in its current form.

[Ahmad Saleem]

I can see this line which this patch is trying to modify in the code:

https://searchfox.org/wubkat/source/Source/WebCore/platform/ScrollView.cpp#808

Only change is change of "shouldPlaceVerticalScrollbarOnLeft" from "shouldPlaceBlockDirectionScrollbarOnLeft".

It does not have test case, so I don't know whether it will fix anything or not.

[Ahmad Saleem]

(In reply to Ahmad Saleem from comment #10)
> I can see this line which this patch is trying to modify in the code:
> 
> https://searchfox.org/wubkat/source/Source/WebCore/platform/ScrollView.
> cpp#808
> 
> Only change is change of "shouldPlaceVerticalScrollbarOnLeft" from
> "shouldPlaceBlockDirectionScrollbarOnLeft".
> 
> It does not have test case, so I don't know whether it will fix anything or
> not.

Just to update, it compiles on WebKit Trunk based of "261814@main" as well.

[Ahmad Saleem]

(In reply to Ahmad Saleem from comment #11)
> (In reply to Ahmad Saleem from comment #10)
> > I can see this line which this patch is trying to modify in the code:
> > 
> > https://searchfox.org/wubkat/source/Source/WebCore/platform/ScrollView.
> > cpp#808
> > 
> > Only change is change of "shouldPlaceVerticalScrollbarOnLeft" from
> > "shouldPlaceBlockDirectionScrollbarOnLeft".
> > 
> > It does not have test case, so I don't know whether it will fix anything or
> > not.
> 
> Just to update, it compiles on WebKit Trunk based of "261814@main" as well.

IntRect scrollViewRect = convertToRootView(IntRect((shouldPlaceVerticalScrollbarOnLeft() && m_verticalScrollbar) ? m_verticalScrollbar->occupiedWidth() : 0, 0, visibleWidth(), visibleHeight()));

^ Just posting for quick copy paste for someone in future.

[Fujii Hironori]

*** Bug 275338 has been marked as a duplicate of this bug. ***

[Fujii Hironori]

attachment#471646 [details] is the test case.
This is only for non-AC mode.
Mac WK2 doesn't support non-AC mode.
Mac WK1, GTK, Windows, PlayStation ports support non-AC mode.
However, Mac WK1 shows the vertial scroll bar on right side. This isn't reproducible for Mac WK1.
GTK port still supports non-AC mode, but uses AC mode by default.

[Fujii Hironori]

Created attachment 471652 [details]
WIP patch

[Fujii Hironori]

Created attachment 471665 [details]
WIP patch

[Fujii Hironori]

Created attachment 471667 [details]
Patch to add a ref test but it doesn't work

I tried to create a ref test. But, it seems to be unable test this paint glitch with a ref test.

[Fujii Hironori]

Pull request: https://github.com/WebKit/WebKit/pull/29807

[EWS]

Committed 280155@main (fe975b3d7ff6): <https://commits.webkit.org/280155@main>

Reviewed commits have been landed. Closing PR #29807 and removing active labels.

[Radar WebKit Bug Importer]

<rdar://problem/130127462>