Closed Bug 1263357 Opened 9 years ago Closed 8 years ago

Caret position wrong after pressing <shift><enter> in paragraph when <tt> or a any other font is used.

Categories

(Core :: DOM: Selection, defect)

Product:
Core
Component:
DOM: Selection
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox48 --- affected
firefox52 --- fixed

People

(Reporter: jorgk-bmo, Assigned: jorgk-bmo)

References

(Blocks 2 open bugs)

Details

Attachments

(3 files, 19 obsolete files)

Test page showing the bug. Works if no "tt" is used. (updated).
(deleted), text/html
Details
Test page automatic collapsing.
(deleted), text/html
Details
Proposed solution - Mark II (v2b).
(deleted), patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
Attached file Test page showing the bug (obsolete) (deleted) —
Caret position wrong after pressing <shift><enter> in paragraph at end of line when <tt> is used. See attached test case. It actually works correctly if the <tt></tt> is removed. (I hate to say it: Works in Chrome.)
Blocks: 1248971
OS: Unspecified → All
Hardware: Unspecified → All
Attached file Test page showing the bug (corrected) (obsolete) (deleted) —
Attachment #8739651 - Attachment is obsolete: true
Not just with <tt> in the composition. Happens with any fixed eidth font selected str choose Arial on the font selection or pre-set it in prefs first shift/enter does nothing second shift/enter produces an extra <br>
Summary: Caret position wrong after pressing <shift><enter> in paragraph when <tt> is used. → Caret position wrong after pressing <shift><enter> in paragraph when <tt>, or a fixed width font is used.
Not sure why you're saying "fixed" width font and then give "Arial" as example. Yes, I can reproduce it with Courier and Arial. I don't think it's an editor bug, since the <br> is correctly inserted into the DOM after hitting <shift><enter> (I checked with the DOM Inspector). I think it's a layout bug that the caret is just shown in the wrong spot. (In reply to Joe Sabash [:JoeS1] from comment #2) > first shift/enter does nothing > second shift/enter produces an extra <br> Not quite. First <shift><enter> does insert a <br> but doesn't show it. Second <shift><enter> inserts another one and shows the first one as well.
Summary: Caret position wrong after pressing <shift><enter> in paragraph when <tt>, or a fixed width font is used. → Caret position wrong after pressing <shift><enter> in paragraph when <tt> or a any other font is used.
David Baron explains in the similar bug 1258085 comment #19 that this type of bug is not a layout bug since adding a <br> at the end of a paragraph has no effect on the layout, where it does have an effect when you're editing.
This is the ugly sister of bug 1258085, so I hope I can get you interested.
Flags: needinfo?(ayg)
This does not look related at all to bug 1258085. Minimal test-case: data:text/html,<!doctype html> <div contenteditable><span>a</span><br></div> Click after "a", hit Shift-Enter. No visible change, but a <br> is inserted at the end of the <span>. If you type a character, it goes onto a new line. This one shows it a bit more clearly: data:text/html,<!doctype html> <div contenteditable><span>a</span><br>b</div> This one will tell you where the selection is: data:text/html,<!doctype html> <div contenteditable><span>a</span><br></div> <script> oninput = function() { document.body.appendChild(document.createTextNode(getSelection().focusNode + " " + getSelection().focusOffset)) } </script> Outputs "[object HTMLSpanElement] 2". So it's after the <br>, yet the caret displays before the <br>. Test-case without user interaction: data:text/html,<!doctype html> <div contenteditable><span>a<br></span><br></div> <script> getSelection().collapse(document.querySelector("span"), 2); document.body.firstChild.focus(); </script> Chrome and IE show the caret in the expected position. So this looks like an actual layout bug. I *could* look at it, but I'd need pointers as to exactly what to look at, since I've never really looked at any layout code in my life.
Component: Editor → Layout
Flags: needinfo?(ayg)
(In reply to :Aryeh Gregor (working until May 8) from comment #6) > This does not look related at all to bug 1258085. Minimal test-case: I didn't say it was related, I said it's the "ugly sister", as in: another caret problem that goes away when you continue typing. David, can you please give us your assessment of this bug, too.
Flags: needinfo?(dbaron)
Attached file some layout testcases (obsolete) (deleted) —
Attachment 8742136 [details] shows the layout that the editor is creating, with the elements more visible. Web compatibility appears to require that even though the span contains a <br>, that no part of the span appears on the next line. In fact, the <br> alone doesn't cause a next line to exist, unless there's another br. (Chrome fails to display the right border of the span anywhere in the first testcase, though, but Gecko and Edge match.) So the editor is asking for the caret to appear on the second line of the span, except Web-compatible layout rules require that no part of the span is actually present on the second line. I think this is a similar problem to the one that led to <br>-insertion existing. In Chromium, the caret placement code seems to manage placing the caret on the second line, though, so it seems that with this problem it's possible that the caret positioning code might be able to work around the issue somehow. But it seems like this either needs to be fixed in the Editor or be fixed by some sort of hack in the caret positioning code.
Component: Layout → Selection
Flags: needinfo?(dbaron)
(In reply to David Baron [:dbaron] ⌚️UTC-7 from comment #9) > So the editor is asking for the caret to appear on the second line of the > span, except Web-compatible layout rules require that no part of the span is > actually present on the second line. Why is this a problem? Is there a rule that the caret needs to be inside the visual boundaries of the element where the selection is? (I can't immediately think of another case where that wouldn't be true.) > I think this is a similar problem to the one that led to <br>-insertion > existing. <br>-insertion is because a block element will collapse to zero height under some circumstances unless it has some content, and we choose to insert a <br> because that's the most convenient. Also, because a <br> at the end of a block element is normally collapsed, so if the content of a block node after <br> is deleted, we need to add a second <br> so that the previously visible <br> doesn't become invisible (e.g., <p>a<br>b</p> -> <p>a<br><br></p> instead of <p>a<br></p>). (&zwsp; would work too for these cases, and IIRC at least some versions of IE would insert some sort of disappearaing &zwsp; instead of a <br>.) What's the similarity to this case? > But it seems like this either needs to be fixed in the Editor or be fixed by > some sort of hack in the caret positioning code. What's the problem with the editor? I.e., what DOM would you want it to produce in this case? The only thing I can think of offhand is to split the inline element at the <br>, like <span>a</span><br><span>|</span><br> where | is the caret position. But this seems much less nice than remaining in the inline element, and may have undesired consequences -- like if the inline element has an id, or some CSS property that doesn't work nicely with splitting (like border).
Flags: needinfo?(dbaron)
(In reply to :Aryeh Gregor (working until May 8) from comment #10) > (In reply to David Baron [:dbaron] ⌚️UTC-7 from comment #9) > > So the editor is asking for the caret to appear on the second line of the > > span, except Web-compatible layout rules require that no part of the span is > > actually present on the second line. > > Why is this a problem? Is there a rule that the caret needs to be inside > the visual boundaries of the element where the selection is? (I can't > immediately think of another case where that wouldn't be true.) Not necessarily, and changing that is likely the best way to fix this. > > I think this is a similar problem to the one that led to <br>-insertion > > existing. > > <br>-insertion is because a block element will collapse to zero height under > some circumstances unless it has some content, and we choose to insert a > <br> because that's the most convenient. Also, because a <br> at the end of > a block element is normally collapsed, so if the content of a block node > after <br> is deleted, we need to add a second <br> so that the previously > visible <br> doesn't become invisible (e.g., <p>a<br>b</p> -> > <p>a<br><br></p> instead of <p>a<br></p>). (&zwsp; would work too for these > cases, and IIRC at least some versions of IE would insert some sort of > disappearaing &zwsp; instead of a <br>.) > > What's the similarity to this case? OK, maybe not a great analogy, but I think they're both cases where the layout results are designed in a way that's particularly bad for editing. (Similar to the design of <br> as a line terminator rather than a line separator.) > > But it seems like this either needs to be fixed in the Editor or be fixed by > > some sort of hack in the caret positioning code. > > What's the problem with the editor? I.e., what DOM would you want it to > produce in this case? The only thing I can think of offhand is to split the > inline element at the <br>, like > > <span>a</span><br><span>|</span><br> > > where | is the caret position. But this seems much less nice than remaining > in the inline element, and may have undesired consequences -- like if the > inline element has an id, or some CSS property that doesn't work nicely with > splitting (like border). No, I think the better fix would be to fix the caret positioning code.
Flags: needinfo?(dbaron)
Is this plausible for me to look at, with no experience in layout? If so, could you give me pointers on how to go about fixing?
Flags: needinfo?(dbaron)
Aryeh, we have two bugs open on paragraph editing: Bug 1263357 (this one here) and bug 1265800. Can we share the work?
(In reply to :Aryeh Gregor (working until May 8) from comment #13) > Is this plausible for me to look at, with no experience in layout? If so, > could you give me pointers on how to go about fixing? I think it's plausible, but I'm not particularly knowledgeable about the relevant code, so I'm not sure how useful my pointers would be. That said, a bunch of the code to start looking is in the selection code, which lives in layout/generic/*Selection*. (I think the classes are horribly named, and I had a proposal for renaming them in bug 298715 which I suppose was partially done in bug 762862.) There's also some relevant code in nsTextFrame and nsInlineFrame, such as their implementations of the nsIFrame methods GetPointFromOffset and GetChildFrameContainingOffset, which I believe are used by the relevant selection code. Beyond that, I'd just have to read code to figure things out, but I'm happy to answer more specific questions. It's probably useful to read https://wiki.mozilla.org/Gecko:Continuation_Model if you're not aware of those concepts.
Flags: needinfo?(dbaron)
Do you know of anyone else who would be able to give me more specific pointers as to what to look at, and how big a change this would be? I'm quite lost with all this layout stuff, and layout/generic/*Selection* is 7710 lines, which is bit much to start reading through all at once.
Flags: needinfo?(dbaron)
I think perhaps the best place to start is probably: nsCaret::GetGeometryForFrame although it might also be worth looking at things like: nsCaret::GetCaretFrameForNodeOffset nsFrameSelection::MoveCaret although I'm not entirely sure; I don't know the code that well.
Flags: needinfo?(dbaron)
I think I'm probably not the one to try tackling this if no one can give me very specific pointers on where to look. I've never looked at anything layout-related before, so I'm completely lost trying to read through even fairly simple functions.
Attached file Test page showing the bug. Works if no "tt" is used. (obsolete) (deleted) —
It if were purely a layout problem, why does the <tt> style matter in the behaviour?
One more observation: If the produced HTML after hitting <shift><enter> is <p><tt>nmnmnmnm<br> </tt><br> </p> the caret is in the wrong position. If no paragraph is present, the produced HTML is this, <tt>klklklk<br> <br> </tt> and the caret is shown correctly. So why do the paragraph make a difference?
Updated test cases. Now the paragraph blocks are coloured to see their dimensions. When <shift><enter> is hit, the blocks grow onto the next line, but in the case where <tt> is used, the caret stays in the wrong location. So as discussed before, no layout bug but a caret mis-positioning bug. Note that last test case is peculiar. Without a paragraph, <shift><enter> should be the same as <enter>, but it's not.
Attachment #8740372 - Attachment is obsolete: true
Attachment #8783485 - Attachment is obsolete: true
Attached patch WIP: Debug patch (v1). (obsolete) (deleted) — — Splinter Review
David, in comment #15 you offered to answer more specific question, so here goes. In my debug patch I'm dumping out the DOM node/offset and the frame/line returned from nsFrameSelection::GetFrameForNodeOffset() in nsCaret::GetCaretFrameForNodeOffset(). I'm comparing two cases, the bad case which uses a <tt> surrounding the text, which produces an inline frame. And the good case. The bad case doesn't work, the good case works. Here's the debug output: Bad case: === nsCaret::GetCaretFrameForNodeOffset: 2 (Note: After the br) tt@04E65D30 state=[40000040000] flags=[00100200] primaryframe=05233780 refcount=9< Text@04E65D80 flags=[08000208] ranges:1 primaryframe=05233870 refcount=15<xyz> br@04E456F0 state=[40000040000] flags=[00100200] primaryframe=05292348 refcount=3<> > === line has InlineFrame (Note: contains the text) === line has BRFrame === returning TextFrame 3 Good case: === nsCaret::GetCaretFrameForNodeOffset: 2 (after the first br) p@0ED3D6A0 style="background:red;" state=[40000040000] flags=[00101208] ranges:1 primaryframe=0D475A60 refcount=18< Text@0D4351F0 flags=[0c000208] ranges:1 primaryframe=0D475B00 refcount=8<xyz> br@04E45F10 state=[40000040000] flags=[00100200] primaryframe=0D4A7348 refcount=3<> br@0D435560 state=[40000040000] flags=[00100200] primaryframe=0D475B90 refcount=3<> > === line has BRFrame === returning BRFrame 2 I looks like in the good case we end up in a line that has no text, so that's the line following the text. The caret gets placed correctly. In the bad case we end up in the line with the inline/tt frame and AdjustCaretFrameForLineEnd() adds insult to injury and moves the caret into the text frame. Given this output, would you agree that the fault is in nsFrameSelection::GetFrameForNodeOffset() ? At a first glance it looks at children of the DOM node passed in, but in fact since the caret is requested to be placed *after* the last child, the <br> in the <tt>, it should ascend to the DOM node's parent. What do you think?
Flags: needinfo?(dbaron)
Mats worked in nsFrameSelection::GetFrameForNodeOffset() recently, so perhaps he can answer my questions.
Flags: needinfo?(mats)
Or maybe it's better to leave nsFrameSelection::GetFrameForNodeOffset() alone and detect the case in nsCaret::GetCaretFrameForNodeOffset() after the call to nsFrameSelection::GetFrameForNodeOffset().
Attached patch WIP: Crude, but works in this particular case. (obsolete) (deleted) — — Splinter Review
How about something like this?
Attachment #8789594 - Attachment is obsolete: true
Attachment #8790249 - Flags: feedback?(mats)
Attached patch WIP: Is this the right approach? (obsolete) (deleted) — — Splinter Review
Ehsan, long time, no see ;-) There has been no traction on this bug for a while, David wasn't interested since it's not layout, Aryeh wasn't interested since it's not editor. This solution is based on your work in IsInvisibleBreak() in nsDocumentEncoder.cpp. Can you please let me know what you think. I haven't tested it greatly, but it seems to solve the problem. If you approve of the approach, I will of cause run some try runs and add some tests.
Attachment #8742136 - Attachment is obsolete: true
Attachment #8790249 - Attachment is obsolete: true
Attachment #8790249 - Flags: feedback?(mats)
Flags: needinfo?(mats)
Flags: needinfo?(dbaron)
Attachment #8790350 - Flags: feedback?(ehsan)
Comment on attachment 8790350 [details] [diff] [review] WIP: Is this the right approach? Review of attachment 8790350 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsCaret.cpp @@ +650,5 @@ > nsIFrame* theFrame = nullptr; > int32_t theFrameOffset = 0; > > + printf("=== nsCaret::GetCaretFrameForNodeOffset: %d\n", aOffset); > + aContentNode->List(); Please remove all debugging code from patches before asking for feedback in the future. These only confuse the reviewer. @@ +660,5 @@ > + NS_ConvertUTF16toUTF8(nsAtomString(theFrame->GetType())).get()); > + if (aOffset > 0 && > + aContentNode->Length() == aOffset && > + theFrame->GetType() == nsGkAtoms::brFrame && > + aContentNode->GetLastChild()->IsHTMLElement(nsGkAtoms::br) You're sort of special casing the condition in the test case here, so this isn't the right approach because it won't work for anything slightly different. We actually have code that's supposed to handle cases such as this: AdjustCaretFrameForLineEnd(). Can you please try to figure out what happens in that function and how we end up leaving the function? I'm specifically interested to know if we ever hit these two places: <http://searchfox.org/mozilla-central/rev/591354c5e5521cf845bf6b6ef44e8b3b0aeda17d/layout/base/nsCaret.cpp#61> <http://searchfox.org/mozilla-central/rev/591354c5e5521cf845bf6b6ef44e8b3b0aeda17d/layout/base/nsCaret.cpp#106> It may be that we'd need to handle this case there somehow, but I'm still unclear as to what a good solution would be like before knowing what we're currently doing. Thanks!
Attachment #8790350 - Flags: feedback?(ehsan)
Thanks for the comments. I left the debug in place in case anyone wanted to run it. As I said in comment #22, nsFrameSelection::GetFrameForNodeOffset() appears to return a frame that belongs to the node passed in. The node passed in is: === nsCaret::GetCaretFrameForNodeOffset: 2 (Note: After the br) tt@04E65D30 state=[40000040000] flags=[00100200] primaryframe=05233780 refcount=9< Text@04E65D80 flags=[08000208] ranges:1 primaryframe=05233870 refcount=15<xyz> br@04E456F0 state=[40000040000] flags=[00100200] primaryframe=05292348 refcount=3<> > We need to place the caret *after* any frame belonging to the node. Please look at the debug in comment #22 carefully (produced with attachment 8789594 [details] [diff] [review] without my crude fix). I'll repeat it here: === nsCaret::GetCaretFrameForNodeOffset: 2 (Note: After the br) tt@04E65D30 state=[40000040000] flags=[00100200] primaryframe=05233780 refcount=9< Text@04E65D80 flags=[08000208] ranges:1 primaryframe=05233870 refcount=15<xyz> br@04E456F0 state=[40000040000] flags=[00100200] primaryframe=05292348 refcount=3<> > === line has InlineFrame (Note: contains the text) === line has BRFrame === returning TextFrame 3 So the frame returned from nsFrameSelection::GetFrameForNodeOffset() is in the line containing the text. As I said in comment #22, (quote) "AdjustCaretFrameForLineEnd() adds insult to injury and moves the caret into the text frame.". To answer your questions: I'm specifically interested to know if we ever hit these two places: <http://searchfox.org/mozilla-central/rev/591354c5e5521cf845bf6b6ef44e8b3b0aeda17d/layout/base/nsCaret.cpp#61> <http://searchfox.org/mozilla-central/rev/591354c5e5521cf845bf6b6ef44e8b3b0aeda17d/layout/base/nsCaret.cpp#106> Line 106: Do you really need to know? We're starting off with the wrong line. Line 61: IMHO also not relevant since we're starting in the wrong line. Nothing in AdjustCaretFrameForLineEnd() moves to the next line, it only adjusts the frame within the given line. I also noted in comment #22 that nsFrameSelection::GetFrameForNodeOffset() only looks at children of the node that's passed in, but not anything that follows which seems to be necessary in this case. Awaiting further instructions ;-)
Flags: needinfo?(ehsan)
Just for completeness: Without the patch, line 61 gets hit before the <br> is inserted. Line 61 and 106 get hit after the <br> is inserted: we're on the wrong line and "xyz" is still at the end of the wrong line, and that gets returned at line 106. With my tweak in place, line 61 is also hit after adding the <br>, most likely since aFrame == aStopAtFrame, but line 106 isn't hit any more since now we're on the correct second line which doesn't have a text.
I've looked at it again: Without the patch, line 61 gets hit on aFrame == aStopAtFrame before adding the <br>. After adding the <br>, it gets hit on the text condition in the "or", and line 106 also gets hit. Surely, you'll be bored to hear again that we're in the wrong line to start with ;-) With the patch, line 61 gets hit on aFrame == aStopAtFrame before and after adding the <br>, since now we're in the right line which has no text. As you observed, my patch is "sort of special casing the condition in the test case here" since this edge case isn't catered for anywhere else: <br> at the end of the node, caret needs to go into a frame behind it and not belonging to the node. I'm not sure what you mean by "it won't work for anything slightly different". Instead of wrapping the "xyz" into a <tt>, I wrapped it into a <tt> and <font color=green> and that "slightly different" case also works. But I'm open to a better idea.
(In reply to Jorg K (GMT+2, never had any PTO during summer) from comment #28) > Thanks for the comments. I left the debug in place in case anyone wanted to > run it. Your debug messages won't help other people trying to dump out information that will help _them_. It will confuse them and make the patch harder to read. (Also FWIW it gets tiring repeating the same request over and over again. I've asked you to not include debugging cruft in patches many times before. It would help if you listen for the next time. :-) > As I said in comment #22, nsFrameSelection::GetFrameForNodeOffset() appears > to return a frame that belongs to the node passed in. The node passed in is: Whatever GetFrameForNodeOffset() is clearly buggy otherwise the caret would be placed in the correct spot, so comment 22 doesn't help much besides showing that something went wrong. The question is what, and why. > So the frame returned from nsFrameSelection::GetFrameForNodeOffset() is in > the line containing the text. As I said in comment #22, (quote) > "AdjustCaretFrameForLineEnd() adds insult to injury and moves the caret into > the text frame.". That's that function's job if you read it more closely. That function isn't misbehaving, and it is the reason why the caret doesn't end up on a (non-existing) second line in |<span>text<br></span>| for example. > To answer your questions: > I'm specifically interested to know if we ever hit these two places: > <http://searchfox.org/mozilla-central/rev/ > 591354c5e5521cf845bf6b6ef44e8b3b0aeda17d/layout/base/nsCaret.cpp#61> > <http://searchfox.org/mozilla-central/rev/ > 591354c5e5521cf845bf6b6ef44e8b3b0aeda17d/layout/base/nsCaret.cpp#106> > Line 106: Do you really need to know? We're starting off with the wrong line. Aha! That's the bug I bet. Just to confirm that I understand things correctly, the wrong line in question is what FindContainingLine() returns, right? If yes, what frame is the one that we're passing to FindContainingLine()? I expect that it's the text frame or the first BRFrame. If that is the case, then my guess is that the bug stems from GetFrameForNodeOffset() returning the incorrect frame. At any rate, the next step is to find out how we're ending up looking at the wrong line. > Line 61: IMHO also not relevant since we're starting in the wrong line. Yes, the bug has happened before we get to this place, I think. > Nothing in AdjustCaretFrameForLineEnd() moves to the next line, it only > adjusts the frame within the given line. > > I also noted in comment #22 that nsFrameSelection::GetFrameForNodeOffset() > only looks at children of the node that's passed in, but not anything that > follows which seems to be necessary in this case. Looking at GetFrameForNodeOffset(), assuming that we're being called with a CARET_ASSOCIATE_AFTER hint, I would expect that we end up in <http://searchfox.org/mozilla-central/rev/f6c298b36db67a7109079c0dd7755f329c1d58e2/layout/generic/nsSelection.cpp#2028>. (My assumption is that when we get there, aOffset is 2, and numChildren is 2 as well, which is where we set childIndex to 1 and then proceed to set theNode to the first BR node.) But this is wrong, since as you noted, our offset and caret association hint tell us that the caret attaches to the node after the (node, offset) pair. This difference is invisible in a test case like |<span>text<img></span>more| since both answers put the caret between the image and "more", but it is visible both in a test case like |<span>text<br></span><br>more| and also |<span>text<br></span>more| (without a second <br>). Doing some quick testing with that last test case seems to suggest that this is indeed the problem, but I would appreciate if you can double check this under a debugger. Now, assuming that I'm not going into the weeds in the above, we need to think about what the right thing to do there is. Restricting the search only to the descendants of the original node passed in is clearly wrong. I think the right thing to do if |aHint == CARET_ASSOCIATE_AFTER && aOffset >= numChildren| is to see if the element has a next sibling. If it does, we should probably continue the search by setting aNode to the next sibling node, aOffset to |aOffset - numChildren| and aHint to CARET_ASSOCIATE_BEFORE (since we're leaving the current node's subtree at that point -- and we should attach the caret to the frame *before* two adjacent ones now.) To express what I mean in plain English, if we get to a point where our found node is at the very end of the subtree we're searching in, and our caret association hint is set to after, we should check to see if a next sibling contains the desired position. The reason we're changing aOffset is to skip over all of the children of the original node. If I have gotten all of the above right, making this change should make GetFrameForNodeOffset() return the second br and an offset of 0 in your case, and the caret should be placed in the correct visual spot. If this idea ends up working out, please push the patch to the try server before asking for review, since this code is very regression prone and often we miss one thing when trying to fix another one. For the tests needed for this patch, you can add a couple of files to layout/base/tests/test_reftests_with_caret.html, and for example simulate clicking at the end of a line. Please make sure to test cases such as having a br after the span, a text node with some text, and a text node with collapsible whitespace (" " would do.) (Also, in response to your question about why special casing algorithms like your patch did isn't a good idea, your patch wouldn't address this test case |<span>text<br></span>more|. This is an example similar case, but generally it's hard to predict what other similar cases are caused by the same root issue without understanding where things go wrong. There _are_ cases where special casing things make sense, but you should usually have a good reason why the special case in question cannot be handled in a more generic way.) Good luck!
Flags: needinfo?(ehsan)
Thanks for the comment. (In reply to :Ehsan Akhgari (intermittently available until 9/21) from comment #31) > Restricting the search > only to the descendants of the original node passed in is clearly wrong. I > think the right thing to do if |aHint == CARET_ASSOCIATE_AFTER && aOffset >= > numChildren| is to see if the element has a next sibling. If it does, we > should probably continue the search by setting aNode to the next sibling > node, aOffset to |aOffset - numChildren| and aHint to CARET_ASSOCIATE_BEFORE > (since we're leaving the current node's subtree at that point -- and we > should attach the caret to the frame *before* two adjacent ones now.) I agree. As I asked in comment #22 (quoting myself): === Given this output, would you agree that the fault is in nsFrameSelection::GetFrameForNodeOffset() ? At a first glance it looks at children of the DOM node passed in, but in fact since the caret is requested to be placed *after* the last child, the <br> in the <tt>, it should ascend to the DOM node's parent. === OK, I meant to say, ascend to the parent to find the next sibling, but you can find it without traversing to the parent first. So since we all agree that we should change nsFrameSelection::GetFrameForNodeOffset() to make it consider siblings, I'll work on that in the next few days/weeks hoping to fix the bug for mozilla52. Thanks again for the detailed instructions.
Oops, I constructed myself this example ... <div contenteditable=""> <p style="background:red;"><font color=green><tt>xyz</tt></font><br></p> </div> ... and there the <tt> doesn't have a sibling. You'd have to use the sibling of the <font>.
Attached patch Solution based on Ehsan's suggestion (v1a). (obsolete) (deleted) — — Splinter Review
This is implementing what Ehsan suggested in comment #31 with the added twist to also look at parents' siblings. Let's see what this breaks. Limited try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4fd19087837b
Assignee: nobody → jorgk
Attachment #8790350 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Comment on attachment 8794338 [details] [diff] [review] Solution based on Ehsan's suggestion (v1a). Review of attachment 8794338 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsSelection.cpp @@ +2025,5 @@ > + } > + if (sibling) { > + aNode = sibling; > + aHint = CARET_ASSOCIATE_BEFORE; > + aOffset = 0; I doubt that this will work. You want to do this work inside the loop below, and |continue;| here, so that this happens every time we come across an after hint with an offset greater than or equal to the number of children at hand. Also, you can distill everything between line 2017 to 2030 inside the chain of if conditions below in the beginning of the loop, but that's more of a nit to integrate with the current code better.
(In reply to :Ehsan Akhgari from comment #35) > I doubt that this will work. Right. It worked with the few manual examples I tried, but the try run looks terrible :-(
Attached patch Solution based on Ehsan's suggestion (v1b). (obsolete) (deleted) — — Splinter Review
Incorporating Ehsan's comments by moving the new code inside the loop. Attached test case still works but I suspect that this will cause equally many test failures. Let's see: https://treeherder.mozilla.org/#/jobs?repo=try&revision=982a84148632
Attachment #8794338 - Attachment is obsolete: true
Looking briefly over the test failures (run for second patch not finished at time of writing), we can see that things go wrong, for example: M(4): layout/base/tests/test_reftests_with_caret.htm M(5): layout/generic/test/test_bug496275.html M(JP): test-selection.js.test (a11y): test_caretmove.xul I think what we have so far is overkill. If the request comes to position after the last child of a node, there is no need to step to the front of its sibling (or worse, a parent's sibling). This is only necessary if there is a *visible break* between that last child and its sibling. I think we should at least check the last child for ->IsHTMLElement(nsGkAtoms::br) before we try to get a sibling as my "special case" patch did. What do you think? BTW: At the bottom of comment #31 you mention |<span>text<br></span>more|. Where is there a problem? Where do you insert the <br>. My test case was: <p style="background:red;"><tt>xyz</tt><br></p> with a <br> inserted here --------^ Sure, instead of <tt> you can have <span> or even double up with <font><tt> like so: <p style="background:red;"><font color=green><tt>xyz</tt></font><br></p> with a <br> inserted here---------------------------^ but the special case would have caught that. What am I missing?
Second try finished with almost the same failures as the first. So here comes the try for the patch that checks for <br> before stepping onto the sibling. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b77299e9d26f
That crashed about every test ;-( Here we go again this time not dereferencing null. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5cc45f878aeb
Attachment #8794416 - Attachment is obsolete: true
Attachment #8794442 - Attachment is obsolete: true
Completely green ;-) https://treeherder.mozilla.org/#/jobs?repo=try&revision=5cc45f878aeb I'll add some tests as per comment #31 (quote): For the tests needed for this patch, you can add a couple of files to layout/base/tests/test_reftests_with_caret.html, and for example simulate clicking at the end of a line. Please make sure to test cases such as having a br after the span, a text node with some text, and a text node with collapsible whitespace (" " would do.)
Here is the patch as you described it. Green try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5cc45f878aeb Try run doesn't include the new test cases, but they pass locally. Do you require another try run with the new tests? Also, you were talking about more tests: === Please make sure to test cases such as having a br after the span, a text node with some text, and a text node with collapsible whitespace (" " would do.) === Please let me know whether my test cases are sufficient, I don't have anything with white-space. My two tests are: <p><tt>xyz</tt><br></p> <p><font color=red><span>xyz</span></font><br></p> In both cases the new <br> is added right after the text. The second one triggers the code where we position at the front of a parent's sibling. To speed up proceedings, kindly just write down in HTML which further tests you require and I'll add them.
Attachment #8794473 - Attachment is obsolete: true
Attachment #8794543 - Flags: review?(ehsan)
(In reply to Jorg K (GMT+2) from comment #38) > BTW: At the bottom of comment #31 you mention |<span>text<br></span>more|. > Where is there a problem? Where do you insert the <br>. I don't insert anything into the DOM. If you take that DOM, and set up a programmatic selection on the span with offset 2, before your patch the caret should show up visually after "text" on the first line. After your patch, it should visually show up before "more" on the second line. (In reply to Jorg K (GMT+2) from comment #42) > Please let me know whether my test cases are sufficient, I don't have > anything with white-space. My two tests are: > <p><tt>xyz</tt><br></p> > <p><font color=red><span>xyz</span></font><br></p> > In both cases the new <br> is added right after the text. > The second one triggers the code where we position at the front of a > parent's sibling. > > To speed up proceedings, kindly just write down in HTML which further tests > you require and I'll add them. I have some comments on your tests which I'm going to submit shortly, but for the test with the whitespace, I had something like this in mind: |<span>text<br></span> more|.
Comment on attachment 8794543 [details] [diff] [review] Solution based on Ehsan's suggestion but with checking for <br> (v2b) and test (v1a). Review of attachment 8794543 [details] [diff] [review]: ----------------------------------------------------------------- Can you please post the next version of the patch together with the tests and get a full try run across the board, using both debug and opt builds? I'm afraid your previous try push may have missed some tests that may be interesting for this change. Thanks! ::: layout/base/tests/bug1263357-1-ref.html @@ +16,5 @@ > + var sel = window.getSelection(); > + // Focus on editable block. > + theDiv = document.getElementById("editable"); > + theDiv.focus(); > + sel.collapse(theDiv, 1); Hmm, this is putting the selection after the <p> element, which isn't exactly testing the reference scenario. You want to collapse the selection to the second BR here. ::: layout/base/tests/bug1263357-2-ref.html @@ +16,5 @@ > + var sel = window.getSelection(); > + // Focus on editable block. > + theDiv = document.getElementById("editable"); > + theDiv.focus(); > + sel.collapse(theDiv, 1); Ditto. ::: layout/base/tests/bug1263357-2.html @@ +8,5 @@ > + <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script> > + <script type="application/javascript" src="/tests/SimpleTest/EventUtils.js"></script> > +</head> > +<body> > +<div id="editable" contenteditable='true' spellcheck=false style="outline: 1px solid;"><p><font color=red><span>xyz</span></font><br></p></div> There's nothing special about <tt> or <font> here. So both your tests are actually testing the same thing. Can you please use the generic |<span>text<br></span><br>more| test case for one of the tests, and |<span>text<br></span>more| for the other one? It would be nice if you set up the selection programmatically on the second test but keep doing the keyboard based manipulation in the first one. And also add a test with the collapsible whitespace as per comment 43. This way we get better coverage here. ::: layout/generic/nsSelection.cpp @@ +2027,5 @@ > if (numChildren > 0) { > + if (theNode->GetLastChild()->IsHTMLElement(nsGkAtoms::br)) { > + // If we should position behind the last child of the node passed in, > + // if that's a break, rather try before its next sibling or the next > + // sibling of a parent if it doesn't have a next sibling. I don't understand what's specific to the BR element here. Please keep the algorithm generic. If you added this to fix test failures, there's a chance that something else is wrong with your approach, and you're masking it by adding this check. With the fix I had in mind, we shouldn't end up having anything special about BR in this algorithm. (If you read comment 31 again, I said that this bug also exists when the last child is something else, such as an image, it's just that you mostly don't see the bug under those cases, but I'm sure one could create a test case with a visible difference in where we place the caret using an element other than BR...) @@ +2033,5 @@ > + nsCOMPtr<nsINode> parent = aNode; > + while (!sibling) { > + parent = parent->GetParentNode(); > + if (!parent) { > + break; What makes bailing out here OK? @@ +2040,5 @@ > + } > + if (sibling) { > + aNode = sibling; > + aHint = CARET_ASSOCIATE_BEFORE; > + aOffset = 0; You can't set the offset to 0. That is only correct if aOffset == numChildren. You should probably do: |aOffset -= numChildren;| instead to account for the "greater than" case as well. @@ +2041,5 @@ > + if (sibling) { > + aNode = sibling; > + aHint = CARET_ASSOCIATE_BEFORE; > + aOffset = 0; > + // Start over. That's what continue means. Please remove the comment explaining the obvious!
Attachment #8794543 - Flags: review?(ehsan) → review-
(In reply to :Ehsan Akhgari from comment #44) > There's nothing special about <tt> or <font> here. So both your tests are > actually testing the same thing. I beg to differ. As I said in comment #42: === <p><tt>xyz</tt><br></p> <p><font color=red><span>xyz</span></font><br></p> In both cases the new <br> is added right after the text. *** The second one triggers the code where we position at the front of a parent's sibling. *** === I agree that <tt>, <font> and <span> test the same thing, but one test had two of them nested. > Can you please use the generic > |<span>text<br></span><br>more| test case for one of the tests, and > |<span>text<br></span>more| for the other one? It would be nice if you set > up the selection programmatically on the second test but keep doing the > keyboard based manipulation in the first one. I have no idea where the caret shows when programmatically collapsing behind the <br>. I can try. As the bug says, the problem shows when editing and being called from the editor. > ::: layout/generic/nsSelection.cpp > @@ +2027,5 @@ > > if (numChildren > 0) { > > + if (theNode->GetLastChild()->IsHTMLElement(nsGkAtoms::br)) { > > + // If we should position behind the last child of the node passed in, > > + // if that's a break, rather try before its next sibling or the next > > + // sibling of a parent if it doesn't have a next sibling. > I don't understand what's specific to the BR element here. Please keep the > algorithm generic. > If you added this to fix test failures, there's a chance that something else > is wrong with your approach, and you're masking it by adding this check. > With the fix I had in mind, we shouldn't end up having anything special > about BR in this algorithm. Yes, I added this after seeing the test failures mentioned in comment #38. It's kind that you call the thing "your approach", as in Jörg's approach, when it is essentially your approach ;-) Since it is your approach, kindly explain this. We agree that in <span>text<br></span><br>more collapsing after the first <br>, the caret should show on the second line which only has one <br>. So in fact, although the <span> is passed in, we return a frame in the <span>'s sibling. We do this to be on the next line. Take: <span>textnode1|textnode2</span><span>text</span> Two text nodes. Again, ask to collapse behind the second child of the span. Well, one would collapse in textnode2, why wouldn't one? In your approach you'd collapse in the sibling, but it's only interesting to collapse in the sibling if there is a *visible break* (as I already said in comment #38). I know that testing for a <br> is not a 100% test for a visible break, there are other functions in the system that test that better. If you look at the test failures of a run without the <br> check: https://treeherder.mozilla.org/#/jobs?repo=try&revision=982a84148632 you can see that a bunch of tests fail because now in your approach the frame selection ends up in a different spot than before without there being any need for that change. > @@ +2033,5 @@ > > + nsCOMPtr<nsINode> parent = aNode; > > + while (!sibling) { > > + parent = parent->GetParentNode(); > > + if (!parent) { > > + break; > What makes bailing out here OK? Well, if there is no more parent, I'm at the top of the tree. If no sibling was found, there just isn't one. > > + aOffset = 0; > You can't set the offset to 0. That is only correct if aOffset == > numChildren. You should probably do: |aOffset -= numChildren;| instead to > account for the "greater than" case as well. Yes, you suggested that before. However, do you really think that makes sense? Say the function gets called with offset 3 where there are only two children. So what do you want to do? Offset 1 into the next sibling you find? IHMO there should be a MOZ_ASSERT on the "greater than" case. That should never happen. > > + // Start over. > That's what continue means. Please remove the comment explaining the > obvious! Actually, "continue" means "start again from the top of the loop". We all understand that. "Start over" as a different meaning. We just set a new node and offset, so we "start over" our search with different input parameters (http://dictionary.cambridge.org/dictionary/english/start-over: to begin to do something again, sometimes *in a different way*). Awaiting further instructions. I think the code is right, I could change the test for <br> to a test for a visible break, and I can do some variations on the tests.
Flags: needinfo?(ehsan)
Attached file Test page automatic collapsing. (deleted) —
This is the test you suggested with programmatic selection via collapse(). It turns out that your approach does not fix the problem. The code I added, as you suggested, only runs in the case where the hint is CARET_ASSOCIATE_AFTER, which is the case in the editing example (attachment 8789087 [details]). A little debugging shows that in the case of this example using collapse(), the hint passed is CARET_ASSOCIATE_BEFORE, so the new code doesn't run. Summarising the problems: 1) Please explain why the new code should always run and not only if there is a visible break, so we must show the caret on the next line (see comment #45). 2) What about your examples that aren't fixed due to the hint? Run the new code always irrespective of the hint?
(In reply to Jorg K (GMT+2) from comment #45) > (In reply to :Ehsan Akhgari from comment #44) > > There's nothing special about <tt> or <font> here. So both your tests are > > actually testing the same thing. > I beg to differ. As I said in comment #42: > === > <p><tt>xyz</tt><br></p> > <p><font color=red><span>xyz</span></font><br></p> > In both cases the new <br> is added right after the text. > *** The second one triggers the code where we position at the front of a > parent's sibling. *** > === > I agree that <tt>, <font> and <span> test the same thing, but one test had > two of them nested. I understand. My point was that the tt and font elements have no significance here whatsoever, so please do not use them. You can keep both of these tests as long as you add the ones I asked for. :-) > > Can you please use the generic > > |<span>text<br></span><br>more| test case for one of the tests, and > > |<span>text<br></span>more| for the other one? It would be nice if you set > > up the selection programmatically on the second test but keep doing the > > keyboard based manipulation in the first one. > I have no idea where the caret shows when programmatically collapsing behind > the <br>. I can try. As the bug says, the problem shows when editing and > being called from the editor. That's just one manifestation of the bug, but nothing here is related to the editor -- you can get this bug without the editor as well. Please try setting the selection programmatically as I described. (FWIW you can press F7 to make the caret visible in non-editable regions, that may help testing this.) > > ::: layout/generic/nsSelection.cpp > > @@ +2027,5 @@ > > > if (numChildren > 0) { > > > + if (theNode->GetLastChild()->IsHTMLElement(nsGkAtoms::br)) { > > > + // If we should position behind the last child of the node passed in, > > > + // if that's a break, rather try before its next sibling or the next > > > + // sibling of a parent if it doesn't have a next sibling. > > I don't understand what's specific to the BR element here. Please keep the > > algorithm generic. > > If you added this to fix test failures, there's a chance that something else > > is wrong with your approach, and you're masking it by adding this check. > > With the fix I had in mind, we shouldn't end up having anything special > > about BR in this algorithm. > Yes, I added this after seeing the test failures mentioned in comment #38. > It's kind that you call the thing "your approach", as in Jörg's approach, > when it is essentially your approach ;-) > > Since it is your approach, kindly explain this. We agree that in > <span>text<br></span><br>more collapsing after the first <br>, the caret > should show on the second line which only has one <br>. So in fact, although > the <span> is passed in, we return a frame in the <span>'s sibling. We do > this to be on the next line. > Take: > <span>textnode1|textnode2</span><span>text</span> > Two text nodes. Again, ask to collapse behind the second child of the span. > Well, one would collapse in textnode2, why wouldn't one? In your approach > you'd collapse in the sibling Yes. >, but it's only interesting to collapse in the > sibling if there is a *visible break* (as I already said in comment #38). Why? You're not explaining the reason for this. > I > know that testing for a <br> is not a 100% test for a visible break, there > are other functions in the system that test that better. I don't know how you define a visible break, but many things about the caret's presentation can be different in the sibling than the stuff inside the span. Think things such as font size, line height, inline-block boxes with relative positioning, and so on. > If you look at the test failures of a run without the <br> check: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=982a84148632 > you can see that a bunch of tests fail because now in your approach the > frame selection ends up in a different spot than before without there being > any need for that change. OK, but you need to provide a detailed analysis of what breaks in every case and why that is. I don't have time to debug this by looking at the try server results unfortunately. (Also, before retesting, please fix the other bugs in your patch that I have pointed out, since it's possible that some of those bugs are causing some of these test failures.) > > @@ +2033,5 @@ > > > + nsCOMPtr<nsINode> parent = aNode; > > > + while (!sibling) { > > > + parent = parent->GetParentNode(); > > > + if (!parent) { > > > + break; > > What makes bailing out here OK? > Well, if there is no more parent, I'm at the top of the tree. If no sibling > was found, there just isn't one. That doesn't mean that you can safely bypass everything else that happens inside the loop body after this point, such as descending into the children subtree, dealing with text nodes, etc. I think in this case you should basically fall back to the existing code path. > > > + aOffset = 0; > > You can't set the offset to 0. That is only correct if aOffset == > > numChildren. You should probably do: |aOffset -= numChildren;| instead to > > account for the "greater than" case as well. > Yes, you suggested that before. However, do you really think that makes > sense? Given the way this code is written right now, it certainly makes sense. > Say the function gets called with offset 3 where there are only two > children. So what do you want to do? Offset 1 into the next sibling you > find? IHMO there should be a MOZ_ASSERT on the "greater than" case. That > should never happen. Maybe... I'm not sure though if we have something somewhere in the tree that actually guarantees that such an assert would never fire. I'm specifically thinking of the case where the next sibling is a text node where we'd need to translate the offset into an offset into the text content. If you want to investigate whether this can actually happen in practice and add the assertion if it cannot, please file a follow-up bug. > > > + // Start over. > > That's what continue means. Please remove the comment explaining the > > obvious! > Actually, "continue" means "start again from the top of the loop". We all > understand that. "Start over" as a different meaning. We just set a new node > and offset, so we "start over" our search with different input parameters > (http://dictionary.cambridge.org/dictionary/english/start-over: to begin to > do something again, sometimes *in a different way*). > > Awaiting further instructions. I think the code is right, I could change the > test for <br> to a test for a visible break, and I can do some variations on > the tests. I would still like to see all of my comments addressed.
Flags: needinfo?(ehsan)
(In reply to Jorg K (GMT+2) from comment #46) > Created attachment 8795291 [details] > Test page automatic collapsing. > > This is the test you suggested with programmatic selection via collapse(). > > It turns out that your approach does not fix the problem. The code I added, > as you suggested, only runs in the case where the hint is > CARET_ASSOCIATE_AFTER, which is the case in the editing example (attachment > 8789087 [details]). A little debugging shows that in the case of this > example using collapse(), the hint passed is CARET_ASSOCIATE_BEFORE, so the > new code doesn't run. Hmm, interesting. Where does the before hint come from in this case?
(In reply to :Ehsan Akhgari from comment #47) > > Take: > > <span>textnode1|textnode2</span><span>text</span> > > Two text nodes. Again, ask to collapse behind the second child of the span. > > Well, one would collapse in textnode2, why wouldn't one? In your approach > > you'd collapse in the sibling > Yes. > >, but it's only interesting to collapse in the > > sibling if there is a *visible break* (as I already said in comment #38). > Why? You're not explaining the reason for this. Well, you're not explaining the reason for wanting to position into the sibling regardless. So please explain to me: <span>textnode1</span><span>textnode2</span> Collapse(firstSpan, 1). Why would you position into textnode2? That is changing current system behaviour and is most likely causing the test failures. As you said, ... > I don't know how you define a visible break, but many things about the > caret's presentation can be different in the sibling than the stuff inside > the span. Think things such as font size, line height, inline-block boxes > with relative positioning, and so on. So why change existing behaviour here? Besides, we're not talking about the DOM insertion point, only where the caret shows. If you position into the next sibling, then you will get cursor corresponding to this siblings text properties, the next character will still be typed into the first span with that span's properties. If there's a visible (line) *break*, like the opposite of IsInvisibleBreak() in nsDocumentEncoder.cpp, so a new *line*, then we have no choice but to position the caret into the new line, since when you keep typing, that's were the new text will go and not into the previous line where the caret currently shows. When you keep typing, the text will still go into the span/tt/font element and carry its properties. You can see that in attachment 8789087 [details], where the <tt> is continues. The DOM insertion is still inside the span, although the caret shows at a frame not belonging to the span. We seem to be on different tracks here. My solution exclusively wants to fix showing the caret on the wrong *line* because the next inserted text goes into the subsequent line after the <br> in the span/tt/font. You're approach is much further reaching and your suggestion is to always position into the next sibling. Before we haven't settled this misunderstanding, I cannot continue since you want to implement something we don't need and which breaks a bunch of tests. > > > @@ +2033,5 @@ > > > > + nsCOMPtr<nsINode> parent = aNode; > > > > + while (!sibling) { > > > > + parent = parent->GetParentNode(); > > > > + if (!parent) { > > > > + break; > > > What makes bailing out here OK? > > Well, if there is no more parent, I'm at the top of the tree. If no sibling > > was found, there just isn't one. > That doesn't mean that you can safely bypass everything else that happens > inside the loop body after this point, such as descending into the children > subtree, dealing with text nodes, etc. I think in this case you should > basically fall back to the existing code path. Is there a misunderstanding here? The code is: + while (!sibling) { + parent = parent->GetParentNode(); + if (!parent) { + break; <<== This break goes ... + } + sibling = parent->GetNextSibling(); + } + if (sibling) { <<===. here. And since no sibling is found we fall though to the normal processing as you requested. + aNode = sibling; + aHint = CARET_ASSOCIATE_BEFORE; + aOffset = 0; + // Start over. + continue; + } + } (In reply to :Ehsan Akhgari from comment #48) > Hmm, interesting. Where does the before hint come from in this case? I have no idea. But obviously collapse() takes two parameters and no hint can be passed though it. So once again, you want to fix that collapse(span,2) in |<span>text<br></span>more| should show the caret in front of the "more". That's pretty esoteric. The real problem at hand is that *editing* doesn't work, since the editor makes all the right calls (including the right hint), but selection shows the caret on the wrong line. Summarising: 1) Can you say goodbye to the idea to always position into the next sibling? I think I have explained why that is not desired. 2) What should we do about the collapse() test you suggested? The fact is that the patch I submitted, once we agree on the "break" being correct and removing/fixing the comment you don't like and changing the offset adjustment, actually fixes the problem presented in this bug. Sure, I can tweak the tests a bit, but they would be the way I had them with editing actions and not collape(). Yes, I can address the review comment re. sel.collapse(theDiv, 1); in the tests. So where do we go from here? May I suggest this: 1) instead of checking theNode->GetLastChild()->IsHTMLElement(nsGkAtoms::br), check for a visible (line) break. Fix the other nits, comment, offset adjustment. 2) fix the |sel.collapse(theDiv, 1);| in the test you complained about and replace the tt/font by one or more spans. I'm not sure I can add more tests since I believe the ones you wanted are based on collapse(), for example |<span>text<br></span> more|.
Flags: needinfo?(ehsan)
(In reply to :Ehsan Akhgari from comment #48) > Hmm, interesting. Where does the before hint come from in this case? nsFrameSelection::GetFrameForNodeOffset(nsIContent * aNode, int aOffset, mozilla::CaretAssociationHint aHint, int * aReturnOffset) Line 2020 C++ nsCaret::GetCaretFrameForNodeOffset(nsFrameSelection * aFrameSelection, nsIContent * aContentNode, int aOffset, mozilla::CaretAssociationHint aFrameHint, unsigned char aBidiLevel, nsIFrame * * aReturnFrame, int * aReturnOffset) Line 653 C++ nsCaret::GetFrameAndOffset(mozilla::dom::Selection * aSelection, nsINode * aOverrideNode, int aOverrideOffset, int * aFrameOffset) Line 406 C++ Comes from here: https://dxr.mozilla.org/comm-central/rev/c55bcb7c777ea09431b4d16903ed079ae5632648/mozilla/layout/base/nsCaret.cpp#408 from frameSelection->GetHint().
Better link: https://dxr.mozilla.org/mozilla-central/rev/c55bcb7c777ea09431b4d16903ed079ae5632648/layout/base/nsCaret.cpp#408 Looks like there is a SetHint(): https://dxr.mozilla.org/comm-central/search?q=SetHint&redirect=false Also looks like CARET_ASSOCIATE_BEFORE is the default, "AFTER" is only set in a few special cases, for example here: https://dxr.mozilla.org/comm-central/source/mozilla/layout/generic/nsSelection.cpp#5115 You were involved in bug 1237236 as the reviewer.
Thanks for digging up where this hint comes from. So if you look at attachment 8730445 [details] (which is a test case for bug 1237236), you'll see that the case there was actually kind of similar to this bug. There, we have a preformatted text node with a newline inside it. Here, we have an inline element with a line break caused by a <BR> after it... Looking around a bit more, we have for example this code <https://dxr.mozilla.org/comm-central/source/mozilla/dom/html/nsTextEditorState.cpp#603> which switches to the after hint when it detects a BR node. So perhaps the idea of using the hint here was a mistake... But before we come to that conclusion, can you please redo the tests and instead of setting the selection programmatically (which may not give you the after hint), use an editable area and place the caret at the beginning and press the Right arrow key to move it to the desired position? That should give you an after hint, but it may not place the selection in the exact position in the DOM to trigger the rest of the conditions. In the mean time, I'll think more about whether I can come up with a better way to fix this. Sorry this is so tricky, but as you can see our code here is a fragile house of cards, and we need to make sure that a change here plays well with everything else.
Flags: needinfo?(ehsan)
(In reply to Jorg K (GMT+2) from comment #49) > (In reply to :Ehsan Akhgari from comment #47) > > > Take: > > > <span>textnode1|textnode2</span><span>text</span> > > > Two text nodes. Again, ask to collapse behind the second child of the span. > > > Well, one would collapse in textnode2, why wouldn't one? In your approach > > > you'd collapse in the sibling > > Yes. > > >, but it's only interesting to collapse in the > > > sibling if there is a *visible break* (as I already said in comment #38). > > Why? You're not explaining the reason for this. > Well, you're not explaining the reason for wanting to position into the > sibling regardless. The reasoning that led to this suggestion wasn't specific to <BR>, which is why I'm resisting adding it. Please note that while things may be obvious to you, it's not obvious to me, since I haven't debugged this myself. So far what you have told me is "without this check, some tests fail" and you're arguing me into accepting the fix without explaining a) what were the reasons each one of the tests you encountered were failing, and b) how exactly restricting this path to <BR> elements make the tests pass. For example, I can't even tell whether this condition only masks things so that the tests pass, or whether you found something while debugging the tests that make it obvious this needs to only apply to <BR> elements. Speaking of this, how does your patch change the caret placement in the last test case in comment 6? That test case places the selection programmatically, so it should get the before hint and not be fixed with your patch... Can you verify? > > I don't know how you define a visible break, but many things about the > > caret's presentation can be different in the sibling than the stuff inside > > the span. Think things such as font size, line height, inline-block boxes > > with relative positioning, and so on. > > So why change existing behaviour here? We _are_ changing the existing behavior here one way or another. The question is: what is the correct way to change it. > Besides, we're not talking about the > DOM insertion point, only where the caret shows. If you position into the > next sibling, then you will get cursor corresponding to this siblings text > properties, the next character will still be typed into the first span with > that span's properties. Attaching the caret to the next sibling shouldn't change where the selection actually is, so it should not affect anything about text insertion. > If there's a visible (line) *break*, like the opposite of IsInvisibleBreak() > in nsDocumentEncoder.cpp, so a new *line*, then we have no choice but to > position the caret into the new line, since when you keep typing, that's > were the new text will go and not into the previous line where the caret > currently shows. When you keep typing, the text will still go into the > span/tt/font element and carry its properties. You can see that in > attachment 8789087 [details], where the <tt> is continues. The DOM insertion > is still inside the span, although the caret shows at a frame not belonging > to the span. Oh, hmm. Now I'm really wondering if this is the right way to fix this... And also whether this case and the similar ones we've been discussing are even the same bug. Hopefully the answers to the questions I asked above will help decide that. (Your current solution seems better to me if it turns out that there are several independent bugs here.) > We seem to be on different tracks here. My solution exclusively wants to fix > showing the caret on the wrong *line* because the next inserted text goes > into the subsequent line after the <br> in the span/tt/font. > > You're approach is much further reaching and your suggestion is to always > position into the next sibling. > > Before we haven't settled this misunderstanding, I cannot continue since you > want to implement something we don't need and which breaks a bunch of tests. Not sure how to read this, but I don't appreciate the tone here. I'm trying to make sure that if there are several manifestations of the same underlying bug we fix all of them. This requires patience and working through the list of issues, and much more importantly, mutual respect. > > > > @@ +2033,5 @@ > > > > > + nsCOMPtr<nsINode> parent = aNode; > > > > > + while (!sibling) { > > > > > + parent = parent->GetParentNode(); > > > > > + if (!parent) { > > > > > + break; > > > > What makes bailing out here OK? > > > Well, if there is no more parent, I'm at the top of the tree. If no sibling > > > was found, there just isn't one. > > That doesn't mean that you can safely bypass everything else that happens > > inside the loop body after this point, such as descending into the children > > subtree, dealing with text nodes, etc. I think in this case you should > > basically fall back to the existing code path. > Is there a misunderstanding here? The code is: > + while (!sibling) { > + parent = parent->GetParentNode(); > + if (!parent) { > + break; <<== This break goes ... > + } > + sibling = parent->GetNextSibling(); > + } > + if (sibling) { <<===. here. And since no sibling is found > we fall though to the normal > processing as you requested. > + aNode = sibling; > + aHint = CARET_ASSOCIATE_BEFORE; > + aOffset = 0; > + // Start over. > + continue; > + } > + } Oh, I totally misread this code, my apologies. Please ignore this comment.
It's past 1 AM, so I will get to your questions tomorrow. Just briefly: data:text/html,<!doctype html> <div contenteditable><span>a<br></span><br></div> <script> getSelection().collapse(document.querySelector("span"), 2); document.body.firstChild.focus(); </script> from comment #6 is basically the same as what I attached in attachment 8795291 [details], so due to the CARET_ASSOCIATE_BEFORE before hint, my code following your approach doesn't fix this as noted in comment #46. As for the tone, please check whether your following statements express patience and mutual respect (see: John 8:7: "He that is without sin among you, let him first cast a stone at her."): Comment #31: Also FWIW it gets tiring repeating the same request over and over again. I've asked you to not include debugging cruft in patches many times before. It would help if you listen for the next time. My comment: We've been working together for a while now and I cannot recall ever submitting a patch with debug in it other when that was explicitly required to debug, for example the vanishing spell check underlines, bug 1100966. Comment #44: That's what continue means. Please remove the comment explaining the obvious! My comment: The use of exclamation marks is really quite inappropriate: https://en.wikipedia.org/wiki/Exclamation_mark. Also, I assume you use "Thanks!" with the best possible intentions, but it comes across as condescending. The (paid) master tells the (unpaid and voluntary) slave what he should do. See: http://www.answers.com/Q/How_should_it_be_conveyed_when_an_exclamation_point_follows_the_word_thanks#slide=1 What does an exclamation point after the word thanks mean? It is usually someone telling you what to do (i.e., a boss) in a mock-friendly way - i.e., "Please have that report on my desk by no later than nine in the morning. Thanks!" The command and the "Thanks!" is usually given in the form of an email, and is considered rhetorical - in other words, the person *giving the command* is not expecting a response; they just expect you to follow the orders they are giving you. "Thanks!" with an exclamation point is typically preceded by the command. The "Thanks" is supposed to denote (mock) politeness, while the exclamation point is supposed to convey firmness or strength (i.e., you better do it, "or else"). So I think we should both acknowledge that we're both good guys trying to fix up good-old Gecko (nicely described as a "fragile house of cards" (comment #52)). Sometimes I don't like what you write and sometimes you don't like what I write, we already discussed this personally on IRC. Coming back to: > > Before we haven't settled this misunderstanding, I cannot continue since you > > want to implement something we don't need and which breaks a bunch of tests. > Not sure how to read this, ... Of course it the best possible way ;-) Surely there is no dispute that I can't continue unless we agree on the solution. I have outlined my solution following your initial idea: Positioning after the last child will show the caret in the next sibling. I added to that: Positioning after the last child *which causes a visible line break* will show the caret in the next sibling. You are correct that I only briefly looked at the test failures, but mostly they said: layout/generic/test/test_bug496275.html | test 3a: Wrong anchor node. layout/generic/test/test_bug496275.html | test 3a: Wrong anchor offset. - got 1, expected 0 etc. So this makes immediately clear what we've unnecessarily changed existing behaviour. The new code (without the <br> check) *always* positions in the sibling although since comment #38 I've been saying that it's only necessary for a visible break. Comment #26 refers to your IsInvisibleBreak() which does check whether a brFrame is causing a break or now. So I thought I made it clear what I meant by visible break, but it wasn't clear. Coming back to the sibling issue. Imagine: <font size=5 color=red>text</font><font size=1 color=green>text</font> Now, it we were going to follow the approach without the break check (and assuming the hint doesn't prevent the new code to be executed), then collapse(firstFont, 1) would return a frame in the second <font> tag causing the caret to be small and green. But when you type, you get big red letters. > Attaching the caret to the next sibling shouldn't change where the selection > actually is, so it should not affect anything about text insertion. Yes, it would. As you pointed out yourself, the colour and size of the caret is determined by the frame it's associated with. This made me say what I said. I'm sorry you didn't like the particular wording. I think this completely answers comment #53, I'll get to the arrow stuff you asked for in comment #52 later.
As an aside. The special case solution from attachment 8790350 [details] [diff] [review] + if (aOffset > 0 && + aContentNode->Length() == aOffset && + theFrame->GetType() == nsGkAtoms::brFrame && + aContentNode->GetLastChild()->IsHTMLElement(nsGkAtoms::br) + ) { (move caret to the next line) works for Aryeh's example from comment #6 and both attached test cases in attachment 8789087 [details] and attachment 8795291 [details] (the latter being mostly Aryeh's example). Frankly, I never understood why you said (comment #31): your patch wouldn't address this test case |<span>text<br></span>more| It does, since a) the length of the span is equal to the offset (2). b) the current GetFrameForNodeOffset() has (maybe erroneously) returned the brFrame belonging to that span. c) the last child of the span is a <br> (crude test for visible line break). So we know that under these circumstances the caret should be shown on the next line and we do just that basically correcting what GetFrameForNodeOffset() has returned.
Finally addressing comment #52. (In reply to :Ehsan Akhgari from comment #52) > But before we come to that conclusion, can you please redo the tests and > instead of setting the selection programmatically (which may not give you > the after hint), use an editable area and place the caret at the beginning > and press the Right arrow key to move it to the desired position? That > should give you an after hint, but it may not place the selection in the > exact position in the DOM to trigger the rest of the conditions. I don't understand what you want me to do. Looking at |<span>text<br></span>more| positioning at the front of "text" and pressing the arrow key four times gets me behind the letter "t". The caret shows there and text insertion continues on the first line as it should. Pressing the arrow key a fifth time gets me in front of the "more" as it should. Using the arrow keys it is impossible to collapse behind the <br> but within the <span>. > In the meantime, I'll think more about whether I can come up with a better way to fix this. That would be much appreciated. So far we have two options, excuse the repetition: 1) Crude special case solution (see comment #55) which works in all known cases. 2) Fixing GetFrameForNodeOffset() by selecting the next sibling under some circumstances, which IHMO should include testing for a <br> or visible line break. That doesn't work for |<span>text<br></span>more| with Collapse() or Aryeh's example, but could be made to work by forcing the hint to "AFTER" just like you did for bug 1237236 (nice description, BTW: "when I start typing, caret teleports to the next line" ;-)
(In reply to Jorg K (GMT+2) from comment #54) > It's past 1 AM, so I will get to your questions tomorrow. Just briefly: > > data:text/html,<!doctype html> > <div contenteditable><span>a<br></span><br></div> > <script> > getSelection().collapse(document.querySelector("span"), 2); > document.body.firstChild.focus(); > </script> > > from comment #6 is basically the same as what I attached in attachment > 8795291 [details], so due to the CARET_ASSOCIATE_BEFORE before hint, my code > following your approach doesn't fix this as noted in comment #46. Sigh, so this probably means we need to adjust our solution then, since this test case is *really* the same as the one using editing operations, and the fact that the patch here fixes one but not the other suggests that the fix isn't exactly right. :-( > As for the tone, please check whether your following statements express > patience and mutual respect In the interest of staying on topic, I'm not going to delve into this, I'll assume good faith and not enough bandwidth in plaintext conversation to convey the tone. :-) I hope you don't take any of my comments as condescending as that's not my intention. > Surely there is no dispute that I can't continue unless we agree on the > solution. I have outlined my solution following your initial idea: > Positioning after the last child will show the caret in the next sibling. > I added to that: > Positioning after the last child *which causes a visible line break* will > show the caret in the next sibling. The difficult part here is detecting a visible break between the two frames. Note that what we really want to pay attention to is whether there's a visual difference with attaching the caret to the left or right side of the selection point. In case there's a line break, there's obviously a difference, but that's not the only such case. If there's basically any visible empty space (horizontal and/or vertical) between the two elements there is a visual distinction between the two options for placing the caret. Detecting only a BR element only addresses one case here (but of course it is the common case.) I've been trying to come up with a solution which works for every case but I'm beginning to think that may be too hard to achieve here sadly. > You are correct that I only briefly looked at the test failures, but mostly > they said: > layout/generic/test/test_bug496275.html | test 3a: Wrong anchor node. > layout/generic/test/test_bug496275.html | test 3a: Wrong anchor offset. - > got 1, expected 0 > etc. > > So this makes immediately clear what we've unnecessarily changed existing > behaviour. The new code (without the <br> check) *always* positions in the > sibling although since comment #38 I've been saying that it's only necessary > for a visible break. OK, taking this test failure as an example, test 3a here is selecting the entire paragraph p1, and then moves the selection forward. The test expects that to put the selection at (p2, 0) but the test failure suggests that the selection is somewhere else. And that's all we can tell from the test failure. We don't know where the selection actually is, how it got there, and which part of your changes caused it to change. I'm asking you to investigate the test failures under a debugger, and get to the bottom of the questions such as above. Once we have those answers, maybe we'll learn something from them that helps us reason about the problem a bit better. One possible answer depending on what causes the tests to fail is that we need to fix our tests because they were previously expecting bad results! IOW, not every test failure means the code is now doing something wrong -- sometimes when you fix a bug you discover some test failures showing that the tests were previously expecting the bug to happen for various reasons. > Comment #26 refers to your IsInvisibleBreak() which > does check whether a brFrame is causing a break or now. So I thought I made > it clear what I meant by visible break, but it wasn't clear. Note that the code there specifically cares about *line breaks* not other types of visible breaks, as I mentioned above. > Coming back to the sibling issue. Imagine: > <font size=5 color=red>text</font><font size=1 color=green>text</font> > > Now, it we were going to follow the approach without the break check (and > assuming the hint doesn't prevent the new code to be executed), then > collapse(firstFont, 1) would return a frame in the second <font> tag causing > the caret to be small and green. But when you type, you get big red letters. Ugh, I forgot that we paint the caret with the text color. :-( This is a good argument against my suggestion before.
(In reply to Jorg K (GMT+2) from comment #55) > As an aside. The special case solution from attachment 8790350 [details] [diff] [review] > [diff] [review] > > + if (aOffset > 0 && > + aContentNode->Length() == aOffset && > + theFrame->GetType() == nsGkAtoms::brFrame && > + aContentNode->GetLastChild()->IsHTMLElement(nsGkAtoms::br) > + ) { > (move caret to the next line) > > works for Aryeh's example from comment #6 and both attached test cases in > attachment 8789087 [details] and attachment 8795291 [details] (the latter > being mostly Aryeh's example). OK, that makes sense. The reason why is that patch doesn't care about the hint. And this basically means doing anything based on the hint is the wrong idea. > Frankly, I never understood why you said > (comment #31): > your patch wouldn't address this test case |<span>text<br></span>more| > > It does, since > a) the length of the span is equal to the offset (2). > b) the current GetFrameForNodeOffset() has (maybe erroneously) returned the > brFrame > belonging to that span. > c) the last child of the span is a <br> (crude test for visible line break). > So we know that under these circumstances the caret should be shown on the > next line and we do just that basically correcting what > GetFrameForNodeOffset() has returned. Reading the patch and the test case again, you're clearly right here. I have no idea why I thought this test case wouldn't be fixed... But now it's clear that I didn't know exactly what I was talking about then! (In reply to Jorg K (GMT+2) from comment #56) > Finally addressing comment #52. > > (In reply to :Ehsan Akhgari from comment #52) > > But before we come to that conclusion, can you please redo the tests and > > instead of setting the selection programmatically (which may not give you > > the after hint), use an editable area and place the caret at the beginning > > and press the Right arrow key to move it to the desired position? That > > should give you an after hint, but it may not place the selection in the > > exact position in the DOM to trigger the rest of the conditions. > > I don't understand what you want me to do. > Looking at > |<span>text<br></span>more| > positioning at the front of "text" and pressing the arrow key four times > gets me behind the letter "t". The caret shows there and text insertion > continues on the first line as it should. Pressing the arrow key a fifth > time gets me in front of the "more" as it should. > > Using the arrow keys it is impossible to collapse behind the <br> but within > the <span>. Thanks, this is what I was hoping to confirm. This is great: it basically means we don't need to worry about the caret movement code here, which reduces the number of things we need to worry about. :-) > > In the meantime, I'll think more about whether I can come up with a better way to fix this. > That would be much appreciated. > > So far we have two options, excuse the repetition: > 1) Crude special case solution (see comment #55) which works in all known > cases. > 2) Fixing GetFrameForNodeOffset() by selecting the next sibling under some > circumstances, which IHMO should include testing for a <br> or visible > line break. > That doesn't work for |<span>text<br></span>more| with Collapse() or > Aryeh's example, > but could be made to work by forcing the hint to "AFTER" just like you > did for bug 1237236 > (nice description, BTW: "when I start typing, caret teleports to the next > line" ;-) Forcing the hint like this is really a hack that should be used as a last resort so I really prefer to not do that here. Sadly I still don't have any better ideas, but I'm still thinking. I'm now more inclined to focus on fixing the <BR> case specifically. But please give me a day or two to convince myself there isn't a better way to fix this... If you have time, can you see if you can come up with an alternative patch that does something similar to attachment 8790350 [details] [diff] [review] but inside GetFrameForNodeOffset() similar to your newer patch? One thing I'd like to know is, can we get away only by detecting the BR frame without having to replicate the code in IsInvisibleBreak()? Or do we absolutely need that part too?
Flags: needinfo?(jorgk)
Midnight has passed so here come some answers. (In reply to :Ehsan Akhgari from comment #57) > In the interest of staying on topic, I'm not going to delve into this, I'll > assume good faith and not enough bandwidth in plaintext conversation to > convey the tone. :-) I hope you don't take any of my comments as > condescending as that's not my intention. It's all good ;-) I met a guy, Michael Henretty, in Berlin at a Mozilla meeting once. He knows you personally and told me that you're a great guy. I have no doubt about that. English is not our mother tongue, I'm a (sometimes) abrasive German, but good at heart, and some stuff gets lost in the plaintext conversation, for sure. So there will be some friction at times, but we're all here with good intentions. > I'm asking you to investigate the test failures under a debugger, and get to > the bottom of the questions such as above. Once we have those answers, > maybe we'll learn something from them that helps us reason about the problem > a bit better. One possible answer depending on what causes the tests to > fail is that we need to fix our tests because they were previously expecting > bad results! IOW, not every test failure means the code is now doing > something wrong -- sometimes when you fix a bug you discover some test > failures showing that the tests were previously expecting the bug to happen > for various reasons. Sure, but there are a lot of *non-obvious* test failures, and debugging each one of them will cost a lot of time. Hopefully it might not be necessary given what we're discussing below. (In reply to :Ehsan Akhgari from comment #58) > Forcing the hint like this is really a hack that should be used as a last > resort so I really prefer to not do that here. Sadly I still don't have any > better ideas, but I'm still thinking. I'm now more inclined to focus on > fixing the <BR> case specifically. But please give me a day or two to > convince myself there isn't a better way to fix this... Sure, as you know, I'm a Thunderbird guy and plenty of work there. I'd like to get this landed in mozilla52, and we still have a few weeks. > If you have time, can you see if you can come up with an alternative patch > that does something similar to attachment 8790350 [details] [diff] [review] > but inside GetFrameForNodeOffset() similar to your newer patch? One thing > I'd like to know is, can we get away only by detecting the BR frame without > having to replicate the code in IsInvisibleBreak()? Or do we absolutely > need that part too? So you'd like to see something that pushes the fix from attachment 8790350 [details] [diff] [review] - which is basically fixing the wrong return of GetFrameForNodeOffset() - into that function. Yes, that makes sense, since cleaning up the mess afterwards is ugly. I'll tinker with it.
Flags: needinfo?(jorgk)
This moves the "special case processing" to the end of GetFrameForNodeOffset(). Quite ugly, but it solves all known test cases. No debug included ;-)
Comment on attachment 8796193 [details] [diff] [review] Special case processing moved to GetFrameForNodeOffset() (In reply to :Ehsan Akhgari from comment #58 of 2016-09-28) > I'm now more inclined to focus on > fixing the <BR> case specifically. But please give me a day or two to > convince myself there isn't a better way to fix this... Any result of your deliberation? If there is no better way, I suggest adding my tests(*) from the previous patch and the ones you requested |<span>text<br></span><br>more| |<span>text<br></span>more| |<span>text<br></span> more| and be done. (*): Changing <font> and <tt> to <span> since you didn't like the former.
Flags: needinfo?(ehsan)
Attachment #8796193 - Flags: feedback?(ehsan)
(In reply to Jorg K (GMT+2) from comment #61) > Comment on attachment 8796193 [details] [diff] [review] > Special case processing moved to GetFrameForNodeOffset() > > (In reply to :Ehsan Akhgari from comment #58 of 2016-09-28) > > I'm now more inclined to focus on > > fixing the <BR> case specifically. But please give me a day or two to > > convince myself there isn't a better way to fix this... > Any result of your deliberation? Ugh, sorry, I meant to comment here, and I forgot... Let's just stick with fixing the BR case, I couldn't come up with anything better!
Flags: needinfo?(ehsan)
Comment on attachment 8796193 [details] [diff] [review] Special case processing moved to GetFrameForNodeOffset() Review of attachment 8796193 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Jorg K (GMT+2) from comment #59) > > If you have time, can you see if you can come up with an alternative patch > > that does something similar to attachment 8790350 [details] [diff] [review] > > but inside GetFrameForNodeOffset() similar to your newer patch? One thing > > I'd like to know is, can we get away only by detecting the BR frame without > > having to replicate the code in IsInvisibleBreak()? Or do we absolutely > > need that part too? > So you'd like to see something that pushes the fix from attachment 8790350 [details] [diff] [review] > [details] [diff] [review] - which is basically fixing the wrong return of > GetFrameForNodeOffset() - into that function. Yes, that makes sense, since > cleaning up the mess afterwards is ugly. I'll tinker with it. Yes, this is what I'd like to see here. Thanks. > (In reply to :Ehsan Akhgari from comment #58 of 2016-09-28) > > I'm now more inclined to focus on > > fixing the <BR> case specifically. But please give me a day or two to > > convince myself there isn't a better way to fix this... > Any result of your deliberation? > > If there is no better way, I suggest adding my tests(*) from the previous > patch and the ones you requested > |<span>text<br></span><br>more| > |<span>text<br></span>more| > |<span>text<br></span> more| > and be done. > > (*): Changing <font> and <tt> to <span> since you didn't like the former. Sure, that sounds good!
Attachment #8796193 - Flags: feedback?(ehsan) → feedback+
Attached patch Proposed solution - Mark II (v1a). (obsolete) (deleted) — — Splinter Review
OK, here is the full patch: 1) Code that received f+ 2) Previous tests. I left the <font> and <tt> since this is closer to the real use case and tests the same as a <span> would. 3) Three new test cases as specified by Ehsan. They use <span> and they are static, so no editing involved, just Collapse(). All tests pass locally so here's a full try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0bace50657febbc976f29887d1d1dad959abcba
Attachment #8794543 - Attachment is obsolete: true
Attachment #8796193 - Attachment is obsolete: true
How silly of me, I put "-u none", grr. Here another correct try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ab6d7875be7a003f69b8d1a8712111b724c6848
The try run is "green" on Windows (apart from the usual intermittent failures) but fails on Linux and Mac. Funnily, two of the five new tests fail: 2180 INFO TEST-UNEXPECTED-FAIL | layout/base/tests/test_reftests_with_caret.html | reftest comparison: == http://mochi.test:8888/tests/layout/base/tests/bug1263357-1.html http://mochi.test:8888/tests/layout/base/tests/bug1263357-1-ref.html 2183 INFO TEST-UNEXPECTED-FAIL | layout/base/tests/test_reftests_with_caret.html | reftest comparison: == http://mochi.test:8888/tests/layout/base/tests/bug1263357-2.html http://mochi.test:8888/tests/layout/base/tests/bug1263357-2-ref.html On Mac, that's all the failures, but on Linux a bunch of other Linux-only tests fail, like: 2177 INFO TEST-UNEXPECTED-FAIL | layout/base/tests/test_reftests_with_caret.html | reftest comparison: == http://mochi.test:8888/tests/layout/base/tests/multi-range-user-select.html#prev1S_ http://mochi.test:8888/tests/layout/base/tests/multi-range-user-select-ref.html#prev1S_ Strangely those passed with the previous solution, see comment #41. Looks like I have to fire up my Linux machine to look into it. I have the impression that the new tests fail since the caret is drawn differently. Meanwhile, here's a try run without the first two new tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=79556e34f14bb38ddc3c7a830dbbee493b0c437d
OK, try https://treeherder.mozilla.org/#/jobs?repo=try&revision=79556e34f14bb38ddc3c7a830dbbee493b0c437d without the two offending new tests came out green. The two new tests fail because synthesizeKey("VK_END", {}); doesn't work as expected on Linux and Mac. I made that experience already in bug 1263909: https://hg.mozilla.org/mozilla-central/rev/761a291f2255#l1.123 So that shouldn't be too hard to fix, updated patch and try run coming up.
Attached patch Proposed solution - Mark II (v1b). (obsolete) (deleted) — — Splinter Review
Same thing, but not using synthesizeKey("VK_END", {}); on Linux and Mac. New Mochitest try for those platforms only: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1194ff40013855a2e47b7d9c9fee2cc6f68c2e02
Attachment #8799964 - Attachment is obsolete: true
Comment on attachment 8800238 [details] [diff] [review] Proposed solution - Mark II (v1b). I think this is ready for review. Notes: I left my original "editing" tests as they were, with <font> and <tt>. For the purposes of the test they have the same effect as <span> and they actually reflect what a user would be doing in a HTML editor. There are two try runs: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ab6d7875be7a003f69b8d1a8712111b724c6848 Linux and Mac Mochitests show failures, but everything else is green (with the usual intermittent failures). https://treeherder.mozilla.org/#/jobs?repo=try&revision=1194ff40013855a2e47b7d9c9fee2cc6f68c2e02 That's a rerun of the Mac and Linux Mochitests with synthesizeKey("VK_END", {}); replaced by something else. This is coming out green slowly but surely. I believe the "a11y" failure is intermittent since it passed on the first try here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ab6d7875be7a003f69b8d1a8712111b724c6848&selectedJob=28987590 Sorry about running the second try on Mac OSX 10.6 as well, that's of course a complete failure (and the try chooser says so if you read the hints). Let me know if you want me to re-run anything so you don't have to combine two try runs mentally.
Attachment #8800238 - Flags: review?(ehsan)
I did another try run to make the reviewer's life easier: https://treeherder.mozilla.org/#/jobs?repo=try&revision=77540c7b6276 a11y failed again, but the sheriff retriggered it for me seven (!) times and it passed seven times.
Comment on attachment 8800238 [details] [diff] [review] Proposed solution - Mark II (v1b). Review of attachment 8800238 [details] [diff] [review]: ----------------------------------------------------------------- I have a couple of comments below, but I feel like it would be cheating for me to review this since I've been involved in getting to this patch. Mats, can you please review this? Thanks! ::: layout/generic/nsSelection.cpp @@ +2128,5 @@ > break; > } // end while > > if (!returnFrame) > return nullptr; You should do all of the below *before* this condition, to correctly deal with the case where returnFrame is null after you modify it (not that I can think of an actual case where that can happen, but better be safe than sorry.) @@ +2137,5 @@ > + aOffset > 0 && > + (uint32_t) aOffset >= aNode->Length() && > + aNode->GetLastChild() == theNode && > + theNode->IsHTMLElement(nsGkAtoms::br)) { > + // Adapted from IsInvisibleBreak() in nsDocumentEncoder.cpp. FWIW I think at this point we need to lift up the common code into nsLayoutUtils or some such...
Attachment #8800238 - Flags: review?(ehsan) → review?(mats)
(In reply to :Ehsan Akhgari from comment #71) > > if (!returnFrame) > > return nullptr; > > You should do all of the below *before* this condition, to correctly deal > with the case where returnFrame is null after you modify it (not that I can > think of an actual case where that can happen, but better be safe than > sorry.) Yeah, I don't think that can happen. First, because "currentLine->IsEmpty()" is certainly true if there is no child frame on 'currentLine', secondly because lines which have no child frames should have been removed by nsBlockFrame so any such lines should not be observable outside it. But sure, it doesn't hurt to null-check again after the assignment I guess. > FWIW I think at this point we need to lift up the common code into > nsLayoutUtils or some such... Yes, that seems like a good idea. It looks to me that: http://searchfox.org/mozilla-central/rev/2142de26c16c05f23e543be4fa1a651c4d29604e/dom/base/nsDocumentEncoder.cpp#364 IsInvisibleBreak returns true if the line that contains the <br> in question is "LineHasNonEmptyContent" *unless* the first non-empty line after it is an IsInline line. (BTW, it could have returned early to avoid the loop when 'lineNonEmpty' is false. And I'm not quite sure why it continues the iteration if it found a non-empty block line. It seems we should "break;" after the IsInline 'if', like in your patch.) Your patch seems to do the same thing, except it doesn't check that the line containing the <br> is non-empty in the "LineHasNonEmptyContent" sense. What happens if you change "if (valid)" to if (valid && LineHasNonEmptyContent(iter.GetLine())) Does that break any tests? (if not, then it seems it should be possible to share the code)
Flags: needinfo?(jorgk)
(In reply to Mats Palmgren (:mats) from comment #72) > And I'm not quite sure why it continues > the iteration if it found a non-empty block line. Perhaps it makes sense to skip empty block lines too actually, so that we handle these cases in the same way: <div contenteditable=true>Hello<br>Kitty</div> <div contenteditable=true>Hello<br><div></div>Kitty</div>
Thanks for looking at it. (In reply to :Ehsan Akhgari from comment #71) > You should do all of the below *before* this condition, to correctly deal > with the case where returnFrame is null after you modify it (not that I can > think of an actual case where that can happen, but better be safe than > sorry.) I'll fix that when addressing further review comments. > FWIW I think at this point we need to lift up the common code into > nsLayoutUtils or some such... Well, IsInvisibleBreak() returns a bool, this patch here wants to dig out a frame and an offset. Is it really worth the effort to squeeze them through the same funnel to shave-off 20 lines of code? As we've already seen, LineHasNonEmptyContent() isn't called in my patch. And if we break in IsInvisibleBreak() like you suggested, are we sure we don't break its callers? Sounds like a lot of work and risk with little net gain.
(In reply to Mats Palmgren (:mats) from comment #73) > Perhaps it makes sense to skip empty block lines too actually, > so that we handle these cases in the same way: > <div contenteditable=true>Hello<br>Kitty</div> > <div contenteditable=true>Hello<br><div></div>Kitty</div> I think you lost me here at 1 AM ;-) Are you referring to IsInvisibleBreak() or the new code?
Flags: needinfo?(jorgk)
(In reply to Jorg K (GMT+2) from comment #74) I don't think it's about shaving off 20 lines of code. I think it's more about having one "is this a visible or invisible line break" check defined in one place to avoid having subtly different checks in different parts of the code. (In reply to Jorg K (GMT+2) from comment #75) > Are you referring to IsInvisibleBreak() or the new code? Ignore that comment, I was temporarily confused. :-)
(In reply to Mats Palmgren (:mats) from comment #76) > (In reply to Jorg K (GMT+2) from comment #74) > I don't think it's about shaving off 20 lines of code. > I think it's more about having one "is this a visible or invisible line > break" > check defined in one place to avoid having subtly different checks in > different parts of the code. OK. How do you solve the problem that the two callers want different things? Bool vs. frame+offset? I third worker function that returns all three and the others calling this one? BTW, my block of code is actually not solving the problem of "is this a visible or invisible line break". It just wants to advance to the next frame in the next line.
Attached patch WIP - trying to use modified IsInvisibleBreak() (obsolete) (deleted) — — Splinter Review
I've copied IsInvisibleBreak() and its helpers here to investigate the possibility of sharing the code. With the attached patch, the new tests still pass locally. So if we're going ahead with this, how would you like to solve the problem that the original IsInvisibleBreak() returned a bool and this one here a frame. Perhaps in the interest of avoiding further loops, write down the new function interface here and I'll implement it. We still need to see whether the added |break;| does any damage.
Attachment #8801001 - Flags: feedback?(mats)
That didn't compile on Linux :-( Also, the null check needs to come first. New try here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e34b810005890abcc96b237c0ead678a247a080b
Attachment #8801001 - Attachment is obsolete: true
Attachment #8801001 - Flags: feedback?(mats)
Attachment #8801032 - Flags: feedback?(mats)
OK, that try went well. So Mats, could you kindly specify the interface for me. Since bool IsInvisibleBreak(nsINode *aNode) has one call, I can easily change the interface. How about void IsInvisibleBreak(nsINode *aNode, bool& aIsInvisible, nsIFrame*& nextLineFrame) The problem is that in the current patch the function returns |return currentLine->mFirstChild;| in one case and nullptr in all others. So I would have a hard time to document what the function does: Checks whether the node passed in is an invisible break. If it's not an invisible break, so it's a visible break or no break at all, the function *sometimes* return the first frame of the next line.
OK, I went ahead and moved IsInvisibleBreak() to nsLayoutUtils.cpp. Let me know what you think.
Attachment #8801032 - Attachment is obsolete: true
Attachment #8801032 - Flags: feedback?(mats)
Attachment #8801148 - Flags: feedback?(mats)
Comment on attachment 8801148 [details] [diff] [review] WIP - trying to use modified IsInvisibleBreak() (v3). This looks good so far. Perhaps this would be a good API: bool IsInvisibleBreak(nsINode* aNode, nsIFrame** aNextLineFrame = nullptr) then you don't have to change any existing call sites. (FYI, we generally prefer to use pointers for out params, not references) Can I assume you copied the LineHasNonEmpty... helper functions verbatim? (so I can skip reviewing those) Please also document the aNextLineFrame with a @param in the header. nit: this needs {} : if (!lineNonEmpty) return; Is "returnFrame" always the primary frame of "theNode" in your new code? Then maybe we can skip the "returnFrame->GetType() == nsGkAtoms::brFrame" now? It seems IsInvisibleBreak does that for you. If so, the "theNode->IsHTMLElement(nsGkAtoms::br)" seems redundant too?
Flags: needinfo?(jorgk)
Attachment #8801148 - Flags: feedback?(mats) → feedback+
Comment on attachment 8800238 [details] [diff] [review] Proposed solution - Mark II (v1b). (Cancelling the review request here since I prefer the other patch.)
Attachment #8800238 - Flags: review?(mats)
Attached patch Proposed solution - Mark II (v2a). (obsolete) (deleted) — — Splinter Review
OK, here we go. To answer your questions: > Can I assume you copied the LineHasNonEmpty... helper functions verbatim? Yes, of course. > nit: this needs {} Done. > Is "returnFrame" always the primary frame of "theNode" in your new code? Yes, retrieved just above: returnFrame = theNode->GetPrimaryFrame(); > Then maybe we can skip the "returnFrame->GetType() == nsGkAtoms::brFrame" > now? It seems IsInvisibleBreak does that for you. Indeed. But there is no harm in checking it, is there? > If so, the "theNode->IsHTMLElement(nsGkAtoms::br)" seems redundant too? Indeed, but there is no harm in checking it. Why would I call a function that digs out the primary frame for a node if I already have a frame I can reject. This is just a corner case and I wouldn't spend many CPU cycles on it if I can discard it early. I'm presenting this for review now. We still need to make sure that the changes in IsInvisibleBreak() don't break anything, we return early (which should change anything looking at it logically) and we added a |break;| - see your comment #72. Ehsan added IsInvisibleBreak() in bug 1143570: https://hg.mozilla.org/mozilla-central/rev/fafce41ffcee https://hg.mozilla.org/mozilla-central/rev/f6a0792e6259 and modified dom/base/test/test_bug116083.html which is a mochitest, so we should be covered by rerunning the mochitests before landing this.
Attachment #8800238 - Attachment is obsolete: true
Attachment #8801148 - Attachment is obsolete: true
Flags: needinfo?(jorgk)
Attachment #8801202 - Flags: review?(mats)
Comment on attachment 8801202 [details] [diff] [review] Proposed solution - Mark II (v2a). Review of attachment 8801202 [details] [diff] [review]: ----------------------------------------------------------------- I noticed two nits. ::: layout/base/nsLayoutUtils.h @@ +2862,5 @@ > + * If not, returns the first frame on the next line if such a next line exists. > + * > + * @return true if the node is an invisible break. > + * false if the node causes a visible break or if the node is no break. > + * @param first frame on the next line if such a next line exists. Should put parameter name here. ::: layout/generic/nsSelection.cpp @@ +40,5 @@ > #include "nsLayoutCID.h" > #include "nsBidiPresUtils.h" > static NS_DEFINE_CID(kFrameTraversalCID, NS_FRAMETRAVERSAL_CID); > #include "nsTextFrame.h" > +#include "nsBlockFrame.h" Most likely not needed any more.
Comment on attachment 8801202 [details] [diff] [review] Proposed solution - Mark II (v2a). > Caret shown in wrong position if positioned behind last child of node Please replace the commit message with something that describes the code changes rather than the bug. Such as, "When the caret is placed after the last node on a line, and there is visible line break there, then associate the caret with the frame on the next line instead." or something like that. >layout/base/nsLayoutUtils.h >+ * @param first frame on the next line if such a next line exists. Please make it a bit more precise and complete, like so: assigned to the first frame [...], or null otherwise >layout/generic/nsSelection.cpp >+#include "nsBlockFrame.h" Yeah, I think you can remove this now. >+ // If we ended up here and were asked to position the caret after a break, s/a break/a visible break/ >+ // let's move it to the next line. "let's return the frame on the next line instead, if it exists" >+ if (returnFrame->GetType() == nsGkAtoms::brFrame && >+ theNode->IsHTMLElement(nsGkAtoms::br) && Please remove these two checks because they are redundant. The reason these redundant lines are harmful is that IsInvisibleBreak might change in the future. If/when it does change, then someone will need to investigate whether these two tests are necessary or not. The motivation for sharing IsInvisibleBreak in the first place is to avoid that as much as possible. Hypothetically, say we want to make IsInvisibleBreak also handle this the same as a <br> in the future: <span style="white-space:pre"> </span> Then these two tests are not redundant - they are now wrong! There are probably a few places where we have such redundant lines, but they are usually well motivated for performance reasons. That's not the case here. >+ bool isInvisBreak = nsLayoutUtils::IsInvisibleBreak(theNode, &newFrame); >+ if (!isInvisBreak && newFrame) { I don't think checking the return value is necessary because 'newFrame' is guarranteed to be null when IsInvisibleBreak returns true. (please make sure that's clear in its documentation!) Feel free to keep it though, if you think it adds clarity, but then please write out "Invisible" in the variable name, or alternatively, write it like so: if (!nsLayoutUtils::IsInvisibleBreak(theNode, &newFrame) && newFrame) { r=mats with those nits addressed Thanks for your persistence in investigating and fixing this bug!
Attachment #8801202 - Flags: review?(mats) → review+
Would you mind looking the changes over again to make sure you're satisfied. I had to shorten your suggested commit message. Before requesting check-in, I will do another mochitest try run in the morning (less oranges during European AM times). (In reply to Mats Palmgren (:mats) from comment #88) > Thanks for your persistence in investigating and fixing this bug! Let me give you some background here. I'm a Thunderbird and Mailnews peer. Sadly, due to lack of a better candidate, I look after the Thunderbird compose window and all editor related issues. In TB 45 we made the default paragraph format "Paragraph", so users produce many <p> blocks. Before, paragraphs where little used and so problems with paragraphs went undetected. After TB 45 was released, we started a meta-bug 1248971 to track these problems. Between myself and Aryeh we fixed almost all problems encountered, when this bug here lands, there will be two uncommon problems left (and I'm still hoping that Aryeh will fix one of them). Sadly I couldn't "recruit" Aryeh for this bug; he didn't want to spend "company time" familiarising himself with Core::Selection, so it was left to fix to an unpaid volunteer who has an endless amount of time at no charge whatsoever. (Coincidentally, today is my fifth wedding anniversary, but my wife and 5 month-old boy hardly get to see me since I started working on Mozilla and Thunderbird.) Since I got used to writing in paragraph mode and I'm old-fashioned and use <tt> mostly, I run into this bug every time I type <shift><enter>. So in brief, it just had to be fixed, preferably in mozilla52 to make TB 52 ESR. Thanks for your help!
Attachment #8801202 - Attachment is obsolete: true
Attachment #8801338 - Flags: review?(mats)
Comment on attachment 8801338 [details] [diff] [review] Proposed solution - Mark II (v2b). (In reply to Jorg K (GMT+2) from comment #89) > I had to shorten your suggested commit message. As far as I know, we don't have any limit on how long those can be. Anyway, this looks good to me. Thanks.
Attachment #8801338 - Flags: review?(mats) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/db8c5ee6d069 When the caret is placed after visible line break, associate caret with frame on the next line instead. r=mats
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/db8c5ee6d069
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Blocks: 1488052
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: