940946 - The WebRTC camera icon and other non-removable buttons can be removed from the toolbar

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Florian Quèze [:florian]]

Attachment: Screenshot of the broken icon

See attached screenshot

Comment 1

[Florian Quèze [:florian]]

Attachment: Screenshot of the icon in the toolbar

Comment 2

[:Gijs (he/him)]

Can you provide more detailed STR, like how to actually get this icon to show up, and stay present in customize mode so it can be moved?

Comment 3

[Florian Quèze [:florian]]

(In reply to :Gijs Kruitbosch from comment #2)
> Can you provide more detailed STR, like how to actually get this icon to
> show up, and stay present in customize mode so it can be moved?

Oops, that seemed obvious to me but I guess that just means I've been poking around WebRTC for too long, sorry about that.

Steps to reproduce:
1. Load https://apprtc.webrtc.org/
2. Grant access to your devices in the prompt that appears
3. Notice the Camera icon that appeared in your toolbar.
4. Click on "customize" and try moving the Camera icon around.

Comment 4

[Florian Quèze [:florian]]

By the way, it's visible from the screenshot that something needs to be fixed, but I'm not sure if the fix is to make the icon look correct when customized, or to ensure the icon is never removed from the toolbar. The point of the icon is to ensure the user is aware that his camera or microphone is currently shared with some website(s), so letting the user hide this icon may be dangerous. It seems Firefox 24 doesn't let me customize this icon.

Comment 5

[:Gijs (he/him)]

(In reply to Florian Quèze [:florian] [:flo] from comment #4)
> By the way, it's visible from the screenshot that something needs to be
> fixed, but I'm not sure if the fix is to make the icon look correct when
> customized, or to ensure the icon is never removed from the toolbar. The
> point of the icon is to ensure the user is aware that his camera or
> microphone is currently shared with some website(s), so letting the user
> hide this icon may be dangerous. It seems Firefox 24 doesn't let me
> customize this icon.

This is a good point. I'm actually confused by the Fx25 behaviour, because it doesn't even seem possible to move the icon itself within its toolbar. I don't understand why that is. Anyway, we should probably just make sure you can't remove the icon from the toolbar.

Comment 6

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #5)
> I'm actually confused by the Fx25 behaviour, because
> it doesn't even seem possible to move the icon itself within its toolbar. I
> don't understand why that is.

Because the 'removable' attribute implies that (or used to, in case this has changed).

Comment 9

[:Gijs (he/him)]

This is because this check:

http://mxr.mozilla.org/mozilla-central/source/browser/components/customizableui/src/CustomizableUI.jsm#884

877         // Normalize the removable attribute. For backwards compat, if
878         // the widget is not defined in a toolbox palette then absence
879         // of the "removable" attribute means it is not removable.
880         if (!node.hasAttribute("removable")) {
881           parent = parent.localName == "toolbarpaletteitem" ? parent.parentNode : parent;
882           // If we first see this in customization mode, it may be in the
883           // customization palette instead of the toolbox palette.
884           node.setAttribute("removable", !parent.customizationTarget);
885         }

Is completely bogus.

The parent is going to be the customization target, not have one. Unfortunately it's not super-trivial to check whether something /is/ a customization target. I'm trying to figure out how to best write this code so it does do what's intended, given that CustomizableUI doesn't know about the visible palette and is still meant to do the right thing here.

Comment 10

[:Gijs (he/him)]

Attachment: items in the navbar without the removable attribute shouldn't be removable in customize mode either,

Fix as discussed plus a test.

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8335675 [details] [diff] [review]
items in the navbar without the removable attribute shouldn't be removable in customize mode either,

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

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +878,5 @@
> +        if ((parent.customizationTarget == nodeInArea.parentNode &&
> +             gBuildWindows.get(aWindow).has(parent.toolbox)) ||
> +            aWindow.gNavToolbox.palette == nodeInArea.parentNode) {
> +          // Normalize the removable attribute. For backwards compat, if
> +          // the widget is not defined in a toolbox palette then absence

s/not defined in/not located in/

"defined" may be a correct word here, but usually when I hear "defined" I think of if the value is null or not, as compared to who the parent is.

Comment 12

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/cf3ed86dfe97

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cf3ed86dfe97