303399 - Bidi: Problems with caret positioning on blank lines

Status: VERIFIED FIXED

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

Description

[Uri Bernstein]

When the caret is logically placed in a blank line in a textarea, it is
sometimes displayed as if it were at the end of the previous line.

This was originally reported in bug 207186 comment 6, and bug 207186 comment 7
(and 8).

Testcase #3 from bug 207186: https://bugzilla.mozilla.org/attachment.cgi?id=128682
In the RTL textarea, place the caret at the end of the text, and press Enter.
The caret remains in its place. Typing some letters verifies that the caret is,
logically, on a new line.
In the LTR textarea, move to the end of the text, then type some RTL (Hebrew)
letters, and then hit "Enter". Again, the caret won't visually move, although it
is logically in the new line.

Testcase #4 from bug 207186: https://bugzilla.mozilla.org/attachment.cgi?id=128683
In the RTL textarea, place the caret at the ond of the text, and press "Enter".
The caret correctly appears on the next line. Now switch the keyboard to an LTR
(English) layout. The caret will be displayed at the end of the first line
(although it's still logically on the second line).
The LTR textarea displays similar behaviour (when you switch the keyboard to RTL).

Comment 1

[Uri Bernstein]

Attachment: tentative patch

The problem, as I understand it, is that nsCaret::SetupDrawingFrameAndOffset()
is using the wrong frameSelection. It is always using the document frame
selection (obtained via nsPresShell), but but a textarea has its own frame
selection (controlled by nsTextInputSelectionImpl) - and that's the selection
that should be used.

What is happening is that when the document frame selection has a different
hint than the textarea frame selection, frameSelection->GetPrevNextBidiLevels()
uses the wrong (doc selection) hint, and therefore gets a different frame as
the current frame, and that confuses the bidi caret positioning algorithm.

So what I wanted to do was to use the actual frame selection associated with
the caret, rather than the document one. However, I couldn't find a way to get
to it through the DOM selection (held as mDomSelectionWeak), so I needed to add
a COM getter method - which I added to nsISelectionPrivate.

Things I'd like to get review on:
- Is my analysis of the problem and my proposed solution (using the "other"
frame selection) correct?
- Do I really have to add a method to a XPCOM reference in order to do that?
- Does my choice of adding the method to nsISelectionPrivate (as opposed to,
say, nsISelection) make sense?
- Did I technically did this correctly? (Is the method named correctly, did I
really need to replace the UUID, etc.)

Blake - these are basically the questions I asked you in my latest e-mail
(before I had a patch), so you can just reply here instead of replying to the
e-mail, if you want to.

Comment 2

[Uri Bernstein]

Attachment: tentative patch

Bah. That one contained some unrelated stuff. This one should be OK.

Comment 3

[Uri Bernstein]

Attachment: patch v 2

Same thing, but also use privateSelection->GetFrameSelection() instead of
presShell->FrameSelection() in nsCaret::GetCaretCoordinates() (for corectness).

Notice that getting the hint is now easier.

Comment 4

[Uri Bernstein]

Comment on attachment 192022 [details] [diff] [review]
patch v 2

Sorry. Wrong patch. right one coming in a minute.

Comment 5

[Uri Bernstein]

Attachment: patch v 2

The correct patch, hopefully.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 192023 [details] [diff] [review]
patch v 2

Thanks for figuring all of this messy frame selection stuff out!
r=me

Comment 7

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

Comment on attachment 192023 [details] [diff] [review]
patch v 2

Looks good, except that GetFrameSelection should AddRef its out parameter, and
you should call GetFrameSelection(getter_AddRefs(frameSelection)) (after
converting frameSelection to an nsCOMPtr)

Comment 8

[Uri Bernstein]

Attachment: Patch v 2.1

Right. I think I'm starting to get the hang of this XPCOM stuff (I originally
tried using getter_AddRefs(), but since I neglected to do the NS_ADDREF part in
the getter itself, I ended up with a null pointer later. Duh!).

Not sure if I have to get r+ again, so asking just to be on the safe side.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 193548 [details] [diff] [review]
Patch v 2.1

>Index: layout/generic/nsSelection.cpp
>+  *aFrameSelection = mFrameSelection;
>+  NS_ADDREF(*aFrameSelection);

This could be written as:
NS_ADDREF(*aFrameSelection = mFrameSelection);

But it's your stylistic choice. Sorry for not catching this before.
r=mrbkap

Comment 10

[Uri Bernstein]

Patch checked in by mrbkap at 2005-08-24 10:44.

Comment 11

[Asaf Romano (gone)]

Nominating for 1.8/fx1.5.

Comment 12

[Uri Bernstein]

Comment on attachment 193548 [details] [diff] [review]
Patch v 2.1

Asking for approval1.8b4. This bug is relatively major (it makes editing bidi
text pretty annoying), and the fix isn't too complicated. It would be a shame
not to have this fixed for 1.5.

Comment 13

[Mike Shaver (:shaver emeritus)]

If we're going to take this, we need a lot of reports of it working well on the
trunk, with bidi, extensions that do weird things with selection, Midas, etc.
before we put it on the branch.

(N.B. that such positive reports are not a guarantee that it'll be on the
branch.  We need to lock that baby down tight tight to avoid regressions that
put 1.5 at risk.)

Also: is this a regression vs. 1.0?  If so, can someone characterize the impact
on real-world sites?

Comment 14

[Uri Bernstein]

(In reply to comment #13)
> Also: is this a regression vs. 1.0?  If so, can someone characterize the impact
> on real-world sites?

This is not a regression vs. 1.0. Bidi editing was practicably unusable on 1.0.
(see the fifth paragraph of bug 207186, comment #6). This is part of an effort
to make it usable on 1.5.

Comment 15

[Asaf Romano (gone)]

Checked into 1.8 branch.

Many thanks, Uri!