Last Comment Bug 757486 - Allow browser frames to bubble some whitelisted events
: Allow browser frames to bubble some whitelisted events
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Mounir Lamouri (:mounir)
:
Mentors:
Depends on: 757764 759017
Blocks: 755648 761060 761067 762349
  Show dependency treegraph
 
Reported: 2012-05-22 09:24 PDT by Mounir Lamouri (:mounir)
Modified: 2013-09-03 20:41 PDT (History)
5 users (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (11.30 KB, patch)
2012-05-22 09:24 PDT, Mounir Lamouri (:mounir)
justin.lebar+bug: review+
Details | Diff | Review
Patch v1.1 (12.94 KB, patch)
2012-05-22 16:44 PDT, Mounir Lamouri (:mounir)
justin.lebar+bug: review+
mounir: checkin+
Details | Diff | Review

Description Mounir Lamouri (:mounir) 2012-05-22 09:24:45 PDT
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
Comment 1 Tim Guan-tin Chien [:timdream] (please needinfo; OOO and on leave until July) 2012-05-22 09:49:40 PDT
Question: would it work if we got <iframe mozbrowser> within <iframe mozbrowser>, like browser?
Comment 2 Mounir Lamouri (:mounir) 2012-05-22 09:57:16 PDT
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.
Comment 3 Tim Guan-tin Chien [:timdream] (please needinfo; OOO and on leave until July) 2012-05-22 10:02:55 PDT
Cool, thanks.
Comment 4 Justin Lebar (not reading bugmail) 2012-05-22 10:48:17 PDT
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 5 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-05-22 10:58:55 PDT
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.
Comment 6 Justin Lebar (not reading bugmail) 2012-05-22 11:06:28 PDT
> (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 7 Justin Lebar (not reading bugmail) 2012-05-22 11:46:13 PDT
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.
Comment 8 Justin Lebar (not reading bugmail) 2012-05-22 13:20:30 PDT
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
Comment 9 Mounir Lamouri (:mounir) 2012-05-22 16:44:36 PDT
Created attachment 626248 [details] [diff] [review]
Patch v1.1

Re-asking a review for the change between |addEventListener| to |addSystemEventListener|.
Comment 10 Mounir Lamouri (:mounir) 2012-05-22 16:45:01 PDT
I will file follow-ups for the OOP issues I've found.
Comment 11 Justin Lebar (not reading bugmail) 2012-05-22 20:15:31 PDT
>+    // 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.
Comment 12 Mounir Lamouri (:mounir) 2012-05-23 00:44:51 PDT
(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.
Comment 14 Mounir Lamouri (:mounir) 2012-05-23 08:35:17 PDT
Finally been able to modify the test so it passes locally *and* in tbpl...
Comment 15 Ed Morley [:emorley] 2012-05-24 10:54:33 PDT
https://hg.mozilla.org/mozilla-central/rev/7ae630f43357
Comment 16 Tim Guan-tin Chien [:timdream] (please needinfo; OOO and on leave until July) 2012-05-27 21:00:22 PDT
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?
Comment 17 Justin Lebar (not reading bugmail) 2012-05-27 21:06:17 PDT
> REOPEN?

Please file a follow-up bug and mark it as blocking this one, if you don't mind.
Comment 18 Tim Guan-tin Chien [:timdream] (please needinfo; OOO and on leave until July) 2012-05-27 21:16:07 PDT
(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...
Comment 19 Justin Lebar (not reading bugmail) 2012-05-28 13:05:45 PDT
> Filed bug 759017. Not sure if I set it correctly...

It looks good to me.

Note You need to log in before you can comment on or make changes to this bug.