866978 - Enforce absence of removable="true" attribute

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

In bug 864355 we're making it so we respect the absence of removable="true" attribute when initially building build areas. We should also respect that when in customization mode.

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

There is the question of whether we want this to mean a widget can't be removed from the area it's defined in, or it can't be removed from all areas (ie, it has to be somewhere, but it doesn't matter where).

Not sure what the old behaviour is - will have to check. But I'm thinking it would be nice to make it so the widget can be moved to different areas.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

It looks like the old behaviour is that if a node doesn't have removable="true", then you can't pick it up and move it, or remove it in customization mode.

I think we should allow items without removable="true" to move within their container. The current implementation allows this (with some difficulty), since the user can simply remove items from the left or right of the item to move it laterally.

So I propose that we interpret removable="true" as grab-able and move-able within its current customizationtarget, but it cannot leave that target.

Comment 3

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

(In reply to Mike Conley (:mconley) from comment #2)
> So I propose that we interpret removable="true" as grab-able and move-able
> within its current customizationtarget, but it cannot leave that target.

I like this proposal.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

(In reply to Mike Conley (:mconley) from comment #2)
> 
> So I propose that we interpret removable="true" as grab-able and move-able
> within its current customizationtarget, but it cannot leave that target.

Whoa - correction, I propose that we interpret the *lack* of removable="true" to mean that a node is grab-able and move-able within its current customizationtarget, but it cannot leave that target.

Comment 5

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

Oh yes, I misread that the first time. I agree with your correction :)

Comment 6

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

Blair, are you still working on this?

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Jared Wein [:jaws] from comment #6)
> Blair, are you still working on this?

Yea, getting close to done. Many yaks have been shaved.

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1

Well this ended up being a bit more involved than expected.

Yaks shaved:
* Refactored getWidgetNode() to not need the toolbox passed in as a parameter (because we don't always have the toolbox handy when we want to use this function)
* Fixed bug in drag&drop code where cancelling a drop wouldn't reset the special effect applied to the dragged item
* Fixed findWidgetInToolbox (now findWidgetInWindow) to work while in customization mode
* When building an area and finding a non-removable node, add that to the area's placements array - otherwise CustomizableUI thinks its in the palette (ie, unused)
* Added an API (canWidgetMoveToArea) that we can extend later, that allows us to enforce areas where certain widgets cannot be placed

And this is keeps backwards compatibility of the old behaviour of:
* If the node is defined in a toolbar without removable="true", it defaults to false
* If the node is defined in the toolbox palette without removable="true", it defaults to true

Comment 10

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

Comment on attachment 750360 [details] [diff] [review]
Patch v1

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

::: browser/base/content/browser.xul
@@ +493,5 @@
>               context="toolbar-context-menu">
>  
>        <hbox id="nav-bar-customizationtarget" class="customization-target" flex="0">
>          <toolbaritem id="unified-back-forward-button" class="chromeclass-toolbar-additional"
> +                     context="backForwardMenu" removable="false"

Can we just drop the 'removable' attribute here and below?

Comment 11

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Changing summary, since this doesn't just affect customization mode - it's supported properly through the entire API now.

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Jared Wein [:jaws] from comment #10)
> Can we just drop the 'removable' attribute here and below?

Technically, yes. But I left it in because it makes the intention far more obvious (and the code just has to set it during runtime anyway).

Comment 13

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

I just pushed a patch for bug 858056 that included part of this patch, namely the ondragend code, since it simplified the work to remove the attribute when the user was no longer dragging. It should make the footprint of this patch smaller.

I should note that I'm getting errors about aWidgetId being an empty string when I apply this patch. I'll continue to investigate.

Comment 14

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Jared Wein [:jaws] from comment #13)
> I should note that I'm getting errors about aWidgetId being an empty string
> when I apply this patch. I'll continue to investigate.

Hm, yea. In which case, I'm starting to suspect this patch was the cause of what I saw in bug 872403.

Comment 15

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Relevant log:

[CustomizableUI] Placements for toolbar-menubar:
	menubar-items
[CustomizableUI] Saving state.
[CustomizableUI] State saved as: {"placements":{"PanelUI-contents":["new-window-button","privatebrowsing-button","save-page-button","print-button","history-panelmenu","fullscreen-button"],"TabsToolbar":["tabbrowser-tabs","new-tab-button","alltabs-button","tabs-closebutton",""],"PersonalToolbar":["personal-bookmarks"],"nav-bar":["unified-back-forward-button","urlbar-container","reload-button","stop-button","bookmarks-menu-button","separator136859613640311368661513681113686621951831","search-container","downloads-button","bookmarks-button","find-button","open-file-button","preferences-button","add-ons-button","home-button","webrtc-status-button","social-toolbar-item","social-share-button"],"toolbar-menubar":["menubar-items",""]},"seen":[]}


Note the empty string in the placements for toolbar-menubar, even though this is with the patch for bug 872403.

Comment 16

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1.1

Un-bitrotten, but apparently with an unfixed bug.

Comment 17

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

Comment on attachment 751621 [details] [diff] [review]
Patch v1.1

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

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +122,5 @@
>   */
>  let gBuildAreas = new Map();
> +
> +/**
> + * gBuildWindows is a map of windows that have registered build areas, mappped

s/mappped/mapped/

Comment 18

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Patch v1.2

Ugh, well this was a pain to track down. Turns out we have elements in the titlebar toolbar that don't have IDs. Before this patch, we were removing them - which likely broke something else. Now we're not removing them, and so need to add any non-removable widgets to the placements array... which of course broken when we hit something without an ID. Instead, AFAICT we need to completely ignore such elements - we should be on the lookout for that type of issue in the future too, in case it affects any other part of our code.

Relevant change is at the end of buildArea(), the rest of just un-bitrotting. Small change, but fundamental, so I want an extra brain on that just in case - thus the re-review.

Comment 19

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

Comment on attachment 752485 [details] [diff] [review]
Patch v1.2

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

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +271,5 @@
>          this._addParentFlex(currentNode);
>          this.setLocationAttributes(currentNode, container, aArea);
> +
> +        // Normalize removable attribute - if it's defined in the area without
> +        // the attribute, it defaults to false.

This sounds confusing to me. Maybe just say "Default the "removable" attribute to false."

@@ +310,5 @@
>              palette.appendChild(node);
>            } else {
>              container.removeChild(node);
>            }
> +        } else if (node.id) {

Since a lot of our code depends on ID's, should we make it a requirement that a "removable"=true element also have an ID?

If we make this requirement, which I think wouldn't hurt, we will need to move the node.id check higher up, before the node.getAttribute("removable") check.

Comment 20

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Jared Wein [:jaws] from comment #19)
> This sounds confusing to me. Maybe just say "Default the "removable"
> attribute to false."

That's not accurate though :\ It only defaults to false if the widget is originally a child of the toolbar. It defaults to true if it's originally a child of the toolbar palette. I'll try to work out better wording to reflect that.

> Since a lot of our code depends on ID's, should we make it a requirement
> that a "removable"=true element also have an ID?
> 
> If we make this requirement, which I think wouldn't hurt, we will need to
> move the node.id check higher up, before the node.getAttribute("removable")
> check.

Hmm. Yea. Ok.

Comment 21

[Blair McBride [:Unfocused] (UNAVAILABLE)]

https://hg.mozilla.org/projects/ux/rev/1cacc35a4801

Comment 22

[Dão Gottwald [:dao]]

(In reply to Blair McBride [:Unfocused] (Limited availability.) from comment #20)
> (In reply to Jared Wein [:jaws] from comment #19)
> > This sounds confusing to me. Maybe just say "Default the "removable"
> > attribute to false."
> 
> That's not accurate though :\ It only defaults to false if the widget is
> originally a child of the toolbar. It defaults to true if it's originally a
> child of the toolbar palette.

The notion of non-removability doesn't seem to make sense for the palette... How and why do you handle removable="false" there?

Comment 23

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/1cacc35a4801