692071 - The Context Menu and Awesomelist are dismissed in the same time, if a tap is performed outside the Context Menu UI

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Cristian Nicolae (:xti)]

Build ID: Mozilla/5.0 (Android;Linux armv7l;rv:9.0a2)Gecko/20111004
Firefox/9.0a2 Fennec/9.0a2
Device: Samsung Galaxy S
OS: Android 2.2

Build ID: Mozilla/5.0 (Android;Linux armv7l;rv:10.0a1)Gecko/20111004
Firefox/10.0a1 Fennec/10.0a1
Device: Samsung Galaxy S
OS: Android 2.2

Steps to reproduce:
1. Open Fennec App
2. Tap on URL Bar for Awesomescreen
3. Long tap on any page for the Context Menu
4. Tap outside the Context Menu UI to dismiss it

Expected result:
After step 4, the Context Menu is dismissed and the Awesomescreen remains on the screen.

Actual result:
After step 4, the Context Menu and the Awesomescreen are dismissed in the same time.

Comment 1

[Aaron Train [:aaronmt]]

Aurora is also affected
Mozilla/5.0 (Android; Linux armv7l; rv:9.0a2) Gecko/20111005 Firefox/9.0a2 Fennec/9.0a2

Comment 2

[Lucas Rocha (:lucasr)]

Attachment: Keep awesome panel open when context menu is dismissed

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 564856 [details] [diff] [review]
Keep awesome panel open when context menu is dismissed

Why isn't handleEscape handling this correctly?

http://mxr.mozilla.org/mozilla-central/source/mobile/chrome/content/browser-ui.js#850

Comment 4

[Matt Brubeck (:mbrubeck)]

(In reply to Mark Finkle (:mfinkle) from comment #3)
> Why isn't handleEscape handling this correctly?

This bug happens when the popup is dismissed by tapping, not when it's dismissed by the escape key.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 564856 [details] [diff] [review]
Keep awesome panel open when context menu is dismissed

* _hasPopup -> _popupShowing
* Should we worry that _popupShowing is not ref counted and could be messed up by more than 1 popup open?

r+, but fix the nit and think about the question

do we have any tests for this situation?

Comment 6

[Cristian Nicolae (:xti)]

This issue doesn't occur on:
Build ID : Mozilla /5.0 (Android;Linux armv7l;rv:9.0a1) Gecko/20110902 Firefox/9.0a1 Fennec/9.0a1 
http://hg.mozilla.org/mozilla-central/rev/0664108eb19d

but it occurs on:
Build ID : Mozilla /5.0 (Android;Linux armv7l;rv:9.0a1) Gecko/20110903 Firefox/9.0a1 Fennec/9.0a1
http://hg.mozilla.org/mozilla-central/rev/472716252ea3

Possible range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0664108eb19d&tochange=472716252ea3

Comment 7

[Lucas Rocha (:lucasr)]

(In reply to Mark Finkle (:mfinkle) from comment #5)
> Comment on attachment 564856 [details] [diff] [review] [diff] [details] [review]
> Keep awesome panel open when context menu is dismissed
> 
> * _hasPopup -> _popupShowing
> * Should we worry that _popupShowing is not ref counted and could be messed
> up by more than 1 popup open?
> 
> r+, but fix the nit and think about the question

My understanding of popup handling in browser is that there's always only one popup open at a time. When a new popup is pushed, the previous one is dismissed. So, we only need to care if there's a popup on top of awesome panel or not.

> do we have any tests for this situation?

No. Writing one now.

Comment 8

[Lucas Rocha (:lucasr)]

Attachment: (1/3) Keep awesome panel open when context menu is dismissed

Same patch with nits fixed. Keeping the review+.

Comment 9

[Lucas Rocha (:lucasr)]

Attachment: (2/3) Ensure awesome panel is closed before each awesome screen test

Comment 10

[Lucas Rocha (:lucasr)]

Attachment: (3/3) Add browser test for popup being dismissed on top of awesome panel

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 565209 [details] [diff] [review]
(3/3) Add browser test for popup being dismissed on top of awesome panel

Just add a comment about using setTimeout to simulate a long tap. We try to avoid using setTimeout with non-zero values, but in this case it's kinda needed.

Comment 12

[Lucas Rocha (:lucasr)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/e5cfda41f2e8
http://hg.mozilla.org/integration/mozilla-inbound/rev/2d17b3815e1c
http://hg.mozilla.org/integration/mozilla-inbound/rev/4749a3d2d66b

Comment 13

[Martijn Wargers (dead)]

Oops, a little bit too early to set that flag yet.

Comment 14

[Matt Brubeck (:mbrubeck)]

No, you can go ahead and set in-testsuite+ when pushing to inbound -
https://wiki.mozilla.org/Inbound_Sheriff_Duty

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e5cfda41f2e8
https://hg.mozilla.org/mozilla-central/rev/2d17b3815e1c
https://hg.mozilla.org/mozilla-central/rev/4749a3d2d66b

Comment 16

[Catalin Suciu [:csuciu]]

This issue is verified fixed on Nightly build:
Mozilla/5.0 (Android;Linux armv7l;rv:10.0a1)Gecko/20111009 Firefox/10.0a1 Fennec/10.0a1
Device: HTC Desire
OS: Android 2.2