Closed
Bug 1067367
Opened 10 years ago
Closed 10 years ago
tapping the icon of a second doorhanger reopens the first doorhanger if it was already open
Categories
(Toolkit Graveyard :: Notifications and Alerts, defect)
Tracking
(firefox33 verified, firefox34 verified, firefox35 verified)
VERIFIED
FIXED
mozilla35
People
(Reporter: florian, Assigned: florian)
References
Details
(Whiteboard: [screensharing-uplift])
Attachments
(1 file)
(deleted),
patch
|
enndeakin
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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+
Assignee | ||
Comment 1•10 years ago
|
||
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)
Updated•10 years ago
|
Iteration: --- → 35.1
Updated•10 years ago
|
Iteration: 35.1 → 35.2
Updated•10 years ago
|
QA Contact: drno
Assignee | ||
Comment 2•10 years ago
|
||
Note: I don't see any obvious way to write a test for this, as the popupnotification tests run with the transitions disabled.
Comment 3•10 years ago
|
||
I'm going to check in bug 1006040, so should now be able to just use hidePopup(true) to prevent the transition.
Assignee | ||
Comment 4•10 years ago
|
||
(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 5•10 years ago
|
||
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+
Assignee | ||
Comment 7•10 years ago
|
||
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 8•10 years ago
|
||
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+
Comment 9•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/2a65f73d9f4c https://hg.mozilla.org/releases/mozilla-beta/rev/3ff9831143fd
Comment 10•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b5a225b162bb
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Assignee | ||
Comment 11•10 years ago
|
||
(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).
Comment 12•10 years ago
|
||
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 13•10 years ago
|
||
Comment on attachment 8489365 [details] [diff] [review] Fix >+ let transitionsEnabled = this.transitionsEnabled; There's no transitionsEnabled getter in PopupNotifications.jsm!
Assignee | ||
Comment 14•10 years ago
|
||
(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.
Comment 15•10 years ago
|
||
(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.
Updated•1 year ago
|
Product: Toolkit → Toolkit Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•