1085691 - Conversation window is missing orange camera icon if the window is not focused when gUM happens

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect, P1)

Description

[Nils Ohlmeier [:drno]]

Attachment: Screen Shot 2014-10-20 at 3.24.52 PM.png

If a call gets placed by clicking on the camera icon next to an existing contact and the call gets established, the callers conversation window is missing the orange camera icon at the top of the conversation window as shown in the attached screen shot.
The callee shows the icon properly.

Comment 1

[Nils Ohlmeier [:drno]]

Attachment: Screen Shot 2014-10-20 at 3.25.52 PM.png

This call is from Aurora to Nightly, showing that the problem also exist on Aurora.

Comment 2

[Nils Ohlmeier [:drno]]

Nightly version: 36.0a1 (2014-10-20)
Aurora version: 35.0a2 (2014-10-20)

Note: resizing the conversation window with the little upper right corner arrow results in the orange camera icon showing up. If the scaled up window gets scaled down again by clicking on the lower left corner arrow the camera icon is also present in the small sized conversation window.

Comment 3

[Nils Ohlmeier [:drno]]

Attachment: Screen Shot 2014-10-20 at 3.32.39 PM.png

This screen shot shows callee and caller missing the orange camera icon. Apparently the rendering of the orange camera icon has something to do with Fx being in the focus.
As I'm testing this with two browsers I have to bring the callee's browser into focus to accept in the incoming call. If I click on the green accept call icon and then switch the focus immediately back to the origination browser both conversation windows are missing the orange icon as shown in the attached screen shot.

Comment 4

[u279076]

Does this also affect Beta?

Comment 5

[Nils Ohlmeier [:drno]]

Yes Beta is affected as well.

While testing this I confirmed that the icon gets rendered if you accept the call and leave the browser in focus until the orange icon shows up. If another application gets the focus before the orange icon got rendered the icon is missing from the conversation window.

Comment 6

[Maire Reavy [:mreavy]]

What happens if you click the icon at the top of the screen (the one which is always there)?

Comment 7

[:shell escalante]

Hi Florian, we suspected this may be a UX behavior from desktop that is bleeding through.  Can you look at the scenario and the screen shots and see if you have any ideas what may be factoring in?

Comment 8

[Florian Quèze [:florian]]

Usually, the PopupNotifications code will only show icons when it believes the related tab is selected. I'm not sure if SocialAPI has any tweak to that behavior.

Are you saying the camera icon isn't shown until the Firefox window is focused, or that it's never shown (even after focusing the window) if the window wasn't focused at the time the icon should have appeared?

Comment 9

[:shell escalante]

at stand-up - Mark and Maire were going to add to this bug.

Comment 10

[Mark Banner (:standard8)]

(In reply to Florian Quèze [:florian] [:flo] from comment #8)
> Usually, the PopupNotifications code will only show icons when it believes
> the related tab is selected. I'm not sure if SocialAPI has any tweak to that
> behavior.
> 
> Are you saying the camera icon isn't shown until the Firefox window is
> focused, or that it's never shown (even after focusing the window) if the
> window wasn't focused at the time the icon should have appeared?

It is never shown. The permission prompt is not available to be clicked on as well.

You're right, that it is a focus issue. If I switch which window is focused just after accepting the call, then I'm able to "move" the issue to the incoming window rather than the outgoing window.

Comment 11

[Florian Quèze [:florian]]

(In reply to Mark Banner (:standard8) from comment #10)

> It is never shown. The permission prompt is not available to be clicked on
> as well.

I thought you skipped the permission prompts for Loop windows? If you do have permission prompts, bug 1000253 may be related.

Comment 12

[Mark Banner (:standard8)]

(In reply to Florian Quèze [:florian] [:flo] from comment #11)
> I thought you skipped the permission prompts for Loop windows? If you do
> have permission prompts, bug 1000253 may be related.

We skip the "accept" permission part of the prompt. The camera icon still gets displayed, and the user can stop sharing.

Comment 13

[Maire Reavy [:mreavy]]

Florian -- Do you think you could take this bug?  (You are the best person to fix this.)

Comment 14

[Florian Quèze [:florian]]

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #13)
> Florian -- Do you think you could take this bug?  (You are the best person
> to fix this.)

In the future yes, right now no. Which time frame are you considering here?

Comment 15

[Maire Reavy [:mreavy]]

I was hoping to fix this ASAP because I'm concerned that it could be confusing to a user (though the big indicator at the top is the source of truth).  

I'm wondering if it would be better (less confusing) to remove the second indicator (the orange camera icon) until we've fixed this bug.

Comment 16

[Maire Reavy [:mreavy]]

Hi Shane -- After talking with Florian and Mark, I suspect this is a social api bug.  Would you have any cycles to look at this?

Comment 17

[Shane Caraveo (:mixedpuppy)]

The problem is here:

https://mxr.mozilla.org/mozilla-central/source/toolkit/modules/PopupNotifications.jsm#333

notification.dismissed is true in the caller, thus the notification icon is not added.  I'm not sure why notification.dismissed is true since the notification was never shown, but I suspect it is in loop and/or webrtc code vs. specifically being a socialapi issue.  Are you passing dismissed as an option to PopupNotifications.show()?  Looks like that is the case, browser/modules/webrtcUI.jsm sets dismissed to true in several places.  I haven't followed all the logic, someone who knows this code should dig in.

Comment 18

[Shane Caraveo (:mixedpuppy)]

ie. not a socialapi bug as far as I can tell

Comment 19

[Florian Quèze [:florian]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #17)
> The problem is here:
> 
> https://mxr.mozilla.org/mozilla-central/source/toolkit/modules/
> PopupNotifications.jsm#333
> 
> notification.dismissed is true in the caller, thus the notification icon is
> not added.

dismissed means that the icon should be added without throwing a panel at the user. The documentation says "dismissed:   Whether the notification should be added as a dismissed notification. Dismissed notifications can be activated by clicking on their anchorElement."
https://mxr.mozilla.org/mozilla-central/source/toolkit/modules/PopupNotifications.jsm#232

The webrtc code intentionally sets dismissed to true because we are not asking the user anything; just showing an icon from which the user can open a panel to control the ongoing device sharing.

Comment 20

[Shane Caraveo (:mixedpuppy)]

(In reply to Florian Quèze [:florian] [:flo] from comment #19)
> The documentation says "dismissed:   Whether the notification
> should be added as a dismissed notification. Dismissed notifications can be
> activated by clicking on their anchorElement.

In that case, seems like that if statement is incorrect.  Does this affect webrtc in a tab?

Comment 21

[Florian Quèze [:florian]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #20)
> (In reply to Florian Quèze [:florian] [:flo] from comment #19)
> > The documentation says "dismissed:   Whether the notification
> > should be added as a dismissed notification. Dismissed notifications can be
> > activated by clicking on their anchorElement.
> 
> In that case, seems like that if statement is incorrect.  Does this affect
> webrtc in a tab?

From reading that specific code, I see nothing SocialAPI-specific, so I would assume the icon also fails to appear if a tab with persistent permissions grabs the device while the browser window is not focused. I would hope the icon appears once the browser window is focused, but I would need to make a testcase to confirm if it actually does.

Comment 22

[Shane Caraveo (:mixedpuppy)]

Attachment: show icon updates regardless of dismissed state

This fixes the issue, and the code being moved is from the initial chat window work in bug 809085.  The getattention shouldn't be called per bug 882188.

Comment 23

[Florian Quèze [:florian]]

Comment on attachment 8522537 [details] [diff] [review]
show icon updates regardless of dismissed state

Looks like this error was introduced at http://hg.mozilla.org/mozilla-central/rev/b96decc34910#l10.73 and yes, that's from the initial SocialAPI changes to this file.

The patch makes sense to me. Is this the kind of feedback you expect, or do you need me to actually try the patch?

Comment 24

[Shane Caraveo (:mixedpuppy)]

(In reply to Florian Quèze [:florian] [:flo] from comment #23)
> Comment on attachment 8522537 [details] [diff] [review]
> show icon updates regardless of dismissed state
> 
> Looks like this error was introduced at
> http://hg.mozilla.org/mozilla-central/rev/b96decc34910#l10.73 and yes,
> that's from the initial SocialAPI changes to this file.
> 
> The patch makes sense to me. Is this the kind of feedback you expect, or do
> you need me to actually try the patch?

Try it, and is there any problem you can anticipate with this patch?

Comment 25

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8522537 [details] [diff] [review]
show icon updates regardless of dismissed state

@dolske or @jaws

Comment 26

[Justin Dolske [:Dolske]]

Comment on attachment 8522537 [details] [diff] [review]
show icon updates regardless of dismissed state

Review of attachment 8522537 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/modules/PopupNotifications.jsm
@@ +338,1 @@
>        }

This is fine, but I think you're the straw that broke the refactoring camel's back here.

I think it would be helpful to clean this chunk up as such... (Basically adding isActiveWindow for clarity, moving the two isActive check up, and lopping off most of the overly verbose comment.)

    let isActiveBrowser = this._isActiveBrowser(browser);
    let fm = Cc["@mozilla.org/focus-manager;1"].getService(Ci.nsIFocusManager);
    let isActiveWindow = fm.activeWindow == this.window;

    if (isActiveBrowser && isActiveWindow) {
      // show panel now
      this._update(notifications, notification.anchorElement, true);
    } else if (isActiveBrowser && !isActiveWindow) {
      // We're a background window, panel will be shown upon focus.

      if (!notification.dismissed) {
        this.window.getAttention();
      }
      if (notification.anchorElement.parentNode != this.iconBox) {
        this._updateAnchorIcon(notifications, notification.anchorElement);
      }
      this._notify("backgroundShow");
    } else {
      // We're not the active browser.
      this._notify("backgroundShow");
    }

Comment 27

[Shane Caraveo (:mixedpuppy)]

Attachment: show icon updates regardless of dismissed state

refactored per review suggestion.  try run:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1ff37080cd0e

Comment 28

[Shane Caraveo (:mixedpuppy)]

https://hg.mozilla.org/integration/fx-team/rev/297245ad4574

Comment 29

[Shane Caraveo (:mixedpuppy)]

@mreavy: Is this fix necessary for beta?

Comment 30

[Maire Reavy [:mreavy]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #29)
> @mreavy: Is this fix necessary for beta?

No, but I do need it for Fx 35 (Aurora).  Thanks, Shane!

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/297245ad4574

Comment 32

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8523987 [details] [diff] [review]
show icon updates regardless of dismissed state

Approval Request Comment
[Feature/regressing bug #]: 809085
[User impact if declined]: user could end up in a situation where they do not have access to camera/microphone permissions
[Describe test coverage new/current, TBPL]: existing test coverage passes in tbpl
[Risks and why]: low, just minor logic change
[String/UUID change made/needed]: none

Comment 33

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 34

[Ada [:adalucinet]]

Reproduced with Nightly 2014-10-23.
Verified as fixed with Aurora 36.0a2 (Build ID: 20141218004002) and Firefox 35 beta 4 (Build ID: 20141216120925) on Mac OS X 10.9.5 - the orange camera icon is present.