Closed Bug 558403 Opened 10 years ago Closed 10 years ago

Plugin crash message on on Lorentz acts like bidi-override: embed is in effect

Categories

(Firefox :: General, defect)

3.6 Branch
x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: tomer, Assigned: smontagu)

References

()

Details

(Keywords: rtl, Whiteboard: [3.6.3plugin1])

Attachments

(6 files, 2 obsolete files)

When a plugin crash message appear on screen with the sad icon, the error message lack RTL direction on RTL builds. 

(Tested with Mozilla/5.0 (X11; U; Linux i686; he; rv:1.9.2.4pre) Gecko/20100409 Namoroka/3.6.4pre and crashed with http://flashcrash.dempsky.org/)
Summary: Plugin crash message on Firefox3.6.3plugin1 (lorenz) lack dir=rtl → Plugin crash message on Firefox3.6.3plugin1 (lorentz) lack dir=rtl
On the following page, I'm able to make the bidi function to fail, and the letters get positioned incorrectly. 

http://reshet.ynet.co.il/12912.aspx
I had reported and patched this previously, and it will hopefully land on Lorentz shortly.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 555289
(In reply to comment #1)
> Created an attachment (id=438128) [details]
> Flash plugin crash message which make the letters to reverse
> 
> On the following page, I'm able to make the bidi function to fail, and the
> letters get positioned incorrectly. 
> 
> http://reshet.ynet.co.il/12912.aspx

Hmm, I can't read Hebrew, but do you mean that the letters appear as HEBREW instead of WERBEH?
(In reply to comment #3)
> Hmm, I can't read Hebrew, but do you mean that the letters appear as HEBREW
> instead of WERBEH?
Indeed. You might be able to compare the text appear on the two attached screenshots.
Attached image Correct error message
I am confused. I am able to get the error message with correct the correct bidi property sometimes. 

URL: http://www.ynet.co.il/articles/0,7340,L-3873434,00.html
One other thing I noticed in the Persian build is if I manually change the chromedir property to ltr (and hence enforcing direction: ltr), the letters will appear in their joined form, but will be layed out from left to right.  I think that something's broken here, and I don't think that the problem is with our CSS styles.

CCing Simon to see if he has any opinion on what's wrong here.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Summary: Plugin crash message on Firefox3.6.3plugin1 (lorentz) lack dir=rtl → Plugin crash message on on Lorentz acts like bidi-override: embed is in effect
Attached image Another screenshot
(In reply to comment #5)
> I am confused. I am able to get the error message with correct the correct bidi
> property sometimes. 
> URL: http://www.ynet.co.il/articles/0,7340,L-3873434,00.html

Still reproduce, although I understood that the patch to the RTL issue has been landed. 

Mozilla/5.0 (X11; U; Linux i686; he; rv:1.9.2.4pre) Gecko/20100410 Lorentz/3.6.4pre

Ehsan (and other people who can't read Hebrew) - The result is wrong when the full-stop character appear at the right side of the word in RTL languages, which appear correct only with the flash crash message in the middle of the attached screenshot
I can think of three things which can cause text to be displayed in reverse as in attachment 438128 [details]: unicode-bidi: bidi-override; RLO/LRO; or bidi resolution failing to split the text into LTR and RTL text runs.

The third one seems most likely in this case -- or as a variation on it, perhaps bidi resolution isn't happening at all. I'm investigating that possibility now.
(In reply to comment #8)
> I can think of three things which can cause text to be displayed in reverse as
> in attachment 438128 [details]: unicode-bidi: bidi-override; RLO/LRO; or bidi resolution
> failing to split the text into LTR and RTL text runs.

I've already ruled out the first two possibilities by testing.

Note that we specifically set unicode-bidi: embed; on the XUL box which contains the box which contains the localized message.

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/plugins/content/pluginProblemContent.css#35

Also, note that the XUL contents do not inherit any styles from the page content:

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/plugins/content/pluginProblem.xml#50

> The third one seems most likely in this case -- or as a variation on it,
> perhaps bidi resolution isn't happening at all.

Agreed.

> I'm investigating that
> possibility now.

Great!
Actually it's none of the above -- http://reshet.ynet.co.il/Static/html/mashina/index.html has <meta http-equiv="Content-Type" content="text/html; charset=iso-8859-8" /> which makes us use visual ordering. I thought there was an old bug about not using visual ordering for content in a visual page generated by javascript, but I can't find it right now.
Hmm, so do you have any idea how the fix might look like?  Is it something which can be crafted easily?
I think we might be able to leverage what we already do to force form elements not to be visual in nsBlockFrame::IsVisualFormElement
Attached patch Patch v.0 (obsolete) — Splinter Review
Something like this, maybe, but I'm open to suggestions whether "is a XUL element" is or isn't the thing to be testing for.

(A final patch will also need to change the name of IsVisualFormControl, and we might also want to move the whole thing into nsBidiPresUtils)
Assignee: nobody → smontagu
Attachment #438439 - Flags: feedback?(ehsan)
I've requested by smontagu to test again the two issues: 

a. The page with the flash video (attachment 438340 [details] and attachment 438145 [details]) - Pass.

b. The page with the streaming flash audio (attachment 438128 [details]) - With the current build the flash doesn't reverse, but the English text. 

Mozilla/5.0 (X11; U; Linux i686; he; rv:1.9.2.4pre) Gecko/20100411 Namoroka/3.6.4pre
(In reply to comment #13)
> Created an attachment (id=438439) [details]
> Patch v.0
> 
> Something like this, maybe, but I'm open to suggestions whether "is a XUL
> element" is or isn't the thing to be testing for.

This will solve the current problem, but I think a slightly better thing to test for is whether the element is inside the XBL bound anonymous content.  At least, that specifies our intention more clearly, and will prevent this from breaking if someone decided to wrap the text in a span or something.

> (A final patch will also need to change the name of IsVisualFormControl, and we
> might also want to move the whole thing into nsBidiPresUtils)

Agreed.
Comment on attachment 438439 [details] [diff] [review]
Patch v.0

That said, if comment 15 is hard to implement, this approach would also be fine, if accompanies by a reftest (which can be based on the below reftests).

http://mxr.mozilla.org/mozilla-central/source/modules/plugin/test/reftest/pluginproblemui-direction-1.html?force=1
http://mxr.mozilla.org/mozilla-central/source/modules/plugin/test/reftest/pluginproblemui-direction-ref.html?force=1
Attachment #438439 - Flags: feedback?(ehsan) → feedback+
(In reply to comment #15)
> This will solve the current problem, but I think a slightly better thing to
> test for is whether the element is inside the XBL bound anonymous content.

That won't work because there can be XBL bound anonymous content which uses text from the document and we don't want to force logical ordering (<marquee> does this).

I'm not sure how to reftest this, because the bug only shows up in RTL l10ns. Is there some way to change a localized property programmatically during a test?
(In reply to comment #17)
> (In reply to comment #15)
> > This will solve the current problem, but I think a slightly better thing to
> > test for is whether the element is inside the XBL bound anonymous content.
> 
> That won't work because there can be XBL bound anonymous content which uses
> text from the document and we don't want to force logical ordering (<marquee>
> does this).

You're right.

> I'm not sure how to reftest this, because the bug only shows up in RTL l10ns.
> Is there some way to change a localized property programmatically during a
> test?

Well, yes, there is, but it's kind of tricky.  ;-)

I've done this once, and there may be better ways of doing this, but anyway.  See this test:

<http://mxr.mozilla.org/mozilla-central/source/toolkit/content/tests/chrome/test_bug437844.xul>

You'll have to register a manifest like this:

<http://mxr.mozilla.org/mozilla-central/source/toolkit/content/tests/chrome/bug437844.manifest>

Which overrides the dtd file containing the strings like this:

<http://mxr.mozilla.org/mozilla-central/source/toolkit/content/tests/chrome/bug437844.dtd>

And of course you need to do this as a mochitest and use canvas snapshots to simulate a reftest.


Or, you could just do the simpler thing, which is writing your own XBL, and binding that to an element of your choice, and just use your own DTD file to inject Hebrew strings in the document.
Attached patch Patch v.1Splinter Review
Attachment #438439 - Attachment is obsolete: true
Attachment #440508 - Flags: review?(roc)
Attached patch Test (obsolete) — Splinter Review
This is put together in the best traditions of cargo-cult programming from the tests Ehsan referred to above, with additions from reftest.js to add support for reftest-wait.
Attachment #440512 - Flags: review?(ehsan)
Comment on attachment 440512 [details] [diff] [review]
Test

I definitely like all the voodoo going on in this test!  I just don't know why you need to make windows rtl as well (i.e., why would you need bug437844.css?)

I think if you remove that, you can also get rid of the cr.refreshSkins calls.  Other than that, everything seems fine to me here.
Comment on attachment 440512 [details] [diff] [review]
Test

r=me with comment 21 addressed.
Attachment #440512 - Flags: review?(ehsan) → review+
Comment on attachment 440508 [details] [diff] [review]
Patch v.1

+nsBidiPresUtils::Resolve(nsBlockFrame*   aBlockFrame)

Fix unnecessary spaces

+     * We always use logical order on form controls, so that they will display
+     * correctly in native widgets in OSs with Bidi support.

Does this comment still make sense since we don't use native widgets?

+  nsresult Resolve(nsBlockFrame*   aBlockFrame);

fix spaces
Attachment #440508 - Flags: review?(roc) → review+
Checked in with comments addressed
http://hg.mozilla.org/mozilla-central/rev/c336b0c25cfa
http://hg.mozilla.org/mozilla-central/rev/1ddda57c4002

and since the topic came up I also put in a reftest for logical and visual marquee
http://hg.mozilla.org/mozilla-central/rev/f32afa31a2e1
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
The test was crashing on some tinderboxes, e.g. http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1272199623.1272201070.18150.gz. I guess it has to be made conditional in some way, but I'm not sure how, so I've backed it out for now.
Flags: in-testsuite+ → in-testsuite?
(In reply to comment #25)
> The test was crashing on some tinderboxes, e.g.
> http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1272199623.1272201070.18150.gz.
> I guess it has to be made conditional in some way, but I'm not sure how, so
> I've backed it out for now.

Two ideas:
 (1) if loading the test file dosen't crash but running it does, you could check whether you're on Mac at the start of the test, and if so, report a single todo(false, ...) and then return
 (2) otherwise, you could ifdef the Makefile for whether you're on Mac, i.e., ifeq($(MOZ_WIDGET_TOOLKIT),cocoa)
Can it be directly conditional on OOPP rather than on platform?
(In reply to comment #27)
> Can it be directly conditional on OOPP rather than on platform?

Yes, check for the value of the dom.ipc.plugins.enabled pref.
Attached patch Test v.2Splinter Review
I added the pref check and then went a little wild trying to consolidate all the duplicated code in the directory. Makes sense?
Attachment #440512 - Attachment is obsolete: true
Attachment #441498 - Flags: review?(ehsan)
Comment on attachment 441498 [details] [diff] [review]
Test v.2

Looks great, thanks!
Attachment #441498 - Flags: review?(ehsan) → review+
http://hg.mozilla.org/mozilla-central/rev/b464cc1789bd
Flags: in-testsuite? → in-testsuite+
I'm removing the test for reasons given in bug 581734 comment 31. I'll write a new one doing "the simpler thing" described in the last line of comment 18.
Flags: in-testsuite+ → in-testsuite?
You need to log in before you can comment on or make changes to this bug.