Bug 158252 - [RTL Scrollbars] Frame scrollbars don't move to the right when text direction changes to RTL
Summary: [RTL Scrollbars] Frame scrollbars don't move to the right when text direction...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antonio Gomes
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2016-05-31 23:51 PDT by Carlos Garcia Campos
Modified: 2016-07-15 12:40 PDT (History)
10 users (show)

See Also:


Attachments
patch v1 (8.39 KB, patch)
2016-07-11 13:16 PDT, Antonio Gomes
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (1.02 MB, application/zip)
2016-07-11 14:07 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews102 for mac-yosemite (790.17 KB, application/zip)
2016-07-11 14:07 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (719.17 KB, application/zip)
2016-07-11 14:14 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-yosemite (1.43 MB, application/zip)
2016-07-11 14:17 PDT, Build Bot
no flags Details
patch v2 - fixed layout tests on mac (9.21 KB, patch)
2016-07-12 09:06 PDT, Antonio Gomes
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-yosemite (791.07 KB, application/zip)
2016-07-12 09:59 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-yosemite-wk2 (818.31 KB, application/zip)
2016-07-12 10:00 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-yosemite (1.43 MB, application/zip)
2016-07-12 10:08 PDT, Build Bot
no flags Details
patch v2.1 - fixed layout tests on mac (take 2) (11.27 KB, patch)
2016-07-12 11:55 PDT, Antonio Gomes
mmaxfield: review-
Details | Formatted Diff | Diff
patch v3 - addressed myles' feedback. (19.83 KB, patch)
2016-07-14 14:43 PDT, Antonio Gomes
mmaxfield: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2016-05-31 23:51:16 PDT
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.
Comment 1 Myles C. Maxfield 2016-06-27 14:07:47 PDT
Does this occur in other ports as well?
Comment 2 Carlos Garcia Campos 2016-06-27 22:36:01 PDT
Only GTK+ and mac ports support RTL scrollbars. This happens in GTK+ for sure, but I don't have a mac to try.
Comment 3 Antonio Gomes 2016-07-11 13:16:47 PDT
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>).
Comment 4 Build Bot 2016-07-11 14:07:02 PDT
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
Comment 5 Build Bot 2016-07-11 14:07:06 PDT
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
Comment 6 Build Bot 2016-07-11 14:07:27 PDT
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
Comment 7 Build Bot 2016-07-11 14:07:31 PDT
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
Comment 8 Build Bot 2016-07-11 14:14:25 PDT
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
Comment 9 Build Bot 2016-07-11 14:14:28 PDT
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
Comment 10 Build Bot 2016-07-11 14:17:24 PDT
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
Comment 11 Build Bot 2016-07-11 14:17:28 PDT
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
Comment 12 Antonio Gomes 2016-07-12 09:06:03 PDT
Created attachment 283413 [details]
patch v2 - fixed layout tests on mac
Comment 13 Build Bot 2016-07-12 09:59:06 PDT
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
Comment 14 Build Bot 2016-07-12 09:59:10 PDT
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
Comment 15 Build Bot 2016-07-12 10:00:18 PDT
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
Comment 16 Build Bot 2016-07-12 10:00:22 PDT
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
Comment 17 Build Bot 2016-07-12 10:07:58 PDT
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
Comment 18 Build Bot 2016-07-12 10:08:02 PDT
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
Comment 19 Antonio Gomes 2016-07-12 11:55:47 PDT
Created attachment 283436 [details]
patch v2.1 - fixed layout tests on mac (take 2)
Comment 20 Myles C. Maxfield 2016-07-12 13:38:07 PDT
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.
Comment 21 Myles C. Maxfield 2016-07-12 14:04:21 PDT
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.
Comment 22 Antonio Gomes 2016-07-14 14:43:02 PDT
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.
Comment 23 Myles C. Maxfield 2016-07-14 19:48:36 PDT
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
Comment 24 Antonio Gomes 2016-07-14 20:58:51 PDT
Committer r203267: <https://trac.webkit.org/r203267>
Comment 25 Antonio Gomes 2016-07-15 12:40:37 PDT
(In reply to comment #24)
> Committer r203267: <https://trac.webkit.org/r203267>

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