Closed Bug 1097894 Opened 5 years ago Closed 5 years ago

[RTL] Caret is mispositioned in empty RTL contenteditable block, when the parent isn't also a block

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: dholbert, Assigned: smontagu)

References

Details

Attachments

(2 files)

Spinning this off from bug 1080832, for the underlying platform issue.

STR:
 1. Load attached testcase.
 2. Click in each of the dotted-border regions.

EXPECTED RESULTS:
Caret should appear at upper-right corner in each case.

ACTUAL RESULTS:
Caret appears at left edge, in the second and third cases. (These are the cases where we have a "contenteditable" block whose parent is not also a block.)
Summary: Caret is mispositioned in a contenteditable block, whose parent is not a block → [RTL] Caret is mispositioned in RTL contenteditable block, when the parent isn't also a block
So, here's what I've got so far.
 - As noted in bug 1080832 comment 10: in the working block-in-a-block case, we get the upper-right corner in "GetPointFromOffset()", due to "isRTL" being true, due to NS_GET_EMBEDDING_LEVEL() returning something nonzero. Code quote:
> 5887 nsFrame::GetPointFromOffset(int32_t inOffset, nsPoint* outPoint)
> 5888 {
[...]
> 5898       bool isRTL = (NS_GET_EMBEDDING_LEVEL(this) & 1) == 1;
> 5899       if ((!isRTL && inOffset > newOffset) ||
> 5900           (isRTL && inOffset <= newOffset)) {
> 5901         pt = contentRect.TopRight();
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp?rev=be1f8816683e#5898

- In the broken cases, isRTL is *false* when we get here, because NS_GET_EMBEDDING_LEVEL() returns 0. (because EmbeddingLevelProperty has not been set)


So, why that difference? (why is EmbeddingLevelProperty set in one case but not the other?)

In the working block-in-a-block case, it EmbeddingLevelProperty gets set here, when we call nsBidiPresUtils::ResolveParagraph() on the outer block:
> 781     if (frame == NS_BIDI_CONTROL_FRAME) {
[...]
> 784     }
> 785     else {
> 786       propTable->Set(frame, nsIFrame::EmbeddingLevelProperty(),
> 787                      NS_INT32_TO_PTR(embeddingLevel));
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsBidiPresUtils.cpp?rev=36f87acce2f9#786

(The local-var "frame" is the inner block here.)

ResolveParagraph() is *only* called on block frames (its argument is "nsBlockFrame* aBlockFrame"), so it's no surprise that we don't reach this code for the examples that have a non-block wrapper frame.

I don't know how this is supposed to work for blocks whose parent is not a block (or really what EmbeddingLevelProperty means exactly).  smontagu, I'm hoping you might be able to help here...
Flags: needinfo?(smontagu)
As soon as the contenteditable block has some content, even an invisible control character, the problem goes away, so I'm fairly sure this is just an over-optimization somewhere.
Flags: needinfo?(smontagu)
Summary: [RTL] Caret is mispositioned in RTL contenteditable block, when the parent isn't also a block → [RTL] Caret is mispositioned in empty RTL contenteditable block, when the parent isn't also a block
I think this is the right thing to do here. We can't just use the CSS direction, because if you get child frames with mixed-direction text we need to determine the direction from the resolved level of each text run set by the Unicode Bidi Algorithm, which is exactly what EmbeddingLevelProperty gives us. However when the property isn't set, we should fall back to the CSS direction rather than just using the zero value returned (which gives us LTR as you already analysed)
Assignee: nobody → smontagu
Attachment #8522436 - Flags: review?(dholbert)
Comment on attachment 8522436 [details] [diff] [review]
Patch: use CSS direction wben embedding level isn't set

>+      bool isRTL = hasEmbeddingLevel
>+        ? (embeddingLevel & 1) == 1
>+        : StyleVisibility()->mDirection == NS_STYLE_DIRECTION_RTL;

Seems like we'd benefit from a utility function (or macro) to abstract away our "embeddinglevel & 1 == 1" checks into something like "IsEmbeddingLevelRTL(level)". Maybe file a followup on that, if you agree? (or a helper-bug if you want to address that first & benefit from it here)  It looks like nsBidiPresUtils.cpp has some "& 1" checks  that could be made clearer with that sort of accessor.

r=me on this, in any case.
Attachment #8522436 - Flags: review?(dholbert) → review+
https://hg.mozilla.org/mozilla-central/rev/d90b4907a287
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
QA Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.