Closed Bug 555289 Opened 10 years ago Closed 10 years ago

Plugin crashed UI should be RTL in RTL locales

Categories

(Core :: Plug-ins, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.3a5
Tracking Status
blocking1.9.2 --- .4+
status1.9.2 --- .4-fixed

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

(Keywords: rtl, verified1.9.2, Whiteboard: [lorentz])

Attachments

(3 files, 3 obsolete files)

Attached patch Patch (v1)Splinter Review
The plugin crashed UI contains localized strings, and therefore needs to be RTL in RTL builds.
Attachment #435258 - Flags: review?(dao)
Attachment #435258 - Flags: review?(dao) → review+
Comment on attachment 435258 [details] [diff] [review]
Patch (v1)

>+[chromedir="rtl"] {

#mainBox[chromedir="rtl"] {

I suppose -moz-locale-dir doesn't work here?
Assignee: nobody → ehsan
OS: Mac OS X → All
Hardware: x86 → All
(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.
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: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a4
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 → ---
Blocks: OOPP
No longer blocks: LorentzBeta1
(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: 10 years ago10 years ago
Resolution: --- → FIXED
So... What should be done with bug 548704 now that you've made this change?(without tests, I might add ;-)
(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.
Duplicate of this bug: 548704
Depends on: 557221
(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.
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 → ---
Attached patch Fixup + Test (obsolete) — Splinter Review
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 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+
Attached patch Fixup + Test (obsolete) — Splinter Review
(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)
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;
}

?
Attached patch Fixup + Test (obsolete) — Splinter Review
(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 on attachment 437704 [details] [diff] [review]
Fixup + Test

I'd put .mainBox[chromedir="rtl"] {...} right after .mainBox {...}.
Attachment #437704 - Flags: review?(dao) → review+
Attached patch Patch to landSplinter Review
Attachment #437704 - Attachment is obsolete: true
This should block 1.9.2.4, because we're planning to ship Lorentz as part of that release.
blocking1.9.2: --- → ?
Blocking, sure, but you'll still need approval on the patch; has it baked in the appropriate places?
blocking1.9.2: ? → .4+
(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.
http://hg.mozilla.org/mozilla-central/rev/c4733ab603ef
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: mozilla1.9.3a4 → mozilla1.9.3a5
Attached patch 1.9.2 patchSplinter Review
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 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+
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)
Duplicate of this bug: 558403
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+
Attachment #437864 - Flags: approval1.9.2.4?
a=LegNeato for 1.9.2.4
Attachment #437864 - Flags: approval1.9.2.4? → approval1.9.2.4+
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
You need to log in before you can comment on or make changes to this bug.