Plugin crashed UI should be RTL in RTL locales

RESOLVED FIXED in mozilla1.9.3a5

Status

()

RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

({rtl, verified1.9.2})

Trunk
mozilla1.9.3a5
rtl, verified1.9.2
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking1.9.2 .4+, status1.9.2 .4-fixed)

Details

(Whiteboard: [lorentz])

Attachments

(3 attachments, 3 obsolete attachments)

(Assignee)

Description

9 years ago
Created attachment 435258 [details] [diff] [review]
Patch (v1)

The plugin crashed UI contains localized strings, and therefore needs to be RTL in RTL builds.
Attachment #435258 - Flags: review?(dao)

Updated

9 years ago
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?

Updated

9 years ago
Assignee: nobody → ehsan
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 2

9 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

9 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
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a4

Comment 4

9 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

9 years ago
Blocks: 478976
No longer blocks: 539055
(Assignee)

Comment 5

9 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
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
So... What should be done with bug 548704 now that you've made this change?(without tests, I might add ;-)
(Assignee)

Comment 7

9 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.

Updated

9 years ago
Duplicate of this bug: 548704

Updated

9 years ago
Depends on: 557221
(Assignee)

Comment 9

9 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

9 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

9 years ago
Created attachment 437697 [details] [diff] [review]
Fixup + Test

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+
(Assignee)

Comment 13

9 years ago
Created attachment 437702 [details] [diff] [review]
Fixup + Test

(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;
}

?
(Assignee)

Comment 15

9 years ago
Created attachment 437704 [details] [diff] [review]
Fixup + Test

(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+
(Assignee)

Comment 17

9 years ago
Created attachment 437710 [details] [diff] [review]
Patch to land
Attachment #437704 - Attachment is obsolete: true
(Assignee)

Comment 18

9 years ago
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+
(Assignee)

Comment 20

9 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

9 years ago
http://hg.mozilla.org/mozilla-central/rev/c4733ab603ef
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: mozilla1.9.3a4 → mozilla1.9.3a5
(Assignee)

Comment 22

9 years ago
Created attachment 437864 [details] [diff] [review]
1.9.2 patch

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+
(Assignee)

Comment 24

9 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)
(Assignee)

Updated

9 years ago
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+
(Assignee)

Updated

9 years ago
Attachment #437864 - Flags: approval1.9.2.4?

Comment 27

9 years ago
a=LegNeato for 1.9.2.4
(Assignee)

Comment 28

9 years ago
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/bf740820e412
status1.9.2: --- → .4-fixed

Updated

9 years ago
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.