555289 - Plugin crashed UI should be RTL in RTL locales

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[(no longer active)]

Attachment: Patch (v1)

The plugin crashed UI contains localized strings, and therefore needs to be RTL in RTL builds.

Comment 1

[Dão Gottwald [:dao]]

Comment on attachment 435258 [details] [diff] [review]
Patch (v1)

>+[chromedir="rtl"] {

#mainBox[chromedir="rtl"] {

I suppose -moz-locale-dir doesn't work here?

Comment 2

[(no longer active)]

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

Comment 3

[(no longer active)]

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

Comment 4

[Benjamin Smedberg]

chormedir="&locale.dir;"

And I'm not going to block beta on this, but keep it on the list for future betas/final

Comment 5

[(no longer active)]

(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?

Comment 6

[Justin Dolske [:Dolske]]

So... What should be done with bug 548704 now that you've made this change?(without tests, I might add ;-)

Comment 7

[(no longer active)]

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

Comment 9

[(no longer active)]

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

Comment 10

[(no longer active)]

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.

Comment 11

[(no longer active)]

Attachment: Fixup + Test

Requesting review from dbaron on reftest.js changes, and from dao on the rest.

Comment 12

[David Baron :dbaron:]

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.

Comment 13

[(no longer active)]

Attachment: 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.

Comment 14

[Dão Gottwald [: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;
}

?

Comment 15

[(no longer active)]

Attachment: 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.

Comment 16

[Dão Gottwald [:dao]]

Comment on attachment 437704 [details] [diff] [review]
Fixup + Test

I'd put .mainBox[chromedir="rtl"] {...} right after .mainBox {...}.

Comment 17

[(no longer active)]

Attachment: Patch to land

Comment 18

[(no longer active)]

This should block 1.9.2.4, because we're planning to ship Lorentz as part of that release.

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

Blocking, sure, but you'll still need approval on the patch; has it baked in the appropriate places?

Comment 20

[(no longer active)]

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

Comment 21

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/c4733ab603ef

Comment 22

[(no longer active)]

Attachment: 1.9.2 patch

I had to add prefs.getBoolPref to reftest.js for 1.9.2, requesting dbaron's review for it.

Comment 23

[David Baron :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

Comment 24

[(no longer active)]

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.

Comment 26

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 27

[LegNeato]

a=LegNeato for 1.9.2.4

Comment 28

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/bf740820e412

Comment 29

[Al Billings [:abillings - ex-MoCo]]

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.