Last Comment Bug 478416 - Replace chromedir with something more sane
: Replace chromedir with something more sane
Status: RESOLVED FIXED
: dev-doc-complete
Product: Toolkit
Classification: Components
Component: XUL Widgets (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Neil Deakin
:
Mentors:
: 304291 (view as bug list)
Depends on: 509602 741293
Blocks: 476423 500282
  Show dependency treegraph
 
Reported: 2009-02-13 07:28 PST by Neil Deakin
Modified: 2012-04-02 08:26 PDT (History)
18 users (show)
enndeakin: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
possible patch (9.91 KB, patch)
2009-03-26 12:41 PDT, Neil Deakin
no flags Details | Diff | Splinter Review
implement the behaviour described above (23.90 KB, patch)
2009-03-30 10:04 PDT, Neil Deakin
no flags Details | Diff | Splinter Review
include a test (36.35 KB, patch)
2009-04-02 06:25 PDT, Neil Deakin
no flags Details | Diff | Splinter Review
use -moz-locale-direction (26.60 KB, patch)
2009-04-03 11:08 PDT, Neil Deakin
no flags Details | Diff | Splinter Review
add tests to test_selectors.html (27.45 KB, patch)
2009-04-04 18:18 PDT, Neil Deakin
no flags Details | Diff | Splinter Review
use preferences instead of the chrome registry (10.56 KB, patch)
2009-04-14 13:04 PDT, Neil Deakin
no flags Details | Diff | Splinter Review
add preference update, support about/resource uris (19.94 KB, patch)
2009-04-15 13:11 PDT, Neil Deakin
no flags Details | Diff | Splinter Review
check full locale (such as ar-QA) then fallback on the two-letter locale (such as ar) (29.55 KB, patch)
2009-04-22 06:41 PDT, Neil Deakin
no flags Details | Diff | Splinter Review
browser/toolkit and themes changes (177.62 KB, patch)
2009-04-24 07:24 PDT, Neil Deakin
no flags Details | Diff | Splinter Review
use -moz-locale-dir instead (29.51 KB, patch)
2009-05-06 12:51 PDT, Neil Deakin
neil: review+
Details | Diff | Splinter Review
patch v2 (33.46 KB, patch)
2009-05-11 11:14 PDT, Neil Deakin
dbaron: review+
dbaron: superreview+
Details | Diff | Splinter Review
browser/toolkit and themes changes v2 (161.07 KB, patch)
2009-05-12 11:34 PDT, Neil Deakin
dao+bmo: review+
Details | Diff | Splinter Review
updated content/layout patch for check in (35.03 KB, patch)
2009-05-12 12:01 PDT, Neil Deakin
no flags Details | Diff | Splinter Review

Description Neil Deakin 2009-02-13 07:28:13 PST
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 [pto-Aug-30][:Pike] 2009-02-15 10:47:11 PST
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 2009-02-15 12:39:02 PST
(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 :Ehsan Akhgari 2009-02-15 12:45:01 PST
(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 2009-03-04 08:42:45 PST
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 :Ehsan Akhgari 2009-03-04 11:39:03 PST
(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 [pto-Aug-30][:Pike] 2009-03-04 16:59:21 PST
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 2009-03-09 11:13:34 PDT
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 2009-03-09 21:04:15 PDT
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 2009-03-26 12:41:38 PDT
Created attachment 369546 [details] [diff] [review]
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 10 Neil Deakin 2009-03-28 05:53:39 PDT
*** Bug 304291 has been marked as a duplicate of this bug. ***
Comment 11 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2009-03-28 12:01:04 PDT
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 2009-03-29 05:58:11 PDT
(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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2009-03-29 08:15:16 PDT
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 2009-03-30 10:04:04 PDT
Created attachment 370024 [details] [diff] [review]
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 :Ehsan Akhgari 2009-03-30 12:24:54 PDT
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 2009-04-02 06:25:16 PDT
Created attachment 370632 [details] [diff] [review]
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] 2009-04-02 06:41:41 PDT
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-7 (review requests must explain patch) 2009-04-02 07:52:39 PDT
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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2009-04-02 08:00:33 PDT
-moz-locale-direction
Comment 20 Neil Deakin 2009-04-02 08:27:34 PDT
> 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] 2009-04-02 08:33:23 PDT
Also, xul.css has the XUL namespace as the default namespace, so :root shouldn't apply to other elements.
Comment 22 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2009-04-02 08:34:31 PDT
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 2009-04-02 08:44:14 PDT
(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 2009-04-03 11:08:00 PDT
Created attachment 370906 [details] [diff] [review]
use -moz-locale-direction
Comment 25 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2009-04-03 11:12:53 PDT
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 2009-04-04 18:18:19 PDT
Created attachment 371093 [details] [diff] [review]
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 [:bsmedberg] 2009-04-09 13:19:46 PDT
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 2009-04-10 06:11:36 PDT
(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 [:bsmedberg] 2009-04-10 06:25:01 PDT
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 2009-04-10 06:46:10 PDT
(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 :Ehsan Akhgari 2009-04-10 11:57:47 PDT
(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 [:bsmedberg] 2009-04-10 12:02:19 PDT
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 :Ehsan Akhgari 2009-04-10 12:14:24 PDT
(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 [:bsmedberg] 2009-04-13 10:56:36 PDT
> 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 :Ehsan Akhgari 2009-04-13 11:01:47 PDT
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 2009-04-14 13:04:25 PDT
Created attachment 372669 [details] [diff] [review]
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 :Ehsan Akhgari 2009-04-15 09:02:08 PDT
(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 2009-04-15 09:17:13 PDT
(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 2009-04-15 13:11:15 PDT
Created attachment 372948 [details] [diff] [review]
add preference update, support about/resource uris
Comment 40 Neil Deakin 2009-04-16 09:17:47 PDT
Ehsan, is it ok to just use the two-character prefix for right-to-left?
Comment 41 :Ehsan Akhgari 2009-04-17 03:00:09 PDT
(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 :Ehsan Akhgari 2009-04-17 03:01:16 PDT
(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 :Ehsan Akhgari 2009-04-17 03:03:53 PDT
(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 2009-04-17 03:46:44 PDT
(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 2009-04-17 04:03:04 PDT
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 2009-04-17 04:50:43 PDT
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 [pto-Aug-30][:Pike] 2009-04-17 05:05:45 PDT
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 2009-04-22 06:41:12 PDT
Created attachment 374057 [details] [diff] [review]
check full locale (such as ar-QA) then fallback on the two-letter locale (such as ar)
Comment 49 Neil Deakin 2009-04-24 07:24:30 PDT
Created attachment 374461 [details] [diff] [review]
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] 2009-04-24 07:54:46 PDT
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 2009-05-06 12:51:07 PDT
Created attachment 376059 [details] [diff] [review]
use -moz-locale-dir instead

OK, so this seems ready now.
Comment 52 neil@parkwaycc.co.uk 2009-05-07 14:25:05 PDT
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-7 (review requests must explain patch) 2009-05-07 14:37:05 PDT
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 neil@parkwaycc.co.uk 2009-05-07 16:17:54 PDT
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 2009-05-11 11:14:42 PDT
Created attachment 376750 [details] [diff] [review]
patch v2
Comment 56 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2009-05-11 13:37:53 PDT
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-7 (review requests must explain patch) 2009-05-11 13:39:02 PDT
(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 2009-05-11 14:06:10 PDT
> 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-7 (review requests must explain patch) 2009-05-11 15:33:14 PDT
(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 2009-05-12 11:34:34 PDT
Created attachment 376960 [details] [diff] [review]
browser/toolkit and themes changes v2
Comment 61 Neil Deakin 2009-05-12 12:01:24 PDT
Created attachment 376967 [details] [diff] [review]
updated content/layout patch for check in
Comment 62 Dão Gottwald [:dao] 2009-05-14 01:37:58 PDT
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 2009-05-28 08:45:47 PDT
(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] 2009-06-01 04:15:35 PDT
(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 2009-06-22 12:08:34 PDT
Is there a good way to inform localizers about this change? Just a post in the localization list?
Comment 66 Phil Ringnalda (:philor) 2009-06-22 21:49:59 PDT
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 2009-07-03 11:45:35 PDT
(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 :Ehsan Akhgari 2009-07-04 06:02:29 PDT
(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 2009-07-28 08:26:29 PDT
OK, so I'm just going to check this in.

Note You need to log in before you can comment on or make changes to this bug.