304291 - The direction of windows should be set inside of them (instead of intl.css)

Status: RESOLVED DUPLICATE of bug 478416

(Toolkit :: UI Widgets, defect, P2)

Description

[Asaf Romano (gone)]

The he-IL l10n has the following style rule in intl.css:
 17 /*make UI RTL */
 18 
 19 window,prefwindow,dialog,wizard,page,menu { direction: rtl; }

This is breaking extension w/o a he-IL localization (i.e. anything but BiDi Mail
UI). We should set the direction (based on locale.dir) on the dialog, wizard and
prefwindow bindings, and also on <window>s.

The rule for <menu> is probably already useless, not sure about <page> (I
_think_ it's only supported by seamonkey).

Comment 1

[Asaf Romano (gone)]

Attachment: widgets/ only patch

I'm not sure what do with <window>s, how about adding a "window" binding?

Comment 2

[Asaf Romano (gone)]

Comment on attachment 192362 [details] [diff] [review]
widgets/ only patch

Actually, that's not going to work for elements with a style attribute.

Comment 3

[Benjamin Smedberg]

I'm not sure I understand why this patch is any better than intl.css: it still
affects all windows, whether or not it's an extension window with a RTL locale.

Comment 4

[Asaf Romano (gone)]

Comment on attachment 192362 [details] [diff] [review]
widgets/ only patch

(And, this will have to be done for each xul file in order to actually solve
something :/ )

Comment 5

[Reuven Gonen]

I've checked this out a bit, and found the following solution. although it may
not be the best solution, it seems to be working fairly good.

we can add a file to "chrome://global/locale/" with the name intl-extensions.css
(or something similar) and refer to it from
"chrome://communicator/skin/communicator.css". like this:

@import url("chrome://global/skin/");

@import url("chrome://global/locale/intl-extensions.css");

and add to the "intl-extensions.css" file the following:

window,prefwindow,dialog,wizard,page,menu { direction: ltr; }

Now, while the extensions will be refering to "chrome://communicator/skin/"
instead of "chrome://global/skin/" the extension will remanin LTR. there are
some extensions that use already "chrome://communicator/skin/" so this will
already solve them. in a case the extension is translated, we can add it's ID to
the intl-extensions file and set it to RTL. 

there are still some minor bugs in this, like arrows being the wrong way, if an
extension uses them (only in browser RTL interface), but i think this state is
already much better. 

Comment 6

[Benjamin Smedberg]

The toolkit apps don't use communicator.

Comment 7

[Asaf Romano (gone)]

Benjamin, how about applying intl.css (from the application locale) only when
the application locale is used?

Comment 8

[Benjamin Smedberg]

How would you do that? Remember that layout has almost no knowledge of locales
or the chrome registry.

Comment 9

[Reuven Gonen]

It seems that since the extensions become part of the interface, it's pretty
difficult to notice which windows belong to the extensions, and which ones not.

The way to solve this, can be by leaving the global settings of the interface
direction, and specifying it for every window. there are two ways i can think of
doing this:

1. add a reference to "global.dtd" in all the files that uses a window style
(window, dialog, wizard, prefwindow) and add to the window proerties
style="direction: &locale.dir;;"

this may look a bit scary, but i've noticed that the total amount of xul files
that sets a window, wizard, etc, is 101, which means that we need to add a
reference to "global.dtd" and add style="direction: &locale.dir;;" in 101 files.
this is way less then the global amount of windows added by extensions. i'm not
sure what effect this has on the code.

2. add all the ID's of window, wizard, etc, to intl.css and set there direction
to RTL. the extensions will remain LTR, since the global setting is LTR, and the
direction is specified for every window.

First method:

advantages: the direction is set inside the xul window, so even if it's ID
changes, the window will still be set to RTL. with intl.css these changes will
need to be tracked. can be used by other languages as well.

disadvantages: needs to be setup in source, needs a review and approval. also 
those it have any effect on the speed?

Second method:

advantages: doesn't need a review and approval, doesn't effect other locales.
can be modified more freely for single windows.

disadvantages: if the window ID changes the intl.css file needs to be modified.
also if new windows are added, they will need to be tracked.

the general idea is to stop using the global setting and set it inside the
windows. The advantage is that browser window ID's are easier to follow then
extensions :-) also there are less windows in the browser, then in all the
extensions  :-)

Comment 10

[Reuven Gonen]

adding the folloing to intl.css, takes care of most windows and dialogs that
exist in the interface:

/* Browser windows  */

#aboutDialog,#SanitizeDialog,#safeModeDialog,#pageReportFirstTime,#pageReport,#main-window,#openLocation,#metadata,#webpanels-window,#viewSource,#selectBookmarkDialog,#bmPropsWindow,#bookmarksPanel,#bookmark-window,#newBookmarkDialog,#addBookmarkDialog,#history-panel,#migrationWizard,#BrowserPreferences,#PermissionsDialog,#LanguagesDialog,#FontsDialog,#DownloadActionsWindow,#CookiesDialog,#ConnectionsDialog,#ColorsDialog,#ChangeActionDialog,#AdvancedJSDialog,#SanitizeDialog,#cookieAcceptDialog,#ssl_warning,#loaddevice,#getpassword,#getp12password,#formsigning,#escrowWarnDialog,#pref-ciphers,#crlUpdatePref,#reset_password,#serverCertExpired,#serverCrlNextupdate,#setp12password,#cacertexists,#certmanager,#certPicker,#certDetails,#set_password,#cipherinfo,#domainMismatch,#crlImportSuccess,#crlviewer,#deleteCertificate,#devicemanager,#download_cert,#editCaCert,#editEmailCert,#editWebsiteCert
{
	direction: rtl !important;
}

/* Toolkit windows  */

#dummyWindow,#CustomizeToolbarWindow,#JSConsoleWindow,#config,#commonDialog,#nsHelperAppDlg,#downloadFontDialog,#findDialog,#alertNotification,#help,#unknownContentType,#editAction,#downloadManager,#updateWizard,#addonList,#extensionsManager,#genericAbout,#plugin-installer-wizard,#UpdateSettingsDialog,#removemp,#OCSPDialog,#FontScalingDialog,#changemp,#profileWindow,#createProfileWizard,#updates,#incompatible,#history,#errors,#xpinstallConfirm,#SignonViewerDialog
{
	direction: rtl !important;
}

/* Builtin Extensions */

#winInspectorMain,#winUtil,#aboutReporter,#reportWizard{
	direction: rtl !important;
}

there seems to be a few files inside toolkit that don't have an ID, so i can't
set there direction through intl.css. i will add a patch for that.

Comment 11

[Benjamin Smedberg]

Hardcoding a bunch of IDs is a really bad idea, because it won't work in
xulrunner anyway. Why can't we just add dir="&locale.dir;" or
chromedir="&locale.dir" to our windows?

Comment 12

[Reuven Gonen]

Attachment: set the direction for windows without an ID

Tha patch adds ths style direction: &locale.dir for the files in toolkit that
don't have an ID.

i think using this method is better then adding the styles to intl.css, since
there maybe an extension that uses the same ID as one of the browser windows.
but i think this maybe easier to get an approval for the branch.

Comment 13

[Reuven Gonen]

(In reply to comment #11)
> Hardcoding a bunch of IDs is a really bad idea, because it won't work in
> xulrunner anyway. Why can't we just add dir="&locale.dir;" or
> chromedir="&locale.dir" to our windows?

i can create an attachment like the one above for the rest of the windows, if
this way is acceptable.

Comment 14

[Benjamin Smedberg]

We certainly want that patch on trunk. I think that it's not a bad idea on
branch, if it lands on trunk and doesn't regress txul or anything like that.

Comment 15

[Asaf Romano (gone)]

What are we going to do with common dialogs (nsIPromptService)... of extensions?

Comment 16

[Reuven Gonen]

Attachment: setting the direction inside the xul files

the direction for the toolbox in browser.xul, is set to "ltr" to force toolbars
(yahoo, google, etc.) to remain LTR, and not inherit the browser direction.

Comment 17

[Reuven Gonen]

Attachment: as above, removing a change from newserver.xul

seems like i changed "Håkan Waara" with "H?kan Waara" inside newserver.xul in
the previous patch.

Comment 18

[Asaf Romano (gone)]

Comment on attachment 194781 [details] [diff] [review]
as above, removing a change from newserver.xul

Index: browser/base/content/metaData.xul
===================================================================

-<!DOCTYPE window SYSTEM "chrome://browser/locale/metaData.dtd" >
- 
+<!DOCTYPE window [
+    <!ENTITY % globalDTD SYSTEM "chrome://global/locale/global.dtd">
+    <!ENTITY % metaDataDTD SYSTEM "chrome://browser/locale/metaData.dtd" >
+  %globalDTD;
+  %metaDataDTD;
+]>
+

fix indent, ditto for similiar instances in this patch.


Index: browser/base/content/searchDialog.xul
===================================================================

-<!DOCTYPE dialog SYSTEM "chrome://browser/locale/searchDialog.dtd">
+<!DOCTYPE dialog [
+<!ENTITY % globalDTD SYSTEM "chrome://global/locale/global.dtd">
+<!ENTITY % searchDialogDTD SYSTEM "chrome://browser/locale/searchDialog.dtd">
+%globalDTD;
+%searchDialogDTD;
+]>


let's make that:
+<!DOCTYPE dialog [
+  <!ENTITY % globalDTD SYSTEM "chrome://global/locale/global.dtd">
+  %globalDTD;
+  <!ENTITY % searchDialogDTD SYSTEM
"chrome://browser/locale/searchDialog.dtd">
+  %searchDialogDTD;
+]>


ditto for similiar instances in this patch.


r=mano. You'll have to fix mail/ as well in order to remove the rtl style rules
from intl.css though.

Comment 19

[Asaf Romano (gone)]

Comment on attachment 194781 [details] [diff] [review]
as above, removing a change from newserver.xul

(please attach the final patch).

In any case, i don't believe we need this on the branch before mail/ is fixed
as well.

Comment 20

[Reuven Gonen]

Attachment: as above, fixing indents

Comment 21

[Asaf Romano (gone)]

Kai, can you mao the security/ changes? thanks.

Comment 22

[Reuven Gonen]

Attachment: parallel patch for messenger

this patch sets the UI direction inside the windows, that are used in
Thunderbird

Comment 23

[Scott MacGregor]

Comment on attachment 197871 [details] [diff] [review]
parallel patch for messenger

There's an extra semi colon here:

style="direction: &locale.dir;;"

is that correct? It occurred in a lot of files.

Comment 24

[Benjamin Smedberg]

that is correct, the first colon is for the entity, the second terminates the css rule

Comment 25

[Kai Engert (:KaiE:)]

moa=kengert granted for changes in security and smime.

Comment 26

[Reuven Gonen]

Attachment: complete patch for browser and messenger

this patch includes the changes for Firefox and Thunderbird

Comment 27

[Reuven Gonen]

I probably won't be able to work on this in the near future.

Comment 28

[(no longer active)]

Mass-assigning the new rtl keyword to RTL-related (see bug 349193).

Comment 29

[(no longer active)]

Mano: is this still wanted?  If so, I can probably put this on my radar.

Comment 30

[Neil Deakin]

A better version of this is pretty much what I implemented in bug 478416.