Closed Bug 1252296 Opened 5 years ago Closed 5 years ago

[e10s] Fix test_contextmenu_input.html to run in e10s

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
e10s + ---
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: jaws, Assigned: jaws)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
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/
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
Flags: needinfo?(adw)
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?
Attachment #8725799 - Flags: review?(gijskruitbosch+bugs) → review+
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?
Flags: needinfo?(adw)
(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).
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.
https://hg.mozilla.org/mozilla-central/rev/c3550a9e2c53
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
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
Attachment #8726351 - Flags: approval-mozilla-aurora?
Comment on attachment 8726351 [details] [diff] [review]
Patch for check-in

Test-only fix.
Attachment #8726351 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.