The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 48

Status

()

Core
Layout
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: Jorg K (GMT+1), Assigned: Jorg K (GMT+1))

Tracking

Trunk
mozilla48
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

a year ago
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.
(Assignee)

Updated

a year ago
Blocks: 1263288
(Assignee)

Updated

a year ago
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.
(Assignee)

Comment 1

a year ago
Created attachment 8740570 [details] [diff] [review]
Enhanced test to cover edit mode actions.

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)
(Assignee)

Updated

a year ago
See Also: → bug 1264088
(Assignee)

Comment 2

a year ago
(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+
(Assignee)

Comment 4

a year ago
Created attachment 8740810 [details] [diff] [review]
Enhanced test to cover edit mode actions (fixed comment and indentation)

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+
(Assignee)

Updated

a year ago
Attachment #8740570 - Attachment is obsolete: true
(Assignee)

Comment 5

a year ago
Dear Sheriff,
this is an enhanced test, no code change. Please bundle with other patches when landing.
Thank you.
Keywords: checkin-needed

Comment 6

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/9db1fc7d3df3
Keywords: checkin-needed
backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=25704118&repo=mozilla-inbound
Flags: needinfo?(mozilla)

Comment 8

a year ago
Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a1f721a3c56
(Assignee)

Comment 9

a year ago
Sorry, test worked locally on Windows. I'll fix it and do some try runs before submitting it again.
Flags: needinfo?(mozilla)
(Assignee)

Comment 10

a year ago
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
(Assignee)

Comment 11

a year ago
Created attachment 8740956 [details] [diff] [review]
Enhanced test to cover edit mode actions (v2)

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)
(Assignee)

Comment 12

a year ago
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)
(Assignee)

Comment 13

a year ago
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", {});
(Assignee)

Updated

a year ago
Attachment #8740956 - Attachment is obsolete: true

Comment 14

a year ago
Created attachment 8740991 [details] [diff] [review]
Suggestion to make test work on Linux (and hopefully mac)

@@ -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");
(Assignee)

Comment 15

a year ago
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.
(Assignee)

Comment 16

a year ago
Hmm, your suggestion works. You reckon it's better than using VK_LEFT 12 times?

Comment 17

a year ago
(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.
(Assignee)

Comment 18

a year ago
Created attachment 8741030 [details] [diff] [review]
Enhanced test to cover edit mode actions (v4)

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
(Assignee)

Comment 19

a year ago
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+
(Assignee)

Comment 22

a year ago
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

Comment 23

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/761a291f2255
Keywords: checkin-needed

Comment 24

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/761a291f2255
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox48: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.