Bug 121224 – Support having scrollbars on the left in right-to-left layout

• Component: Layout and Rendering
• Status: NEW

[Allan Sandfeld Jensen]

Qt application places the scrollbars to the left when using right-to-left layout. This reverse layout is not currently supported by WebCore though.

[Allan Sandfeld Jensen]

Upstream https://bugreports.qt-project.org/browse/QTBUG-22696

[Allan Sandfeld Jensen]

Created attachment 211420 [details]
Patch

[Simon Hausmann]

Comment on attachment 211420 [details]
Patch

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

I'm not really familiar with the ScrollView Qt, would be great if somebody else could take a look at that part. One comment inside though :)

> Source/WebCore/page/Frame.cpp:739
> +#endif

Perhaps this could be done on the caller side (FrameLoaderClientQt), to reduce the amount of #ifdefs here?

[Antonio Gomes]

Comment on attachment 211420 [details]
Patch

[Qt] is misleading in the title.

[Antonio Gomes]

How does this code play together with code under RTL_SCROLLBAR ?

[Simon Fraser (smfr)]

Comment on attachment 211420 [details]
Patch

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

> Source/WebCore/platform/ScrollView.h:141
> +    bool reverseLayout() const;
> +    void setReverseLayout(bool);

I think we need a better term than "reverseLayout", for several reasons:
1. We use layout() as a verb with a specific meaning
2. Unclear what "reverse" means. Does it put the horizontal scrollbar at the top?

Maybe scrollbarOnLeft()?

[Allan Sandfeld Jensen]

(In reply to comment #5)
> How does this code play together with code under RTL_SCROLLBAR ?

This only sets the direction on scrollview scrollbars. It appears RTL_SCROLLBAR does the same for CSS scrollbars based on CSS writing direction.

[Allan Sandfeld Jensen]

Created attachment 211540 [details]
Patch

[Allan Sandfeld Jensen]

Created attachment 211541 [details]
Patch

Remove unrelated change

[Sam Weinig]

No tests?

[Antonio Gomes]

Comment on attachment 211541 [details]
Patch

Is that testable, Allan?

[Allan Sandfeld Jensen]

(In reply to comment #11)
> (From update of attachment 211541 [details])
> Is that testable, Allan?

Not as it is. It would need to be a setting or we would need another interface to force it before we can test it in a layout test.

[Allan Sandfeld Jensen]

Created attachment 212593 [details]
Patch

Now with an attempted pixel test

[Build Bot]

Comment on attachment 212593 [details]
Patch

Attachment 212593 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/2288025

[Build Bot]

Comment on attachment 212593 [details]
Patch

Attachment 212593 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/2260065

[Build Bot]

Comment on attachment 212593 [details]
Patch

Attachment 212593 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/2283005

[Allan Sandfeld Jensen]

Created attachment 212689 [details]
Patch

[Build Bot]

Comment on attachment 212689 [details]
Patch

Attachment 212689 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/2423043

[Allan Sandfeld Jensen]

Created attachment 212700 [details]
Patch

Export symbol to windows too

[Ryosuke Niwa]

(In reply to comment #0)
> Qt application places the scrollbars to the left when using right-to-left layout. This reverse layout is not currently supported by WebCore though.

It is supported in the context of RTL text. See bug 85856 for example.

[Allan Sandfeld Jensen]

(In reply to comment #20)
> (In reply to comment #0)
> > Qt application places the scrollbars to the left when using right-to-left layout. This reverse layout is not currently supported by WebCore though.
> 
> It is supported in the context of RTL text. See bug 85856 for example.

That bug has been closed as WONTFIX. The only part landed (as far as I can tell), is the support for placing scrollbars on left on CSS overflow. But css overflow is handled by RenderLayer and not by ScrollView.

Besides that, this bug is based on placing the scrollbar based on the direction of the parent container and not based on the container itself. I can see arguments against this mechanism, but currently we have neither support scrollbar placement for internal text-direction or external text-direction in ScrollView.

[Antonio Gomes]

Comment on attachment 212700 [details]
Patch

r- . Qt has been removed.

This can possibly be an Application settings, as per the chrome client hook, but it should likely obey what container renderer style says too.