The default bug view has changed. See this FAQ.

implement the global getUserMedia indicator

VERIFIED FIXED in Firefox 33

Status

()

Firefox
Device Permissions
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: florian, Assigned: florian)

Tracking

(Depends on: 2 bugs)

Trunk
Firefox 33
Points:
13
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 3 obsolete attachments)

(Assignee)

Description

3 years ago
Created attachment 8454411 [details]
Global indicator mockup

With the addition of screensharing support in getUserMedia (bug 983504), we are concerned that the existing toolbar indicator (http://i.imgur.com/NHidncc.png) may not be visible enough, and we decided (in bug 1031424) to replace it with a global indicator always visible at the top of the screen.

This new global indicator should be a small floating window, on top of all other windows. It should be possible to drag it to the left or right of the screen, but it should stick to the top of the screen.

The attached mockup for this is a subset of attachment 8453837 [details].
(Assignee)

Updated

3 years ago
Blocks: 1037415
(Assignee)

Updated

3 years ago
Flags: firefox-backlog+
(Assignee)

Updated

3 years ago
Depends on: 1037418
(Assignee)

Comment 1

3 years ago
If we can't get this ready before 33 merges to aurora, we should think about pre-landing the strings. The changes here don't require many strings, they only need the strings for the tooltip that should be displayed when hovering the icons.

Philipp provided these strings in bug 1031424 comment 30, we then discussed a bit on IRC and agreed to drop the "with <hostname>"/"with <count> pages" part to simplify, as this is only tooltips that we don't expect to be the primary way of informing the user.

So the strings to use are:

Left side (camera and microphone):
Your camera and microphone are being shared. Click to control sharing.
Your microphone is being shared. Click to control sharing.
Your camera is being shared. Click to control sharing.

Right side (screen sharing):
Your screen is being shared. Click to control sharing.
The application <application name> is being shared. Click to control sharing.
(Assignee)

Updated

3 years ago
Blocks: 1035577
Added to Iteration 33.3
Status: NEW → ASSIGNED
Iteration: --- → 33.3
QA Whiteboard: [qa?]

Updated

3 years ago
QA Whiteboard: [qa?] → [qa+]
(Assignee)

Comment 3

3 years ago
(In reply to Florian Quèze [:florian] [:flo] from comment #1)

> The application <application name> is being shared. Click to control sharing.

This should now say "window" rather than "application", as it's finally window sharing that will be implemented for Fx33.

Comment 4

3 years ago
Created attachment 8456511 [details] [diff] [review]
add global indicator for webrtc things,

Checkpointing. This doesn't do anything but creates strings and vaguely approximates the suggested styling. Missing icons, click behaviour, integration (showing/hiding), button showing/hiding depending on what is shared, dropdowns for multiple tabs... and probably more that I've forgotten. Oh, and it shows if you click the home button, just to have a way to test it and check styling.

Comment 5

3 years ago
(In reply to Florian Quèze [:florian] [:flo] from comment #3)
> (In reply to Florian Quèze [:florian] [:flo] from comment #1)
> 
> > The application <application name> is being shared. Click to control sharing.
> 
> This should now say "window" rather than "application", as it's finally
> window sharing that will be implemented for Fx33.

(I still need to update this in the patch, too)
(Assignee)

Updated

3 years ago
Depends on: 1039529

Comment 6

3 years ago
Created attachment 8457505 [details] [diff] [review]
add global indicator for webrtc things,

This patch does:
- fix the appearance of the indicator
- integrate in some way with the existing code (although it'll conflict with the other indicator patches, but it should be easy to resolve...)
- implement the tooltips
- implement the hiding/showing of the buttons
- implement opening/focusing the right tab/window.
- hide the indicator when webrtc stops again
- stays on top

This patch doesn't:
- have tests
- have the requested box-shadow
- do dragging the indicator
- ??? I'm sure I've forgotten something, but at this point I'm not sure what.

In future, I guess we should split bugs such as these up in smaller bits, but either way... this is how far I got.
Attachment #8457505 - Flags: feedback?(florian)

Updated

3 years ago
Attachment #8456511 - Attachment is obsolete: true
Florian: With Gijs out, were you planning to pick up this work (I gather you two have discussed something like that), or should I find someone else to take it?
Flags: needinfo?(florian)
(Assignee)

Comment 8

3 years ago
(In reply to Justin Dolske [:Dolske] from comment #7)
> Florian: With Gijs out, were you planning to pick up this work (I gather you
> two have discussed something like that), or should I find someone else to
> take it?

Yes, I'll work on this today, but I would still appreciate if you could suggest someone who would be available to review the patch immediately (or at least quickly) once I have it ready.
Flags: needinfo?(florian)
(Assignee)

Updated

3 years ago
Blocks: 1040061
(Assignee)

Comment 9

3 years ago
Created attachment 8458802 [details] [diff] [review]
Part 1 (add booleans flags in webrtcUI for each kind of sharing)
Assignee: gijskruitbosch+bugs → florian
Attachment #8458802 - Flags: review?(dolske)
(Assignee)

Comment 10

3 years ago
Created attachment 8458803 [details] [diff] [review]
Part 2 (give parameters to the activeStreams getter)
Attachment #8458803 - Flags: review?(dolske)
(Assignee)

Comment 11

3 years ago
Created attachment 8458809 [details] [diff] [review]
Part 3 (add the global indicator window)

This patch is based on Gijs' attachment 8457505 [details] [diff] [review].

The indicator.css file is almost unmodified (I only added a license header and fixed the preprocessing), and I haven't actually fully reviewed it.

The JS code is changed significantly compared to Gijs' version because I've moved most of the logic to webrtcUI.jsm so that the logic can be shared with the Mac-only indicator (bug 1037430) with minimal changes.

Things that aren't handled by this patch, and that I think would be better addressed in follow-ups given the time constraints:
- moving the window (dragging, but also repositioning it if the screen resolution changes)
- adding tests.
Attachment #8457505 - Attachment is obsolete: true
Attachment #8457505 - Flags: feedback?(florian)
Attachment #8458809 - Flags: review?(dolske)
Attachment #8458802 - Flags: review?(dolske) → review+
Attachment #8458803 - Flags: review?(dolske) → review+
Comment on attachment 8458809 [details] [diff] [review]
Part 3 (add the global indicator window)

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

New Binary File: browser/themes/shared/webrtc/camera-white-16@2x.png
New Binary File: browser/themes/shared/webrtc/microphone-white-16@2x.png

I noticed these were a bit blurry in the UI, seems the source files are just blurry upscales. But sounds like we're waiting for final icons in bug 1037418?

New Binary File: browser/themes/shared/webrtc/firefox-22.png
New Binary File: browser/themes/shared/webrtc/firefox-22@2x.png

These two Firefox logos need to go into browser/branding.

Alas the current branding has become a bit of a mess. There is already a browser/branding/official/default22.png, but there's no @2x / 44px version, and the other branding flavors don't even have a 22px version...

So, I'd suggest using the (existing) chrome://branding/content/icon48.png in your CSS, downscaling to 22/44 as needed. We can deal with getting properly sized icons in a followup bug.

::: browser/locales/en-US/chrome/browser/webrtcIndicator.properties
@@ +1,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +# LOCALIZATION NOTE : FILE This file contains the webrtc global indicator strings -->

I was trying to figure out why you have an ascii arrow here, and then realized this is probably a cut'n'paste leftover from a .dtd file. :)

@@ +13,5 @@
> +webrtcIndicator.sharingCameraAndMicrophone.tooltip = Your camera and microphone are being shared. Click to control sharing.
> +webrtcIndicator.sharingCamera.tooltip              = Your camera is being shared. Click to control sharing.
> +webrtcIndicator.sharingMicrophone.tooltip          = Your microphone is being shared. Click to control sharing.
> +webrtcIndicator.sharingScreen.tooltip = Your screen is being shared. Click to control sharing.
> +webrtcIndicator.sharingWindow.tooltip = A window is being shared. Click to control sharing.

Random aside, not really something to deal with here.

I wonder if "sharing" is too vague of a term in the context of a screen/window. I think it's ok with a camera/mic, since those are understandable devices, and the user is likely to understand what's happening without thinking about it deeply. Might be worth a ponder if we can find a more descriptive/understandable phrasing (especially in the permission prompt).
Attachment #8458809 - Flags: review?(dolske) → review+
(In reply to Florian Quèze [:florian] [:flo] from comment #11)

> Things that aren't handled by this patch, and that I think would be better
> addressed in follow-ups given the time constraints:
> - moving the window (dragging, but also repositioning it if the screen
> resolution changes)
> - adding tests.

That's fine, please file these if they're not already filed.

Speaking of tests, I'd suggest giving this a spin through Try. I would not be shocked to find that one of our existing tests is now confused by the presence of the indicator window. :)

Oh, and out of curiosity, any particular reason this is a <window> and not a <panel>?
(Assignee)

Comment 14

3 years ago
(In reply to Justin Dolske [:Dolske] from comment #13)

> Speaking of tests, I'd suggest giving this a spin through Try. I would not
> be shocked to find that one of our existing tests is now confused by the
> presence of the indicator window. :)

I was wondering the same thing (more specifically, I was afraid that new window could cause focus issues).

The test queue is huge currently so I don't have the full results yet, but I pushed https://tbpl.mozilla.org/?tree=Try&rev=e9d90fff1a8d a few hours ago, and the results I have for now are all green :).

> Oh, and out of curiosity, any particular reason this is a <window> and not a
> <panel>?

I don't think we really investigated using a <panel>.
(Assignee)

Comment 15

3 years ago
(In reply to Justin Dolske [:Dolske] from comment #12)

> I noticed these were a bit blurry in the UI, seems the source files are just
> blurry upscales. But sounds like we're waiting for final icons in bug
> 1037418?

Yes, we are. And I expect most (if not all) the webrtc icons to change soon.
(Assignee)

Comment 16

3 years ago
Created attachment 8459049 [details] [diff] [review]
Part 3 (add the global indicator window) v2

Addressed review comments, carrying the r+ forward.
Attachment #8458809 - Attachment is obsolete: true
Attachment #8459049 - Flags: review+
(Assignee)

Comment 17

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f875dc5e0df7
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9cfdc0bbfbc
https://hg.mozilla.org/integration/mozilla-inbound/rev/8833bb7e90de
All three backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/876659559bd8 along with bug 1037430's patch for devtools orange: 
https://tbpl.mozilla.org/php/getParsedLog.php?id=44161687&tree=Mozilla-Inbound
Flags: needinfo?(florian)
(Assignee)

Updated

3 years ago
Depends on: 1041155
(Assignee)

Comment 19

3 years ago
Re-landing with a fix for the orange (bug 1041155):
https://hg.mozilla.org/integration/mozilla-inbound/rev/5cc2ca221443
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1b8ec714d79
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe54b4c9be09
Flags: needinfo?(florian)
https://hg.mozilla.org/mozilla-central/rev/5cc2ca221443
https://hg.mozilla.org/mozilla-central/rev/f1b8ec714d79
https://hg.mozilla.org/mozilla-central/rev/fe54b4c9be09
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Hi Florin, can you assign a QA contact for verification of this bug.
Flags: needinfo?(florin.mezei)
Flags: needinfo?(florin.mezei)
QA Contact: bogdan.maris
(Assignee)

Updated

3 years ago
Depends on: 1041658
(Assignee)

Updated

3 years ago
Depends on: 1041663
(Assignee)

Updated

3 years ago
Depends on: 1041667
(Assignee)

Updated

3 years ago
Depends on: 1041677
(Assignee)

Updated

3 years ago
Depends on: 1041679
(Assignee)

Updated

3 years ago
Depends on: 1041687

Updated

3 years ago
Iteration: 33.3 → 34.1
Blocks: 1042163
Verified that the global getUserMedia indicator is implemented plus did some exploratory around this on Windows 7 64bit, Ubuntu 13.04 64bit and Mac OS X 10.9.4 using latest Nightly.
The dependencies will be verified individually.
Status: RESOLVED → VERIFIED
QA Whiteboard: [qa+] → [qa!]

Updated

3 years ago
Depends on: 1043372
Blocks: 1048230
(Assignee)

Updated

3 years ago
Depends on: 1067444
(Assignee)

Updated

2 years ago
Duplicate of this bug: 819343
(Assignee)

Updated

2 years ago
Component: General → Device Permissions
You need to log in before you can comment on or make changes to this bug.