The default bug view has changed. See this FAQ.

Allow browser frames to bubble some whitelisted events

RESOLVED FIXED in mozilla15

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

(Depends on: 1 bug)

Trunk
mozilla15
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

12.94 KB, patch
Justin Lebar (not reading bugmail)
: review+
mounir
: checkin+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
Created attachment 626054 [details] [diff] [review]
Patch v1

For the moment, the whitelisted events are the following hardware button:
- search
- menu
- volume up
- volume down
- back
Attachment #626054 - Flags: review?(justin.lebar+bug)
Question: would it work if we got <iframe mozbrowser> within <iframe mozbrowser>, like browser?
(Assignee)

Comment 2

5 years ago
There is no reason why it wouldn't: <iframe mozbrowser>'s child will simply send those events to the parent if .preventDefault() wasn't called.
Cool, thanks.
(Assignee)

Updated

5 years ago
Attachment #626054 - Flags: review?(bugs)
Comment on attachment 626054 [details] [diff] [review]
Patch v1

smaug, I don't think you necessarily need to review this (unless you want to of course).
Comment on attachment 626054 [details] [diff] [review]
Patch v1

Note, there is nsIFrameLoader::activateFrameEvent
I think it doesn't support key events atm, but perhaps keyboardevent
could implement Serialize/Deserialize, if that is useful for b2g.

And I don't see reason to review this. This is not real DOM code.
(Hmm, BrowserElement* should be somewhere in b2g/ )

Btw, do you mean bubble or propagate. Bubble is just one phase of the event dispatch.
Attachment #626054 - Flags: review?(bugs)
> (Hmm, BrowserElement* should be somewhere in b2g/ )

BrowserElement* works in normal Firefox, too.  The intent is for this to be "part of the Web".
Comment on attachment 626054 [details] [diff] [review]
Patch v1

>+// Event whitelisted for bubbling.
>+let whitelistedEvents = [
>+  Ci.nsIDOMKeyEvent.DOM_VK_ESCAPE,   // Back button.
>+  Ci.nsIDOMKeyEvent.DOM_VK_CONTEXT_MENU,
>+  Ci.nsIDOMKeyEvent.DOM_VK_F5,       // Search button.
>+  Ci.nsIDOMKeyEvent.DOM_VK_PAGE_UP,  // Volume up.
>+  Ci.nsIDOMKeyEvent.DOM_VK_PAGE_DOWN // Volume down.
>+];

Ugh, this this will break so hard as soon as someone pairs a real keyboard to their device...

Anyway, I think this is sane for now.

> 		test_browserFrame9.html \
>+		test_browserFrame_keyEvents.html \

Thanks for naming this test properly.  I have a todo to rename the numbered tests (and all my new tests have descriptive names); I just haven't gotten around to renaming the existing tests yet.

>diff --git a/dom/tests/mochitest/browser-frame/file_focus.html b/dom/tests/mochitest/browser-frame/file_focus.html
>new file mode 100644
>--- /dev/null
>+++ b/dom/tests/mochitest/browser-frame/file_focus.html
>@@ -0,0 +1,26 @@
>+<html>
>+<body>
>+
>+Aloha!  My URL is <span id='url'></span>.
>+<script>
>+document.getElementById('url').innerHTML = window.location;
>+</script>
>+<input autofocus>
>+
>+<script>
>+  // The input element is getting synthesized key events and will block the
>+  // first ESC keydown event.

Nit: s/block/call preventDefault() on/.

>+  var alreadyBlocked = false;
>+
>+  document.getElementsByTagName('input')[0].addEventListener('keydown', function(e) {
>+    if (e.keyCode == Components.interfaces.nsIDOMKeyEvent.DOM_VK_ESCAPE &&
>+        alreadyBlocked == false) {
>+      alreadyBlocked = true;
>+      e.preventDefault();
>+    }
>+  });
>+</script>
>+</body>
>+</html>
>+
>diff --git a/dom/tests/mochitest/browser-frame/test_browserFrame_keyEvents.html b/dom/tests/mochitest/browser-frame/test_browserFrame_keyEvents.html
>new file mode 100644
>--- /dev/null
>+++ b/dom/tests/mochitest/browser-frame/test_browserFrame_keyEvents.html
>@@ -0,0 +1,97 @@
>+<!DOCTYPE HTML>
>+<html>
>+<!--
>+https://bugzilla.mozilla.org/show_bug.cgi?id=710231
>+-->
>+<head>
>+  <title>Test for Bug 710231</title>
>+  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
>+  <script type="application/javascript" src="/tests/SimpleTest/EventUtils.js"></script>
>+  <script type="application/javascript" src="browserFrameHelpers.js"></script>
>+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
>+</head>
>+<body>
>+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=710231">Mozilla Bug 710231</a>
>+
>+<!--
>+  Test that an iframe with the |mozbrowser| attribute does not emit
>+  mozbrowserX events when they're globally pref'ed off.
>+-->

Fix comment please and bug numbers please.

>+
>+<script type="application/javascript;version=1.7">
>+"use strict";
>+
>+let Ci = Components.interfaces;
>+
>+let whitelistedEvents = [
>+  Ci.nsIDOMKeyEvent.DOM_VK_ESCAPE,   // Back button.
>+  Ci.nsIDOMKeyEvent.DOM_VK_CONTEXT_MENU,
>+  Ci.nsIDOMKeyEvent.DOM_VK_F5,       // Search button.
>+  Ci.nsIDOMKeyEvent.DOM_VK_PAGE_UP,  // Volume up.
>+  Ci.nsIDOMKeyEvent.DOM_VK_PAGE_DOWN // Volume down.
>+];
>+
>+SimpleTest.waitForExplicitFinish();
>+
>+browserFrameHelpers.setEnabledPref(true);
>+browserFrameHelpers.addToWhitelist();
>+browserFrameHelpers.setOOPDisabledPref(true); // this is breaking the autofocus.

:(  We'll need a follow-up, of course.

In fact, I'm not sure we should even land this without OOP working.

>+function runTest() {
>+  is(document.activeElement, iframe, "iframe should be focused");
>+
>+  addEventListener('keydown', eventHandler);
>+  addEventListener('keypress', eventHandler);
>+  addEventListener('keyup', eventHandler);
>+
>+  // Those event should not be received because not whitelisted.
>+  synthesizeKey("VK_A", {});
>+  synthesizeKey("VK_B", {});
>+
>+  // Those events should not be received because prevent default is called.
>+  synthesizeKey("VK_ESCAPE", {});
>+
>+  // Those events should be received.
>+  synthesizeKey("VK_F5", {}); // F5 key is going to be canceled by ESC key.
>+  synthesizeKey("VK_ESCAPE", {});
>+  synthesizeKey("VK_PAGE_UP", {});
>+  synthesizeKey("VK_PAGE_DOWN", {});
>+  synthesizeKey("VK_CONTEXT_MENU", {});

I presume you'd need to send these keys via a script injected via the mm in an OOP test.

>+}
>+
>+iframe.addEventListener('load', runTest);

Remote mozbrowsers don't send load; you have to listen for mozbrowserloadend.

r+ if you file a bug on making this work OOP, but I'd kind of prefer just to make it work OOP here.
Attachment #626054 - Flags: review?(justin.lebar+bug) → review+
WRT OOP + focus, here's what felipe had to say:

<felipe> on desktop and fennec, there is an ActivateRemoteFrame msg sent through ipc whenever <browser> got focus. Maybe that is not happening for mozbrowser
<felipe> see this changeset and specially this line: http://hg.mozilla.org/mozilla-central/rev/238b9a9479ed#l3.115
<felipe> with that changeset, fennec didn't have to handle focus in the front-end anymore
<felipe> but you can still test by calling .activateRemoteFrame manually from js
(Assignee)

Comment 9

5 years ago
Created attachment 626248 [details] [diff] [review]
Patch v1.1

Re-asking a review for the change between |addEventListener| to |addSystemEventListener|.
Attachment #626054 - Attachment is obsolete: true
Attachment #626248 - Flags: review?(justin.lebar+bug)
(Assignee)

Comment 10

5 years ago
I will file follow-ups for the OOP issues I've found.
>+    // We are using the system group for those events so if something in the
>+    // content called .stopPropagation() this will still be called.
>+    els.addSystemEventListener(global, 'keydown',
>+                               this._keyEventHandler.bind(this),
>+                               /* useCapture = */ true);

Ah, stopPropagation() is not the same thing as preventDefault().  Shows how
much I know...

So just to check that I understand: If content calls stopPropagation or
preventDefault, our keyEventHandler gets called.  But if they called
preventDefault, the key event handler doesn't do anything.  Which means that
content can for example stop the volume key from doing anything.

We'll probably want to place some limits around that somehow, but if I'm
understanding it right, this is fine for now.
Attachment #626248 - Flags: review?(justin.lebar+bug) → review+
(Assignee)

Comment 12

5 years ago
(In reply to Justin Lebar [:jlebar] from comment #11)
> So just to check that I understand: If content calls stopPropagation or
> preventDefault, our keyEventHandler gets called.  But if they called
> preventDefault, the key event handler doesn't do anything.  Which means that
> content can for example stop the volume key from doing anything.

If the content calls stopPropagation(), only, we will send the event to the outer window.
If the content calls preventDefault() amongst other things, we will not send the event to the outer window.

The later was the initial plan and is what we defined with Vivien. The former is smaug's opinion: he thinks there is no reason to not the the event to the outer window if stopPropagation() has been called.

I also believe we will be able to tweak that later depending on feedback we will get.
(Assignee)

Updated

5 years ago
Flags: in-testsuite+
Target Milestone: --- → mozilla15
(Assignee)

Updated

5 years ago
Attachment #626248 - Flags: checkin+
(Assignee)

Updated

5 years ago
Depends on: 757764
Landed as:
https://hg.mozilla.org/integration/mozilla-inbound/rev/baee7c540118

Unfortunately backed out for M3 orange:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&onlyunstarred=1&rev=baee7c540118

https://hg.mozilla.org/integration/mozilla-inbound/rev/59ec4eabd9ce
Target Milestone: mozilla15 → ---
(Assignee)

Comment 14

5 years ago
Finally been able to modify the test so it passes locally *and* in tbpl...
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/7ae630f43357
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Hi, I did get key events in the system app from this fix, but all the events came with keyCode = 0. 

You can try it by replacing system/index.html of Gaia with the following content:
https://gist.github.com/2817145

I/GeckoDump( 2327): ==== keydown, 0, capturing
I/GeckoDump( 2327): ==== keydown, 0, bubbling
I/GeckoDump( 2327): ==== keypress, 0, capturing
I/GeckoDump( 2327): ==== keypress, 0, bubbling
I/GeckoDump( 2327): ==== keyup, 0, capturing
I/GeckoDump( 2327): ==== keyup, 0, bubbling


REOPEN?
> REOPEN?

Please file a follow-up bug and mark it as blocking this one, if you don't mind.
Blocks: 759017
No longer blocks: 759017
Depends on: 759017
(In reply to Justin Lebar [:jlebar] from comment #17)
> > REOPEN?
> 
> Please file a follow-up bug and mark it as blocking this one, if you don't
> mind.

Filed bug 759017. Not sure if I set it correctly...
> Filed bug 759017. Not sure if I set it correctly...

It looks good to me.
Blocks: 761060
Blocks: 761067
Blocks: 761093
No longer blocks: 761093
Blocks: 762349
You need to log in before you can comment on or make changes to this bug.