Closed Bug 1085691 Opened 6 years ago Closed 6 years ago

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

Categories

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

x86
macOS
defect

Tracking

(firefox34 wontfix, firefox35 verified, firefox36 verified)

VERIFIED FIXED
mozilla36
Tracking Status
firefox34 --- wontfix
firefox35 --- verified
firefox36 --- verified
Blocking Flags:
backlog Fx35+

People

(Reporter: drno, Assigned: mixedpuppy)

Details

Attachments

(4 files, 1 obsolete file)

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.
This call is from Aurora to Nightly, showing that the problem also exist on Aurora.
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.
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.
Does this also affect Beta?
Summary: Callers conversation window is missing orange camera icon → Conversation window is missing orange camera icon if not in focus
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.
What happens if you click the icon at the top of the screen (the one which is always there)?
backlog: --- → Fx35+
Priority: -- → P1
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?
Flags: needinfo?(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?
Flags: needinfo?(florian)
at stand-up - Mark and Maire were going to add to this bug.
Flags: needinfo?(standard8)
Flags: needinfo?(mreavy)
(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.
Flags: needinfo?(standard8)
Summary: Conversation window is missing orange camera icon if not in focus → Conversation window is missing orange camera icon if the window is not focused when gUM happens
(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.
(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.
Florian -- Do you think you could take this bug?  (You are the best person to fix this.)
Flags: needinfo?(mreavy) → needinfo?(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?
Flags: needinfo?(florian)
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.
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?
Flags: needinfo?(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.
Flags: needinfo?(mixedpuppy)
ie. not a socialapi bug as far as I can tell
(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.
(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?
(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.
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.
Attachment #8522537 - Flags: feedback?(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?
Attachment #8522537 - Flags: feedback?(florian) → feedback+
(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 on attachment 8522537 [details] [diff] [review]
show icon updates regardless of dismissed state

@dolske or @jaws
Attachment #8522537 - Flags: review?(jaws)
Attachment #8522537 - Flags: review?(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");
    }
Attachment #8522537 - Flags: review?(jaws)
Attachment #8522537 - Flags: review?(dolske)
Attachment #8522537 - Flags: review+
refactored per review suggestion.  try run:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1ff37080cd0e
Assignee: nobody → mixedpuppy
Attachment #8522537 - Attachment is obsolete: true
Attachment #8523987 - Flags: review+
@mreavy: Is this fix necessary for beta?
Flags: needinfo?(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!
Flags: needinfo?(mreavy)
https://hg.mozilla.org/mozilla-central/rev/297245ad4574
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
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
Attachment #8523987 - Flags: approval-mozilla-aurora?
Attachment #8523987 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: qe-verify+
Flags: in-testsuite?
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.