Steps to reproduce: 1) Type right-to-left characters followed by left-to-right characters in a text area, or copy and paste the following line אבגabc 2) Position the caret visually between "א" and "a" 3) Press backspace. Expected results: depending on bug 1034337 and the value of bidi.edit.delete_immediately, the first press of backspace may or may not delete "ג" (the leftmost character visually), but in either case the caret should move to the left of the line Actual results: the caret doesn't move

Regression range ftp://ftp.mozilla.org/pub/firefox/nightly/2012/10/2012-10-29-03-05-53-mozilla-central/firefox-19.0a1.en-US.linux-x86_64.tar.bz2 Bug 805697?

Simon, is it okay if I take this?

Go for it!

I think it was Bug 1048752. But don't worry, there's only 39 patch files I have to go through to try to find the problem.

Oops, I just saw that I pasted the wrong link as "regression range" in comment 1 -- it should be http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e069342dc665&tochange=e19e170d2f6d

Ah. Thanks, Simon. I've been looking in the wrong place.

Okay, there are going to be multiple patches on this one, Simon.

This is the core of the problem. As you know, when a character is deleted, the bidi level of the caret should be set to the bidi level of the removed character. That was happening, but then some code in nsTextFrame::AdjustOffsetsForBidi() was unsetting the caret bidi level. I've moved that code to a place where it makes more sense, and where it won't happen at the wrong time.

This patch fixes a couple of problems with the positioning of the caret in bidi contexts. * Firstly, the bidi-related code in nsCaret::GetCaretFrameForNodeOffset() needs to be executed whenever the text is bidi, not just when the bidi.browser.ui pref is set. * Secondly, nsFrameSelection::MoveCaret() was resetting the caret bidi level to the paragraph default after a Home (start of line) or End (end of line) caret movement. There's no reason for that, and it was interfering with the correct positioning of the caret at the start or end of a line. These problems aren't specifically related to deleting characters (they are broader than that), but they need to be fixed in order for the previous patch to be effective.

Unfortunately, test_bug496275.html relied on incorrect caret behaviour that was fixed by the previous patch. To work again, the test needs to manually set the bidi level of the caret. (At least, I figured that was the least confusing way to amend the test.) There currently isn't a way for Javascript code to set the caret bidi level. (There used to be a method called Selection.selectionLanguageChange() that could set the caret bidi level to either 0 or 1, but it was removed by Ehsan in Bug 949445.) So this patch adds a new attribute to the Selection object called caretBidiLevel. To help justify the existence of the caretBidiLevel attribute, I'll note that the Selection object already has an interlinePosition attribute, which is quite similar in purpose. Both caretBidiLevel and interlinePosition are used to translate the logical caret position (as set by Selection.collapse()) into a physical caret position. In fact, I'd suggest that it's useless having one without the other. (With only one, you can partially, but not fully, specify the position of the physical caret. And what's the point in that?) Like interlinePosition, I've made the caretBidiLevel attribute ChromeOnly, so it's not exposed to the public, and hopefully won't need to be reviewed by a committee.

And now we have to talk about test_reftests_with_caret.html. This test executes a whole bunch of subtests regarding caret positioning. Unfortunately, there are a whole bunch of timeouts happening. Furthermore, these timeouts obscure the fact that A LOT of the subtests are currently failing. (For some stupid reason, the first time a timeout happens, it aborts the rest of the subtests.) I figured out that the timeouts are caused by exceptions in multi-range-user-select.html, caused by out-of-bounds indicies. This patch: * Fixes the timeouts * Amends test_reftests_with_caret.html so that a timeout does not abort the rest of the subtests. This patch does not: * Fix the currently failing subtests * Update the subtests to match the changes in my previous patches. (Those last couple things will be in my next patch(es), which will have to wait until tomorrow.)

blocking a blocker

Comment on attachment 8609185 [details] [diff] [review] bug-1067788-no-invalidate-bidi-caret-after-delete.patch Review of attachment 8609185 [details] [diff] [review]: ----------------------------------------------------------------- Essentially this looks good. I was afraid that it would regress bug 664087 (which it more or less reverts) but AFAICT it doesn't. Maybe because of the various caret refactorings that have happened since then. You do need to patch nsHTMLEditRules.cpp as well as nsTextEditRules.cpp, though. ::: editor/libeditor/nsTextEditRules.cpp @@ +250,5 @@ > > // my kingdom for dynamic cast > nsTextRulesInfo *info = static_cast<nsTextRulesInfo*>(aInfo); > > switch (info->action) { I don't really like having two consecutive switch statements on the same field. What do you think about putting the UndefineCaretBidiLevel into a helper method (which would also have the advantage that you wouldn't have to duplicate the code in nsHTMLEditRules) and calling it from both cases in the existing switch?

Comment on attachment 8609190 [details] [diff] [review] bug-1067788-bidi-caret-fixes.patch Review of attachment 8609190 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsCaret.cpp @@ +674,5 @@ > // Direction Style from visibility->mDirection > // ------------------ > // NS_STYLE_DIRECTION_LTR : LTR or Default > // NS_STYLE_DIRECTION_RTL > + if (theFrame->GetStateBits() & NS_FRAME_IS_BIDI) Can you make this test |theFrame->PresContext()->BidiEnabled()| instead? I think that will be more stable. It's also (mutatis mutandis) what it did before bug 418513, which probably should have left this line alone. ::: layout/generic/nsSelection.cpp @@ -1012,5 @@ > switch (aAmount) { > case eSelectBeginLine: > case eSelectEndLine: > - // set the caret Bidi level to the paragraph embedding level > - SetCaretBidiLevel(NS_GET_BASE_LEVEL(theFrame)); What use case are you fixing here? This feels like the kind of change which will fix some things and break others. This might be better as a separate bug.

Comment on attachment 8609197 [details] [diff] [review] bug-1067788-fix-timeouts-in-caret-tests.patch Review of attachment 8609197 [details] [diff] [review]: ----------------------------------------------------------------- I think Ehsan should review this.

Comment on attachment 8609197 [details] [diff] [review] bug-1067788-fix-timeouts-in-caret-tests.patch Review of attachment 8609197 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/tests/test_reftests_with_caret.html @@ +69,5 @@ > var docEl = iframe.contentDocument.documentElement; > if (docEl.className.indexOf("reftest-wait") >= 0) { > if (currentIteration++ > MAX_ITERATIONS) { > ok(false, "iframe load for " + url + " timed out"); > + nextTest(); Please revert this line, since one timeout aborting the test is intentional. The timeout can leave the iframes belonging to the test on the screen, which may affect our ability to simulate things such as click events for the future tests (because they would end up being off-screen.)

> What do you think about putting the UndefineCaretBidiLevel into a helper method (which would also have the advantage that you wouldn't have to duplicate the code in nsHTMLEditRules) I think that sounds like a good idea!

> you make this test |theFrame->PresContext()->BidiEnabled()| instead? Alrighty. > > - // set the caret Bidi level to the paragraph embedding level > > - SetCaretBidiLevel(NS_GET_BASE_LEVEL(theFrame)); > What use case are you fixing here? This is an issue that affects test_bug646382.html and test_496275.html. You know, although I think it's the right thing to do, I'm going to move it to a different patch, where its purpose will be more apparent.

> > + nextTest(); > Please revert this line, since one timeout aborting the test is intentional. Okay.

Hi Simon, You r+'d this before, but I've added a little more stuff to this patch, so it needs to be reviewed again. It turns that I was wrong about many tests failing. Only a few tests were failing. (The others appeared to be failing because of issues in my test environment, and because earlier failures can cause later tests to fail.) The only tests that need to be updated are test_bug496275.html, bug646382-1.html, and bug646382-2.html. This patch contains all the necessary fixes. 1) Changes to the test files themselves. 2) Adding a caretBidiLevel property to Selection, as discussed before. (For test_bug496275.html.) 3) A change to nsFrameSelection::MoveCaret(). This is necessary for all three tests. Now, you expressed concern about this change to nsFrameSelection::MoveCaret() before, and you were right. I had it wrong before. But I've fixed it now. The change affects the situation where the user uses the Home (or End) key on a line that begins (or ends) with text that is opposite the paragraph's normal direction. For example, an English paragraph that says: "אבגדה are the first five letters of the Hebrew alphabet." In this situation, when the user hits the Home key, nsFrameSelection::MoveCaret() is called with aAmount = eSelectBeginLine. The method then calls nsFrame::PeekOffset() with the appropriate parameters, which returns the *logical* position which corresponds to the visual start of the line. (In our example above, it would be 5.) This logical position is used to position the caret. However, this logical position corresponds to two visual positions. The caret will only appear in the correct visual position if the caret bidi level is set to the same bidi level as the frame at the start of the line. Right now it's being set to the paragraph bidi level, which isn't always the same thing. We already have the frame at the start of the line, thanks to GetFrameForNodeLevel() (which uses the logical position and the "caret association hint" returned by PeekOffset()). My patch sets the caret bidi level to the embedding level of this frame.

(In reply to Ted Clancy [:tedders1] from comment #21) > We already have the frame at the start of the line, thanks to > GetFrameForNodeLevel() I mean GetFrameForNodeOffset().

Comment on attachment 8612543 [details] [diff] [review] bug-1067788-tests-involving-setcaretbidilevel.patch See https://bugzilla.mozilla.org/show_bug.cgi?id=1067788#c21

(In reply to Simon Montagu :smontagu from comment #15) > This might be better as a separate bug. You know what? On second thoughts, I think you're right. I should have split that off into a different bug. I'm gonna do that. I've cancelled the relevant review.

Sorry, Simon. I tried to split this off into a new bug, but I can't do it. It leads to mutual dependency between the two bugs where I can't land without the other (at least, not without breaking tests).

Green treeherder run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5cb46d183ea

Hi, part 1 failed to apply: renamed 1067788 -> bug-1067788-no-invalidate-bidi-caret-after-delete.patch applying bug-1067788-no-invalidate-bidi-caret-after-delete.patch patching file editor/libeditor/nsTextEditRules.h Hunk #1 FAILED at 193 1 out of 1 hunks FAILED -- saving rejects to file editor/libeditor/nsTextEditRules.h.rej patch failed, unable to continue (try -v) patch failed, rejects left in working directory could you take a look, thanks!

Hey Carsten. No prob. It just needed a slight rebase.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d9e2afd7c146 https://hg.mozilla.org/integration/mozilla-inbound/rev/e6e2759a1205 https://hg.mozilla.org/integration/mozilla-inbound/rev/102b7beb6039 https://hg.mozilla.org/integration/mozilla-inbound/rev/237690e8e193 https://hg.mozilla.org/integration/mozilla-inbound/rev/90bdd7a731ef

https://hg.mozilla.org/mozilla-central/rev/d9e2afd7c146 https://hg.mozilla.org/mozilla-central/rev/e6e2759a1205 https://hg.mozilla.org/mozilla-central/rev/102b7beb6039 https://hg.mozilla.org/mozilla-central/rev/237690e8e193 https://hg.mozilla.org/mozilla-central/rev/90bdd7a731ef

Comment on attachment 8613899 [details] [diff] [review] bug-1067788-no-invalidate-bidi-caret-after-delete.patch NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings. [Approval Request Comment] Bug caused by (feature/regressing bug #): RTL support for B2G (Bug 906270) User impact if declined: The caret would move in confusing ways in bidi text. Testing completed: Green treeherder run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5cb46d183ea Risk to taking this patch (and alternatives if risky): None foreseen. String or UUID changes made by this patch: None.

Comment on attachment 8612376 [details] [diff] [review] bug-1067788-bidi-caret-fixes.patch NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings. [Approval Request Comment] Bug caused by (feature/regressing bug #): RTL support for B2G (Bug 906270) User impact if declined: The caret would move in confusing ways in bidi text. Testing completed: Green treeherder run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5cb46d183ea Risk to taking this patch (and alternatives if risky): None foreseen. String or UUID changes made by this patch: None.

Comment on attachment 8612378 [details] [diff] [review] bug-1067788-fix-timeouts-in-caret-tests.patch NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings. [Approval Request Comment] Bug caused by (feature/regressing bug #): RTL support for B2G (Bug 906270) User impact if declined: The caret would move in confusing ways in bidi text. Testing completed: Green treeherder run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5cb46d183ea Risk to taking this patch (and alternatives if risky): None foreseen. String or UUID changes made by this patch: None.

Comment on attachment 8609193 [details] [diff] [review] bug-1067788-tests-involving-setcaretbidilevel.patch NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings. [Approval Request Comment] Bug caused by (feature/regressing bug #): RTL support for B2G (Bug 906270) User impact if declined: The caret would move in confusing ways in bidi text. Testing completed: Green treeherder run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5cb46d183ea Risk to taking this patch (and alternatives if risky): None foreseen. String or UUID changes made by this patch: None.

Comment on attachment 8613208 [details] [diff] [review] Bug 1067788 - Part 5: Fix for parts of test_bug496275.html. NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings. [Approval Request Comment] Bug caused by (feature/regressing bug #): RTL support for B2G (Bug 906270) User impact if declined: The caret would move in confusing ways in bidi text. Testing completed: Green treeherder run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5cb46d183ea Risk to taking this patch (and alternatives if risky): None foreseen. String or UUID changes made by this patch: None.

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/8fd26357b306 https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/c130d03a7173 https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/88a00625bf9e https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/15dd8a6fbbb3 https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/2475c846e016

Ted on the latest Nightly Flame build I'm seeing some odd behavior that I'm not sure is intended. When attempting to copy אבגabc into a text field (or make similar text on my own), the phone always relocates the ltr text (abc) to the left of the RTL text (אבג). Environmental Variables: Device: Flame 3.0 BuildID: 20150609081840 Gaia: ea27c4ed5b6083c9e21d233d4804372ac4d5d353 Gecko: e10e2e8d8bf2 Version: 41.0a1 (3.0) Firmware Version: v18D-1 User Agent: Mozilla/5.0 (Mobile; rv:41.0) Gecko/41.0 Firefox/41.0

Hi Jayme. I think that's fine. What's happening is that you're moving text from an LTR field to an RTL field. (Or rather, the field switches to RTL after you paste that text into it.) In both cases, the Hebrew text appears before the Latin text. In an LTR field, that means the Hebrew text is on the left. In an RTL field, that means the Hebrew text is on the right. It might seem confusing, but in the context of an actual Bidi sentence, it's usually clear what's happening.

In that case using the build from comment 42 I can mark this as verified fixed on Master. Leaving the verifyme keyword so that this can be checked in 2.2 later by myself or someone else. Actual Results: The character to the left of the text caret is deleted when pressing the delete button.

I was able to reproduce this issue on Firefox 41.0a1 (2015-05-28) using Windows 7 64-bit. In order to verify this issue there are 2 options to place the caret between the Left-to-right letters and Right-to-left letters. And therefore there are 2 different behaviors: 1.Placing the caret between the Latin letters and right-to-left letters using the mouse. - Pressing backspace, the cursor moves to the left of the line and no letter is deleted 2.Placing the caret between the Latin letters and right-to-left characters using the left arrow of the keyboard - Pressing backspace, the cursor moves to the left of the line and the right-to-left letters are deleted one by one. The Latin letters are not erased. Are these the expected results? I obtained these results when I pasted the example from Description in location bar and search bar (near location bar). But there is another behaviour when the same word “ אבגabc “ is pasted in the search bar from about:newtab page and about:home page. See screenshot: http://i.imgur.com/bDPIL1F.jpg The RTL characters are reversed with LTR characters and the word is displayed as in a RTL build. In this case, the caret is automatically place between those 2 types of characters and pressing backspace button deletes all the letters. This behaviour does not occur in a right-to-left build, where “ אבגabc “ word is placed from right to left in all 4 cases: location bar, search bar (near location bar), search bar from about:newtab page and search bar from about:home page. Ted, could you explain me which of these results are the right ones and if is there some issue that need to be filed.

Long story short, everything is fine. > 1.Placing the caret between the Latin letters and right-to-left letters using the mouse. > - Pressing backspace, the cursor moves to the left of the line and no letter is deleted This happens when you click closer to the Hebrew letters than the Latin letters, or if you position the cursor using the right arrow key. (You'll see a difference in the direction the cursor faces.) Yes, it's fine. > 2.Placing the caret between the Latin letters and right-to-left characters using the left arrow of the keyboard > - Pressing backspace, the cursor moves to the left of the line and the right-to-left letters are > deleted one by one. The Latin letters are not erased. Again, this is fine. > But there is another behaviour when the same word “ אבגabc “ is pasted in the search bar from > about:newtab page and about:home page. See screenshot: http://i.imgur.com/bDPIL1F.jpg > The RTL characters are reversed with LTR characters and the word is displayed as in a RTL build. The search bars on those pages switch directionality based on the first character of the content. Because אבגabc starts with a Hebrew letter, the search bar switches to RTL. It's debatable about whether that's the best behaviour, but that's a UX decision. (I think it's useful, especially since there's no obvious way to change the directionality of a text box in an LTR build. Perhaps if the search bar contains bidi text, the context menu should show options for switching the directionality of the text, just like in RTL builds? Feel free to file a bug for that.) > This behaviour does not occur in a right-to-left build, where “ אבגabc “ word is placed from right to > left in all 4 cases Hmmm. It does seem a bit odd that it doesn't behave the same as LTR builds. I don't know if this was a deliberate styling choice or an oversight. It may have been deliberate, since you can change the directionality of the search bar from the context menu in an RTL build. In any case, it's a UX issue, not related to this bug.

Filed Bug 1205669 as follow-up bug Verified again on Firefox 41 (20150916203902) under Windows 7 64-bit and Ubuntu 14.04 32-bit. Marking this bug as Verified since the other issue is tracked separately.