1029943 - [Text selection] Enable selection carets on B2G and fix the existing test case failures

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect, P1)

Description

[StevenLee[:slee]]

Bug 1016184 comment 29 enable touch caret on B2G test cases. This bug is to enable the preference on B2G and remove the unnecessary pref in testing/profiles/prefs_b2g_unittest.js.

Comment 1

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Since bug 1016184 which turn on touch caret on B2G will be landed soon, let me to steal this bug, and change it to "Enable selection carets on B2G."

Comment 2

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Try result after selection carets is enabled.
https://tbpl.mozilla.org/?tree=Try&rev=695d172644e3

Comment 4

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Fix reftest fails on 824080-2/3/4/5.html.

These four reftests have document.designMode='on' in test html, but they
have noneditable elements in the ref html. Since selection carets are
supported in editable elements for now. Those reftests will fail.

To fix them, turn off "selectioncaret.enabled" until we support
selection carets in non-editable elements.

Comment 5

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Enable selection carets by default on b2g.

Comment 6

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Fix reftest fails on 824080-3/5/7.html, bug558663.html.

These test cases compare between non-edit and editable elements. So we should disable these tests.

Comment 7

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Hide selection carets when blurred.

Some test cases would capture screenshot after element is blurred. If element is blurred, we should hide the selection carets.

Comment 8

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

https://tbpl.mozilla.org/?tree=Try&rev=56c8c6182bfa

latest try run

Comment 9

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Enable selection carets by default on b2g. r=ehsan (v2)

Rebased. We should wait until all tests are green.

Comment 10

[(no longer active)]

Comment on attachment 8476431 [details] [diff] [review]
Fix reftest fails on 824080-3/5/7.html, bug558663.html.

Review of attachment 8476431 [details] [diff] [review]:
-----------------------------------------------------------------

(Splinter doesn't seem to like this patch...)

+  <script>
+  // Selection caret's pref is checked only when PresShell is initialized. To turn
+  // off the pref, we test bug 558663 in an iframe.
+  SpecialPowers.pushPrefEnv({"set": [['selectioncaret.enabled', false]]}, function() {
+    var iframe = document.createElement("iframe");
+    iframe.src = "bug558663.html";
+    document.getElementById('container').appendChild(iframe);
+  });
+  </script>

You need to use SimpleTest.waitForExplicitFinish and SimpleTest.finish to make sure the test framework waits for these tests to be finished before loading the next page.  Also, in the test iframe you should not use your own copy of the SimpleTest.js API.  Usually people define shim functions like this at the beginning of their test frame code to forward all of the calls to these testing functions to the SimpleTest framework loaded in the main test page:

var ok = parent.ok;
var is = parent.is;
// same for todo, isnot, etc if you need them.

Comment 11

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

About B2G ICS Emulator Debug mochitest 9 timeout, I did some experiments. I found disable selection carets, test_collapse.html would running about 78 seconds. But when I enabled selection carets, it running about 476 seconds so that it would timeout mochitest in try server. 

Next, I commented out sendAsyncMsg at BrowserElementChildPreload.js [1], then it reduced to 170 seconds.
Then, I commented out addEventListener for mozselectionchange at BrowserElementChildPreload.js [2], then it reduced to 79 seconds.

So I think selection carets increased too much overhead here, Ehsan, do you have any suggestions?
I have some opinions,
1) Only send mozselectionchange when we have specific reason, so that we can reduce overhead. Add this change test case running about 80 seconds which is good!
2) Increase timeout from 330 seconds to maybe 600 seconds so the testcase won't timeout.
3) Split test case into many smaller test cases.  

[1]: http://dxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementChildPreload.js#658
[2]: http://dxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementChildPreload.js#210

Comment 12

[(no longer active)]

I would be fine with increasing the timeout on that test, I think.  But note that this is an upstream test (from https://dvcs.w3.org/hg/editing), so you need to convince either Aryeh or Ms2ger about increasing the timeout.

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Feel free to fork the editing ones as needed.

Comment 14

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

If you're asking about test_runtest.html, it's not really maintained upstream, so it's safe enough to fork it.  I do have a project to split it up so it's not so ridiculously unwieldy (bug 1008146), but I don't know if I'll get to it in the foreseeable future.  The current location in the tree does suggest it's imported, though, so if you make any significant changes there's always the risk they'll get overwritten sometime in the future.  Maybe disable the import script for editing somehow with a comment explaining why, if you're doing anything that you don't want overwritten.

Comment 15

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Fix reftest fails on 824080-3/5/7.html, bug558663.html v2.

Update to address comment.

Comment 16

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Double timeout for test_collapse.html. (for m-c)

This patch is for mozilla-central. But I need modify upstream as well. Ms2ger, how do I modify upstream? Provide a patch for upstream or there is other way to do this? Thanks.

Comment 17

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Just land on m-c.

Comment 18

[(no longer active)]

Comment on attachment 8478817 [details] [diff] [review]
Fix reftest fails on 824080-3/5/7.html, bug558663.html v2.

Review of attachment 8478817 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/tests/bug558663.html
@@ +8,2 @@
>    <script type="application/javascript" src="/tests/SimpleTest/EventUtils.js"></script>
>    <script type="application/javascript" src="/tests/SimpleTest/WindowSnapshot.js"></script>

You should forward the stuff coming from these two scripts to the parent similar to ok and SimpleTest below...

Comment 19

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Fix reftest fails on 824080-3/5/7.html, bug558663.html v3.

Update address comment.

Comment 20

[(no longer active)]

Comment on attachment 8479685 [details] [diff] [review]
Fix reftest fails on 824080-3/5/7.html, bug558663.html v3.

Review of attachment 8479685 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great!

Comment 21

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Enable selection carets by default on b2g. r=ehsan (v3, carry r+)

Rebased.

Comment 22

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Enable selection carets by default on b2g. r=ehsan (v4, carry r+)

Update since v3 is corrupt.

Comment 23

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Latest try result. We still have an orange M-10 on B2G ICS Emulator Debug.
https://tbpl.mozilla.org/?tree=Try&rev=3b865f4dcb21

Comment 24

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

https://tbpl.mozilla.org/?tree=Try&rev=7ff721c347c8

Here is revision from August 1st and apply 4 patches which are provided in this bug. All tries are green. So there is something wrong since August 1st.

Comment 25

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Apply all patches in this bug on master, and disable touch caret. All green ...
https://tbpl.mozilla.org/?tree=Try&rev=bbb1a64c8b1c

Comment 26

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Another try results which have M-10 failure on different test case, but the call stack are the same with Comment 23. Both failures are at the end of the test, which seem like a shutdown crash.

https://tbpl.mozilla.org/?tree=Try&rev=0ca253c83134

Comment 27

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: CheckAcquire_callstack.txt

Steps to efficiently reproduce M-10 crash locally:
1) Modify dom/tests/mochitest/bugs/mochitest.ini so that it only contains [test_bug265203.html] and [test_bug346659.html].
2) $ ./mach mochitest-remote dom/tests/mochitest/bugs/

The callstack is attached as CheckAcquire_callstack.txt

Comment 28

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

latest try run: https://tbpl.mozilla.org/?tree=Try&rev=9e56b6d715e4

Looks like only Cpp test failed. But other tries also failed on this test recently. So it should be ok.

Comment 29

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

The Cpp test fail is a known issue.
https://bugzilla.mozilla.org/show_bug.cgi?id=1062937

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/c0e37fa863dd
https://hg.mozilla.org/integration/b2g-inbound/rev/f125a36df1d5
https://hg.mozilla.org/integration/b2g-inbound/rev/af2b2c407cf7
https://hg.mozilla.org/integration/b2g-inbound/rev/5d1c0e2b1bd2

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c0e37fa863dd
https://hg.mozilla.org/mozilla-central/rev/f125a36df1d5
https://hg.mozilla.org/mozilla-central/rev/af2b2c407cf7
https://hg.mozilla.org/mozilla-central/rev/5d1c0e2b1bd2

Comment 32

[(no longer active)]

Awesome!  Thanks for all of your hard work on this guys.

Comment 33

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/b2g-inbound/rev/ac4bfb631c1e

Comment 34

[[:fabrice] Fabrice Desré]

This was the backout asked in bug 1066515

Comment 35

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

After resolving bug 1066515 and bug 1065955, the performance of word suggestion and correction in message app shouldn't be a problem now. We can enable copy/paste on B2G again.
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=08d4f7dfd26f

Comment 36

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Keep in mind I have observed there is a timeout failure in test_extend.html test. Like this try push ( https://tbpl.mozilla.org/?tree=Try&rev=248161574608 ). Please see emulator mochitest-5. So, let wait and see if latest try run still have this failure!

Comment 37

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

The test_extend.html is actually timeout on B2G ICS Emulator opt Mochitest-4 in comment 35 after enabling selection carets. (Treeherder is green though ...) And I found that the way we double the timeout for test_collapse.html in attachment 8478819 [details] [diff] [review] might not be correct. The running time of test_collapse.html is too short, and it makes feel that the test does not run.

I will try rebase those patches, and double the timeout for test_extend.html and test_collapse.html by using SimpleTest.requestLongerTimeout();

Comment 38

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Part 1 - Hide selection carets when blurred. r=roc (v2, carry r+)

Comment 39

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Part 2 - Fix test fails on 824080-3/5/7.html, bug558663.html. r=ehsan (v4, carry r+)

Comment 40

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Part 3 - Double timeout for test_collapse.html and test_extend.html. (v2)

Comment 41

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Part 4 - Enable selection carets by default on b2g. r=ehsan (v5, carry r+)

Comment 42

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Try result for the new part 3.
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=03be5056d309

Comment 43

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Per comment 37, if I enable selection caret on b2g, test_extend.html and test_collapse.html will timeout on B2G ICS Emulator. Since we set "explicit_timeout": true in [1], no timeout should be set by  testharness.js. So the only timeout should be set by TestRunner.js. However, my attempt to use SimpleTest.requestLongerTimeout(2); in part3 does not work as comment 42 shown. Simply including SimpleTest.js in the two tests introduces errors.

Aryeh and Ms2ger, I need your advice on how to correctly increase the timeout limit on these two tests.

Disabling tests on b2g should be treated as a last resort, right? Thought some tests had being disabled on b2g in [2].

[1] http://hg.mozilla.org/mozilla-central/annotate/0e631971b841/dom/imptests/testharnessreport.js#l307
[2] http://dxr.mozilla.org/mozilla-central/source/dom/imptests/editing/mochitest.ini

Comment 44

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8515057 [details] [diff] [review]
Part 3 - Double timeout for test_collapse.html and test_extend.html. (v2)

No, this isn't going to work. Perhaps something like

if (W3CTest.runner) {
  W3CTest.runner.requestLongerTimeout(2);
}

will.

Comment 45

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Part 3 - Double timeout for test_collapse.html and test_extend.html. r=Ms2ger (v3)

test_extend.html and test_collapse.html run in B2G ICS Emulator opt Mochitest-4
and debug Mochitest-10.

Try result:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5991c01071b3

Comment 46

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8515605 [details] [diff] [review]
Part 3 - Double timeout for test_collapse.html and test_extend.html. r=Ms2ger (v3)

Review of attachment 8515605 [details] [diff] [review]:
-----------------------------------------------------------------

r=me if it works.

Comment 47

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

The try fails in comment 45 seem to be known issues on m-c.

Comment 48

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/64d22f9fa38f
https://hg.mozilla.org/integration/mozilla-inbound/rev/7466944d5a5d
https://hg.mozilla.org/integration/mozilla-inbound/rev/0dff0452893f
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb2252b32e57

Comment 49

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/64d22f9fa38f
https://hg.mozilla.org/mozilla-central/rev/7466944d5a5d
https://hg.mozilla.org/mozilla-central/rev/0dff0452893f
https://hg.mozilla.org/mozilla-central/rev/eb2252b32e57