Closed
Bug 555289
Opened 14 years ago
Closed 14 years ago
Plugin crashed UI should be RTL in RTL locales
Categories
(Core Graveyard :: Plug-ins, defect)
Core Graveyard
Plug-ins
Tracking
(blocking1.9.2 .4+, status1.9.2 .4-fixed)
RESOLVED
FIXED
mozilla1.9.3a5
People
(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)
References
Details
(Keywords: rtl, verified1.9.2, Whiteboard: [lorentz])
Attachments
(3 files, 3 obsolete files)
2.83 KB,
patch
|
dao
:
review+
|
Details | Diff | Splinter Review |
4.78 KB,
patch
|
Details | Diff | Splinter Review | |
6.80 KB,
patch
|
dbaron
:
review+
mrbkap
:
feedback+
christian
:
approval1.9.2.4+
|
Details | Diff | Splinter Review |
The plugin crashed UI contains localized strings, and therefore needs to be RTL in RTL builds.
Attachment #435258 -
Flags: review?(dao)
Updated•14 years ago
|
Attachment #435258 -
Flags: review?(dao) → review+
Comment 1•14 years ago
|
||
Comment on attachment 435258 [details] [diff] [review] Patch (v1) >+[chromedir="rtl"] { #mainBox[chromedir="rtl"] { I suppose -moz-locale-dir doesn't work here?
Updated•14 years ago
|
Assignee: nobody → ehsan
OS: Mac OS X → All
Hardware: x86 → All
Assignee | ||
Comment 2•14 years ago
|
||
(In reply to comment #1) > (From update of attachment 435258 [details] [diff] [review]) > >+[chromedir="rtl"] { > > #mainBox[chromedir="rtl"] { > > I suppose -moz-locale-dir doesn't work here? Yes, that's right. -moz-locale-dir can't be used in HTML documents.
Assignee | ||
Comment 3•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/313ecb8fb0f9 (In reply to comment #2) > > #mainBox[chromedir="rtl"] { It should actually be .mainBox[chromedir="rtl"]. Landed this as: http://hg.mozilla.org/mozilla-central/rev/980e49c1ab8a
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a4
Comment 4•14 years ago
|
||
chormedir="&locale.dir;" And I'm not going to block beta on this, but keep it on the list for future betas/final
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Updated•14 years ago
|
Assignee | ||
Comment 5•14 years ago
|
||
(In reply to comment #4) > chormedir="&locale.dir;" Bah! :-( http://hg.mozilla.org/mozilla-central/rev/fea9d008f342 > And I'm not going to block beta on this, but keep it on the list for future > betas/final I don't actually know how the process of landing this on Lorentz works like. Should I ask for approval?
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Resolution: --- → FIXED
Comment 6•14 years ago
|
||
So... What should be done with bug 548704 now that you've made this change?(without tests, I might add ;-)
Assignee | ||
Comment 7•14 years ago
|
||
(In reply to comment #6) > So... What should be done with bug 548704 now that you've made this > change?(without tests, I might add ;-) Heh, I had forgot about that bug! I guess one is a dupe of the other! _But_, I'm surprised that the patch in bug 548704 works, because reading the implementation of the :moz-locale-dir, I think that should only work where there is a XUL document... Also, you mentioned you have tests for this in that bug. Can you please attach a patch with those tests? I was going to add tests here, but I hesitated, because the similar test for RTL handling of video controls proved to be too painful.
Assignee | ||
Comment 9•14 years ago
|
||
(In reply to comment #7) > _But_, I'm surprised that the patch in bug 548704 works, because reading the > implementation of the :moz-locale-dir, I think that should only work where > there is a XUL document... I tested this, and indeed :moz-locale-dir cannot be used here.
Assignee | ||
Comment 10•14 years ago
|
||
One case was not handled in this patch: where document is RTL in an LTR browser. I'll attach a patch which fixes this and includes a reftest as well.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 11•14 years ago
|
||
Requesting review from dbaron on reftest.js changes, and from dao on the rest.
Attachment #437697 -
Flags: review?(dbaron)
Attachment #437697 -
Flags: review?(dao)
Comment 12•14 years ago
|
||
Comment on attachment 437697 [details] [diff] [review] Fixup + Test >diff --git a/layout/tools/reftest/reftest.js b/layout/tools/reftest/reftest.js r=dbaron on the reftest.js changes >diff --git a/modules/plugin/test/reftest/reftest.list b/modules/plugin/test/reftest/reftest.list > fails-if(!haveTestPlugin) == border-padding-2.html border-padding-2-ref.html >+skip-if(!prefs.getBoolPref("dom.ipc.plugins.enabled") || !haveTestPlugin) == pluginproblemui-direction-1.html pluginproblemui-direction-ref.html This isn't going to work because each skip-if or fails-if token can't have whitespace inside it. In any case, I think what you really want is: fails-if(!haveTestPlugin) skip-if(!prefs.getBoolPref(...)) since the last matching item wins.
Attachment #437697 -
Flags: review?(dbaron) → review+
Assignee | ||
Comment 13•14 years ago
|
||
(In reply to comment #12) > In any case, I think what you really want is: > fails-if(!haveTestPlugin) skip-if(!prefs.getBoolPref(...)) > since the last matching item wins. Done.
Attachment #437697 -
Attachment is obsolete: true
Attachment #437702 -
Flags: review?(dao)
Attachment #437697 -
Flags: review?(dao)
Comment 14•14 years ago
|
||
Wouldn't it make more sense this way: .mainBox { direction: ltr; unicode-bidi: embed; } .mainBox[chromedir="rtl"] { direction: rtl; } instead of: .mainBox { direction: ltr; } .mainBox[chromedir="rtl"] { direction: rtl; unicode-bidi: embed; } ?
Assignee | ||
Comment 15•14 years ago
|
||
(In reply to comment #14) > Wouldn't it make more sense this way: > > .mainBox { > direction: ltr; > unicode-bidi: embed; > } > .mainBox[chromedir="rtl"] { > direction: rtl; > } > > instead of: > > .mainBox { > direction: ltr; > } > .mainBox[chromedir="rtl"] { > direction: rtl; > unicode-bidi: embed; > } > > ? Shouldn't really matter, but changed it anyway.
Attachment #437702 -
Attachment is obsolete: true
Attachment #437704 -
Flags: review?(dao)
Attachment #437702 -
Flags: review?(dao)
Comment 16•14 years ago
|
||
Comment on attachment 437704 [details] [diff] [review] Fixup + Test I'd put .mainBox[chromedir="rtl"] {...} right after .mainBox {...}.
Attachment #437704 -
Flags: review?(dao) → review+
Assignee | ||
Comment 17•14 years ago
|
||
Attachment #437704 -
Attachment is obsolete: true
Assignee | ||
Comment 18•14 years ago
|
||
This should block 1.9.2.4, because we're planning to ship Lorentz as part of that release.
blocking1.9.2: --- → ?
Comment 19•14 years ago
|
||
Blocking, sure, but you'll still need approval on the patch; has it baked in the appropriate places?
blocking1.9.2: ? → .4+
Assignee | ||
Comment 20•14 years ago
|
||
(In reply to comment #19) > Blocking, sure, but you'll still need approval on the patch; has it baked in > the appropriate places? It will, when I land it in a few minutes! ;-) I'll create a rolled-up patch for 1.9.2 and request approval on that.
Assignee | ||
Comment 21•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/c4733ab603ef
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: mozilla1.9.3a4 → mozilla1.9.3a5
Assignee | ||
Comment 22•14 years ago
|
||
I had to add prefs.getBoolPref to reftest.js for 1.9.2, requesting dbaron's review for it.
Attachment #437864 -
Flags: review?(dbaron)
Comment 23•14 years ago
|
||
Comment on attachment 437864 [details] [diff] [review] 1.9.2 patch r=dbaron on the reftest.js changes; you might also want to double-check with mrbkap that this is the right thing on 1.9.2 if you don't already have some other source of that information
Attachment #437864 -
Flags: review?(dbaron) → review+
Assignee | ||
Comment 24•14 years ago
|
||
Comment on attachment 437864 [details] [diff] [review] 1.9.2 patch (In reply to comment #23) > (From update of attachment 437864 [details] [diff] [review]) > r=dbaron on the reftest.js changes; you might also want to double-check with > mrbkap that this is the right thing on 1.9.2 if you don't already have some > other source of that information Asking for his feedback on this patch.
Attachment #437864 -
Flags: feedback?(mrbkap)
Comment 26•14 years ago
|
||
Comment on attachment 437864 [details] [diff] [review] 1.9.2 patch Yeah, this'll work fine on the 1.9.2 branch (the __exposedProps__ won't take effect until I backport a couple of patches there, though).
Attachment #437864 -
Flags: feedback?(mrbkap) → feedback+
Assignee | ||
Updated•14 years ago
|
Attachment #437864 -
Flags: approval1.9.2.4?
Comment 27•14 years ago
|
||
a=LegNeato for 1.9.2.4
Assignee | ||
Comment 28•14 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/bf740820e412
status1.9.2:
--- → .4-fixed
Attachment #437864 -
Flags: approval1.9.2.4? → approval1.9.2.4+
Comment 29•14 years ago
|
||
Verified for 1.9.2 using Hebrew builds: Mozilla/5.0 (Windows; U; Windows NT 5.1; he; rv:1.9.2.4pre) Gecko/20100412.
Keywords: verified1.9.2
Updated•2 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•