Bug 158252 – [RTL Scrollbars] Frame scrollbars don't move to the right when text direction changes to RTL

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Carlos Garcia Campos]

You can try this with the theme GTK+ manual test (ManualTests/gtk/theme.html). When switching to RTL using the option menu, all scrollbars (text areas, list boxes) move to the right, but the main frame and iframe scrollbars stay on the left. I haven't tried this on mac, but this doesn't look like GTK+ specific issue, could someone with a mac confirm it? If I change the html to start in RTL mode, and then reload the page, the the frame scrollbars are on the right indeed.

[Myles C. Maxfield]

Does this occur in other ports as well?

[Carlos Garcia Campos]

Only GTK+ and mac ports support RTL scrollbars. This happens in GTK+ for sure, but I don't have a mac to try.

[Antonio Gomes]

Created attachment 283337 [details]
patch v1

(In reply to comment #1)
> Does this occur in other ports as well?
(In reply to comment #2)
> Only GTK+ and mac ports support RTL scrollbars. This happens in GTK+ for
> sure, but I don't have a mac to try.

This happens on both Gtk+ and Mac OSX ports.

To reproduce the problem on OSX, attach a clicky-scroll-wheel-mouse (so that scrollbars are non-overlay), then load ManualTests/gtk/theme.html and play with switching RTL and LTR modes (see the <select>).

[Build Bot]

Comment on attachment 283337 [details]
patch v1

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

New failing tests:
fast/scrolling/rtl-switch-rtl-ltr-updates-scrollbar-placement.html

[Build Bot]

Created attachment 283339 [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

[Build Bot]

Comment on attachment 283337 [details]
patch v1

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

New failing tests:
fast/scrolling/rtl-switch-rtl-ltr-updates-scrollbar-placement.html

[Build Bot]

Created attachment 283340 [details]
Archive of layout-test-results from ews102 for mac-yosemite

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

[Build Bot]

Comment on attachment 283337 [details]
patch v1

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

New failing tests:
fast/scrolling/rtl-switch-rtl-ltr-updates-scrollbar-placement.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 283337 [details]
patch v1

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

New failing tests:
fast/scrolling/rtl-switch-rtl-ltr-updates-scrollbar-placement.html

[Build Bot]

Created attachment 283343 [details]
Archive of layout-test-results from ews115 for mac-yosemite

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

[Antonio Gomes]

Created attachment 283413 [details]
patch v2 - fixed layout tests on mac

[Build Bot]

Comment on attachment 283413 [details]
patch v2 - fixed layout tests on mac

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

New failing tests:
fast/scrolling/rtl-switch-rtl-ltr-updates-scrollbar-placement.html

[Build Bot]

Created attachment 283420 [details]
Archive of layout-test-results from ews101 for mac-yosemite

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

[Build Bot]

Comment on attachment 283413 [details]
patch v2 - fixed layout tests on mac

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

New failing tests:
fast/scrolling/rtl-switch-rtl-ltr-updates-scrollbar-placement.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 283413 [details]
patch v2 - fixed layout tests on mac

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

New failing tests:
fast/scrolling/rtl-switch-rtl-ltr-updates-scrollbar-placement.html

[Build Bot]

Created attachment 283423 [details]
Archive of layout-test-results from ews116 for mac-yosemite

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

[Antonio Gomes]

Created attachment 283436 [details]
patch v2.1 - fixed layout tests on mac (take 2)

[Myles C. Maxfield]

Comment on attachment 283436 [details]
patch v2.1 - fixed layout tests on mac (take 2)

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

> LayoutTests/ChangeLog:3
> +        [RTL Scrollbars] Frame scrollbars don't move to the right when text direction changes to RTL

In WebKit, there are three kinds of scrolling content:

1. Main document content
2. overflow:scroll content
3. Iframe content

It looks like this patch only fixes one of those three cases. It's good that you have fixed one of the cases, but the title of the bug should probably be updated to reflect the narrow scope.

I'll file new bugs and add tests (which are marked as expected to fail) for the remaining types.

> LayoutTests/fast/scrolling/rtl-scrollbars-alternate-body-rtl-ltr-does-not-update-scrollbar-placement.html:30
> +            if (scroller.scrollTop != previousScrollPosition)

Why not use shouldNotBe() defined in js-test-pre.js?

> LayoutTests/fast/scrolling/rtl-scrollbars-alternate-body-rtl-ltr-does-not-update-scrollbar-placement.html:46
> +            if (scroller.scrollTop != previousScrollPosition)

Ditto

> LayoutTests/fast/scrolling/rtl-scrollbars-alternate-body-rtl-ltr-does-not-update-scrollbar-placement.html:56
> +            if (window.eventSender) {

I think it would be valuable to add a simple reference test so we aren't so reliant on eventSender.

> Source/WebCore/page/FrameView.cpp:1132
> +    updateScrollbars(scrollPosition());

Are we sure this is guaranteed not to be stale during style resolution time? It seems scary to be updating the scrollbars outside of layout where they were being updated before.

[Myles C. Maxfield]

Comment on attachment 283436 [details]
patch v2.1 - fixed layout tests on mac (take 2)

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

> Source/WebCore/rendering/RenderBox.cpp:405
> +            view().frameView().topContentDirectionDidChange();

topContentDirectionDidChange() calls updateScrollbars() calls updateContentsSize() calls layout(). We shouldn't be doing a synchronous layout here. Instead, we should set a flag (hopefully a flag already exists for this purpose) and during the next layout (which there will definitely be one because "dir" changed) check the flag and do the updateScrollbars() there.

[Antonio Gomes]

Created attachment 283686 [details]
patch v3 - addressed myles' feedback.

(In reply to comment #20)
> Comment on attachment 283436 [details]
> patch v2.1 - fixed layout tests on mac (take 2)
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=283436&action=review
> 
> > LayoutTests/ChangeLog:3
> > +        [RTL Scrollbars] Frame scrollbars don't move to the right when text direction changes to RTL
> 
> In WebKit, there are three kinds of scrolling content:
> 
> 1. Main document content
> 2. overflow:scroll content
> 3. Iframe content

Patch here focus on frame-level scrollbars cases, (1) and (3), hence the tests added to them
(2) already updates the vertical scrollbar placement when 'dir' changes.

> It looks like this patch only fixes one of those three cases. It's good that
> you have fixed one of the cases, but the title of the bug should probably be
> updated to reflect the narrow scope.
> 
> I'll file new bugs and add tests (which are marked as expected to fail) for
> the remaining types.

See above.

> > LayoutTests/fast/scrolling/rtl-scrollbars-alternate-body-rtl-ltr-does-not-update-scrollbar-placement.html:30
> > +            if (scroller.scrollTop != previousScrollPosition)
> 
> Why not use shouldNotBe() defined in js-test-pre.js?

Done.
 
> > LayoutTests/fast/scrolling/rtl-scrollbars-alternate-body-rtl-ltr-does-not-update-scrollbar-placement.html:46
> > +            if (scroller.scrollTop != previousScrollPosition)
> 
> Ditto

Done.

> > LayoutTests/fast/scrolling/rtl-scrollbars-alternate-body-rtl-ltr-does-not-update-scrollbar-placement.html:56
> > +            if (window.eventSender) {
> 
> I think it would be valuable to add a simple reference test so we aren't so
> reliant on eventSender.

I think this test is also valid, given that it drags the scrollbar either on the left or right side.
But, a reference tests really sounds like a good idea here. I added one.
 
> > Source/WebCore/page/FrameView.cpp:1132
> > +    updateScrollbars(scrollPosition());
>
> (..)
> 
> > Source/WebCore/rendering/RenderBox.cpp:405
> > +            view().frameView().topContentDirectionDidChange();
> 
> topContentDirectionDidChange() calls updateScrollbars() calls
> updateContentsSize() calls layout(). We shouldn't be doing a synchronous
> layout here. Instead, we should set a flag (hopefully a flag already exists
> for this purpose) and during the next layout (which there will definitely be
> one because "dir" changed) check the flag and do the updateScrollbars()
> there.

You are right. The way patch v2 was implemented, it calls ScrollView::updateScrollbars in the middle of a style recalc (outside of a layout call), which is wrong.

Patch v3 tries a different approach: as part of the style recalc, it only "flags" the associated FrameView in case that it needs to update its scrollbars placement due to a 'dir' attribute change.
In the next FrameView::layout() call, ScrollView::updateScrollbars is actually called and does the job.

[Myles C. Maxfield]

Comment on attachment 283686 [details]
patch v3 - addressed myles' feedback.

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

> LayoutTests/fast/scrolling/rtl-scrollbars-alternate-body-dir-attr-does-not-update-scrollbar-placement-2-expected.html:10
> +            internals.settings.setUserInterfaceDirectionPolicy("View");

\o/

> Source/WebCore/page/FrameView.cpp:1143
> +    ASSERT(m_layoutPhase == InPostLayerPositionsUpdatedAfterLayout);

:D

[Antonio Gomes]

Committer r203267: <https://trac.webkit.org/r203267>

[Antonio Gomes]

(In reply to comment #24)
> Committer r203267: <https://trac.webkit.org/r203267>

Fixed TestExpectation entry in https://trac.webkit.org/r203281 (by Ryan Haddad).