Closed Bug 1096146 Opened 10 years ago Closed 10 years ago

No default actions of keydown event should be made if mozbrowserbeforekeydown event is defaultPrevented

Categories

(Core :: DOM: Events, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
2.2 S1 (5dec)

People

(Reporter: dwi2, Assigned: gyeh)

References

Details

(Whiteboard: [ft:conndevices])

Attachments

(2 files, 2 obsolete files)

STR:
1. open any web page in a browser frame.
2. scroll down.
3. tap the "home" button to go to the homescreen.
4. use the cardview to go back to the web page.

Expected:
The scroll position hasn't change.

Actual:
We scrolled up to the top.
Note that bad things happen with the power button too. For instance, install the Macaw app (https://marketplace.firefox.com/app/macaw). If you hit the power button to switch off the screen, that opens the twit edition dialog. That didn't happen before bugs 989198 and 1014418.
I did a experiment. 

Because I didn't see this issue happens on v1 patch of bug 1014418 (https://bugzilla.mozilla.org/attachment.cgi?id=8471485), I made a comparison:

  1. use latest Gecko and Gaia, follow STR => Web page scroll up to the top after pressing home key
  2. remove permission of 'before-after-keyboard-event' from manifest of system app but leave Gaia code untouched. => Web page don't scroll up after pressing home key, which is expected.

I am not sure why this permission make such different behavior. I did call event.preventDefault() in https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/browser_key_event_manager.js#L60 to prevent home key from affecting embedded frame.


Gina, I might need your help on investigating this. Thanks
Flags: needinfo?(gyeh)
To post my findings here:

I did another experiment. I backtrack gaia to https://github.com/mozilla-b2g/gaia/commit/3ca3172b9122fedf46c8411e92c48b724104aae5 which is right before landing of bug 1014418 v2. And I also add 'before-after-keyboard-event' to system app on purpose, then this issue happens.


However the permission 'before-after-keyboard-event' is essential to bug 1014418. Without this permission, if there is any plain iframe resides in system app (for example, FX account login dialog) and the iframe is focused, system app will not receive any key event and user will get stuck in it, just as the case in bug 1093955.
I'm digging into the problem and it seems like that the default actions (which is scrolling up in this case) is made although we prevent them in System App.

Should be an Gecko issue and I'm going to steal this one. Thanks for your help, Tzu-Lin.
Flags: needinfo?(gyeh)
Component: Gaia::System → DOM: Events
Product: Firefox OS → Core
Summary: 'Home' button event should not dispatch into browser iframe → No default actions of keydown event should be made if mozbrowserbeforekeydown event is defaultPrevented
Whenever the before events are default prevented, no key events should be dispatched and none of default actions of key events should be made. Thus, I set the event status as nsEventStatus_eConsumeNoDefault, and the issue is fixed.

smaug, please help to review the patch and let me know if you have any concerns. Thanks.
Assignee: nobody → gyeh
Attachment #8521936 - Flags: review?(bugs)
Comment on attachment 8521936 [details] [diff] [review]
Patch 1(v1): Set event status as nsEventStatus_eConsumeNoDefault

This needs a test too.
Perhaps add a something to the existing before/afterkeyevent tests.
Attachment #8521936 - Flags: review?(bugs) → review+
Can we land this?
Flags: needinfo?(gyeh)
Should we land this with a test? Can we file a follow-up for adding test?
Flags: needinfo?(gyeh)
Whiteboard: [ft:conndevices]
Attached patch Patch 2(v1): Mochitest. (obsolete) — Splinter Review
Attached patch Patch 2(v1): Mochitest. (obsolete) — Splinter Review
Attachment #8525293 - Attachment is obsolete: true
Comment on attachment 8525294 [details] [diff] [review]
Patch 2(v1): Mochitest.

Please review the test. Thanks.
Attachment #8525294 - Flags: review?(bugs)
Comment on attachment 8525294 [details] [diff] [review]
Patch 2(v1): Mochitest.

Could you please test also the case when .preventDefault() isn't called for
mozbrowserbeforekeydown.
So basically run the same test twice, first with preventDefault() and nothing should happen, and then without preventDefault() and something should happen.
Attachment #8525294 - Flags: review?(bugs) → review-
Comment 14 is addressed.
Attachment #8525294 - Attachment is obsolete: true
Attachment #8525812 - Flags: review?(bugs)
Comment on attachment 8525812 [details] [diff] [review]
Patch 2(v2): Mochitest.

>+function testDefaultAction()
>+{
>+  synthesizeKey('VK_END', {}, document.getElementById("embedded").contentWindow);
>+  setTimeout(function () {
>+    is(kTests[gCurrentTest].expectedEvents,
>+       kTests[gCurrentTest].resultEvents,
>+       "verify result");
>+    runTests();
>+  }, 100);
This is racy. Nothing guarantees 100ms timer will run after the next animation frame (which will trigger the possible scroll event).
But of course testing that something did not (and will not) happen is hard. Perhaps increase timer to 500ms and push this to try and trigger this there on
different platforms for few times, please.

Thanks
Attachment #8525812 - Flags: review?(bugs) → review+
set the timeout to 500ms and push to try:

https://tbpl.mozilla.org/?tree=Try&rev=e39ee5cdd6a6
Target Milestone: --- → 2.2 S1 (5dec)
You need to log in before you can comment on or make changes to this bug.