Closed Bug 1263288 Opened 4 years ago Closed 4 years ago

Caret no longer displays on ContentEditable Line where <BR> follows readonly span containing html comment.

Categories

(Core :: Layout, defect)

39 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: firefox, Assigned: firefox)

References

Details

(Keywords: regression)

Attachments

(3 files, 12 obsolete files)

303 bytes, text/html
Details
698 bytes, text/html
Details
11.89 KB, patch
mats
: review+
Details | Diff | Splinter Review
Attached file Test case (Non minimal) (obsolete) —
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.
Attached patch Possible fix (obsolete) — Splinter Review
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
Blocks: 756984
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(mozilla)
Keywords: regression
Version: 45 Branch → 39 Branch
Attached file Minimal and self-explanatory test case (obsolete) —
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".)
Attachment #8739585 - Attachment is obsolete: true
Flags: needinfo?(mozilla)
Summary: contenteditable regression - IP no longer displays on ContentEditable Line where <BR> follows readonly span containing html comment. → Caret no longer displays on ContentEditable Line where <BR> follows readonly span containing html comment.
Even more self-explanatory ;-)
Attachment #8739647 - Attachment is obsolete: true
(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
Attachment #8739597 - Attachment filename: 314415.patch → 1263288.patch
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.
Attachment #8739597 - Attachment is obsolete: true
Attachment #8740059 - Flags: review?(mozilla)
The patch is for Core::Layout.
Component: Editor → 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 ;-)
Attachment #8740059 - Flags: review?(mozilla)
Attachment #8740059 - Flags: review?(mats)
Attachment #8740059 - Flags: feedback+
Attachment #8740059 - Attachment is obsolete: true
Attachment #8740059 - Flags: review?(mats)
Attachment #8740088 - Flags: review?(mats)
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.
Attachment #8740088 - Flags: review?(mats)
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.
Attached patch More tweaks. (obsolete) — Splinter Review
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.
Blocks: 1237286
Let's fix this since it also fixes the problem described in bug 1237286.
Depends on: 1263909
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.
Attachment #8740151 - Attachment is obsolete: true
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.
Attachment #8740088 - Flags: feedback-
Attached patch chome mochitest for this bug. (obsolete) — Splinter Review
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.
Flags: needinfo?(mats)
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.
Flags: needinfo?(mats)
Attached patch wip (obsolete) — Splinter Review
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?
Attachment #8748225 - Flags: review?(mats)
Attachment #8748225 - Flags: feedback?(mats)
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.
Attachment #8748225 - Flags: review?(mats)
Attachment #8748225 - Flags: feedback?(mats)
Attachment #8748225 - Flags: feedback+
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!
Attachment #8740088 - Attachment is obsolete: true
Attachment #8741866 - Attachment is obsolete: true
Attachment #8741870 - Attachment is obsolete: true
Attachment #8748225 - Attachment is obsolete: true
Attachment #8750859 - Flags: review?(mats)
Attachment #8750859 - Flags: feedback?(mats)
Attached patch caret test patch (obsolete) — Splinter Review
(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)
Attachment #8750859 - Flags: review?(mats)
Attachment #8750859 - Flags: review+
Attachment #8750859 - Flags: feedback?(mats)
(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
Attachment #8750859 - Attachment is obsolete: true
Keywords: checkin-needed
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.)
Attachment #8752331 - Flags: review?(mats)
Attachment #8742070 - Attachment is obsolete: true
Attachment #8751421 - Attachment is obsolete: true
Assignee: nobody → firefox
Keywords: checkin-needed
Attachment #8752331 - Flags: review?(mats) → review+
https://hg.mozilla.org/mozilla-central/rev/cea4108d4918
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
No longer blocks: 1237286
You need to log in before you can comment on or make changes to this bug.