Bug 156948 – [WK2] [OS X] Create API for switching RTL scrollbar policy

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

[WK2] [OS X] Create API for switching RTL scrollbar policy

[Myles C. Maxfield]

Created attachment 277141 [details]
Needs a test

[Myles C. Maxfield]

Created attachment 277269 [details]
WIP

[Myles C. Maxfield]

Created attachment 277300 [details]
Patch

[WebKit Commit Bot]

Attachment 277300 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/text/WritingMode.h:37:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 97 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Myles C. Maxfield]

<rdar://problem/25707757>

[Myles C. Maxfield]

Comment on attachment 277300 [details]
Patch

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

> Source/WebCore/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=156948

<rdar://problem/25707757>

[Myles C. Maxfield]

Created attachment 277319 [details]
Patch

[WebKit Commit Bot]

Attachment 277319 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/text/WritingMode.h:37:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 98 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

Comment on attachment 277319 [details]
Patch

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

> Source/WebCore/page/Settings.h:77
> +    Content = 0,
> +    View = 1

Is it necessary to explicitly assign values to these enumerators?

> Source/WebCore/platform/text/WritingMode.h:37
> +enum TextDirection { LTR = 0, RTL = 1 };

Is it necessary to explicitly assign values to these enumerators?

Can we make this an enum class? This can be done in a separate bug/patch.

> Source/WebCore/rendering/RenderLayer.cpp:3371
> +    RenderBox& box = *renderBox();
> +    ASSERT(&box);

r- because this. This makes use of undefined behavior by asserting &box is not zero. Specifically, (5) of [dcl.ref] of the C++ standard states "A reference shall be initialized to refer to a valid object or function. [Note: in particular, a null reference cannot exist in a well-defined program, because the only way to create such a reference would be to bind it to the "object" obtained by dereferencing a null pointer, which causes undefined behavior...]". I am unclear of clang's behavior, but a compiler can elide the ASSERT() because its expression will always evaluate to true in a well-formed program by definition of a reference. We should ASSERT(renderBox()) before dereferencing it.

> Source/WebCore/testing/InternalSettings.cpp:572
> +    default:
> +        ASSERT_NOT_REACHED();
> +        return String();

We should remove this default case so that the compiler enforces that the switch block covers all enumerators of enum class VerticalScrollbarLocationPolicy.

[Daniel Bates]

For completeness, I am referencing the C++ standard <http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3337.pdf>.

[Myles C. Maxfield]

Created attachment 277337 [details]
Patch

[WebKit Commit Bot]

Attachment 277337 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/text/WritingMode.h:37:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 98 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 277337 [details]
Patch

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

> Source/WebCore/ChangeLog:25
> +        Because WebKit doesn't know which of these situations it is operating within, this
> +        patch adds a new API property, verticalScrollbarLocationPolicy, to
> +        WKWebViewConfigurationPolicy. This allows clients to instruct us which policy to
> +        abide by. It is plumbed to the web process inside the WebPreferencesStore and is
> +        ultimately held inside the Page's Settings object.

Can this API be combined with other "website vs. web technology used to implement native content" settings? I am concerned that we will offer too many separate settings, which is flexible, but makes it too easy to get something wrong.

> Source/WebCore/page/FrameView.cpp:4977
> +bool FrameView::shouldPlaceBlockDirectionScrollbarOnLeft() const
> +{
> +    if (auto* renderView = this->renderView())
> +        return renderView->shouldPlaceBlockDirectionScrollbarOnLeft();
> +    return false;
> +}

I prefer early return or && in functions like this, even though it means we can’t scope the local variable inside the if.

> Source/WebCore/page/FrameView.h:560
> +    bool shouldPlaceBlockDirectionScrollbarOnLeft() const override;

Can this be final instead of override?

> Source/WebCore/page/Settings.h:78
> +enum class VerticalScrollbarLocationPolicy {
> +    Content = 0,
> +    View = 1
> +};

Do we really need the "= 0" and "= 1" here? Do we really need to spread this over two lines? Can this be a boolean instead of an enum?

> Source/WebCore/platform/ScrollableArea.h:318
> +    virtual bool shouldPlaceBlockDirectionScrollbarOnLeft() const = 0;

I am confused by this name, even though it already exists. What is a "block direction scrollbar"? And does this policy really mean "place it on the left"?

> Source/WebCore/platform/text/WritingMode.h:37
> +enum TextDirection { LTR = 0, RTL = 1 };

Why add these? Does this enhance something?

> Source/WebCore/platform/win/PopupMenuWin.h:111
> +    bool shouldPlaceBlockDirectionScrollbarOnLeft() const override { return false; }

Can this be final instead of override?

> Source/WebCore/rendering/RenderLayer.h:674
> +    bool shouldPlaceBlockDirectionScrollbarOnLeft() const override { return renderer().shouldPlaceBlockDirectionScrollbarOnLeft(); }

Can this be final instead of override?

> Source/WebCore/rendering/RenderLayerModelObject.cpp:227
> +#if PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101200
> +    switch (frame().settings().verticalScrollbarLocationPolicy()) {
> +    case VerticalScrollbarLocationPolicy::Content:
> +        return style().shouldPlaceBlockDirectionScrollbarOnLeft();
> +    case VerticalScrollbarLocationPolicy::View:
> +        return frame().settings().systemLayoutDirection() == RTL;
> +    }
> +#else
> +    return false;
> +#endif

This seems peculiar. Why would this code be checking for OS X explicitly and for a specific version. Can’t that policy be applied somewhere that policy naturally belongs and where platform specifics should be instead of inside RenderLayerModelObject?

> Source/WebCore/rendering/RenderListBox.h:159
> +    bool shouldPlaceBlockDirectionScrollbarOnLeft() const override { return RenderBlockFlow::shouldPlaceBlockDirectionScrollbarOnLeft(); }

Can this be final instead of override?

> Source/WebCore/rendering/style/RenderStyle.cpp:2083
> +#if PLATFORM(MAC) || USE(RTL_SCROLLBAR)
>      return !isLeftToRightDirection() && isHorizontalWritingMode();
>  #else
>      return false;

Can’t this policy be applied somewhere that policy naturally belongs and where platform specifics should be instead of inside RenderStyle?

> LayoutTests/fast/scrolling/rtl-scrollbars-elementFromPoint-static.html:7
> +    window.internals.settings.setVerticalScrollbarLocationPolicy("View");
> +    window.internals.settings.setSystemLayoutDirection("RTL");

No need for "window." here. Same for all the other cases below.

[Simon Fraser (smfr)]

Comment on attachment 277337 [details]
Patch

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

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:419
> +    // Therefore, according to the docs, "this property contains the value reported by the appâs userInterfaceLayoutDirection property."

Which docs?

> Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.h:83
> + specified, the placement of vertical scrollbars is affected by the direction of the system.

"direction of the system" is a bit vague.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:226
> +    self.verticalScrollbarLocationPolicy = static_cast<WKVerticalScrollbarLocationPolicy>([coder decodeIntegerForKey:@"verticalScrollbarLocationPolicy"]);

I don't think you should blindly static_cast<>. You should check that the value is an allowed one.

[Anders Carlsson]

Comment on attachment 277337 [details]
Patch

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

>> Source/WebCore/platform/text/WritingMode.h:37
>> +enum TextDirection { LTR = 0, RTL = 1 };
> 
> Why add these? Does this enhance something?

I agree, no need to give these values.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3076
> +    settings.setVerticalScrollbarLocationPolicy(!store.getUInt32ValueForKey(WebPreferencesKey::verticalScrollbarLocationKey()) ? VerticalScrollbarLocationPolicy::Content : VerticalScrollbarLocationPolicy::View);
> +    settings.setSystemLayoutDirection(static_cast<TextDirection>(store.getUInt32ValueForKey(WebPreferencesKey::verticalScrollbarLocationKey())));

Don't use static casts here, instead add a conversion function from the WK2 enum to the WebCore enum.

[Anders Carlsson]

Comment on attachment 277337 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.h:83
>> + specified, the placement of vertical scrollbars is affected by the direction of the system.
> 
> "direction of the system" is a bit vague.

WKVerticalScrollbarLocationPolicy sounds awfully specific. How about something like WKUserInterfaceDirectionPolicy, with values .Content and .System?

[Myles C. Maxfield]

Comment on attachment 277337 [details]
Patch

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

>> Source/WebCore/ChangeLog:25
>> +        ultimately held inside the Page's Settings object.
> 
> Can this API be combined with other "website vs. web technology used to implement native content" settings? I am concerned that we will offer too many separate settings, which is flexible, but makes it too easy to get something wrong.

I agree with this.

I can think of 3 scenarios:
1. I piggyback off of an existing setting. However, it doesn't seem like the names of any existing settings are general enough for it not to be weird that it also changes scrollbars.
2. I make a new setting: BrowserMode vs AppMode. The implementation of this mode sets a bunch of existing settings. (However, I can't remove the existing settings because they are API.)
3. Keep fine grained controls

I'm not quite sure which of these is the best way to go.

>> Source/WebCore/page/Settings.h:78
>> +};
> 
> Do we really need the "= 0" and "= 1" here? Do we really need to spread this over two lines? Can this be a boolean instead of an enum?

Yeah, this is for the serialization between the UI process and the web process (the enum is encoded as an int). There are already some examples of code which serializes an enum, and they simply cast the WK2 type as the WebCore type. I'm adding these values to guarantee that the cast works correctly.

>> Source/WebCore/platform/ScrollableArea.h:318
>> +    virtual bool shouldPlaceBlockDirectionScrollbarOnLeft() const = 0;
> 
> I am confused by this name, even though it already exists. What is a "block direction scrollbar"? And does this policy really mean "place it on the left"?

It's the scrollbar which scrolls in the direction of block progression (So it's vertical in regular writing-modes, and horizontal in vertical-writing-modes)

>> Source/WebCore/rendering/RenderLayerModelObject.cpp:227
>> +#endif
> 
> This seems peculiar. Why would this code be checking for OS X explicitly and for a specific version. Can’t that policy be applied somewhere that policy naturally belongs and where platform specifics should be instead of inside RenderLayerModelObject?

This is because of the animation of overlay scrollbars. On most versions of OS X, the scrollbars always animate from right to left when you hover the cursor over them (because they are expected to be placed on the right edge of their contents). If this same animation is used when the scroller is placed on the left edge of the contents, the result is very unnatural. We have elected to simply disable this feature entirely on those versions of OS X.

However, the story I just told only holds for OS X. Perhaps I should enable this feature for the other ports.

I describe in the ChangeLog why the renderer is a good place for this logic, but I do agree that it is surprising that scrollbar placement logic in the renderer should be platform-specific. Do you have any ideas about a better place to put this PLATFORM check? Maybe the best solution is a comment.

>> Source/WebCore/rendering/style/RenderStyle.cpp:2083
>>      return false;
> 
> Can’t this policy be applied somewhere that policy naturally belongs and where platform specifics should be instead of inside RenderStyle?

Maybe I should force RTL_SCROLLBAR to be on for all ports.

> LayoutTests/fast/scrolling/rtl-scrollbars-iframe-position-absolute.html:-1
> -<!DOCTYPE html><!-- webkit-test-runner [ rtlScrollbars=true ] -->

I should delete the handling for this "rtlScrollbars" stuff in WKTR

[Anders Carlsson]

(In reply to comment #17)
> > Do we really need the "= 0" and "= 1" here? Do we really need to spread this over two lines? Can this be a boolean instead of an enum?
> 
> Yeah, this is for the serialization between the UI process and the web
> process (the enum is encoded as an int). There are already some examples of
> code which serializes an enum, and they simply cast the WK2 type as the
> WebCore type. I'm adding these values to guarantee that the cast works
> correctly.

We don't need these. Please add a conversion function between these.

[Myles C. Maxfield]

Created attachment 277418 [details]
Addressed some review comments

[Myles C. Maxfield]

Created attachment 277422 [details]
Addressed some review comments

[Myles C. Maxfield]

Comment on attachment 277337 [details]
Patch

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

>>> Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.h:83
>>> + specified, the placement of vertical scrollbars is affected by the direction of the system.
>> 
>> "direction of the system" is a bit vague.
> 
> WKVerticalScrollbarLocationPolicy sounds awfully specific. How about something like WKUserInterfaceDirectionPolicy, with values .Content and .System?

I like "System" because it allows us to improve the implementation of this in the future to honor post-creation changes to userInterfaceLayoutDirection on the NSView.

[Myles C. Maxfield]

Committed r200116: <http://trac.webkit.org/changeset/200116>

Comment on attachment 277422 [details]
Addressed some review comments

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

> Source/WebCore/ChangeLog:23
> +        WKWebViewConfigurationPolicy. This allows clients to instruct us which policy to

WKWebViewConfiguration.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.h:87
> +    WKVerticalScrollbarLocationPolicyContent = 0,
> +    WKVerticalScrollbarLocationPolicyView = 1,

We typically don’t assign specific integers to enum values like  this.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.h:145
> +@property (nonatomic) WKVerticalScrollbarLocationPolicy verticalScrollbarLocationPolicy WK_AVAILABLE(WK_MAC_TBA, NA);

Cocoa API typically refers to control as a scroller. In the few occasions that the API uses the term “scroll bar”, it is made up of two separate words, such as in +[NSColor scrollBarColor] or NSAccessibilityScrollBarRole. I think the naming here should align with the rest of the Cocoa API.

Why is this not available in iOS? Are scrolling indicators never used for inner scrollable areas in iOS?

If you decide to keep this unavailable in iOS for some reason, then the enum definition and the property declaration should be guarded by #if !TARGET_OS_IPHONE.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:103
> +#include "WebPreferencesKeys.h"

Why did this become necessary?

[Csaba Osztrogonác]

(In reply to comment #22)
> Committed r200116: <http://trac.webkit.org/changeset/200116>

It broke the Apple Mac cmake build.

[Csaba Osztrogonác]

fix landed in http://trac.webkit.org/changeset/200122

[Myles C. Maxfield]

Comment on attachment 277422 [details]
Addressed some review comments

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

>> Source/WebKit2/UIProcess/API/Cocoa/WKWebViewConfiguration.h:145
>> +@property (nonatomic) WKVerticalScrollbarLocationPolicy verticalScrollbarLocationPolicy WK_AVAILABLE(WK_MAC_TBA, NA);
> 
> Cocoa API typically refers to control as a scroller. In the few occasions that the API uses the term “scroll bar”, it is made up of two separate words, such as in +[NSColor scrollBarColor] or NSAccessibilityScrollBarRole. I think the naming here should align with the rest of the Cocoa API.
> 
> Why is this not available in iOS? Are scrolling indicators never used for inner scrollable areas in iOS?
> 
> If you decide to keep this unavailable in iOS for some reason, then the enum definition and the property declaration should be guarded by #if !TARGET_OS_IPHONE.

I honored Anders's comment below and renamed this to userInterfaceDirectionPolicy.

There is no reason why this shouldn't exist in iOS; however, all my work has only been on OS X.