1056179 - WebRTC global indicator fails to open the sharing doorhanger if there's another notification before it

Status: VERIFIED FIXED

(Firefox :: Site Permissions, defect)

Description

[alex_mayorga]

Pre-requisites:
- Plug an external monitor
- Move the Nightly window to secondary monitor

Steps:
1 Load https://appear.in/nightly
2 Click "Share Selected Devices" button

Result:
An orange indicator appears on the primary monitor and does nothing when clicked.

Expected result:
The orange indicator is clickable and lets you control sharing.

Comment 1

[Florian Quèze [:florian]]

(In reply to alex_mayorga from comment #0)

> An orange indicator appears on the primary monitor and does nothing when
> clicked.

Is there an error message in the browser console? ("Tools -> Web Developer -> Browser Console" on Mac; not sure where it is exactly on Windows.)

Comment 2

[:Gijs (he/him)]

(In reply to Florian Quèze [:florian] [:flo] from comment #1)
> (In reply to alex_mayorga from comment #0)
> 
> > An orange indicator appears on the primary monitor and does nothing when
> > clicked.
> 
> Is there an error message in the browser console? ("Tools -> Web Developer
> -> Browser Console" on Mac; not sure where it is exactly on Windows.)

Comment 3

[alex_mayorga]

The relevant bits from "Browser Console":

Unknown property '-moz-font-smoothing'.  Declaration dropped. f35ccfdc.main.css:1
Unknown property 'font-smoothing'.  Declaration dropped. f35ccfdc.main.css:1
Unknown property 'speak'.  Declaration dropped. f35ccfdc.main.css:1
Unknown property 'background-position-y'.  Declaration dropped. f35ccfdc.main.css:1
Error in parsing value for 'display'.  Declaration dropped. f35ccfdc.main.css:1
Unknown property 'webkit-flex-wrap'.  Declaration dropped. f35ccfdc.main.css:1
Unknown pseudo-class or pseudo-element '-webkit-full-screen'.  Ruleset ignored due to bad selector. f35ccfdc.main.css:1
Unknown pseudo-class or pseudo-element 'full-screen'.  Ruleset ignored due to bad selector. f35ccfdc.main.css:1
Error in parsing value for 'top'.  Declaration dropped. f35ccfdc.main.css:1
Unknown property 'touch-callout'.  Declaration dropped. f35ccfdc.main.css:1
Unknown property 'user-select'.  Declaration dropped. f35ccfdc.main.css:1
Error in parsing value for 'background'.  Declaration dropped. f35ccfdc.main.css:1
Unknown pseudo-class or pseudo-element '-webkit-input-placeholder'.  Ruleset ignored due to bad selector. f35ccfdc.main.css:1
Unknown pseudo-class or pseudo-element '-ms-input-placeholder'.  Ruleset ignored due to bad selector. f35ccfdc.main.css:1
Unknown pseudo-class or pseudo-element 'input-placeholder'.  Ruleset ignored due to bad selector. f35ccfdc.main.css:1
Error in parsing value for 'max-height'.  Declaration dropped. f35ccfdc.main.css:1
Error in parsing value for '-moz-transition'.  Declaration dropped. f35ccfdc.main.css:1
Error in parsing value for 'transition'.  Declaration dropped. f35ccfdc.main.css:1
Unknown property '-moz-osx-font-smoothing'.  Declaration dropped. font-awesome.min.css:4
Expected 'none' or URL but found 'progid'.  Error in parsing value for 'filter'.  Declaration dropped. font-awesome.min.css:4
"This appears to be Firefox" dedb9407.main.js:5
"ScreenShareAction: check-extension message: " undefined angular.min.js:86
"ChromeNotifierAction: check-extension message: " undefined angular.min.js:86
Error in parsing value for 'background-color'.  Declaration dropped. nightly
Expected end of value but found '{'.  Error in parsing value for 'border'.  Declaration dropped. nightly
Invalid URI. Load of media resource  failed. nightly
"ChromeNotifierAction: visited-room-data message: " Object { /pruebamia: Object, /alex_mayorga: Object, /nightly: Object } angular.min.js:86
"Updating stream: {stream.id: 0 stream.streamId: undefined}" angular.min.js:86
"Attaching media stream" dedb9407.main.js:5
Unknown property '-moz-border-radius'.  Declaration dropped. main.css:12
Unknown property '-moz-box-shadow'.  Declaration dropped. main.css:30
Unknown property '-moz-box-shadow'.  Declaration dropped. main.css:35
Unknown property '-moz-osx-font-smoothing'.  Declaration dropped. font-awesome.min.css:4
Expected 'none' or URL but found 'progid'.  Error in parsing value for 'filter'.  Declaration dropped. font-awesome.min.css:4
Expected media feature name but found 'touch-enabled'. appearin:1
Expected media feature name but found '-webkit-touch-enabled'. appearin:1
Expected media feature name but found '-o-touch-enabled'. appearin:1
Expected media feature name but found '-ms-touch-enabled'. appearin:1
Expected media feature name but found 'modernizr'. appearin:1
Unknown property '-moz-opacity'.  Declaration dropped. appearin
Error in parsing value for 'background-image'.  Declaration dropped. appearin
Expected color but found 'left'.  Error in parsing value for 'background-image'.  Declaration dropped. appearin

Comment 4

[:Gijs (he/him)]

Can you take a screenshot of the indicator, and can you clear the console, then click the indicator, and then check if anything appears there?

Comment 5

[alex_mayorga]

Attachment: webrtc-controls-multi-monitor.png

Expected media feature name but found '-webkit-min-device-pixel-ratio'. from DOM
Expected media feature name but found '-o-min-device-pixel-ratio'. from DOM
Expected media feature name but found 'min-device-pixel-ratio'. from DOM

Comment 6

[:Gijs (he/him)]

I'm confused. Is the indicator at the bottom of the screen? :-\

So the desired behaviour is that clicking the microphone should focus the Firefox window and drop down the doorhanger. Does it do neither of those things? (the errors you pasted are not related)

Comment 7

[alex_mayorga]

Attachment: webrtc-indicator.PNG

I'm referring to the white on orange button next to the Nightly logo.

Clicking it just make the Nightly window flash for a split second here.

Comment 8

[u279076]

I think what Alex is referring to here is the overlay icon that appears on the screen, outside of the Firefox window. When you load https://appear.in/nightly and share your devices, you will get a yellow "camera" icon in the awesomebar. This icon only ever appears in the window that instantiated it (this is normal AFAIK).

However, there is also a small overlay that appears at the top-center of the main screen. This overlay contains the application icon, a yellow camera icon, and a yellow microphone icon. Clicking this overlay triggers the sharing door hanger in the Firefox window that instantiated it. Even if the window is on the secondary monitor, this overlay will appear on the main screen.

Comment 9

[u279076]

I also checked on https://meet.tokbox.com/mozilla-qa and it does the same thing:

Device sharing overlay appears on the main screen and clicking it triggers the sharing doorhanger in the browser window on the screen where that window exists.

Comment 10

[:Gijs (he/him)]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #9)
> I also checked on https://meet.tokbox.com/mozilla-qa and it does the same
> thing:
> 
> Device sharing overlay appears on the main screen and clicking it triggers
> the sharing doorhanger in the browser window on the screen where that window
> exists.

Right. This is the expected behaviour. Alex is saying clicking the indicator doesn't do anything. It seems you can't reproduce that...

Comment 11

[alex_mayorga]

On https://meet.tokbox.com/mozilla-qa the indicator does show the door hanger when clicked and this shows on "Browser Console":

A promise chain failed to handle a rejection. Did you forget to '.catch', or did you forget to 'return'?
See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise

Date: Thu Aug 21 2014 16:15:05 GMT-0500 (Central Standard Time (Mexico))
Full Message: null

Comment 12

[:Gijs (he/him)]

(In reply to alex_mayorga from comment #11)
> On https://meet.tokbox.com/mozilla-qa the indicator does show the door
> hanger when clicked and this shows on "Browser Console":
> 
> A promise chain failed to handle a rejection. Did you forget to '.catch', or
> did you forget to 'return'?
> See
> https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/
> Promise
> 
> Date: Thu Aug 21 2014 16:15:05 GMT-0500 (Central Standard Time (Mexico))
> Full Message: null

What file name is associated with this error?

Anthony and Alex, do you perhaps have different equipment, ie one of you can/does only share a mic, and the other camera and microphone?

Alex, what if you test with: http://queze.net/goinfre/ff_gum_test.html ?

Comment 13

[alex_mayorga]

Attachment: click-to-play.PNG

Bingo!

I think I've found the trigger for this bug.

I've set Flash to be click-to-play.

https://appear.in/ has a Flash elements that throw things off.
https://meet.tokbox.com/ has no Flash elements

Comment 14

[Florian Quèze [:florian]]

Ah, so is the doorhanger not opening when there's also a plugin icon displayed in the url bar?

Is this problem really specific to having the window on a secondary screen?

Comment 15

[u279076]

I can reproduce this now too, with Flash set to click-to-play on https://appear.in

This also seems to happen for me if the window is on the main screen so dual monitors seems to be a moot point.

Comment 16

[Florian Quèze [:florian]]

Attachment: Patch

This fixes the bug reported here and another bug occuring with multiple notification icons shown at once.

Not requesting review yet because I haven't had time to verify that this doesn't break any test.

Comment 17

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] [:flo] from comment #16)
> and another bug occuring with multiple notification icons shown at once.

I moved this to its own bug: bug 1067367.

Comment 18

[Florian Quèze [:florian]]

Attachment: Patch v2

Here is what's happening:
- the notification.reshow call from webrtcUI.jsm causes _update from PopupNotifications to be called with a specific notification and anchor specified.
Because of this, the doorhanger is briefly (not long enough to be visible, unless one is stepping through the code using a debugger) shown (the "showing" and "shown" events are notified to the notification's event callback).
- the browser window is activated (due to the browserWindow.focus() call in webrtcUI.jsm), the "activate" event is received by the PopopNotifications code which then calls _update without parameter out of a setTimeout. Without any specified anchor, the _update code will pick the first one (the click to play one in this case), see that all the notifications attached to that anchor are dismissed, decide that there's nothing to show and close the panel; which will cause the webrtc doorhanger notification to be dismissed (a "dismissed" event is received by the notification's event callback).

I think a reasonable solution here is to make _update called without parameter be a little smarter about which anchor it picks, ie. pick the first anchor that has non-dismissed notifications attached to it.

Comment 19

[Florian Quèze [:florian]]

Alternative steps to reproduce that don't involve click-to-play:
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, and the global indicator popup window at the top of the screen.
6. Click the screen share icon of the global indicator.

Expected result:
The screensharing doorhanger should open.

Actual result:
The browser window gets focused, but the screensharing doorhanger isn't open.

Note: I only reproduced on Windows. Not sure if it's possible to reproduce on other OSes.

Note2: The reason why on https://appear.in/nightly the bug can only be reproduced if flash is blocked is that for some reason PopupNotifications._getNotificationsForBrowser returns the click-to-play notification before the webrtc one when flash is blocked, but after the webrtc notification when flash is allowed. I haven't further looked into what causes the ordering difference, but my guess is that it's related to in which order things are loaded.

Comment 20

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8489379 [details] [diff] [review]
Patch v2

Can you write a test for this?

Comment 21

[Florian Quèze [:florian]]

Attachment: Patch v2 (with test)

Comment 22

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8493780 [details] [diff] [review]
Patch v2 (with test)

Can you lower/raise the window rather than calling _update() directly in the test? I suppose that introduces greater change of intermittent failures :/

Comment 23

[Florian Quèze [:florian]]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #22)

> Can you lower/raise the window rather than calling _update() directly in the
> test? I suppose that introduces greater change of intermittent failures :/

Yes, that would cause more asynchronicity and more chances of intermittent failures. Especially as the 'activate' event is handled from a setTimeout, and lowering the window would dismiss the notifications.

Try was green on the new test: https://tbpl.mozilla.org/?tree=Try&rev=5845cf9f75a3

https://hg.mozilla.org/integration/fx-team/rev/44a820e40ec5

Comment 24

[Florian Quèze [:florian]]

Comment on attachment 8493780 [details] [diff] [review]
Patch v2 (with test)

Approval Request Comment
[Feature/regressing bug #]: The bug has been here for a long time, but it became visible with bug 1037408 that landed for Firefox 33.
[User impact if declined]: Clicking the icons in the global indicator may not show the sharing doorhanger if there's another notification icon before (eg. the click to play icon).
[Describe test coverage new/current, TBPL]: the patch contains a test.
[Risks and why]: small risk as there's a small behavior change. If we were earlier in the cycle, I would request beta approval too, as the bug is visible on 33.
[String/UUID change made/needed]: none.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/44a820e40ec5

Comment 26

[alex_mayorga]

Thanks for fixing this Florian =)

It now works on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0 ID:20140925030203 CSet: 1735ff2bb23e

Comment 27

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Thanks for verifying this Alex. Setting correct status flag.

Comment 28

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8493780 [details] [diff] [review]
Patch v2 (with test)

Aurora+

Comment 29

[Florian Quèze [:florian]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c9a8fd295d5b

Comment 30

[Catalin Varga [QA][:VarCat]]

Verified as fixed using:

FF 34.10b
Build Id: 20141117202603
OS: Win 7 x64