For a document containing element layout something like this: <div contentEditable="true"><BR> <span contentEditable="false><!--IP won't show after this span--><span><BR> </div> Clicking to the right of the line with comment causes IP to not display. See attached example html. Change in behaviour caused by change bf414f68291c https://paratext.fogbugz.com/f/cases/756984/ Firefox previous to above change displayed IP. Chrome also displays IP.

Need to send this to try server.

Regression range: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ad587ca628cf&tochange=385840329d91 I guess the culprit is: Jorg K — Bug 756984 - Collapse the selection on the last text node on the line, skipping br and inline frames when clicking past the end of line; r=roc,ehsan

I looked at the patch and I can't see anything wrong with it (apart from the fact that the coding standard calls for indentation by two spaces and we always us braces in conditions). As you said, you'd need to push it to the try server. You also need to add a test. (Note: It took me a while to work out what the problem is since I've never seen "IP" (insertion point?) used for the word "caret".)

Even more self-explanatory ;-)

(In reply to Jorg K (GMT+2) from comment #3) > You also need to add a test. "Caret wrong" bugs usually require a reftest (which compares what is visible on the screen). However, maybe you can just add a test case to the existing test here: https://dxr.mozilla.org/mozilla-central/source/layout/generic/test/test_bug756984.html#28

Try server Result for patch- https://treeherder.mozilla.org/#/jobs?repo=try&revision=21cd60e08bb3&selectedJob=19283604 Patch contains both (updated) fix and updated test for bug 756984 Jorg K - Thank you for pointing out the test I could modify rather than writing a new one and sorry for using non standard terminology for caret.

The patch is for Core::Layout.

Comment on attachment 8740059 [details] [diff] [review] 1263288.patch - updated fix and updated mochitest to test for regression. Review of attachment 8740059 [details] [diff] [review]: ----------------------------------------------------------------- I'm a Thunderbird developer, so I have no authority to review this. Let's see who can review this. Also, for further reference, to not use too many server resources, this is usually enough for a try server run involving mochitests: try: -b do -p linux64 -u mochitests[x64] -t none or even try: -b o -p linux64 -u mochitests[x64] -t none You know the try choser at http://trychooser.pub.build.mozilla.org/, right? ::: layout/generic/nsFrame.cpp @@ +3879,5 @@ > // Skip brFrames. Can only skip if the line contains at least > // one selectable and non-empty frame before > if (!SelfIsSelectable(frame, aFlags) || frame->IsEmpty() || > + (canSkipBr && frame->GetType() == nsGkAtoms::brFrame && > + !frame->GetContent()->IsEditable())) { Nit: I think that according to the coding standard this needs to be indented one space more, so the ! comes under the c. Anyway, let's see what the reviewer will say to this. ::: layout/generic/test/test_bug756984.html @@ +58,4 @@ > is(selRange.endContainer.nodeName, "DIV", "selection should be in DIV"); > is(selRange.endOffset, 0, "offset should be 0"); > > + // Click beyond a empty, apart from a readonly span, contenteditable line. expect caret to be after <br> Nit. Upper case after the period and period at the end. They are very picky around here ;-)

In bug 756984 we changed the behaviour of FF to position into a text node when getting to the end of the line. Getting to the end of the line can be achieved in various ways: 1) Clicking to the right of the line 2) Pressing left-arrow from the beginning of the following line 3) Pressing the end key 4) Pressing up/down from longer following/preceding lines. The change I made in https://hg.mozilla.org/mozilla-central/diff/bf414f68291c/layout/generic/nsFrame.cpp made changes to cases 1) 2) and 3), although the test sadly doesn't cover that. Inspired by this knowledge, I looked at this some more. The first observation is that your new test sadly proves nothing. It would give the same outcome in an unpatched version. If you click behind the first xyz using current FF, you get DIV(1), just as in the your test. Have you run your test without the patch? It's always a good idea to run tests with and without the fix so you see that the fail without the fix and succeed with the fix. Also, I think we need to exercise the other cases a little. I've done this: Case 1: Clicked behind the second xyz. No caret, typed 123. Then used right-arrow to go to the third line. Tried to get to the end of the line using "end" or right-arrow, but that doesn't work. Case 2: Clicked behind the second xyz. No caret, typed 123. Use left-arrow a few times to get to the end of the previous line. Caret shows, however, I can't type anything. Are you interested in looking into this some more? Also try the two cases in Chrome, there they work OK.

Comment on attachment 8740088 [details] [diff] [review] Update patch to reflect Nits from review by Jorg K. Maybe to early to ask for review. Also, looking at the test again: <div id="div5" contenteditable=true><span contentEditable=false>xyz<!-- comment --></span><br></div> Offset 1 in the div is where exactly? Your comment says: "Expect caret to be after <br>". That would be offset 2, right? - on the next line. Offset 1 is after the span. That's were we want to be and that's where we are, with and without your patch. Only with your patch, we get a caret, which is nice.

Looks like I messed up - I will have another go at this shorty. Thanks for pointing this out. (In reply to Jorg K (GMT+2) from comment #11) > Comment on attachment 8740088 [details] [diff] [review] > Update patch to reflect Nits from review by Jorg K. > > Maybe to early to ask for review. > > Also, looking at the test again: > <div id="div5" contenteditable=true><span contentEditable=false>xyz<!-- > comment --></span><br></div> > > Offset 1 in the div is where exactly? Your comment says: "Expect caret to be > after <br>". That would be offset 2, right? - on the next line. Offset 1 is > after the span. That's were we want to be and that's where we are, with and > without your patch. Only with your patch, we get a caret, which is nice.

Carrying through Tom's ideas to the other two spots that were changed in bug 756984. With these two additional tweaks the two test cases 1) and 2) from comment #10 work a whole lot better: 1) Can now go to the end of the third line with the "end" key. Right-arrow still doesn't work. 2) When going to the end of the first line with left-arrow, typing there actually works. I guess now we need to write a few decent tests.

Let's fix this since it also fixes the problem described in bug 1237286.

Tom, would you mind if I fixed test_bug756984.html in bug 1263909 quickly to have a good basis for this bug here and make sure we don't break anything.

(In reply to Jorg K (GMT+2) from comment #15) > Tom, would you mind if I fixed test_bug756984.html in bug 1263909 quickly to > have a good basis for this bug here and make sure we don't break anything. I don't mind at all. :)

Maybe you will mind, since I've just made our lives so much harder ;-) I've done the test over in bug 1263909. Sadly, it fails big time with the patches from this bug here applied.

Comment on attachment 8740151 [details] [diff] [review] More tweaks. As stated in comment #17, this patch makes the enhanced test test_bug756984.html fail, so this is not the correct way to fix it. Even Tom's original change if (!SelfIsSelectable(frame, aFlags) || frame->IsEmpty() || - (canSkipBr && frame->GetType() == nsGkAtoms::brFrame)) { + (canSkipBr && frame->GetType() == nsGkAtoms::brFrame) && + !frame->GetContent()->IsEditable()) { continue; } already causes a test failure. The failure is that after clicking beyond the end of the line, the selection is placed in the surrounding DIV, although it should be in the text node. Being in the text node is very important for guaranteeing that the text properties are carried over to newly entered text. Selecting at the end of the text node is also what Chrome does, so we decided on this modified behaviour in bug 756984. Sadly it has caused two known (minor) regressions, this bug here and bug 1237286.

Comment on attachment 8740088 [details] [diff] [review] Update patch to reflect Nits from review by Jorg K. Not a viable solution as per comment #18. Sorry that I didn't realise it earlier.

Copies window image before and after selection that should display caret. Checks that the two images differ.

Experimental fix - for proof of concept feedback. 1. Not very Elegant (introducing two new variables) 2. Doesn't deal with the third brFrame special case added in Bug 756984

Hmm, I'm not enough of a Mozilla man to know whether a Mochitest or a Reftest is the appropriate test here. See for example bug 1237236 or bug 1258308 for recent new reftests. (I only had to deal with reftests once in bug 1200533 (attachment 8658241 [details] [diff] [review])).

(In reply to Tom Hindle from comment #21) > Experimental fix - for proof of concept feedback. Looks like I'm not the right guy to ask here, perhaps Mats can comment.

Comment on attachment 8741866 [details] [diff] [review] chome mochitest for this bug. This looks nice but I think you can add your test here instead: layout/base/tests/test_reftests_with_caret.html

Comment on attachment 8741870 [details] [diff] [review] Experimental fix - for proof of concept feedback. I think this is the wrong place to address this problem - the existing code here seems correct to me. The result we get from GetContentOffsetsFromPoint in nsFrame::HandlePress is "DIV : 3" which is between the SPAN and the BR. This is the correct point to place the caret from an editing POV. The problem is that we fail to render the caret there. So the relevant painting path is: nsCaret::PaintCaret nsCaret::GetFrameAndOffset nsCaret::GetCaretFrameForNodeOffset nsFrameSelection::GetFrameForNodeOffset where the last one call returns a null frame - this is why nsCaret fails to render the caret. The reason GetFrameForNodeOffset returns a null frame is that it gets the child content at the given offset (the SPAN) and drills down to its deepest decendant at the end-edge, the COMMENT node in this case, which has no frame. Then it gives up. We have the same problem if we replace the comment with <x style="display:none"></x> for example.

I think something like this might work - i.e. if the desired content has no frame then iterate its siblings until we find one. This patch needs work though since we shouldn't recurse over siblings like this (it'll blow up the stack with a large number of frame-less siblings). Perhaps we could break out the code between these two hunks into a separate method and then iterate instead?

Tom, FYI, it would help if you configure 'hg diff' to include more context, like so: [diff] git = 1 showfunc = true unified = 8 (in your $HOME/.hgrc)

How are we going to continue here? There's a proposal by Mats. Are we going to test that, write a test for it and land it?

(In reply to Jorg K (GMT+2) from comment #28) > How are we going to continue here? There's a proposal by Mats. Are we going > to test that, write a test for it and land it? I'm planning on attempting this, should clear other TODO's by end of this week.

Sorry for the delay. I've updated the patch by Mats so that GetFrameForNodeOffset is no longer recursive. try here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5dc2b05038c9 I did try and change the mochitest test to a reftest but couldn't get synthesizeMouse to work in the reftest. I was including the following script lines: <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script> <script type="application/javascript" src="/tests/SimpleTest/EventUtils.js"></script> (My first ref test file, bug1263288.html, looked like this: http://pastebin.com/q95aU8Xc )

I hope you're not waiting for my feedback. I'm just a poor Thunderbird developer at times venturing out into M-C core territory, mostly editor/. I'm pretty much lost in layout/ and my knowledge of reftests is close to zero. You'd have to get Mats' help/feedback/review via NI, f? and r?

Comment on attachment 8748225 [details] [diff] [review] Modified patch by Mats so that GetFrameForNodeOffset is no longer recursive >Bug 1263288 - Caret no longer displays on ContentEditable Line where <BR> follows readonly span containing html comment. It'd be better if the commit message describes the code changes instead of describing the bug. >+ while(!done) { nit: add a space beetwen keyword and '(' please >+ } >+ else { nit: } else { >+ // Now that we have the child node, check if it too >+ // can contain children. If so, call this method again! The last sentence is now inaccurate - it should say "If so, restart the loop!" or something. >+ if (theNode->IsElement() && >+ theNode->GetChildCount() && >+ !theNode->HasIndependentSelection()) { >+ int32_t newOffset = 0; >+ >+ if (aOffset > childIndex) { >+ numChildren = theNode->GetChildCount(); >+ newOffset = numChildren; >+ } >+ >+ aNode = theNode; >+ aOffset = newOffset; >+ continue; nit: this can be simplified. We don't need |newOffset| at all, nor do we need to assign |numChildren|. This should do: aNode = theNode; aOffset = aOffset > childIndex ? theNode->GetChildCount() : 0; continue; >+ } else { >+ // Check to see if theNode is a text node. If it is, translate >+ // aOffset into an offset into the text node. >+ >+ nsCOMPtr<nsIDOMText> textNode = do_QueryInterface(theNode); >+ >+ if (textNode) { nit: remove the blank line >+ if (theNode->GetPrimaryFrame()) { >+ if (aOffset > childIndex) { >+ uint32_t textLength = 0; >+ >+ nsresult rv = textNode->GetLength(&textLength); nit: remove the blank line >+ theNode = newChildNode; >+ int32_t newOffset = >+ aHint == CARET_ASSOCIATE_BEFORE ? theNode->GetChildCount() : 0; >+ aNode = theNode; >+ aOffset = newOffset; >+ continue; nit: the above can also be simplified by not assigning |theNode| (assign aNode directly instead) and removing |newOffset| (assign aOffset directly instead. Make sure you use the new aNode value for the GetChildCount() call though) >+ int32_t end = theNode->GetChildCount(); >+ if (aOffset < end) { >+ ++aOffset; >+ continue; nit: indentation is off >+ done = true; >+ } // end while >+ >+ if (!done) > return nullptr; It seems we don't really need the |done| flag? Can we make the loop |while (true)| and test "if (!returnFrame)" here instead? (if we assign it to nullptr at the top) >layout/generic/test/test_bug1263288.html It would be better to add this as a test in layout/base/tests/test_reftests_with_caret.html to avoid code duplication.

Updated patch based upon review in comment #32 I have NOT changed test_1263288.html to a reftest as I was previously unable to get synthesizeMouse to work in the reftest. (comment #30) (although I'm willing to have another go at this if it 'should' work and it's likely I was just doing something wrong) Thanks!

(In reply to Tom Hindle from comment #33) > I have NOT changed test_1263288.html to a reftest as I was previously unable > to get synthesizeMouse to work in the reftest. (comment #30) Oops, I missed that comment for some reason, sorry. synthesizeMouse should definitely work in a plain mochitest - we don't need to use a chrome mochitest for event stuff. Also, it appears VK_END works fine on all platforms. It looks green so far: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9294aa939460 It might have been a missing SimpleTest.waitForFocus call that made it fail when you tried it?

Comment on attachment 8750859 [details] [diff] [review] Update patch to reflect review from mats >- theNode = newChildNode; >- int32_t newOffset = >- aHint == CARET_ASSOCIATE_BEFORE ? theNode->GetChildCount() : 0; >- return GetFrameForNodeOffset(theNode, newOffset, aHint, aReturnOffset); [...] >+ int32_t newOffset = >+ aHint == CARET_ASSOCIATE_BEFORE ? aNode->GetChildCount() : 0; >+ aNode = newChildNode; >+ aOffset = newOffset; >+ continue; In the old code we use "theNode->GetChildCount()" after it's been assigned. In the new code we don't, but I think we should. I think we can remove the local variable (newOffset) here and assign |aOffset| directly instead. Just move the expression to after |aNode| has been assigned and it should work. r=mats with that change (r- on the test part though - use the test I attached above instead)

(In reply to Mats Palmgren (:mats) from comment #34) > Created attachment 8751421 [details] [diff] [review] > caret test patch > > (In reply to Tom Hindle from comment #33) > > I have NOT changed test_1263288.html to a reftest as I was previously unable > > to get synthesizeMouse to work in the reftest. (comment #30) > > Oops, I missed that comment for some reason, sorry. > synthesizeMouse should definitely work in a plain mochitest - we don't > need to use a chrome mochitest for event stuff. > > Also, it appears VK_END works fine on all platforms. It looks green so far: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=9294aa939460 > > It might have been a missing SimpleTest.waitForFocus call that made it > fail when you tried it? I sent this test to the try server WITHOUT the bug fix patch. However it did NOT show the expected test fail. https://treeherder.mozilla.org/#/jobs?repo=try&revision=3020d56f5a20 It seems I was having problem with the ref tests because, individual reftests don't seem to run successfully from the command line(following instructions from here): https://developer.mozilla.org/en-US/docs/Mozilla/Creating_reftest-based_unit_tests IE. running: ./mach reftest layout/base/tests/reftest.list (where reftest.list is): == bug1263288.html bug1263288-ref.html both yours and my previous reftests give "line 26: ReferenceError: SimpleTest is not defined" when run from the command line.

I think I confused you when talking about "reftest" previously - it's actually a plain mochitest (as I later said) that is loading a bunch of sub-tests (with caret drawing enabled) and compare the test to its reference using snapshots (much like reftests do): http://mxr.mozilla.org/mozilla-central/source/layout/base/tests/test_reftests_with_caret.html?force=1 See my test patch. You run it as: ./mach mochitest layout/base/tests/test_reftests_with_caret.html

Patch now contains tests from mats. try server results here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f3d2209f13b93ad7b51e75043a54badc7d97112c

Comment on attachment 8752331 [details] [diff] [review] Update patch again to reflect review from mats Sorry to jump in here. There are three patches here, none of which carry an r+. The sheriffs doing the check-in wouldn't know what to do. Typically you would obsolete the ones which aren't required. If you believe that the final patch has been sufficiently reviewed, you can add the r+ yourself with a comment like "Carrying forward Mats' r+". Strictly speaking, since you added tests to a code patch which had r+, this should get its final review from Mats, so I'll put another r? onto it. Thanks a lot for working on this. Since my change in bug 756984 revealed the problem, I'm happy that it got fixed finally. Could you please check whether bug 1237286 is fixed by this patch as well? Oh, and finally, you would assign the bug to yourself to get all the credit ;-) Which bug are you fixing next? (I've done all the above for you.)