Closed Bug 1263909 Opened 8 years ago Closed 8 years ago

Enhance test for bug 756984 to cover other code paths also changed in bug 756984.

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

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

References

Details

Attachments

(1 file, 5 obsolete files)

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
as part of
https://hg.mozilla.org/mozilla-central/rev/bf414f68291c
made changes to code path for cases 1) 2) and 3), but the test only covers case 1).

This bug is about adding more tests for cases 2) 3) and 4).

This is necessary since this code will be touched again in bug 1263288 and I want to make sure that nothing breaks.
Blocks: 1263288
Summary: Enhance test for bug 756984 to cover other code path also changed in bug 756984. → Enhance test for bug 756984 to cover other code paths also changed in bug 756984.
Sorry about the white space changes at the top.

We need this test to secure the current operation of the editor when moving the cursor to the end of the line before making changes in bug 1263288. Sadly the original test was deficient.

I hope you agree. Ehsan actually called for more tests in bug 756984 comment #114. There was more talk about additional tests in bug 756984 comment #119, last two lines.

As a by-product we learn that when not it quirks mode, the up and down arrow act differently. I guess that's another bug.
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Attachment #8740570 - Flags: review?(masayuki)
See Also: → 1264088
(In reply to Jorg K (GMT+2) from comment #1)
> As a by-product we learn that when not it quirks mode, the up and down arrow
> act differently. I guess that's another bug.
Raised bug 1264088 for this.
Comment on attachment 8740570 [details] [diff] [review]
Enhanced test to cover edit mode actions.

>+    // Now we do a more complex test, this time with an editable div.
>+    // We have four lines:
>+    // 1234567890
>+    // abc
>+    // defghijklm
>+    // end

Great comment for easier to read!

>+    for (i = 5; i <= 7; i++) {
>+      // We do these steps:
>+      // 1) Click behind the first line, add "X".
>+      theDiv = document.getElementById("div" + i.toString());
>+      theDiv.focus();
>+      var originalHTML = theDiv.innerHTML;
>+      sel.collapse(theDiv, 0);
>+      synthesizeMouse(theDiv, 100, 2, {});
>+
>+      selRange = sel.getRangeAt(0);
>+      is(selRange.endContainer.nodeName, "#text", "selection should be in text node (1)");

Could you test the text node contents as expected? Because there are other text nodes in the editor. (Or store each text node to variables and compare with the stored nodes)

>+      is(selRange.endOffset, 10, "offset should be 10");
>+      synthesizeKey("X", {});
>+
>+      // 2) Down arrow to the end of the second line, add "Y".
>+      synthesizeKey("VK_DOWN", {});
>+      selRange = sel.getRangeAt(0);

Looks like that Selection.getRangeAt() returns a pointer to the range, not copied. So, doesn't work this test without calling this method a lot? If so, please remove the redundant calls. Otherwise, keep them.
Attachment #8740570 - Flags: review?(masayuki) → review+
Thanks for the quick review.

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #3)
> Could you test the text node contents as expected? Because there are other
> text nodes in the editor. (Or store each text node to variables and compare
> with the stored nodes)
I don't think that's necessary. I add text to the end of each line I visit and at the end I compare the produced HTML. If I had positioned to the wrong text, that test wouldn't work.

> Looks like that Selection.getRangeAt() returns a pointer to the range, not
> copied. So, doesn't work this test without calling this method a lot? If so,
> please remove the redundant calls. Otherwise, keep them.
Without the repeated calls, the test fails.

So in the end I'm only enhanced a comment and fixed more indentation (the original was really badly indented).

Carrying forward Masayuki's r+.
Attachment #8740810 - Flags: review+
Attachment #8740570 - Attachment is obsolete: true
Dear Sheriff,
this is an enhanced test, no code change. Please bundle with other patches when landing.
Thank you.
Keywords: checkin-needed
backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=25704118&repo=mozilla-inbound
Flags: needinfo?(mozilla)
Sorry, test worked locally on Windows. I'll fix it and do some try runs before submitting it again.
Flags: needinfo?(mozilla)
Looking at the test failure, the VK_END on Mac doesn't do what it's supposed to do, it doesn't move the caret to the end of the line. We can see in other tests that special stuff if done for Mac, for example here:
http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/tests/test_bug596506.html?force=1#46
Masayuki-san, I'm sorry, you'll have to look at this again.

These are the changes I made since your review:
- Fixed some more white-space issues.
- Fixed a comment.
- I don't think it's necessary to check the text content, but
  I increased the last line to 12 characters, so you can distinguish it
  from the first one.

As you can see from previous comments, the test fails on Mac due to the VK_END key not working.

Here is one failed test:
unexpected HTML - got
"12345678A90X<br>aTbcY<br>Zdefghijklm<br>end", expected
"1234567890XA<br>abcYT<br>defghijklmZ<br>end"

The upper case characters should all go to the end of the lines. On Mac, the X and the Y are inserted correctly, the Z should go to the end of defghijklm but it goes to the start because the "end" key doesn't take us to the end. It's all downhill from there. So I'm skipping most of the new part of the test on Mac.
Attachment #8740810 - Attachment is obsolete: true
Attachment #8740956 - Flags: review?(masayuki)
Comment on attachment 8740956 [details] [diff] [review]
Enhanced test to cover edit mode actions (v2)

Changed my mind, I will modify the test for Mac and Linux, yes, same problem on Linux.
Attachment #8740956 - Flags: review?(masayuki)
OK, try run here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0dcbfa58fdca

Masayuki-san, do you have any idea why the end key wouldn't work as expected on Linux and Mac? I tried on a Linux machine I have and the end key does work. I have no idea why it wouldn't work in the test. For now I'm trying:

+      // 3) Right arrow and end key to the end of the third line, add "Z".
+      //    We've seen problems with the end key on Mac and Linux, so let's
+      //    go right a few more times before hitting the end key.
+      synthesizeKey("VK_RIGHT", {});
+      synthesizeKey("VK_RIGHT", {});
+      synthesizeKey("VK_RIGHT", {});
+      synthesizeKey("VK_END", {});
Attachment #8740956 - Attachment is obsolete: true
@@ -95,11 +95,5 @@
       is(selRange.endOffset, 3, "offset should be 3");
       synthesizeKey("Y", {});
 
-      // Sadly on Mac the virtual end key doesn't take us to the end of the line,
-      // so we're done with one this platform.
-      if (navigator.platform.indexOf("Mac") == 0) {
-        continue;
-      }
-
       // 3) Right arrow and end key to the end of the third line, add "Z".
       synthesizeKey("VK_RIGHT", {});
@@ -104,6 +98,6 @@
       // 3) Right arrow and end key to the end of the third line, add "Z".
       synthesizeKey("VK_RIGHT", {});
-      synthesizeKey("VK_END", {});
+      window.getSelection().modify("move", "right", "lineboundary");
       selRange = sel.getRangeAt(0);
       is(selRange.endContainer.nodeName, "#text", "selection should be in text node (3)");
       is(selRange.endOffset, 12, "offset should be 12");
I booted up my Linux box and I tried what I said in comment #13 locally.
Ain't working. Thanks for your suggestion, but I'd like to use a virtual key, if at all possible. Any idea why that doesn't work? I can do it by hand and it works, but not in the test.
Hmm, your suggestion works. You reckon it's better than using VK_LEFT 12 times?
(In reply to Jorg K (GMT+2) from comment #16)
> Hmm, your suggestion works. You reckon it's better than using VK_LEFT 12
> times?

I think so - The test would be less fragile to line length changes.
Hopefully last attempt incorporating Tom's suggestion. Thanks!
Try here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6bffe401125
Attachment #8740973 - Attachment is obsolete: true
Attachment #8740991 - Attachment is obsolete: true
Comment on attachment 8741030 [details] [diff] [review]
Enhanced test to cover edit mode actions (v4)

Assuming that the try run
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6bffe401125
was successful this time (it has come out green on Linux, where it wasn't when this was first landed here:
  https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=9db1fc7d3df35bb343bc8d83231737b0c900bc63)
I'm asking for review again.

Repeating from comment #11:

Masayuki-san, I'm sorry, you'll have to look at this again.

These are the changes I made since your review:
- Fixed some more white-space issues, it was badly indented initially.
- Fixed a comment.
- declared loop variable i.
- I don't think it's necessary to check the text content, but
  I increased the last line to 12 characters, so you can distinguish it
  from the first one.
- Implemented a workaround for non-functioning "end" key on Mac and Linux.
Attachment #8741030 - Flags: review?(masayuki)
(In reply to Jorg K (GMT+2) from comment #13)
> Created attachment 8740973 [details] [diff] [review]
> Enhanced test to cover edit mode actions (v3)
> 
> OK, try run here:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=0dcbfa58fdca
> 
> Masayuki-san, do you have any idea why the end key wouldn't work as expected
> on Linux and Mac? I tried on a Linux machine I have and the end key does
> work. I have no idea why it wouldn't work in the test. For now I'm trying:
> 
> +      // 3) Right arrow and end key to the end of the third line, add "Z".
> +      //    We've seen problems with the end key on Mac and Linux, so let's
> +      //    go right a few more times before hitting the end key.
> +      synthesizeKey("VK_RIGHT", {});
> +      synthesizeKey("VK_RIGHT", {});
> +      synthesizeKey("VK_RIGHT", {});
> +      synthesizeKey("VK_END", {});

Ah, probably, End key binding is not defined by XUL <key> element. Instead, using native key bindings information. synthesizeKey() generates keyboard events just via widget. So, some information are not set different from native key event.
Comment on attachment 8741030 [details] [diff] [review]
Enhanced test to cover edit mode actions (v4)

Sigh, we don't retrieve native key bindings for non-printable keys...
https://dxr.mozilla.org/mozilla-central/source/widget/cocoa/TextInputHandler.mm#4148,4210
Attachment #8741030 - Flags: review?(masayuki) → review+
Dear Sheriff,
this is an enhanced test, no code change. Please bundle with other patches when landing.
Should work this time on all platforms ;-)
Thank you.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/761a291f2255
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.