454420 - layout.scrollbar.side=0 doesn't work as advertised

Status: NEW

(Core :: Layout, defect)

Description

[Uri Bernstein]

Bug 330863 implemented the layout.scrollbar.side preference, for which the value "0" (the default) is described as "end-side in UI direction".
However, in practice, when layout.scrollbar.side=0, the scrollbar is always on the right, even in RTL localizations.

This is because the layout.scrollbar.side=0 case looks at the value of bidi.direction in order to determine "UI direction", but bidi.direction appears to be a fossil that's always set to 1 (LTR), and is not tied to the locale direction (at least in Firefox).

Comment 1

[(no longer active)]

I think it would be enough to change layout.scrollbar.side to look at the direction of a chrome window to determine the UI direction instead of bidi.direction, right?

Comment 2

[Uri Bernstein]

(In reply to comment #1)

As far as I can tell, yes.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Looking at the direction of a chrome window is hard.  (Which chrome window?  What if they disagree?)

There should be a simple way to find out the appropriate direction here.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(And is what you propose really what's desirable as a default behavior?)

Comment 5

[(no longer active)]

(In reply to comment #3)
> Looking at the direction of a chrome window is hard.  (Which chrome window? 
> What if they disagree?)

I think we would want to use the chrome window containing the browser element for which we want to draw a scrollbar.

(In reply to comment #4)
> (And is what you propose really what's desirable as a default behavior?)

I'm not really sure, to be honest.  Basically, we want as much UI as possible reversed in RTL mode, but since the scrollbar can be considered associated with the content from a user's perspective, and the content might itself be LTR, this can be debated.

Comment 6

[(no longer active)]

Sorry if this idea is too stupid, but is there any way to walk up the frames hierarchy to get our hands on a frame belonging to the chrome document containing the content document (for example, that of <xul:browser>), and then see if the RTL bit for that frame is set?

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'd really prefer not making layout code know about how Firefox sets the browser UI direction.

Comment 8

[(no longer active)]

This pref has the same effect on both nsHTMLScrollFrame and nsXULScrollFrame, so if it's set to 1, it will be content sensitive for HTML content, and will place the scrollbar on the left side for XUL scrollbars (because the XUL content has the RTL flag set).

So, if RTL locales want the scrollbar to always appear on the left or right, they can use values 3 and 2 respectively.  With 1 they get the above behavior.  With 0, what they get is effectively the same as 2.

The way I see it, the 0 value may be redundant anyway, because the locale *knows* its direction, so it can either choose 2 or 3.  So, what is exactly the use case for 0 value anyway?

On a side note, has setting the default value of this pref to 1 ever been discussed?

Comment 9

[(no longer active)]

Another question: what if RTL locales set bidi.direction to 2?  I understand that bidi.direction is not used for this purpose currently, but my question is, would it hurt something?  A quick test on a build of the Persian locale with bidi.direction == 2 didn't show anything broken...

What I'm trying to get at is if the RTL locales set bidi.direction to 2, then they'd get the advertised behavior for layout.scrollbar.side==0.

Comment 10

[Simon Montagu :smontagu]

According to http://www.w3.org/International/docs/html-bidi-requirements/#vertical-scrollbar, the default value of 0 is what we should be doing (if it worked).

Ideally, I would like this to work just from the setting of intl.uidirection, without having to set any other prefs, but I suppose that would contradict comment 7.

Comment 11

[Kang-Hao (Kenny) Lu [:kennyluck]]

(In reply to comment #9)
> Another question: what if RTL locales set bidi.direction to 2?

I think this is what was asked for at the beginning of this bug issue.

> I understand
> that bidi.direction is not used for this purpose currently, but my question is,
> would it hurt something?  A quick test on a build of the Persian locale with
> bidi.direction == 2 didn't show anything broken...

When I set my LTR locale's bidi.direction to 2, the text is aligned on the right side of any LTR page. Does changing bidi.direction change the default value of 'text-align' or the 'ltr' attribute? What's the use case of this property by the way?

Comment 12

[Simon Montagu :smontagu]

The primary use case of bidi.direction was removed in bug 151407, and I plan to remove other cases where it affects the default value of direction.

This opens the door to fixing this bug by making RTL locales set bidi.direction to 2 as suggested above, without the side effect described in comment 11 -- in other words recycling bidi.direction to mean "UI direction".

Or we could remove bidi.direction completely altogether and WONTFIX this.

Thoughts?

Comment 13

[Simon Montagu :smontagu]

On further reflection, I don't think it makes sense to have a pref setting which means "use another pref setting". If we don't want to query for actual chrome direction in layout code, I think we should:

(a) remove 0 from the possible values of layout.scrollbar.side
(b) make the default value 2 (right side)
(c) make it a localizable pref and encourage RTL l10ns to customize the default to 3 (left side)

Comment 14

[(no longer active)]

That sounds good to me.

Comment 15

[Simon Montagu :smontagu]

Attachment: Patch

I don't know if there's a better hoop to jump through to get a pref with a localizable int value.

Comment 16

[Axel Hecht [:Pike]]

Can't we do what CSS does, or query what CSS does? That code path ends up in http://mxr.mozilla.org/mozilla-central/source/content/xul/document/src/XULDocument.cpp#4658.

I'd really hate to have yet another pref that nobody understands in intl.properties.

Comment 17

[Simon Montagu :smontagu]

AIUI that was already proposed and rejected -- see comment 6 and comment 7

Comment 19

[Axel Hecht [:Pike]]

How should the scrollbars behave in a scenario of mixed directions within the UI, say, you're using a hebrew build and an addon dialog that's in English?

Independent of that answer, I'd strongly lobby to get the UI direction from some other value than a number in l10n, those just go wrong too often.

Comment 20

[(no longer active)]

(In reply to Axel Hecht [:Pike] from comment #19)
> How should the scrollbars behave in a scenario of mixed directions within
> the UI, say, you're using a hebrew build and an addon dialog that's in
> English?

They should still adhere to the value of this pref, although perhaps it makes sense to file a follow-up bug to request the context-sensitive value of the pref only be honored for content.

> Independent of that answer, I'd strongly lobby to get the UI direction from
> some other value than a number in l10n, those just go wrong too often.

Even given the fact that we only have three RTL locales?

Comment 21

[(no longer active)]

Comment on attachment 746799 [details] [diff] [review]
Patch

Review of attachment 746799 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsPresContext.cpp
@@ +739,5 @@
>    mEnableJapaneseTransform =
>      Preferences::GetBool("layout.enable_japanese_specific_transform");
>  
> +  nsAdoptingCString pref =
> +    Preferences::GetLocalizedCString("layout.scrollbar.side"); 

Trailing space.

Comment 22

[Axel Hecht [:Pike]]

Comment on attachment 746799 [details] [diff] [review]
Patch

Review of attachment 746799 [details] [diff] [review]:
-----------------------------------------------------------------

Adding a feedback- here for now.

I think we can hack around our three known RTL locales, what I'm concerned about is for all those that are not RTL to get it right. And my experience with the intl.menuitems.insertseparatorbeforeaccesskeys is, those that should use it don't, and those that shouldn't get it wrong.

It's important that the code does the right thing for wrong values, thus my question below.

(And yes, I really should file a bug on accesskeys doing the opposite per default)

::: layout/base/nsPresContext.cpp
@@ +740,5 @@
>      Preferences::GetBool("layout.enable_japanese_specific_transform");
>  
> +  nsAdoptingCString pref =
> +    Preferences::GetLocalizedCString("layout.scrollbar.side"); 
> +  mPrefScrollbarSide = int32_t(atoi(pref.get()));

What happens if the string is garbage here? "yes" or some odd unichar character denoting a number in devangari(sp?) script?

Comment 23

[Simon Montagu :smontagu]

Attachment: alternative patch

Axel, do you like this any better? It should be proof against garbage in the pref, plus I added a heavy warning in the localization note (assuming anybody reads them)

Comment 24

[Axel Hecht [:Pike]]

Comment on attachment 747469 [details] [diff] [review]
alternative patch

Review of attachment 747469 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the lag.

I like this better, I have a few concerns though:

We're changing the existing pref from an int to a string, which means that anybody that had that pref is now borked, right?

I don't think we should expose "end", if there are no good reasons to make it the default.

Not sure how to get out of that.

Comment 25

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.