Closed Bug 757486 Opened 12 years ago Closed 12 years ago

Allow browser frames to bubble some whitelisted events

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla15

People

(Reporter: mounir, Assigned: mounir)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Patch v1.1
12.94 KB, patch
justin.lebar+bug
: review+
mounir
: checkin+
Details | Diff | Splinter Review
Attached patch Patch v1 (obsolete) — — Splinter Review 
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?
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.
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
Attached patch Patch v1.1 — — Splinter Review 
Re-asking a review for the change between |addEventListener| to |addSystemEventListener|.
Attachment #626054 - Attachment is obsolete: true
Attachment #626248 - Flags: review?(justin.lebar+bug)
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+
(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.
Flags: in-testsuite+
Target Milestone: --- → mozilla15
Attachment #626248 - Flags: checkin+
Depends on: 757764
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
Closed: 12 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.
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.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: