Bug 155149 – [OS X] Main frame scrollbars should appear on the left on RTL systems

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

[Myles C. Maxfield]

[Not For Review] Dummy bug: move scrollbars around

[Myles C. Maxfield]

Created attachment 273242 [details]
WIP

[Myles C. Maxfield]

Created attachment 273399 [details]
WIP

[Myles C. Maxfield]

Created attachment 273547 [details]
WIP

[Myles C. Maxfield]

Created attachment 273548 [details]
WIP

[Myles C. Maxfield]

Created attachment 273554 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 273554 [details]
Patch

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

> Source/WebCore/platform/ScrollAnimator.cpp:247
> +    return true;

Whoops. This needs to be put back the way it was.

[Build Bot]

Comment on attachment 273554 [details]
Patch

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

Number of test failures exceeded the failure limit.

[Build Bot]

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

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

[Build Bot]

Comment on attachment 273554 [details]
Patch

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

New failing tests:
fast/scrolling/rtl-scrollbars-simple.html

[Build Bot]

Created attachment 273559 [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.10.5

[Antonio Gomes]

Created attachment 273563 [details]
webkit-gtk+ with patch applied.

Please observe how webkit-gtk+ gets leftmost content cropped by the rtl scrollbar with patch applied.
Also observe that the rightmost content area now has a dead space/area (that used to correspond to the non-overlay scrollbar area).

basically, for ports that use non-overlay scrollbars for mainframe, patch breaks up content.

[Antonio Gomes]

Comment on attachment 273554 [details]
Patch

Hi Myles. Logic seems to be on the right track, and should work fine for ports that use overlay scrollbars for mainframe.

However, for ports that dont (e.g. minibrowser --gtk), RTL web content gets cropped by the scrollbar itself (see attachment 273563 [details]). Setting r- for now due to that.

[Myles C. Maxfield]

Created attachment 273578 [details]
Patch

[Myles C. Maxfield]

(In reply to comment #12)
> Comment on attachment 273554 [details]
> Patch
> 
> Hi Myles. Logic seems to be on the right track, and should work fine for
> ports that use overlay scrollbars for mainframe.
> 
> However, for ports that dont (e.g. minibrowser --gtk), RTL web content gets
> cropped by the scrollbar itself (see attachment 273563 [details]). Setting
> r- for now due to that.

This patch is intended to be OS X only... I accidentally left ScrollAnimator::scrollbarsAreRTL() broken from my testing in the patch I uploaded. There should be no behavior change on non-OS X ports.

[Myles C. Maxfield]

Created attachment 273579 [details]
Patch

[Antonio Gomes]

(In reply to comment #14)
> (In reply to comment #12)
> > Comment on attachment 273554 [details]
> > Patch
> > 
> > Hi Myles. Logic seems to be on the right track, and should work fine for
> > ports that use overlay scrollbars for mainframe.
> > 
> > However, for ports that dont (e.g. minibrowser --gtk), RTL web content gets
> > cropped by the scrollbar itself (see attachment 273563 [details]). Setting
> > r- for now due to that.
> 
> This patch is intended to be OS X only... I accidentally left
> ScrollAnimator::scrollbarsAreRTL() broken from my testing in the patch I
> uploaded. There should be no behavior change on non-OS X ports.

I see.

Are there plans to support | dir="rtl" | on the frame level to control scrollbar placement?

[Myles C. Maxfield]

(In reply to comment #16)
> (In reply to comment #14)
> > (In reply to comment #12)
> > > Comment on attachment 273554 [details]
> > > Patch
> > > 
> > > Hi Myles. Logic seems to be on the right track, and should work fine for
> > > ports that use overlay scrollbars for mainframe.
> > > 
> > > However, for ports that dont (e.g. minibrowser --gtk), RTL web content gets
> > > cropped by the scrollbar itself (see attachment 273563 [details]). Setting
> > > r- for now due to that.
> > 
> > This patch is intended to be OS X only... I accidentally left
> > ScrollAnimator::scrollbarsAreRTL() broken from my testing in the patch I
> > uploaded. There should be no behavior change on non-OS X ports.
> 
> I see.
> 
> Are there plans to support | dir="rtl" | on the frame level to control
> scrollbar placement?

Different platforms may wish to use different signals to trigger this. The implementation mechanism should be (is) completely divorced from the triggering logic.

On OS X, our goal is to enable this for all pages when the user has their system set up in an RTL environment.

[Myles C. Maxfield]

(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #14)
> > > (In reply to comment #12)
> > > > Comment on attachment 273554 [details]
> > > > Patch
> > > > 
> > > > Hi Myles. Logic seems to be on the right track, and should work fine for
> > > > ports that use overlay scrollbars for mainframe.
> > > > 
> > > > However, for ports that dont (e.g. minibrowser --gtk), RTL web content gets
> > > > cropped by the scrollbar itself (see attachment 273563 [details]). Setting
> > > > r- for now due to that.
> > > 
> > > This patch is intended to be OS X only... I accidentally left
> > > ScrollAnimator::scrollbarsAreRTL() broken from my testing in the patch I
> > > uploaded. There should be no behavior change on non-OS X ports.
> > 
> > I see.
> > 
> > Are there plans to support | dir="rtl" | on the frame level to control
> > scrollbar placement?
> 
> Different platforms may wish to use different signals to trigger this. The
> implementation mechanism should be (is) completely divorced from the
> triggering logic.
> 
> On OS X, our goal is to enable this for all pages when the user has their
> system set up in an RTL environment.

This is why the patch is OS X only - the trigger we are using is a bit of state in the platform environment, rather than the page itself.

[Myles C. Maxfield]

Created attachment 273580 [details]
Patch

[Antonio Gomes]

(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #16)
> > > (In reply to comment #14)
> > > > (In reply to comment #12)
> > > > > Comment on attachment 273554 [details]
> > > > > Patch
> > > > > 
> > > > > Hi Myles. Logic seems to be on the right track, and should work fine for
> > > > > ports that use overlay scrollbars for mainframe.
> > > > > 
> > > > > However, for ports that dont (e.g. minibrowser --gtk), RTL web content gets
> > > > > cropped by the scrollbar itself (see attachment 273563 [details]). Setting
> > > > > r- for now due to that.
> > > > 
> > > > This patch is intended to be OS X only... I accidentally left
> > > > ScrollAnimator::scrollbarsAreRTL() broken from my testing in the patch I
> > > > uploaded. There should be no behavior change on non-OS X ports.
> > > 
> > > I see.
> > > 
> > > Are there plans to support | dir="rtl" | on the frame level to control
> > > scrollbar placement?
> > 
> > Different platforms may wish to use different signals to trigger this. The
> > implementation mechanism should be (is) completely divorced from the
> > triggering logic.
> > 
> > On OS X, our goal is to enable this for all pages when the user has their
> > system set up in an RTL environment.
> 
> This is why the patch is OS X only - the trigger we are using is a bit of
> state in the platform environment, rather than the page itself.

Right. I was wondering because dir="rtl" actually triggers a rtl oriented layout/rendering of the web content.

I got a bit confused that with the patch (on Mac OSX) when user sets its  system preference to show RTL scrollbars, but content it non-rtl, scrollbars are on the left.

However, when content sets dir="rtl" and the system preference is OFF, scrollbars are on the right, although content is rendered starting on the right too.

Patch itself looks ok to me. I would make ::scrollbarsAreRTL a ScrollView method rather than ScrollAnimator, but this should be fine as is.

[Simon Fraser (smfr)]

Comment on attachment 273580 [details]
Patch

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

> Source/WebCore/platform/ScrollAnimator.cpp:244
> +#if !(PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101200)

I think it would be better if this were just #if !PLATFORM(MAC). Put the __MAC_OS_X_VERSION_MIN_REQUIRED version check in the Mac implementation.

> Source/WebCore/platform/ScrollView.cpp:712
> +        IntRect hBarRect(ScrollAnimator::scrollbarsAreRTL() && m_verticalScrollbar ? m_verticalScrollbar->occupiedWidth() : 0,

Feels like some helper functions would make this easier to grok.

> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:1403
> +bool ScrollAnimator::scrollbarsAreRTL()

Seems weird for this to be on ScrollAnimator. ScrollableArea would make more sense.

> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:1405
> +    return [NSScrollerImpPair scrollerLayoutDirection] == NSUserInterfaceLayoutDirectionRightToLeft;

How expensive is it to call this every time we rejigger scrollbars?

> Source/WebCore/platform/spi/mac/NSScrollerImpSPI.h:113
> ++ (NSUserInterfaceLayoutDirection)scrollerLayoutDirection;

Does this need some version guards?

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1763
> +    int xPosition = ScrollAnimator::scrollbarsAreRTL() ? m_renderView.frameView().scrollbarIntrusion().width() : 0;

Shame to call scrollbarIntrusion(), which computes for both scrollbars, then only use its width.

> Tools/WebKitTestRunner/InjectedBundle/mac/InjectedBundleMac.mm:100
> +        [newDictionary setValue:@YES forKey:@"AppleTextDirection"];
> +        [newDictionary setValue:@YES forKey:@"NSForceRightToLeftWritingDirection"];

Do these settings just affect scrollbars, or other things too (like form control rendering?)

[Myles C. Maxfield]

(In reply to comment #20)
> (In reply to comment #18)
> > (In reply to comment #17)
> > > (In reply to comment #16)
> > > > (In reply to comment #14)
> > > > > (In reply to comment #12)
> > > > > > Comment on attachment 273554 [details]
> > > > > > Patch
> > > > > > 
> > > > > > Hi Myles. Logic seems to be on the right track, and should work fine for
> > > > > > ports that use overlay scrollbars for mainframe.
> > > > > > 
> > > > > > However, for ports that dont (e.g. minibrowser --gtk), RTL web content gets
> > > > > > cropped by the scrollbar itself (see attachment 273563 [details]). Setting
> > > > > > r- for now due to that.
> > > > > 
> > > > > This patch is intended to be OS X only... I accidentally left
> > > > > ScrollAnimator::scrollbarsAreRTL() broken from my testing in the patch I
> > > > > uploaded. There should be no behavior change on non-OS X ports.
> > > > 
> > > > I see.
> > > > 
> > > > Are there plans to support | dir="rtl" | on the frame level to control
> > > > scrollbar placement?
> > > 
> > > Different platforms may wish to use different signals to trigger this. The
> > > implementation mechanism should be (is) completely divorced from the
> > > triggering logic.
> > > 
> > > On OS X, our goal is to enable this for all pages when the user has their
> > > system set up in an RTL environment.
> > 
> > This is why the patch is OS X only - the trigger we are using is a bit of
> > state in the platform environment, rather than the page itself.
> 
> Right. I was wondering because dir="rtl" actually triggers a rtl oriented
> layout/rendering of the web content.
> 
> I got a bit confused that with the patch (on Mac OSX) when user sets its 
> system preference to show RTL scrollbars, but content it non-rtl, scrollbars
> are on the left.
> 
> However, when content sets dir="rtl" and the system preference is OFF,
> scrollbars are on the right, although content is rendered starting on the
> right too.
> 

This is accurate.

[Radar WebKit Bug Importer]

<rdar://problem/25090346>

[Myles C. Maxfield]

Comment on attachment 273580 [details]
Patch

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

>> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:1405
>> +    return [NSScrollerImpPair scrollerLayoutDirection] == NSUserInterfaceLayoutDirectionRightToLeft;
> 
> How expensive is it to call this every time we rejigger scrollbars?

We can put this in a static.

[Myles C. Maxfield]

Comment on attachment 273580 [details]
Patch

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

>> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:1403
>> +bool ScrollAnimator::scrollbarsAreRTL()
> 
> Seems weird for this to be on ScrollAnimator. ScrollableArea would make more sense.

I'll add a ScrollableAreaMac.mm (there is no .mm file for ScrollableArea)

>> Source/WebCore/platform/spi/mac/NSScrollerImpSPI.h:113
>> ++ (NSUserInterfaceLayoutDirection)scrollerLayoutDirection;
> 
> Does this need some version guards?

It's just a declaration, so no.

>> Tools/WebKitTestRunner/InjectedBundle/mac/InjectedBundleMac.mm:100
>> +        [newDictionary setValue:@YES forKey:@"NSForceRightToLeftWritingDirection"];
> 
> Do these settings just affect scrollbars, or other things too (like form control rendering?)

Other things too. This is by design. (This is the proper way to mock this setting.)

[Myles C. Maxfield]

Comment on attachment 273580 [details]
Patch

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

>> Source/WebCore/platform/ScrollView.cpp:712
>> +        IntRect hBarRect(ScrollAnimator::scrollbarsAreRTL() && m_verticalScrollbar ? m_verticalScrollbar->occupiedWidth() : 0,
> 
> Feels like some helper functions would make this easier to grok.

I actually don't agree. I can't re-use any of the logic on these 4 lines, and the rest of the addition/subtraction positioning math is already right here. I think that spreading the positioning logic out would actually make it more difficult to understand.

I would agree with you if the positioning math was complicated, but in this case, I don't think it's worth it.

Please reconsider. If you still think that I should move it out, I would be happy to.

[Myles C. Maxfield]

Created attachment 273615 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 273615 [details]
Patch

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

> Source/WebCore/platform/mac/ScrollableAreaMac.mm:34
> +#include "NSScrollerImpSPI.h"

Probably only want to include this for PLATFORM(MAC)

> Tools/WebKitTestRunner/TestController.cpp:1470
> +        WKRetainPtr<WKStringRef> key = adoptWK(WKStringCreateWithUTF8CString("UseRTLScrollbars"));
> +        WKRetainPtr<WKBooleanRef> value = adoptWK(WKBooleanCreate(true));
> +        const WKStringRef keyArray[] = { key.get() };
> +        const WKTypeRef valueArray[] = { value.get() };
> +        return adoptWK(WKDictionaryCreate(keyArray, valueArray, WTF_ARRAY_LENGTH(keyArray)));

Not your fault, but the amount of code need for this is ridiculous.

[Myles C. Maxfield]

Committed r197956: <http://trac.webkit.org/changeset/197956>

[Csaba Osztrogonác]

(In reply to comment #29)
> Committed r197956: <http://trac.webkit.org/changeset/197956>

Apple Mac cmake buildfix landed in http://trac.webkit.org/changeset/198415