Open Bug 454420 Opened 13 years ago Updated 8 years ago

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

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

People

(Reporter: uriber, Assigned: smontagu)

References

(Blocks 2 open bugs)

Details

(Keywords: rtl)

Attachments

(2 files)

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).
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?
(In reply to comment #1)

As far as I can tell, yes.
Flags: wanted1.9.1?
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.
(And is what you propose really what's desirable as a default behavior?)
(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.
Flags: wanted1.9.1?
Flags: wanted1.9.1-
Flags: blocking1.9.1-
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?
I'd really prefer not making layout code know about how Firefox sets the browser UI direction.
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?
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.
Blocks: html5bidi
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.
(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?
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?
Blocks: 867936
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)
Flags: needinfo?(ehsan)
That sounds good to me.
Flags: needinfo?(ehsan)
Attached patch PatchSplinter Review
I don't know if there's a better hoop to jump through to get a pref with a localizable int value.
Assignee: nobody → smontagu
Attachment #746799 - Flags: review?(ehsan)
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.
AIUI that was already proposed and rejected -- see comment 6 and comment 7
Duplicate of this bug: 859974
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.
(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 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.
Attachment #746799 - Flags: review?(ehsan) → review+
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?
Attachment #746799 - Flags: feedback-
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)
Attachment #747469 - Flags: feedback?
Attachment #747469 - Flags: feedback? → feedback?(l10n)
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.
Attachment #747469 - Flags: feedback?(l10n) → feedback+
You need to log in before you can comment on or make changes to this bug.