Last Comment Bug 691175 - Unable to back out of Awesome screen
: Unable to back out of Awesome screen
Status: VERIFIED FIXED
[pushed]
: regression
Product: Fennec Graveyard
Classification: Graveyard
Component: General (show other bugs)
: Firefox 10
: ARM Android
: -- normal (vote)
: Firefox 10
Assigned To: Matt Brubeck (:mbrubeck)
:
:
Mentors:
Depends on: 692123
Blocks: 684558
  Show dependency treegraph
 
Reported: 2011-10-02 11:17 PDT by Aaron Train [:aaronmt]
Modified: 2011-10-06 04:10 PDT (History)
5 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (4.19 KB, patch)
2011-10-03 10:29 PDT, Matt Brubeck (:mbrubeck)
mark.finkle: review+
Details | Diff | Splinter Review

Description Aaron Train [:aaronmt] 2011-10-02 11:17:06 PDT
Pressing my devices back button will not back out of the awesome screen; a recent regression.

STR:
1. New profile, start Fennec
2. Tap the Awesomebar
3. Tap your devices back button twice

ER: Back looking at about:home
AR: Stuck in the Awesome screen

--
Mozilla/5.0 (Android; Linux armv7l; rv:10.0a1) Gecko/20111002 Firefox/10.0a1 Fennec/10.0a1

Samsung Galaxy SII
Samsung Nexus S
Comment 1 Aaron Train [:aaronmt] 2011-10-03 06:34:30 PDT
Under the tablet UI, the panel remains open but one can tap outside of it to dismiss it

--
Mozilla/5.0 (Android; Linux armv7l; rv:10.0a1) Gecko/20111003 Firefox/10.0a1 Fennec/10.0a1

Samsung Galaxy Tab 10.1
Comment 3 Matt Brubeck (:mbrubeck) 2011-10-03 09:49:57 PDT
Regression from bug 684558.  Specifically, this broke because I changed the BrowserUI "keypress" handler from capture=true to capture=false.
Comment 4 Matt Brubeck (:mbrubeck) 2011-10-03 10:29:24 PDT
Created attachment 564230 [details] [diff] [review]
patch

This fixes the bug, and does not regress bug 684558 or bug 683736.

This also backs out a change I made to browser_awesomescreen.js that I think caught this bug but I thought it was a bug in the test.  Oops.  :(  Tests are passing on desktop except for browser_awesomescreen which is crashing on my desktop build for some unrelated reason (with or without this patch).  Pushed to try to test on device:
https://tbpl.mozilla.org/?tree=Try&rev=f09d8a58271b

It feels kind of hacky that we have to do this.  I'd like to file a followup bug on making the platform side of this better so that we don't need to do our own event re-dispatching (though I don't know exactly how that would work).  But this is no hackier than the code that was removed by bug 682017, so I think the best way forward is to stick with the platform event forwarding, fix this bug, and try to clean up the platform to eventually remove the need for these hacks.  (The alternative is to back out bug 682017 and all its dependencies and just go back to doing all the forwarding with JavaScript messages.)
Comment 5 Mark Finkle (:mfinkle) (use needinfo?) 2011-10-03 10:37:44 PDT
Comment on attachment 564230 [details] [diff] [review]
patch


>       case "keypress":
>+        // Ignore events headed toward the browser; they will be
>+        // re-dispatched after content has a chance to handle them.
>+        if (aEvent.target == Browser.selectedBrowser)
>+          break;

I suppose keypresses would only be targeted to the selected browser and not any browser. If we think it could be any browser, maybe use | aEvent.target.localName == "browser" | check instead
Comment 6 Matt Brubeck (:mbrubeck) 2011-10-03 12:56:55 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/2c0eb6a0e6e7
Comment 7 Matt Brubeck (:mbrubeck) 2011-10-03 16:51:02 PDT
https://hg.mozilla.org/mozilla-central/rev/2c0eb6a0e6e7
Comment 8 Carla Nadastean 2011-10-05 02:39:20 PDT
Retested bug with:
Mozilla/5.0 (Android; Linux armv7l; rv:10.0a1) Gecko/20111004 Firefox/10.0a1 Fennec/10.0a1

Devices: 
Samsung Nexus S (Android 2.3)
LG Optimus 2X (Android 2.2)

Bug is no longer reproducible. Back button is working properly after Awesome screen is displayed.

Verifying bug.

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