Last Comment Bug 582181 - Incorrect rendering of directionality in mixed LTR and RTL texts
: Incorrect rendering of directionality in mixed LTR and RTL texts
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout: Text (show other bugs)
: Trunk
: x86 All
: -- normal with 1 vote (vote)
: ---
Assigned To: Simon Montagu :smontagu
:
:
Mentors:
Depends on: 263359
Blocks:
  Show dependency treegraph
 
Reported: 2010-07-27 01:10 PDT by Huji
Modified: 2011-06-16 19:56 PDT (History)
6 users (show)
smontagu: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Correct rendering (10.43 KB, image/png)
2011-05-11 00:41 PDT, ebrahim
no flags Details
Patch (5.67 KB, patch)
2011-06-14 00:29 PDT, Simon Montagu :smontagu
no flags Details | Diff | Splinter Review
Mochitests (4.72 KB, patch)
2011-06-14 00:30 PDT, Simon Montagu :smontagu
roc: review+
Details | Diff | Splinter Review
Patch with comments addressed (5.80 KB, patch)
2011-06-16 09:02 PDT, Simon Montagu :smontagu
roc: review+
Details | Diff | Splinter Review

Description Huji 2010-07-27 01:10:27 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2) Gecko/20100115 Firefox/3.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2) Gecko/20100115 Firefox/3.6

If you have a textarea which is in RTL layout, and you put in a piece of text which is a mixture of RTL and LTR content, if the LTR content starts with specific characters (like braces, curly braces or < or >), the directionality of the character is affected by the text "before" it, not the text on the same line. This causes confusion.

Reproducible: Always

Steps to Reproduce:
1. Create a page in RTL layout nd put a textarea in it which also flows from right to left. As a shortcut, you can use Persian Wikipedia's Edit Box ( http://fa.wikipedia.org/w/index.php?title=testpage&action=edit&section=new )
2. Inside the text area, paste this sample text:

Blah blah
فلان فلان
<ref>ooo</ref>
<references />


Actual Results:  
The opening < for the REF tag and the REFERENCES tag is not shown in the desired place.


See this screenshot please: http://upload.wikimedia.org/wikipedia/test/e/ed/Rtlproblem2.png

The top two windows belong to IE and Google Chrome (latest versions), and the bottom box belongs to Firefox. You can clearly get the difference in the rendering of the text.

This bug has been present in Firefox at least since version 2.5 (IIRC), and apparently no one has bothered to report it.
Comment 1 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2010-09-29 05:34:06 PDT
Is this bug and bug 505685 duplicate?
Comment 2 ebrahim 2011-03-24 15:27:19 PDT
i know this was related to Bug 263359 that today fixed :)
Comment 3 Tyler Downer [:Tyler] 2011-05-10 20:09:36 PDT
Reporter, are you still seeing this issue with Firefox 4.0.1 or later in safe mode or a fresh profile? If not, please close. These links can help you in your testing.
http://support.mozilla.com/kb/Safe+Mode
http://support.mozilla.com/kb/Managing+profiles
Comment 4 ebrahim 2011-05-11 00:32:08 PDT
Sadly, bug 263359 is backed out so this can be seen again, (in versions this was committed you couldn't find that, same "Gecko/20110413 Firefox/6.0a1")
Comment 5 ebrahim 2011-05-11 00:41:32 PDT
Created attachment 531560 [details]
Correct rendering

about above comment
Comment 6 Huji 2011-05-11 07:43:47 PDT
(In reply to comment #3)
> Reporter, are you still seeing this issue with Firefox 4.0.1 or later in
> safe mode or a fresh profile? If not, please close. These links can help you
> in your testing.
> http://support.mozilla.com/kb/Safe+Mode
> http://support.mozilla.com/kb/Managing+profiles

Yes, the bug is still present in Firefox 4.0.1 in safe mode and using a fresh profile.
Comment 7 Tyler Downer [:Tyler] 2011-06-03 17:33:01 PDT
This bug was reported using a pre-release version of Firefox 4. Now that Firefox 4.0.1 final has been released, can you please update and retest your bug? A fresh profile would be a good starting place to test, 
http://support.mozilla.com/kb/Managing+profiles. If you continue to see the issue, can you please update this bug with your results?

Filter: firefox4prebugsunco
Comment 8 Huji 2011-06-04 07:37:58 PDT
Tested with a clean profile on Firefox 4.0.1 in Ubuntu and in Win, and the bug still persists.
Comment 9 :Ehsan Akhgari 2011-06-09 13:18:12 PDT
Can you please try it in the latest nightly now that bug 263359 is fixed?
Comment 10 ebrahim 2011-06-09 13:28:25 PDT
Ehsan, I downloaded last nightly, it is generally fixed but in this testcase:
data:text/html;charset=utf-8,<textarea style="direction:rtl">فارسی%0A[[en:Farsi]]%0A</textarea>
please do a backspace in last line of textarea (remove last enter of textarea), something going wrong I thing. it is too much better from previous Firefox, but i think this little problem can be fixed also
Comment 11 Huji 2011-06-09 14:03:13 PDT
I also see another problem. Using the original example:

Blah blah
فلان فلان
<ref>ooo</ref>
<references />

If you put the cursor at the end of the last line, hit BACKSPACE once (so the > will be removed) and then type ">" again, it will not be shown correctly.
Comment 12 :Ehsan Akhgari 2011-06-09 14:30:39 PDT
I can reproduce both comment 10 and 11.  Simon, I think that dynamic changes somehow screw up the bidi calculations here...
Comment 13 Simon Montagu :smontagu 2011-06-14 00:29:22 PDT
Created attachment 539146 [details] [diff] [review]
Patch
Comment 14 Simon Montagu :smontagu 2011-06-14 00:30:24 PDT
Created attachment 539147 [details] [diff] [review]
Mochitests
Comment 15 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-14 04:29:18 PDT
Comment on attachment 539146 [details] [diff] [review]
Patch

Review of attachment 539146 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsBidiPresUtils.cpp
@@ +650,5 @@
>      || !aFrame->IsFrameOfType(nsIFrame::eBidiInlineContainer);
>  }
>  
>  void
> +nsBidiPresUtils::AppendFrameAndAdvance(nsIFrame*& aFrame,

Shouldn't this be called AdvanceAndAppendFrame?

I think aFrame and aNextSibling should be nsIFrame** so that it's clear in the callers that the frame references are modified.
Comment 16 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-14 04:30:11 PDT
Comment on attachment 539147 [details] [diff] [review]
Mochitests

Review of attachment 539147 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 18 Simon Montagu :smontagu 2011-06-16 09:02:25 PDT
Created attachment 539803 [details] [diff] [review]
Patch with comments addressed

Oops, I thought you had marked this as r+ with comments, and I checked it in already. Can you r+ it retroactively (or not, and I'll back out)?
Comment 19 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-16 19:56:01 PDT
Comment on attachment 539803 [details] [diff] [review]
Patch with comments addressed

Review of attachment 539803 [details] [diff] [review]:
-----------------------------------------------------------------

Note You need to log in before you can comment on or make changes to this bug.