Closed Bug 1067367 Opened 6 years ago Closed 6 years ago

tapping the icon of a second doorhanger reopens the first doorhanger if it was already open

Categories

(Toolkit :: Notifications and Alerts, defect)

x86
Windows 7
defect
Not set
normal
Points:
2

Tracking

()

VERIFIED FIXED
mozilla35
Iteration:
35.2
Tracking Status
firefox33 --- verified
firefox34 --- verified
firefox35 --- verified

People

(Reporter: florian, Assigned: florian)

References

Details

(Whiteboard: [screensharing-uplift])

Attachments

(1 file)

Steps to reproduce:
1. Load http://people.mozilla.org/~fqueze2/webrtc/
2. In about:config, add people.mozilla.org in the media.getusermedia.screensharing.allowed_domains list.
3. Click the "Screen & Audio" button.
4. Share a microphone and your screen.
5. See an orange microphone icon and an orange screensharing icon in the URL bar.
6. Click the microphone icon, a doorhanger saying "You are currently sharing your microphone with this page." appears.
7. Tap (ie. a brief touch of a laptop touchpad; not a click) the screensharing icon.

Expected result:
The microphone doorhanger closes and the screensharing doorhanger (saying "You are currently sharing your screen with this page.") appears.

Actual result:
The microphone doorhanger is closed then reopened.

Notes:
- I could reproduce on Mac and Windows. I think doorhanger CSS transitions are currently disabled on Linux, so the bug may not be reproducible on Linux.
- I can reproduce most of the times but not all the time.
- Doing a real click (which lets some code get executed between the mousedown and mouseup events) doesn't trigger the bug.
- Marking as screensharing-uplift because this makes using the screensharing url bar icon difficult on Windows/Mac.
Flags: qe-verify+
Flags: firefox-backlog+
Attached patch FixSplinter Review
Here is what's happening:
- The popuphidden handler marks all the currently shown notifications as dismissed and then calls update.
- The click handler causes the clicked notification to be (re)shown.
- If the click handler is fired before the panel (dismissed on mousedown) is done closing, the panel marked as dismissed by the popuphidden handler is the one the user clicked (which was marked as shown by the click handler), and then update causes the first notification to be reshown.

Disabling the closing transition ensures the popuphidden event is received (and correctly ignored due to _ignoreDismissal) before we attempt to reshow another notification.
Attachment #8489365 - Flags: review?(enndeakin)
Blocks: 1056179
Blocks: 610545
Iteration: --- → 35.1
Iteration: 35.1 → 35.2
QA Contact: drno
Note: I don't see any obvious way to write a test for this, as the popupnotification tests run with the transitions disabled.
I'm going to check in bug 1006040, so should now be able to just use hidePopup(true) to prevent the transition.
(In reply to Neil Deakin from comment #3)
> I'm going to check in bug 1006040, so should now be able to just use
> hidePopup(true) to prevent the transition.

Assuming you mean:

diff --git a/toolkit/modules/PopupNotifications.jsm b/toolkit/modules/PopupNotifications.jsm
--- a/toolkit/modules/PopupNotifications.jsm
+++ b/toolkit/modules/PopupNotifications.jsm
@@ -490,5 +490,5 @@ PopupNotifications.prototype = {
   _hidePanel: function PopupNotifications_hide() {
     this._ignoreDismissal = true;
-    this.panel.hidePopup();
+    this.panel.hidePopup(true);
     this._ignoreDismissal = false;
   },

then this doesn't prevent the transition (and so doesn't fix the bug here). Note: I tried in a current local build that contains the patch from bug 1006040.
Comment on attachment 8489365 [details] [diff] [review]
Fix

You're right. I for some reason has this backwards. Let's do this for now.
Attachment #8489365 - Flags: review?(enndeakin) → review+
Comment on attachment 8489365 [details] [diff] [review]
Fix

Approval Request Comment
[Feature/regressing bug #]: screensharing, more specifically bug 1037405.
[User impact if declined]: users who tap (instead of actually clicking) a touchpad may have a hard time getting the screensharing doorhanger to open.
[Describe test coverage new/current, TBPL]: hoping QA will verify the fix this week.
[Risks and why]: very low risk: only disabling a CSS transition.
[String/UUID change made/needed]: none.
Attachment #8489365 - Flags: approval-mozilla-beta?
Attachment #8489365 - Flags: approval-mozilla-aurora?
Comment on attachment 8489365 [details] [diff] [review]
Fix

Approving even it is not in m-c to make sure it is in beta 8
Attachment #8489365 - Flags: approval-mozilla-beta?
Attachment #8489365 - Flags: approval-mozilla-beta+
Attachment #8489365 - Flags: approval-mozilla-aurora?
Attachment #8489365 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/b5a225b162bb
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
(In reply to Florian Quèze [:florian] [:flo] from comment #0)
> Steps to reproduce:
> 1. Load http://people.mozilla.org/~fqueze2/webrtc/

This should be: https://people.mozilla.org/~fqueze2/webrtc/ (ie. load the page over https to be able to use screensharing).
I took this bug for verification. 

Reproduced the issue on MacBook Pro OSX 10.9.4 on Nightly 2014-09-15. 

Verified fixed on MacBook Pro OSX 10.9.4 using latest Nightly (buildID: 20140929030205), latest Aurora (buildID: 20140929004005) and Firefox 33 Beta 8 (buildID: 20140929180120).
Status: RESOLVED → VERIFIED
QA Contact: drno → camelia.badau
Comment on attachment 8489365 [details] [diff] [review]
Fix

>+    let transitionsEnabled = this.transitionsEnabled;
There's no transitionsEnabled getter in PopupNotifications.jsm!
(In reply to neil@parkwaycc.co.uk from comment #13)
> Comment on attachment 8489365 [details] [diff] [review]
> Fix
> 
> >+    let transitionsEnabled = this.transitionsEnabled;
> There's no transitionsEnabled getter in PopupNotifications.jsm!

We noticed (a bit late though :-/), see bug 1079303.
(In reply to Florian Quèze from comment #14)
> (In reply to comment #13)
> > There's no transitionsEnabled getter in PopupNotifications.jsm!
> 
> We noticed (a bit late though :-/), see bug 1079303.

Whoops, sorry I hadn't noticed the dependent bug.
You need to log in before you can comment on or make changes to this bug.