Closed Bug 1260102 Opened 8 years ago Closed 8 years ago

[e10s] Click on context <menuitem> is not counted as a user input, which makes fullscreen request, clipboard manipluation, etc. fail to execute

Categories

(Core :: DOM: Events, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
e10s m9+ ---
firefox46 --- wontfix
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: mardeg, Assigned: xidorn)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(2 files)

In Nightly builds, possibly after bug 743198 landed, clicking on the "Full Screen" context <menuitem> in the attached testcase fails, giving the message:

Request for fullscreen was denied because Element.requestFullscreen() was not called from inside a short running user-generated event handler.

Was the onclick handler for <menuitem> usecase missed when the new Fullscreen API was implemented? The prefixed Fullscreen API works in Firefox 46 and earlier for this testcase.
Xidorn, wdyt?
Flags: needinfo?(quanxunzhen)
No, it is not a regression from bug 743198, it is a regression from e10s. The testcase works perfectly fine if it is opened in a non-e10s window.
No longer blocks: 743198
tracking-e10s: --- → ?
Flags: needinfo?(quanxunzhen)
And this is about whether IsHandlingUserInput works for <menuitem> click, which could affect not only fullscreen, but also various other things including window.open and copy/cut.
Summary: Fullscreen request from context <menuitem> click custom event handler denied → [e10s] Click on context <menuitem> is not counted as a user input, which makes fullscreen request, clipboard manipluation, etc. fail to execute
Assignee: nobody → quanxunzhen
Comment on attachment 8735697 [details]
MozReview Request: Bug 1260102 - Pass isHandlingUserInput through process boundary for content menu command. r=mrbkap

https://reviewboard.mozilla.org/r/42913/#review39503

I'm marking ship-it but it seems like the test could use some helpers more effectively.

::: dom/html/test/browser_content_contextmenu_userinput.js:35
(Diff revision 1)
> +add_task(function* () {
> +  let tab = gBrowser.addTab(kPage);
> +  let browser = tab.linkedBrowser;
> +  gBrowser.selectedTab = tab;
> +  registerCleanupFunction(() => gBrowser.removeTab(tab));
> +  yield waitForDocLoadComplete();

Any reason not to use BrowserTestUtils.withNewTab?

::: dom/html/test/browser_content_contextmenu_userinput.js:39
(Diff revision 1)
> +  registerCleanupFunction(() => gBrowser.removeTab(tab));
> +  yield waitForDocLoadComplete();
> +
> +  gMessageManager = browser.messageManager;
> +  gMessageManager.loadFrameScript(
> +    `data:,(${frameScript.toString()})();`, false);

Likewise, why not use ContentTask to avoid having to do your own serialization?
Attachment #8735697 - Flags: review?(mrbkap) → review+
Priority: -- → P2
https://reviewboard.mozilla.org/r/42913/#review39503

> Any reason not to use BrowserTestUtils.withNewTab?

Just because I was not aware of them :)

> Likewise, why not use ContentTask to avoid having to do your own serialization?

Same here. But actually these two helper functions do not affect the length of the test a lot in terms of line count.

I'd really hope if there could be a waitForMessage helper.
Comment on attachment 8735697 [details]
MozReview Request: Bug 1260102 - Pass isHandlingUserInput through process boundary for content menu command. r=mrbkap

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42913/diff/1-2/
Attachment #8735697 - Attachment description: MozReview Request: Bug 1260102 - Pass isHandlingUserInput through process boundary for content menu command. r?mrbkap → MozReview Request: Bug 1260102 - Pass isHandlingUserInput through process boundary for content menu command. r=mrbkap
From IRC:
> mrbkap> xidorn: Hey, a couple of things: you probably want to yield ContentTask.spawn to give it a chance to run in the child.
> mrbkap> xidorn: But also, you can do |let promise = ContentTask.spawn(browser, null, function* () { yield new Promise(resolve => { ...addEventListener(..., resolve); }); }); yield promise;|

Add ni? myself so that I remember to push a followup later.
Flags: needinfo?(quanxunzhen)
https://hg.mozilla.org/mozilla-central/rev/928fe146f1c1
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Flags: needinfo?(quanxunzhen)
Xidorn, should this fix be uplifted to Aurora 47 in preparation for our e10s experiments on Beta 47?
Flags: needinfo?(quanxunzhen)
Comment on attachment 8735697 [details]
MozReview Request: Bug 1260102 - Pass isHandlingUserInput through process boundary for content menu command. r=mrbkap

Approval Request Comment
[Feature/regressing bug #]: e10s
[User impact if declined]: window.open/fullscreen/copy etc. may not work if they are invoked from context menu created from <menu>/<menuitem> element in the content
[Describe test coverage new/current, TreeHerder]: new test added
[Risks and why]: doesn't seem to be risky
[String/UUID change made/needed]: n/a
Flags: needinfo?(quanxunzhen)
Attachment #8735697 - Flags: approval-mozilla-aurora?
Hi Xidorn, this is a pretty big change. Could it negatively affect the non-e10s behavior? Since 47 will ship with e10s off I need to make sure we don't regression quality on that front while taking new uplifts to enable Beta 47 e10s experiment.
Flags: needinfo?(bugzilla)
This doesn't seem to be a big change. Not counting the test, it is just +24 -10 lines.

I cannot predict, but I think it shouldn't affect non-e10s behavior, especially given that there is a new test is added.
Flags: needinfo?(bugzilla)
Comment on attachment 8735697 [details]
MozReview Request: Bug 1260102 - Pass isHandlingUserInput through process boundary for content menu command. r=mrbkap

e10s related, Aurora47+
Attachment #8735697 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
has problems/conflicts uplifting to aurora

Tomcats-MacBook-Pro-2:mozilla-central Tomcat$ hg graft --edit -r 928fe146f1c1
grafting 337073:928fe146f1c1 "Bug 1260102 - Pass isHandlingUserInput through process boundary for content menu command. r=mrbkap"
merging browser/base/content/content.js
merging dom/html/test/browser.ini
warning: conflicts while merging dom/html/test/browser.ini! (edit, then use 'hg resolve --mark')
Flags: needinfo?(bugzilla)
Flags: needinfo?(bugzilla)
You need to log in before you can comment on or make changes to this bug.