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)
Tracking
()
RESOLVED
FIXED
2.2 S1 (5dec)
People
(Reporter: dwi2, Assigned: gyeh)
References
Details
(Whiteboard: [ft:conndevices])
Attachments
(2 files, 2 obsolete files)
1.20 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
6.11 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 1•10 years ago
|
||
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.
Reporter | ||
Comment 2•10 years ago
|
||
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)
Reporter | ||
Comment 3•10 years ago
|
||
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.
Assignee | ||
Comment 4•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Component: Gaia::System → DOM: Events
Product: Firefox OS → Core
Assignee | ||
Updated•10 years ago
|
Summary: 'Home' button event should not dispatch into browser iframe → No default actions of keydown event should be made if mozbrowserbeforekeydown event is defaultPrevented
Assignee | ||
Comment 5•10 years ago
|
||
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 6•10 years ago
|
||
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)
Assignee | ||
Comment 9•10 years ago
|
||
Should we land this with a test? Can we file a follow-up for adding test?
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(gyeh)
Whiteboard: [ft:conndevices]
A test would be nice, yes.
Assignee | ||
Comment 11•10 years ago
|
||
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #8525293 -
Attachment is obsolete: true
Assignee | ||
Comment 13•10 years ago
|
||
Comment on attachment 8525294 [details] [diff] [review] Patch 2(v1): Mochitest. Please review the test. Thanks.
Attachment #8525294 -
Flags: review?(bugs)
Comment 14•10 years ago
|
||
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.
Updated•10 years ago
|
Attachment #8525294 -
Flags: review?(bugs) → review-
Assignee | ||
Comment 15•10 years ago
|
||
Comment 14 is addressed.
Attachment #8525294 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8525812 -
Flags: review?(bugs)
Comment 16•10 years ago
|
||
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+
Assignee | ||
Comment 17•10 years ago
|
||
set the timeout to 500ms and push to try: https://tbpl.mozilla.org/?tree=Try&rev=e39ee5cdd6a6
Assignee | ||
Updated•10 years ago
|
Target Milestone: --- → 2.2 S1 (5dec)
Assignee | ||
Comment 18•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1043bff752db https://hg.mozilla.org/integration/mozilla-inbound/rev/b7d1ec54b2ef
https://hg.mozilla.org/mozilla-central/rev/1043bff752db https://hg.mozilla.org/mozilla-central/rev/b7d1ec54b2ef
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•