664087 - Regression in caret positioning in bidi text

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Simon Montagu :smontagu]

In an LTR textarea, enter two lines of RTL text, e.g.
מוזילה
פיירפוקס

Note that although the textarea is LTR, the caret stays at the end (left edge) of the text.

Then move up and add more RTL text at the end of the first line.

Expected result: the caret stays at the end (left edge) of the text.
Actual result: the caret is at the right edge of the text, far from the characters being entered.

This is a regression from bug 263359 (tested by reverting http://hg.mozilla.org/mozilla-central/rev/3d475b322365).

Comment 1

[Simon Montagu :smontagu]

This is in fact an existing bug which bug 263359 has made more obvious: before bug 263359 the two lines of RTL text were treated as a single RTL run, but after it the newline at the end of the first line is resolved as LTR.

To see the bug in versions without bug 263359, enter RTL text followed by LTR text, e.g. "מוזילה Firefox". Then move back and enter more RTL text after the existing RTL word. The caret is displayed before the LTR text instead of after the new RTL text.

This doesn't seem to have been reported before, but that may only be because nobody knows WTF the caret should be doing in situations like that ;-)

Comment 2

[(no longer active)]

(In reply to comment #1)
> This doesn't seem to have been reported before, but that may only be because
> nobody knows WTF the caret should be doing in situations like that ;-)

Indeed!  :-)

Comment 3

[Simon Montagu :smontagu]

Attachment: Possible patch

This is caused by a kind of race condition: after entering text we set the caret bidi level to UNDEFINED in editor code. As the comment there says, what is supposed to happen is that the caret will reset the bidi level to the correct level after reflow. What actually happens is that the caret gets redisplayed before reflow, so the caret bidi level can get set to the wrong level. (In the specific case of the STRs above, it is getting set to the level of the LTR text after the new entered text before bidi resolution splits the frames).

This patch makes us set the level to UNDEFINED later, during bidi resolution. It means that we will be doing so a lot more often, not only when text is actually entered, so I need to do some kind of performance assessment.

It also causes failures in layout/base/tests/bug646382*, but I *think* that that is because the tests depend on the buggy behaviour and need to change, possibly by making the reference page set the caret to the end of the text. Ehsan, does that make sense?

Comment 4

[(no longer active)]

(In reply to comment #3)
> Created attachment 539757 [details] [diff] [review] [review]
> Possible patch
> 
> This is caused by a kind of race condition: after entering text we set the
> caret bidi level to UNDEFINED in editor code. As the comment there says,
> what is supposed to happen is that the caret will reset the bidi level to
> the correct level after reflow. What actually happens is that the caret gets
> redisplayed before reflow, so the caret bidi level can get set to the wrong
> level. (In the specific case of the STRs above, it is getting set to the
> level of the LTR text after the new entered text before bidi resolution
> splits the frames).
> 
> This patch makes us set the level to UNDEFINED later, during bidi
> resolution. It means that we will be doing so a lot more often, not only
> when text is actually entered, so I need to do some kind of performance
> assessment.

This seems fine to me...

> It also causes failures in layout/base/tests/bug646382*, but I *think* that
> that is because the tests depend on the buggy behaviour and need to change,
> possibly by making the reference page set the caret to the end of the text.
> Ehsan, does that make sense?

What sort of broken behavior are you talking about?  I'd like to make sure that we still test what we need to test in those cases...

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 539757 [details] [diff] [review]
Possible patch

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

Comment 6

[Simon Montagu :smontagu]

Attachment: patch the tests from bug 646382 to prevent regression

(In reply to comment #4)
> What sort of broken behavior are you talking about?  I'd like to make sure
> that we still test what we need to test in those cases...

Good point. I reverted bug 646382 and found new STR for the test that would show that bug with the patch from here applied.

Comment 7

[(no longer active)]

Comment on attachment 540762 [details] [diff] [review]
patch the tests from bug 646382 to prevent regression

Yes, this looks correct, thanks!

Comment 8

[Simon Montagu :smontagu]

Comment on attachment 539757 [details] [diff] [review]
Possible patch

OK, I've profiled this in a number of scenarii and I don't see any perf. problems, so I think it's good to go.

Comment 9

[Simon Montagu :smontagu]

Attachment: Mochitests

Comment 10

[(no longer active)]

Comment on attachment 542939 [details] [diff] [review]
Mochitests

Note that I backed out bug 664152, which means that test_reftests_with_caret is now a mochitest-plain test again (the backout is on mozilla-inbound until it gets merged).

You need to retest your patch, as the tests have not actually been run at all before this backout.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 539757 [details] [diff] [review]
Possible patch

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

Comment 12

[Simon Montagu :smontagu]

(In reply to comment #10)
> You need to retest your patch, as the tests have not actually been run at
> all before this backout.

They were run on Linux -- I wouldn't have requested review without seeing that the tests fail without the patch and pass with it -- but yes, I'll do another tryserver run before checking in.

Comment 13

[(no longer active)]

(In reply to comment #12)
> (In reply to comment #10)
> > You need to retest your patch, as the tests have not actually been run at
> > all before this backout.
> 
> They were run on Linux -- I wouldn't have requested review without seeing that
> the tests fail without the patch and pass with it -- but yes, I'll do another
> tryserver run before checking in.

Did you run the test standalone or in the testrunner?  In the latter case, the test should be aborted before it hits your new test, but I think that it would work just fine in standalone  mode (without actually having tested it).

Comment 14

[Simon Montagu :smontagu]

(In reply to comment #13)
> Did you run the test standalone or in the testrunner? 

Both, since you ask ;-) They show up in the Linux tryserver logs at http://tbpl.mozilla.org/?tree=Try&rev=7986ea485409 also, e.g. from http://tinderbox.mozilla.org/showlog.cgi?log=Try/1309360705.1309362854.13319.gz&fulltext=1:

10543 INFO TEST-PASS | chrome://mochitests/content/chrome/layout/base/tests/chrome/test_reftests_with_caret.html | Reftest chrome://mochitests/content/chrome/layout/base/tests/chrome/bug664087-1.html
10544 INFO TEST-PASS | chrome://mochitests/content/chrome/layout/base/tests/chrome/test_reftests_with_caret.html | Reftest chrome://mochitests/content/chrome/layout/base/tests/chrome/bug664087-2.html

Comment 15

[(no longer active)]

Oh, yes.  What I meant to say was, these tests were not run _on Mac_!  :-)

Comment 16

[Simon Montagu :smontagu]

http://hg.mozilla.org/mozilla-central/rev/b552fdfe62a6
http://hg.mozilla.org/mozilla-central/rev/3f27dc203e62

Checked in with some tweaks to the tests for Mac, and after a tryserver run with the order of the tests in test_reftests_with_caret.html changed so that all the tests ran on all the tryservers.