Bidi: Caret sometimes moves incorrectly when moving into HTML paragraph starting/ending with reverse-direction text

RESOLVED FIXED

Status

()

Core
Layout: Text
RESOLVED FIXED
12 years ago
10 years ago

People

(Reporter: Uri Bernstein (Google), Assigned: Uri Bernstein (Google))

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

12 years ago
This is a follow-up to bug 330815.

Sometimes (depending on whether there is or isn't whitespace betwen the paragraphs), when moving the caret into a pargraph starting [ending] with text of reverse direction (relative to that pargraph), the caret moves to the wrong side of the reverse-direction text (that is, not to the visual start/end of the paragraph). 

In bug 330815, comment #6, I wrote about this:

> As far as I can tell, there are two hint-related problems:
>
> 1. The hint is flipped for each line-jump, so in the case of unrendered
> whitespace between two paragraphs, a single step involves two line-jumps, which
> results in two hint-flips, which is wrong. The solution to this is fairly
> simple: the hint should be flipped (in GetFrameForDirection) if the amount is
> eSelectCharacter, but not if it is eSelectNoAmount (indicating that we already
> jumped a line).
> This problem is not bidi-related, but in practice I couldn't come up with a
> non-bidi case which shows a visible bug due to this.
> 
> 2. For bidi, it seems that the correct rule is to flip the hint if the final
> destination frame is a reverse-direction frame (relative to its own paragraph).
> The current code flips the hint for a reverse-direction frame when the amount
> is eSelectCharacter, (which is not necessarily the last step), and also flips
> it back during a line jump if we moved to a frame with an opposite direction.

I'll attach a testcase soon.
(Assignee)

Comment 1

12 years ago
Created attachment 215990 [details]
testcase

Use in caret browsing mode or composer.

Notice that in some cases whitespace between the paragraphs is what's causing the problem, and in other cases it's the lack of such whitespace.
(Assignee)

Updated

12 years ago
Assignee: mozilla → uriber
(Assignee)

Updated

12 years ago
Blocks: 328656
(Assignee)

Comment 2

12 years ago
Created attachment 216232 [details] [diff] [review]
patch

Enough with the hint-flipping madness!

It took me a while, but finally I realized that if we can count on PeekOffset to return the correct frame in mResultFrame, we can use that to easily infer the correct hint. It turned out that usually this was true, except for one case where PeekOffset didn't return immediately after calling itself recursively, but continued to override the correct frame. I couldn't think of any reason why that was correct, so I changed it.
The hint is now determined solely by nsSelection::MoveCaret().

I'd really like to altogether remove mPreferLeft from nsPeekOffsetStruct, but there seem to be other unrelated methods using it, so I'm leaving it there for now.
Attachment #216232 - Flags: superreview?(roc)
Attachment #216232 - Flags: review?(smontagu)
(Assignee)

Comment 3

12 years ago
The patch also fixes bug 328656, as expected.
Status: NEW → ASSIGNED
Comment on attachment 216232 [details] [diff] [review]
patch

Sweet! As you will have seen in the comments you removed, I wanted to do exactly this in the original Bidi check-in, but I didn't have the guts.
Attachment #216232 - Flags: review?(smontagu) → review+
(Assignee)

Comment 5

12 years ago
Created attachment 216306 [details] [diff] [review]
patch v.1.1

After some more testing, I found out that in the eSelectLine (up/down) case, mResultFrame might be an inline containing the target frame, not the actual text frame. So I need to use nsBidiPresUtils::GetFrameEmbeddingLevel instead of just NS_GET_EMBEDDING_LEVEL.

Also, removed preferLeft completely from nsFrame::GetFrameFromDirection().
Attachment #216232 - Attachment is obsolete: true
Attachment #216306 - Flags: superreview?(roc)
Attachment #216306 - Flags: review?(smontagu)
Attachment #216232 - Flags: superreview?(roc)
(Assignee)

Comment 6

12 years ago
(In reply to comment #5)
 
> Also, removed preferLeft completely from nsFrame::GetFrameFromDirection().

Or at least I meant to. I'll do this before checkin, if this patch gets r/sr. 

(Assignee)

Comment 7

12 years ago
Comment on attachment 216306 [details] [diff] [review]
patch v.1.1

No, sorry. This was not a perfect solution to the up/down-into-inline problem, because the hint calculation is now wrong. I guess I'll have to revert to the previous way of doing things for everything but left/right, where I know I can rely on mResultFrame.
Attachment #216306 - Attachment is obsolete: true
Attachment #216306 - Flags: superreview?(roc)
Attachment #216306 - Flags: review?(smontagu)
(Assignee)

Comment 8

12 years ago
Created attachment 216311 [details] [diff] [review]
patch v2

Like I said above:
- For left/right, use frame and offset to determine hint
- For up/down and home/end, use offset and hint to determine frame (as before).
Attachment #216311 - Flags: superreview?(roc)
Attachment #216311 - Flags: review?(smontagu)
(Assignee)

Updated

12 years ago
Blocks: 299240
Attachment #216311 - Flags: superreview?(roc) → superreview+

Updated

12 years ago
Attachment #216311 - Flags: review?(smontagu) → review+
(Assignee)

Comment 9

12 years ago
Checked in:
Checking in layout/generic/nsSelection.cpp;
/cvsroot/mozilla/layout/generic/nsSelection.cpp,v  <--  nsSelection.cpp
new revision: 3.222; previous revision: 3.221
done
Checking in layout/generic/nsTextFrame.cpp;
/cvsroot/mozilla/layout/generic/nsTextFrame.cpp,v  <--  nsTextFrame.cpp
new revision: 1.560; previous revision: 1.559
done
Checking in layout/generic/nsFrame.cpp;
/cvsroot/mozilla/layout/generic/nsFrame.cpp,v  <--  nsFrame.cpp
new revision: 3.635; previous revision: 3.634
done
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Updated

10 years ago
Component: Layout: BiDi Hebrew & Arabic → Layout: Text
QA Contact: zach → layout.fonts-and-text
You need to log in before you can comment on or make changes to this bug.