Closed
Bug 1097894
Opened 10 years ago
Closed 10 years ago
[RTL] Caret is mispositioned in empty RTL contenteditable block, when the parent isn't also a block
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: dholbert, Assigned: smontagu)
References
Details
Attachments
(2 files)
812 bytes,
text/html
|
Details | |
1.66 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
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.)
Reporter | ||
Updated•10 years ago
|
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
Reporter | ||
Comment 1•10 years ago
|
||
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)
Assignee | ||
Comment 2•10 years ago
|
||
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
Assignee | ||
Comment 3•10 years ago
|
||
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)
Assignee | ||
Comment 4•10 years ago
|
||
Try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5b327906283d
Reporter | ||
Comment 5•10 years ago
|
||
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+
Assignee | ||
Comment 6•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d90b4907a287
https://hg.mozilla.org/mozilla-central/rev/d90b4907a287
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Updated•9 years ago
|
QA Whiteboard: [good first verify]
You need to log in
before you can comment on or make changes to this bug.
Description
•