Closed Bug 437844 Opened 17 years ago Closed 16 years ago

[RTL] about:plugins is hardcoded to LTR

Categories

(Firefox :: General, defect)

Product:
Firefox
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 3.6a1

People

(Reporter: tomer, Assigned: ehsan.akhgari)

References

Details

(Keywords: fixed1.9.1, rtl)

Attachments

(3 files, 4 obsolete files)

Current result (screenshot)
22.46 KB, image/png
Details
Expected result (screenshot)
22.55 KB, image/png
Details
Patch (v2 + unit test)
11.93 KB, patch
bzbarsky
: review+
beltzner
: approval1.9.1+
Details | Diff | Splinter Review
bug 437797 includes a fix to make about:mozilla RTL-friendly, and about:plugins has similar issue we wish to fix. about:plugins is more complicated than about:mozilla, and will require us to either convert the file to xhtml or add locale.dir entity to a dot-properties file.
steps to reproduce: a. Get an RTL build (Hebrew/Arabic/Farisi). b. Type about:plugins into the address bar. c. Text should be aligned to the right.
I'll take care of this bug, but please guide me how you expect it to be resolved. As I see it, it is impossible to insert the dir=&locale.dir; property without converting the page to a more native format.
Given the dynamic nature of the page, it should be trivial to convert it to XHTML, and use $locale.dir; like everywhere else. Can you do this Tomer, or do you want me to give it a shot?
Keywords: l12y
Version: 3.0 Branch → Trunk
Attached patch Patch (v1) (obsolete) — — Splinter Review
r,sr request from bz on the docshell part (which is trivial). r request from gavin on the toolkit part. I rewrote the script inside the page so that it does not use document.write which is XHTML incompatible. The resulting page is identical to the current page in LTR, and is correctly reversed in RTL mode.
Assignee: nobody → ehsan.akhgari
Status: NEW → ASSIGNED
Attachment #358708 - Flags: superreview?(bzbarsky)
Attachment #358708 - Flags: review?(gavin.sharp)
Attachment #358708 - Flags: review?(bzbarsky)
Actually, I'm probably the right reviewer for this whole thing... I assume there are no other plugins.html files around? I don't seem to see any, at least.
Comment on attachment 358708 [details] [diff] [review] Patch (v1) (In reply to comment #5) > Actually, I'm probably the right reviewer for this whole thing... I assume > there are no other plugins.html files around? I don't seem to see any, at > least. No, there aren't.
Attachment #358708 - Flags: review?(gavin.sharp)
OK, some questions about the changes: 1) Is there a reason for the left/right text-align stuff, instead of just using "text-align: start"? 2) Is there a reason for the border-right/border-left stuff instead of using "-moz-border-start"? 3) As I recall, some plug-ins actually put HTML in plugin.description and expect that to work for various purposes (e.g. user settings for the plug-in). Won't this patch break that, unless they happen to have XML-compatible HTML in the string (e.g. no <br>, no <input>, etc without trailing slashes)? 4) If issues 1 and 2 are addressed, then you don't need a 'dir' attribute on the body. You just need to set the direction of the document to rtl, right? Is there any way you can get this information in your script? If so, you could just set document.dir directly, or add the attribute to the <body> or whatever...
If we have no sane way of getting this value in JS we'd need to add it to a properties file, as comment 0 says. That said, it seems to me that not being able to get to it from JS is a problem.
Comment on attachment 358708 [details] [diff] [review] Patch (v1) Don't think we want to take this as-is.
Attachment #358708 - Flags: superreview?(bzbarsky)
Attachment #358708 - Flags: superreview-
Attachment #358708 - Flags: review?(bzbarsky)
Attachment #358708 - Flags: review-
I wonder if we would like grant chrome privs to about:plugins, in particular if plugins add content to that page. And I recall that we had to if we wanted to access .properties files.
Attached patch Patch (v1.1) (obsolete) — — Splinter Review
(In reply to comment #7) > OK, some questions about the changes: > > 1) Is there a reason for the left/right text-align stuff, instead of just > using > "text-align: start"? > 2) Is there a reason for the border-right/border-left stuff instead of using > "-moz-border-start"? I addressed these two. I didn't know about "text-align: start" actually, so thanks for pointing it out! > 3) As I recall, some plug-ins actually put HTML in plugin.description and > expect that to work for various purposes (e.g. user settings for the > plug-in). Won't this patch break that, unless they happen to have > XML-compatible HTML in the string (e.g. no <br>, no <input>, etc without > trailing slashes)? Hmm, yes that will break non-XML compatible HTML in plugin.description. Is there any way to sanitize such an HTML string into XHTML (for example using an XPCOM component)? That would of course require giving about:plugins the privilege to access XPCOM objects. > 4) If issues 1 and 2 are addressed, then you don't need a 'dir' attribute on > the body. You just need to set the direction of the document to rtl, > right? Is there any way you can get this information in your script? If > so, you could just set document.dir directly, or add the attribute to the > <body> or whatever... AFAIK, there is no way to access a value defined in a dtd file in js. But yes if there were a way to do that, then the attribute wouldn't be needed. BTW, would trying to read that dtd file and parse it in js be too stupid to give it a try? (In reply to comment #9) > If we have no sane way of getting this value in JS we'd need to add it to a > properties file, as comment 0 says. Hmm, this requires having four places in the codebase which define directions instead of three (the current ones being global.dtd, intl.css and the crashreporter properties file). This isn't ideal, but if we can't solve 3 or 4 then I don't know what other alternative there would be. > That said, it seems to me that not being able to get to it from JS is a problem. Yes. In general if we had a way to read dtd values in js code we would be able to do a lot of stuff more nicely throughout the code base.
Attachment #358708 - Attachment is obsolete: true
(In reply to comment #11) > I wonder if we would like grant chrome privs to about:plugins, in particular if > plugins add content to that page. And I recall that we had to if we wanted to > access .properties files. Yes. How about HTML encoding the description? Do we officially allow HTML content in descriptions?
> Do we officially allow HTML content in descriptions? There's nothing official about any of this, but we've had bugs in the past when we broke this stuff, as I recall. Here's a stupid question. If you set up an XML string pointing to the DTD in question and using the relevant entity and then parse it with DOMParser, will the resulting document have the text you want?
it probably requires perms to load chrome docs, as we only load DTDs for chrome urls.
Hmm... good point. Can we cheat? Include intl.css and use computed style to read off the direction set there?
(In reply to comment #16) > Can we cheat? Include intl.css and use computed style to read off the > direction set there? This may actually work! One tiny point though: would it be OK to assume that the styles set in intl.css do not clash with actual HTML elements in about:plugins?
You could create a hidden (height = width = 0, visibility: hidden) subframe that loads intl.css and that you read the data from, if you want to be safe.
To answer my own question, I examined every intl.css file in <http://mxr.mozilla.org/l10n-mozilla1.9.1/find?string=toolkit%2Fchrome%2Fglobal%2Fintl.css&tree=l10n-mozilla1.9.1&hint=>, and I think we can safely assume that it will work, given that the IDs/classes used in about:plugins are not used elsewhere in the XUL UI, and the XUL element names do not clash with HTML elements which are used in about:plugins. I'll come up with a patch using this approach shortly.
(In reply to comment #18) > You could create a hidden (height = width = 0, visibility: hidden) subframe > that loads intl.css and that you read the data from, if you want to be safe. I'll try this first.
Attached patch WIP (obsolete) — — Splinter Review
I tried implementing the suggested idea, but it seems like I can't figure out how to get the detected direction in the about:plugin document. I have tested a number of methods to try to get this work (this patch tries to expose the direction as the textContent of a node) and none of them work (for example, in this patch, the textContent property read from plugins.html is a blank string. Boris: do you have any pointers here?
Attachment #359305 - Attachment is obsolete: true
> + frame.setAttribute("style", "display: none;"); That means no presentation inside the frame, no prescontext, no style resolution, no computed style. That's why I said to use "width:0; height:0; visibility:hidden" in comment 18.
Attached patch Patch (v2) (obsolete) — — Splinter Review
(In reply to comment #22) > > + frame.setAttribute("style", "display: none;"); > > That means no presentation inside the frame, no prescontext, no style > resolution, no computed style. That's why I said to use "width:0; height:0; > visibility:hidden" in comment 18. Ah right. OK, this patch works and is ready for review. Thanks for your help on this! BTW, I wanted to use a data: URI originally, but I figured that this directionDetector.html would help me with bug 348233 as well (at least).
Attachment #359384 - Attachment is obsolete: true
Attachment #359482 - Flags: review?(bzbarsky)
Attachment #359482 - Attachment description: Patch → Patch (v2)
I managed to come up with an automated test to test the correct direction of the about:plugins page in RTL locales. The code part of this patch is the same as that of attachment 359482 [details] [diff] [review].
Attachment #359482 - Attachment is obsolete: true
Attachment #359510 - Flags: review?(bzbarsky)
Attachment #359482 - Flags: review?(bzbarsky)
Comment on attachment 359510 [details] [diff] [review] Patch (v2 + unit test) Looks good. Gotta love our ugly hacks. ;)
Attachment #359510 - Flags: review?(bzbarsky) → review+
http://hg.mozilla.org/mozilla-central/rev/121fbf0736cc
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.2a1
Comment on attachment 359510 [details] [diff] [review] Patch (v2 + unit test) This would be a nice fix for our RTL locales to be included in branch. It includes an automated test suite.
Attachment #359510 - Flags: approval1.9.1?
Comment on attachment 359510 [details] [diff] [review] Patch (v2 + unit test) a191=beltzner
Attachment #359510 - Flags: approval1.9.1? → approval1.9.1+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: