478416 - Replace chromedir with something more sane

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Neil Deakin]

Currently all these chromedir="&locale.dir;" attributes have been scattered about all of the widgets. This is a rather stupid way to specify the chrome direction as it requires hundreds of extra attributes to be set to something which always has the same value (&locale.dir; is an application-global value which cannot be changed without restarting)

Instead, as this is mostly done for the benefit of supporting rtl for themes, this should be done as a css feature, something like:

button:chrome-rtl { ... }

Comment 1

[Axel Hecht [:Pike]]

Note, this may raise interesting problems when facing a en-US extension in a RTL-locale Firefox. Menus shouldn't bother, probably, but dialogs would look funky.

locale.dir is only app-global, it doesn't need to (and probably shouldn't) be used in extensions for that reason.

Comment 2

[Neil Deakin]

(In reply to comment #1)
> Note, this may raise interesting problems when facing a en-US extension in a
> RTL-locale Firefox. Menus shouldn't bother, probably, but dialogs would look
> funky.
> 
> locale.dir is only app-global, it doesn't need to (and probably shouldn't) be
> used in extensions for that reason.

This is a toolkit widgets bug. The XUL widgets will use the same global value of locale.dir whether they are used in Firefox or in any extension.

Comment 3

[(no longer active)]

(In reply to comment #1)
> Note, this may raise interesting problems when facing a en-US extension in a
> RTL-locale Firefox. Menus shouldn't bother, probably, but dialogs would look
> funky.
> 
> locale.dir is only app-global, it doesn't need to (and probably shouldn't) be
> used in extensions for that reason.

What kinds of problems are you referring to?  In the current system where
chromedir attributes are used, any extension which make use of &locale.dir;
will get the correct value for both RTL and LTR versions of Firefox.  In the
proposed version where the chrome-rtl pseudo class is available, then all
extensions which use it also get the correct value.

BTW, this assumption that the chrome direction is constant until a restart is
certainly invalid in the current system.  In any future overhaul of this
feature, I don't think that we should attempt to make it so.  In fact, I'd like
future facilities to support changing the chrome direction on the fly through a
well-defined API, instead of the current hacks which are necessary now.

Comment 4

[Neil Deakin]

Should we assume that the entire application in all one direction, or should we allow each window to have a different direction?

If the latter, is it sufficient to assume that the computed value of the direction property on the root element (window, dialog, etc) is able to accurately determine the direction of a window?

Currently the direction is specified in both locale.dtd, in intl.css as well as the bidi preferences. Is there some preferred way either now or in the future in which either localizers or users change the direction?

Comment 5

[(no longer active)]

(In reply to comment #4)
> Should we assume that the entire application in all one direction, or should we
> allow each window to have a different direction?

Direction should normally be set in an application specific manner.  I'm not sure if there actually is a use case where we would want different windows in different directions, and this is certainly not possible currently.

> If the latter, is it sufficient to assume that the computed value of the
> direction property on the root element (window, dialog, etc) is able to
> accurately determine the direction of a window?

Yes, it is.  As I understand it though, the problem has always been not being able to specify different CSS rules for LTR and RTL windows, because computed styles are not available to use in selectors.  That's why the chromedir attribute is being used.

> Currently the direction is specified in both locale.dtd, in intl.css as well as
> the bidi preferences. Is there some preferred way either now or in the future
> in which either localizers or users change the direction?

I don't think bidi.direction is something we want to worry about too much, because AFAIK that isn't used in RTL locales, and affects both chrome and content, which is definitely not what we want.

I think the preferred way to improve the current situation is to offer a service of some sort which is supposed to manage the UI direction.  The service may be able to read the current direction from a properties file, and that would be the place that localizers would determine whether they want their UI to be RTL or LTR.  We can then expose a pseudo-class like :rtl for example which would apply only in case that service specifies that the direction is RTL.  This way we can use this pseudo-class instead of chromedir to specify alternate style rules for RTL UI.  We can also support changing the direction via that same service, so that things such as the Force RTL extension can be easily implemented in terms of that service (and so that they wouldn't have to resort to scary DOM tree walking hacks to get all chromedir attributes right!).

Also, if we do this, we can easily have something like:

window:rtl, dialog:rtl, ...
{
  direction: rtl;
}

in xul.css so that RTL locales wouldn't have to worry about editing intl.css for setting the direction.  This way, they can just switch from ltr to rtl in the properties file I mentioned, and everything will work right away.

What do you think?

Comment 6

[Axel Hecht [:Pike]]

We have a rather week multi-locale story in general. There are a few different ways to get into that scenario:

- partial translation, i.e., a RTL locale has remaining English strings in it. A reasonable scenario would be XSLT error messages in the error console.
- mixed translations due to extensions
-- here we have overlays messing with existing windows and
-- new UI

From a UE perspective, I'd think that a per-window granularity is the right context to do RTL-ness of things like groupboxes. For preference panes, I'm torn. For the error console, I don't have any good idea, really. AFAICT, there's even strings from the web showing up there, isn't it? Like
throw "Dies ist kein Liebeslied"
would literally show up there? Same goes for a RTL string?

My point is, no, we cannot generally say which direction a window has. Even worse, not even per string or widget. I'm not sure how much it helps that we can in theory tell from the string whether it's RTL or LTR (and if that'd be perfy). I recall smontagu having a bug on an API for that. The chrome registry isn't of much help either, as it's only exposing information per package, the package doesn't really say anything about from which shippable bundle the localized strings are coming from, though.

We should get to a point where hacks in intl.css are not required, hopefully. Not that I have anything really constructive to say about that, sadly.

Comment 7

[Neil Deakin]

Both of the last two comments conflict with each other. Can you work out what exactly you want here?

I'm not really concerned right now about adding any new functionality. I just want to replace the very inefficient current method of using chromedir with something else. I think the idea of using a global flag is sufficient for now as in:

window:rtl { ... }

Comment 8

[Neil Deakin]

Ehsan, where were you thinking that the service should live? And where would it determine the desired direction from? A properties file? Preference? dtd?

Comment 9

[Neil Deakin]

Attachment: possible patch

This patch does the following:

- expects to use chromedir only on the root element (window, dialog, etc)
- :-moz-doc-direction(rtl) may be used to match if the root element has chromedir="rtl"
- to change the direction for a window, just change the chromedir attribute on it. I was thinking also that a pref could be added with provided a general override.

Thoughts?

Comment 11

[Asaf Romano (gone)]

Most extension don't provide he/ar/etc string, and would not override "chromedir" on their windows, thus, I think we should set chromedir on each window, and not on the toolkit side.

That's, practically. Ideally, the "supports rtl ui" "flag" should be set in a place like chrome.manifest, in which a locale package would define itself as "RTL". Then, if any[1] of the locale files referenced in a xul file is loaded from a RTL package, we would mark that window as RTL. I have no idea how to implement this sort of behavior

---
1. "Any" and not "All", because if an overlay add a locale file to say, browser.xul, the direction of it should not be altered. An interesting theoretical case is installing a rtl-string-only extension into a LTR browser.

Comment 12

[Neil Deakin]

(In reply to comment #11)
> Most extension don't provide he/ar/etc string, and would not override
> "chromedir" on their windows, thus, I think we should set chromedir on each
> window, and not on the toolkit side.

That seems like it would mean that if a window didn't set the chromedir attribute at all then it couldn't be localized into a right-to-left language.

> That's, practically. Ideally, the "supports rtl ui" "flag" should be set in a
> place like chrome.manifest, in which a locale package would define itself as
> "RTL".

OK, what about if we do this, then when a document's locale is right-to-left, we make the document right-to-left. For instance, for the document chrome://something/content/file.xul, if the locale for 'something' is right to left. Overlays don't affect this.

Then the chromedir isn't used as all, or perhaps is just used to override this and hardcode a direction.

Comment 13

[Asaf Romano (gone)]

I "expected" overlays to be a problem, If they indeed aren't, this proposal seems more than fine. I actually cannot think of a use case for overriding it.

Comment 14

[Neil Deakin]

Attachment: implement the behaviour described above

This patch adds a righttoleft flag for the locale lines in chrome .manifest files. When set, it enables a document flag for xul files loaded from the corresponding package. Then, one can use:

element:moz-doc-direction(rtl) { ... }

I'll still need to figure out a way of testing this

Comment 15

[(no longer active)]

Sorry for missing the discussion here (it turns out that I had mistakenly muted this thread in gmail :( )

Neil, I think what you're doing here is great.  We can track the discussion of how to extend the existing functionality in a separate bug (or a .platform thread).  I still need to think more on this.

But in the mean time, I have one suggestion for your patch.  How about defaulting some of the locales which we already know that are RTL to righttoleft, so that for example a he locale entry in chrome.manifest is set to righttoleft, unless |righttoleft=false| is specified in chrome.manifest?  The current RTL locales in Mozilla include "he", "ar" and "fa".  We could store this list in a pref if needed.

In order to write an automated test, you can start by doing something similar to <http://hg.mozilla.org/mozilla-central/file/57dc99fc19e6/toolkit/content/tests/chrome/test_bug437844.xul#l126> for registering a chrome.manifest file during runtime.  After that, you can load XUL documents which reference the special locale files in iframe's, and have them load a stylesheet which provides different styles for :moz-doc-direction(rtl) and :moz-doc-direction(ltr).  From there you can get their computed style to verify that the correct style has been applied, and hence it has all worked correctly.

Comment 16

[Neil Deakin]

Attachment: include a test

This should test the feature sufficiently. I haven't included any default settings for some languages.

I'll make a .platform post to see if anyone has any objections/additional ideas.

Comment 17

[Dão Gottwald [:dao]]

Comment on attachment 370632 [details] [diff] [review]
include a test

>diff --git a/toolkit/content/xul.css b/toolkit/content/xul.css

>+window:-moz-doc-direction(rtl),
>+dialog:-moz-doc-direction(rtl),
>+wizard:-moz-doc-direction(rtl),
>+prefwindow:-moz-doc-direction(rtl),
>+page:-moz-doc-direction(rtl)
>+{
>+  direction: rtl;
>+}

Are you intentionally avoiding :root?

Comment 18

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

Maybe the pseudo-class should be called :-moz-chrome-direction rather than :-moz-doc-direction?  It seems to be more about the overall chrome than about a specific document; :-moz-doc-direction sounds like something that would change given :root { direction:rtl }, which this doesn't (and shouldn't).

Comment 19

[Asaf Romano (gone)]

-moz-locale-direction

Comment 20

[Neil Deakin]

> Are you intentionally avoiding :root?

I think only to avoid it matching non-xul elements. This patch only implements the pseudoclass for xul documents currently though.

> -moz-locale-direction

I like this option

Comment 21

[Dão Gottwald [:dao]]

Also, xul.css has the XUL namespace as the default namespace, so :root shouldn't apply to other elements.

Comment 22

[Asaf Romano (gone)]

Isn't xul.css applied only onto xul elements? That is, if the root element isn't a xul element, xul.css wouldn't be applied on it unless it's loaded explicitly, would it?

Comment 23

[Neil Deakin]

(In reply to comment #21)
> Also, xul.css has the XUL namespace as the default namespace, so :root
> shouldn't apply to other elements.

Ah, yes, that makes sense. I'll change to use :root then.

(In reply to comment #22)
> Isn't xul.css applied only onto xul elements?

xul.css is loaded in the user agent style sheet and is applied to every document that is loaded.

Comment 24

[Neil Deakin]

Attachment: use -moz-locale-direction

Comment 25

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

Could you add some tests for the selector parsing to layout/style/test/test_selectors.html ?  In particular, some calls to test_parseable and test_balanced_unparseable to make sure that identifiers are accepted as arguments and strings are not, and that only the identifiers you expect are accepted, and that extra stuff inside the arguments is not accepted (e.g., ":-moz-locale-direction(ltr, foo)"), and that the pseudo-class without any arguments ":-moz-locale-direction()" and without any parentheses at all ":-moz-locale-direction" are rejected?

It would probably be good to add tests for the selector matching as well if there's an easy way to do it.

Comment 26

[Neil Deakin]

Attachment: add tests to test_selectors.html

Also fixes the case for -moz-locale-direction(other) which wasn't being set to false.

The actual selector matching is tested in test_righttoleft.xul, by loading a file using an ltr-marked document and as an rtl-marked document and comparing the computed style for the background-color (set in the testcase) and direction (which comes from xul.css)

Comment 27

[Benjamin Smedberg]

So, there seem to be two separate questions here, and I'm not sure I have clear how we're solving them:

1) How do you set the RTL-ness of a Window?
2) How can random CSS know the direction in order to set icons and other details correctly?

-moz-locale-direction solves problem #2, right? Once we've established the RTL-ness of a XUL window (or chrome window?... looks like right now this only applies to XUL windows), the CSS selector can be used to make the necessary theme adjustments.

But I'm not sure that using the chrome registry is the right way to solve problem #1. HTML already has a dir="" attribute, right? Can we just re-use that on the root element of the XUL document, to specify the RTL-ness?

Adding a chrome registry flag for RTL locales will require some build-system goop for locale repackaging which I'm not sure is the best way to go about this.

Comment 28

[Neil Deakin]

(In reply to comment #27)
> 1) How do you set the RTL-ness of a Window?

You don't. It needs to be determined automatically based on the locale the user is using.

> But I'm not sure that using the chrome registry is the right way to solve
> problem #1. HTML already has a dir="" attribute, right? Can we just re-use that
> on the root element of the XUL document, to specify the RTL-ness?
> 

No, because that would require a xul author to set a dir attribute just to get rtl working even though the author isn't aware of rtl locales. The author should not have to do anything, and the user should only need to install a rtl locale.

Comment 29

[Benjamin Smedberg]

I disagree. The RTLness of a document (XUL or HTML) should be determined by the document itself. Authors should have to set something on the root element if they want their window to change direction with the locale.

I agree that spreading chromedir="" throughout XBL bindings and using attribute selectors is bad, and it's good we're fixing that. But I really think we should keep dir="" or lang="" (inferring RTLness from the language) as a toplevel attribute to set the document direction.

Comment 30

[Neil Deakin]

(In reply to comment #29)
> I disagree. The RTLness of a document (XUL or HTML) should be determined by the
> document itself. Authors should have to set something on the root element if
> they want their window to change direction with the locale.

I think the point is that authors don't know they 'want their window to change direction'.

I've based my implementation on the ideas given by the two people here who use right-to-left locales. If I've made some incorrect assessment I'd like to know what it is.

Comment 31

[(no longer active)]

(In reply to comment #30)
> (In reply to comment #29)
> > I disagree. The RTLness of a document (XUL or HTML) should be determined by the
> > document itself. Authors should have to set something on the root element if
> > they want their window to change direction with the locale.
> 
> I think the point is that authors don't know they 'want their window to change
> direction'.
> 
> I've based my implementation on the ideas given by the two people here who use
> right-to-left locales. If I've made some incorrect assessment I'd like to know
> what it is.

Neil is right, here.  The whole point is that authors should not care/know what direction their window will appear in.

Here is one use case: suppose that extension A is written by someone who doesn't even know that some languages are written from right to left.  A's author would create the extension's windows/etc as they do always, and package their extension.  When I install A in an RTL build (assuming that A doesn't have any matching translation to my locale), A's windows should appear LTR, since English text on an RTL locale looks weird.  Now, if I submit a translation for A into my locale, the next time that A is updated it should automagically figure out that the extension is now available in my RTL language, and the UI should appear RTL this time.

Please note that if an author wants their extension to always appear in a given locale (for whatever reason, although I can't imagine one now), they can specify |window { direction: ltr/rtl !important; }| in their CSS and their desired direction is used instead of the locale direction.

The root issue here is that the directionality of a window is a property of the locale the window appears in, not of the window itself, and tying this to the window as it's currently the case is a mistake.

Comment 32

[Benjamin Smedberg]

The locale a window appears in is a property of the document itself, because we use entities to localize the window. If you write a XUL document with English text and no entities, it should be LTR no matter what other things are registered in the chrome registry.

If you then localize the XUL document using entities, you should also localize the document direction using dir="&mypackage.dir;" or some equivalent mechanism.

Comment 33

[(no longer active)]

(In reply to comment #32)
> The locale a window appears in is a property of the document itself, because we
> use entities to localize the window.

That's right.  My point is that for example if a window is localized into Hebrew/Arabic/Persian, then it should appear RTL.  This RTL-ness is a property of the locale (because the language is RTL).

> If you write a XUL document with English
> text and no entities, it should be LTR no matter what other things are
> registered in the chrome registry.

If you do that, then you're probably not using a locale chrome package at all, so your window will be LTR.  That's the behavior of this patch, IINM.  Neil, please correct me if I'm wrong.

> If you then localize the XUL document using entities, you should also localize
> the document direction using dir="&mypackage.dir;" or some equivalent
> mechanism.

99+% of the authors of XUL documents assume that the direction is always LTR (and it doesn't need to change for some locales), so this solution will fail in practice.

Comment 34

[Benjamin Smedberg]

> 99+% of the authors of XUL documents assume that the direction is always LTR
> (and it doesn't need to change for some locales), so this solution will fail in
> practice.

That may be so, but I think adding additional magic to the chrome registry just makes it more confusing, rather than requiring in-tree XUL to use the correct entity and education extension authors. We could even make it part of the new extension verification steps that each toplevel XUL doc has a dir="" attribute.

Comment 35

[(no longer active)]

Neil, what do you feel about moving the knowledge about which locale should be RTL to the preferences service, for example?  We could have prefs like "locale.xx.direction" with values either "rtl" or "ltr" (default being "ltr" in case the pref doesn't exist, and use them instead of adding the direction knowledge to the chrome registry.  This way RTL locales can add this pref for their locale, and extension authors can query/set this pref if needed...

Comment 36

[Neil Deakin]

Attachment: use preferences instead of the chrome registry

This implements Ehsan's idea of using a preference, with the default set for the three locales which are rtl. It also changes 'chromedir' to 'localedir'.

This means:

- an unlocalized english window will appear ltr
- an unlocalized arabic window will appear ltr; the extension should set localedir="rtl" to appear rtl
- a localized window will appear in the appropriate direction for its locale

I'm not sure what should happen for non-chrome files (say a locally stored xul file). One can set the localedir attribute if needed, otherwise, it will just use the browser's locale to retrieve the preference.

I really don't like the idea of requiring people to add an attribute to every window just to get localization to work. It's bad enough that the global stylesheet has to be included.

This solution in this patch requires neither xul developers nor locales to change anything.

Comment 37

[(no longer active)]

(In reply to comment #36)
> Created an attachment (id=372669) [details]
> use preferences instead of the chrome registry
> 
> This implements Ehsan's idea of using a preference, with the default set for
> the three locales which are rtl. It also changes 'chromedir' to 'localedir'.
> 
> This means:
> 
> - an unlocalized english window will appear ltr
> - an unlocalized arabic window will appear ltr; the extension should set
> localedir="rtl" to appear rtl
> - a localized window will appear in the appropriate direction for its locale

Looks great, thanks!

One more idea: would it be possible to set up a pref listener to catch changes to intl.uidirection.<locale> so that we can update chrome UI direction when that pref changes?  (Yes, I'm thinking of a way to make Force RTL obsolete.  If you want, I can file a bug and do that myself).

> I'm not sure what should happen for non-chrome files (say a locally stored xul
> file). One can set the localedir attribute if needed, otherwise, it will just
> use the browser's locale to retrieve the preference.

I'm thinking that if such files are loaded as content, then we should make them LTR by default and have authors specify localedir if they need their document to appear RTL.  This is the same behavior that we get with HTML content; in RTL builds HTML content remains LTR by default.

If however chrome windows are loaded using file:// URIs, then I think we should give them the default locale direction.  Of course I'm not sure if that's possible at all, or if there is actually a use case for that.  At any rate, this is an edge case I think.

BTW, what happens with about: URIs (such as about:config)?

> I really don't like the idea of requiring people to add an attribute to every
> window just to get localization to work. It's bad enough that the global
> stylesheet has to be included.
> 
> This solution in this patch requires neither xul developers nor locales to
> change anything.

I think this is the best tradeoff that we can get.  Great stuff, Neil!  :-)

Comment 38

[Neil Deakin]

(In reply to comment #37)
> One more idea: would it be possible to set up a pref listener to catch changes
> to intl.uidirection.<locale> so that we can update chrome UI direction when
> that pref changes?

Yes, I just didn't do that for this patch.


> I'm thinking that if such files are loaded as content, then we should make them
> LTR by default and have authors specify localedir if they need their document
> to appear RTL.  This is the same behavior that we get with HTML content; in RTL
> builds HTML content remains LTR by default.

OK, I'll make that change.


> If however chrome windows are loaded using file:// URIs, then I think we should
> give them the default locale direction.

That isn't a case we really need to worry about too much, but I think it should behave the same as other content-loaded file urls.


> BTW, what happens with about: URIs (such as about:config)?

We should probably have about and resource urls use the default locale.


> I think this is the best tradeoff that we can get.  Great stuff, Neil!  :-)

Yes, it still allows people to use Benjamin's model of using localedir="&dir;" if they wanted to hardcode it, but picks a suitable default otherwise.

Comment 39

[Neil Deakin]

Attachment: add preference update, support about/resource uris

Comment 40

[Neil Deakin]

Ehsan, is it ok to just use the two-character prefix for right-to-left?

Comment 41

[(no longer active)]

(In reply to comment #38)
> (In reply to comment #37)
> > One more idea: would it be possible to set up a pref listener to catch changes
> > to intl.uidirection.<locale> so that we can update chrome UI direction when
> > that pref changes?
> 
> Yes, I just didn't do that for this patch.

Filed bug 488820 for that.

> > If however chrome windows are loaded using file:// URIs, then I think we should
> > give them the default locale direction.
> 
> That isn't a case we really need to worry about too much, but I think it should
> behave the same as other content-loaded file urls.

Agreed.

> > BTW, what happens with about: URIs (such as about:config)?
> 
> We should probably have about and resource urls use the default locale.

Sounds reasonable.

Comment 42

[(no longer active)]

(In reply to comment #39)
> Created an attachment (id=372948) [details]
> add preference update, support about/resource uris

Oh, I didn't see this before filing bug 488820.  I'll mark that INVALID then.  Thanks!

Comment 43

[(no longer active)]

(In reply to comment #40)
> Ehsan, is it ok to just use the two-character prefix for right-to-left?

Yes.  fa/ar/he are always RTL, no matter what country they're spoken in.

BTW, your latest patch lacks the test_righttoleft.xul file, probably missed an |hg add|.

Comment 44

[Simon Montagu :smontagu]

(In reply to comment #43)
> (In reply to comment #40)
> > Ehsan, is it ok to just use the two-character prefix for right-to-left?
> 
> Yes.  fa/ar/he are always RTL, no matter what country they're spoken in.

Yes, but we might have locales in the future where this isn't true. Examples of languages that can be written in different directions are Kurdish (which is written in Latin script in Turkey and Arabic script in Iraq and Iran), Punjabi (either LTR Gurmukhi script or RTL Shahmukhi), and I think several languages from the Asian former USSR republics that can be written in either Cyrillic or Arabic scripts.

Comment 45

[Simon Montagu :smontagu]

It also seems a little strange to me that you're adding locale-specific prefs to all.js rather than using intl.properties or something. What will new l10ns that need to set direction to RTL do?

Comment 46

[Neil Deakin]

Set(In reply to comment #45)
> It also seems a little strange to me that you're adding locale-specific prefs
> to all.js rather than using intl.properties or something. What will new l10ns
> that need to set direction to RTL do?

Set it in firefox-l10n.js

Using prefs instead of a properties file allows it to be changed while running. See comment 37.

> Yes, but we might have locales in the future where this isn't true.

OK, I'll change the patch to check for the specific language code as well, falling back on the two-letter code if specified.

Comment 47

[Axel Hecht [:Pike]]

I know that that kind of string processing is ugh in c++, but in theory, we should have a set of languages with default scripts set to LTR, aka, ar, fa, he. And then see if the language tag contains a script tag Arab, or other RTL scripts.

http://www.w3.org/International/articles/language-tags/#script has the construct, and http://unicode.org/iso15924/iso15924-codes.html has the list of codes. smontagu probably knows a few that are RTL.

Comment 48

[Neil Deakin]

Attachment: check full locale (such as ar-QA) then fallback on the two-letter locale (such as ar)

Comment 49

[Neil Deakin]

Attachment: browser/toolkit and themes changes

This patch removes chromedir from all elements and uses -moz-locale-direction instead. Seems to work on Mac. Haven't tested it extensively.

Comment 50

[Dão Gottwald [:dao]]

Comment on attachment 374461 [details] [diff] [review]
browser/toolkit and themes changes

>         // Make sure the bookmark properties dialog hangs toward the middle of
>         // the location bar in RTL builds
>-        var position = "after_end";
>-        if (gURLBar.getAttribute("chromedir") == "rtl")
>-          position = "after_start";
>+        var position = (getComputedStyle(gURLBar, "").direction == "rtl") ? 'after_start' : 'after_end';

We actually want the locale direction there, I think, even if the location bar has direction: ltr; as done in pinstripe's browser.css. Using gNavToolbox instead of gURLBar should work.

What I don't like is that :-moz-locale-direction(rtl) is rather long. That's unfortunate for something that's used extensively. "direction" could be trimmed to "dir", or maybe the whole thing could be simplified to :-moz-rtl-locale and :-moz-ltr-locale.

Comment 51

[Neil Deakin]

Attachment: use -moz-locale-dir instead

OK, so this seems ready now.

Comment 52

[[e-mail]]

Failed 2nd hunk of patch to toolkit/content/tests/chrome/Makefile.in and build fails to find toolkit/content/tests/chrome/rtltest/Makefile.in

Comment 53

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

Comment on attachment 376059 [details] [diff] [review]
use -moz-locale-dir instead

>+
>+  /**
>+   * Returns true if the document's concept of default direction is right to left.
>+   */
>+  virtual PRBool IsDocumentRightToLeft() { return PR_FALSE; }

This explanation sounds pretty vague to me.  Reading the comment doesn't really help me understand why one would use this method or what exactly it returns.

>+        // if the localedir changed on the root element, the document needs
>+        // to be reflowed.
>+        if (aAttribute == nsGkAtoms::localedir) {
>+            nsIDocument* doc = GetCurrentDoc();
>+            if (doc && doc->GetRootContent() == this) {
>+                nsCOMPtr<nsIXULDocument> xuldoc = do_QueryInterface(doc);
>+                if (xuldoc)
>+                    xuldoc->ResetDocumentDirection();
>+                retval = NS_STYLE_HINT_FRAMECHANGE;
>+            }


Why do you need a framechange hint here?  I'd think you'd need no extra hint at all; the style change should cause an adequate hint.

However, nsCSSRuleProcessor::HasAttributeDependentStyle needs to be updated to reflect that a change of the localedir attribute on the root element (of some documents?) causes a style change.  (You're probably ok just assuming that it's unconditional rather than bothering to store which style sheets have :-moz-locale-dir in them.)

>+int
>+nsXULDocument::DirectionChanged(const char* aPrefName, void* aData)

>+  nsIPresShell *shell = doc->GetPrimaryShell();
>+  if (shell) {
>+    nsPresContext *context = shell->GetPresContext();
>+    if (context)
>+       context->RebuildAllStyleData(NS_STYLE_HINT_REFLOW);      
>+  }

You shouldn't need this either given appropriate code in nsCSSRuleProcessor (nor do I see why you'd have needed NS_STYLE_HINT_REFLOW here rather than NS_STYLE_HINT_NONE, even without that patch... if you did, there's probably another bug somewhere).


>+    test_parseable(":-moz-locale-dir(other)");

My gut feeling is that this should be unparseable; you could do that pretty easily by adding a one-off special case in CSSParserImpl::ParsePseudoClassWithIdentArg, and it would let you simplify the matching code.




I've only skimmed this; I'll want to go through more carefully before granting review.  I'm also wondering if the current list of reviewers is sufficient.

Comment 54

[[e-mail]]

Comment on attachment 376059 [details] [diff] [review]
use -moz-locale-dir instead

>+        // setting the localedir attribute on the root element forces a
>+        // specific direction for the document.
>+        nsIContent* content = GetRootContent();
>+        if (content && content->HasAttr(kNameSpaceID_None, nsGkAtoms::localedir)) {
>+            mRightToLeft = (content->AttrValueIs(kNameSpaceID_None,
>+                              nsGkAtoms::localedir, nsGkAtoms::rtl, eCaseMatters)) ? 1 : 0;
Is there a reason you accept anything other than null and rtl as ltr, rather than just accepting ltr and rtl?

>+                    // use the 'global' package for about and resource uris.
[For about: could you poke the about redirector?]

>+                        return 0;
Should be return PR_FALSE; as this is a PRBool.

>+                if (locale.Length() >= 2) {
[Can this be false?]

>+                    nsCAutoString prefStringPrefix(NS_LITERAL_CSTRING("intl.uidirection."));
>+                    nsCAutoString prefString = prefStringPrefix + locale;
>+                    nsAdoptingCString dir =
>+                        nsContentUtils::GetCharPref(prefString.get());
>+                    if (dir.IsEmpty()) {
>+                        prefString = prefStringPrefix;
>+
>+                        PRInt32 hyphen = locale.FindChar('-');
Why not find the hyphen in the prefString? (I don't think we support multipart locales otherwise I would suggest truncating at the rightmost hyphen each time until you run out.) This would also resolve the odd calculation of prefString this time around.

>+    return (mRightToLeft == 1);
I don't suppose you can return PRBool(mRightToLeft);
(you might then want to specify PR_FALSE/PR_TRUE instead of 0 and 1)

>+    virtual PRBool IsDocumentRightToLeft();
>+
>+    void ResetDocumentDirection() { mRightToLeft = -1; }
This one's virtual too, no?

>     /**
>+     * right-to-left flag for use with the -moz-locale-dir property
>+     *   -1 : uninitialized, 0 : left to right, 1 : right-to-left
>+     */
>+    PRInt8               mRightToLeft;
This won't pack, so why not a PRInt32?

Comment 55

[Neil Deakin]

Attachment: patch v2

Comment 56

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

Comment on attachment 376750 [details] [diff] [review]
patch v2

>+        if (aName== nsGkAtoms::localedir &&

space before == (two occurrences)

>+XULDocument::DirectionChanged(const char* aPrefName, void* aData)

You shouldn't need to call RebuildAllStyleData here.  (The changes to
CSSRuleProcessor::HasAttributeDependentStyle should lead the attribute
change codepath to call nsCSSFrameConstructor::PostRestyleEvent for the
root.)

Also, please use consistent indentation in this function; right now
you're indenting some levels 2 spaces; all should be 4.

>+     * right-to-left flag for use with the -moz-locale-dir property
>+     *   -1 : uninitialized, 0 : left to right, 1 : right-to-left

It might be cleaner to use an enum here.

In SelectorMatches, when data.mContent is null, you should set result to
PR_FALSE (or, perhaps better, get the document from the pres context...
although you'd need to null-check that too).  (This covers anonymous
boxes that aren't associated with content nodes.)

In nsCSSRuleProcessor::HasAttributeDependentStyle, you should remove the
null check for aData->mContent (see the assertion in the constructor of
AttributeRuleProcessorData).  You might also want to check namespace
before GetRootContent, just because it's faster.


r+sr=dbaron with those changes

Comment 57

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

(In reply to comment #56)
> In SelectorMatches, when data.mContent is null, you should set result to
> PR_FALSE (or, perhaps better, get the document from the pres context...
> although you'd need to null-check that too).  (This covers anonymous
> boxes that aren't associated with content nodes.)

But even if you fall back to getting the document from the pres context, you should still set result to false when you have no document.

Comment 58

[Neil Deakin]

> You shouldn't need to call RebuildAllStyleData here.  (The changes to
> CSSRuleProcessor::HasAttributeDependentStyle should lead the attribute
> change codepath to call nsCSSFrameConstructor::PostRestyleEvent for the
> root.)

Hmmm. It seems my comments for this iteration of the patch got lost. This code path doesn't change the attribute; it's due entirely to a preference change.

Comment 59

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

(In reply to comment #58)
> > You shouldn't need to call RebuildAllStyleData here.  (The changes to
> > CSSRuleProcessor::HasAttributeDependentStyle should lead the attribute
> > change codepath to call nsCSSFrameConstructor::PostRestyleEvent for the
> > root.)
> 
> Hmmm. It seems my comments for this iteration of the patch got lost. This code
> path doesn't change the attribute; it's due entirely to a preference change.

ok, in that case you should call PostRestyleEvent(rootElement, eReStyle_Self, NS_STYLE_HINT_NONE) yourself (rather than RebuildAllStyleData).  You don't need to rebuild style data since all the changes are caused by changes in which selectors match, which means the cached data in the rule tree are still good.

Comment 60

[Neil Deakin]

Attachment: browser/toolkit and themes changes v2

Comment 61

[Neil Deakin]

Attachment: updated content/layout patch for check in

Comment 62

[Dão Gottwald [:dao]]

Comment on attachment 376960 [details] [diff] [review]
browser/toolkit and themes changes v2

Is global.dtd still needed at all?

I would have preferred :-moz-rtl-locale and :-moz-ltr-locale, as "rtl" and "ltr" imply "direction". Arguments are useful if the value set is infinite or extensible, which doesn't seem to be the case here.

Comment 63

[Neil Deakin]

(In reply to comment #62)
> (From update of attachment 376960 [details] [diff] [review])
> Is global.dtd still needed at all?

Probably not, but other code or extensions might still be using it.

> I would have preferred :-moz-rtl-locale and :-moz-ltr-locale, as "rtl" and
> "ltr" imply "direction". Arguments are useful if the value set is infinite or
> extensible, which doesn't seem to be the case here.

That's would mean multiple pseudoclasses though I doubt we'll need to support other values than 'ltr' or 'rtl' for ui direction.

Comment 64

[Dão Gottwald [:dao]]

(In reply to comment #63)
> > Is global.dtd still needed at all?
> 
> Probably not, but other code or extensions might still be using it.

Do we want to support it forever, or get rid of it, but just not now?

> > I would have preferred :-moz-rtl-locale and :-moz-ltr-locale, as "rtl" and
> > "ltr" imply "direction". Arguments are useful if the value set is infinite or
> > extensible, which doesn't seem to be the case here.
> 
> That's would mean multiple pseudoclasses though I doubt we'll need to support
> other values than 'ltr' or 'rtl' for ui direction.

Right, and I'm assuming that two pseudo-classes instead of one is a neglectable overhead. But dbaron should chime in here.

Comment 65

[Neil Deakin]

Is there a good way to inform localizers about this change? Just a post in the localization list?

Comment 66

[Phil Ringnalda (:philor)]

It'd be really handy for me if global.dtd could stick around at least until after comm-central forks for 1.9.1.x - right now we'd need some horrible huge number of ifdefs (I got tired of counting at around 80) to keep building on both 1.9.1 and mozilla-central.

Comment 67

[Neil Deakin]

(In reply to comment #66)
> It'd be really handy for me if global.dtd could stick around at least until
> after comm-central forks for 1.9.1.x - right now we'd need some horrible huge
> number of ifdefs (I got tired of counting at around 80) to keep building on
> both 1.9.1 and mozilla-central.

I don't plan on removing global.dtd

Comment 68

[(no longer active)]

(In reply to comment #65)
> Is there a good way to inform localizers about this change? Just a post in the
> localization list?

CCing some l10n community aliases.  Do you think this is sufficient?

Comment 69

[Neil Deakin]

OK, so I'm just going to check this in.

Comment 70

[Neil Deakin]

http://hg.mozilla.org/mozilla-central/rev/d78664d9530c

Comment 71

[Eric Shepherd [:sheppy]]

https://developer.mozilla.org/en/Making_Sure_Your_Theme_Works_with_RTL_Locales#Gecko_1.9.2_and_later

https://developer.mozilla.org/en/CSS::-moz-locale-dir(ltr)
https://developer.mozilla.org/en/CSS::-moz-locale-dir(rtl)