1252296 - [e10s] Fix test_contextmenu_input.html to run in e10s

Status: RESOLVED FIXED

(Firefox :: General, defect)

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: MozReview Request: Bug 1252296 - Convert test_contextmenu_input.html to browser_contextmenu_input.html to run in e10s. r?gijs

Review commit: https://reviewboard.mozilla.org/r/37645/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37645/

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8725799 [details]
MozReview Request: Bug 1252296 - Convert test_contextmenu_input.html to browser_contextmenu_input.html to run in e10s. r?gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37645/diff/1-2/

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f0dc6b603186

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Drew, what changed in bug 1005601 to let selectall now have the same value on OSX and Windows? See https://bugzilla.mozilla.org/show_bug.cgi?id=1005601#c18 and https://hg.mozilla.org/mozilla-central/rev/8b0a999a23f9#l4.19

Comment 5

[:Gijs (he/him)]

Comment on attachment 8725799 [details]
MozReview Request: Bug 1252296 - Convert test_contextmenu_input.html to browser_contextmenu_input.html to run in e10s. r?gijs

https://reviewboard.mozilla.org/r/37645/#review34379

Nice work, thanks!

::: browser/base/content/test/general/contextmenu_common.js:271
(Diff revision 2)
> +  if (options.preCheckContextMenuFn) {
> +    yield options.preCheckContextMenuFn();
> +    info("Completed preCheckContextMenuFn");
> +  }

You moved this block compared to where it used to live in this function (below the spell check wait block). Why?

::: browser/base/content/test/general/browser_contextmenu_input.js:45
(Diff revision 2)
> -                          "context-inspect",     true]);
> +      *preCheckContextMenuFn() {
> +        yield ContentTask.spawn(gBrowser.selectedBrowser, null, function*() {
> +          let doc = content.document;
> +          let input = doc.getElementById("input1");
> +          input.setAttribute("spellcheck", "true");
> +        });
> +      },

It seems like adding separate input boxes for these cases might be an easier way to do this, making the test less verbose and reducing the need for layout flushes (in some of the other preCheckContextMenuFns)? So add #input1-spellchecked that has the spellcheck attribute, #input1-disabled that is disabled, etc.

::: browser/base/content/test/general/browser_contextmenu_input.js:141
(Diff revision 2)
> +  todo(false, "context-selectall is enabled on osx-e10s, and windows when" +
> +              " it should be disabled");

Please ensure we file a followup bug for this and other todo()s here. They look like they could be genuine bugs?

Comment 6

[Drew Willcoxon :adw]

Hmm, I don't remember.  Wish I had noted it in the bug.

The general situation around that bug was basically that async spell check broke that test (e.g., bug 1005601 comment 13).  The OS X/Windows change in that patch was to expect the spell check menu items to all be disabled when the context menu is opened on an input that has never been focused because at that point the input's spell check hadn't finished its async initialization -- or something like that.  So I guess that for some reason on OS X that had always been the case -- spell check menu items were always disabled in that case, but not on Windows.  Maybe due to their different focus models?

But looking at the code now, I don't understand what selectall has to do with that.  Even before that bug, selectall's enabled state was tied to the OS.  Maybe unconditionally setting `value` to false for all the edit menu items was actually an error?  But the test is passing, right?

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Drew Willcoxon :adw from comment #6)
> But looking at the code now, I don't understand what selectall has to do
> with that.  Even before that bug, selectall's enabled state was tied to the
> OS.  Maybe unconditionally setting `value` to false for all the edit menu
> items was actually an error?  But the test is passing, right?

Yeah, very confusing. My refactored patch shows the previous behavior (selectall enabled on windows and disabled on osx).

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

https://reviewboard.mozilla.org/r/37645/#review34379

> You moved this block compared to where it used to live in this function (below the spell check wait block). Why?

The previous ordering was arbitrary. This test sets the "spellcheck" attribute in the preCheckContextMenuFn function, so it is useful to wait for the spell check *after* we execute the preCheckContextMenuFn function.

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for check-in

https://treeherder.mozilla.org/#/jobs?repo=try&revision=29f9002dd87f

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/c3550a9e2c53

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c3550a9e2c53

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8726351 [details] [diff] [review]
Patch for check-in

Approval Request Comment
[Feature/regressing bug #]: e10s test fix
[User impact if declined]: n/a
[Describe test coverage new/current, TreeHerder]: automated test
[Risks and why]: no risk, doesn't touch production code
[String/UUID change made/needed]: none

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8726351 [details] [diff] [review]
Patch for check-in

Test-only fix.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2926f8020821