886323 - Can't drag Facebook button to main customization palette

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[:Margaret Leibovic]

I can drag the social API Facebook button to the toolbar and to the main menu panel, but not to the palette on the left side of customization mode.

I am told on IRC that this button is not designed to be removable, so the fix here is probably just to not let the user drag it out of the toolbar (this would also fix a problem where the button breaks the layout of the menu panel).

Comment 1

[:Margaret Leibovic]

After updating and restarting the browser, the button moved itself from the menu panel to the toolbar.

Also, I failed to mention this in the description, but when I try to drag the button out of the menu panel back into the toolbar, I can't do it. So basically, I can move the button to the menu panel, and then it's stuck there forever.

Comment 2

[:Gijs (he/him)]

(In reply to :Margaret Leibovic from comment #1)
> After updating and restarting the browser, the button moved itself from the
> menu panel to the toolbar.
> 
> Also, I failed to mention this in the description, but when I try to drag
> the button out of the menu panel back into the toolbar, I can't do it. So
> basically, I can move the button to the menu panel, and then it's stuck
> there forever.

I can't move the button to the panel anymore (which is correct; it's not removable so it shouldn't be possible to move it outside of the navbar). We have a different bug on file about making those constraints (which also apply to, for instance, the URL bar bits) more intuitive/visible and seem more like "Oh, I'm not supposed to be able to do this" rather than "why can't I do this" ?

Comment 3

[:Gijs (he/him)]

Found the bug I was thinking of: bug 879981. Margaret, if you can no longer remove the social API widget to the panel or elsewhere, can you mark this as WFM? Thanks!

Comment 4

[:Margaret Leibovic]

At some point I disabled the social API on my UX build, but when I re-enabled it to test just now, I found that the Facebook button started off in the panel, and I can't drag it anywhere else.

Comment 5

[:Gijs (he/him)]

(In reply to :Margaret Leibovic from comment #4)
> At some point I disabled the social API on my UX build, but when I
> re-enabled it to test just now, I found that the Facebook button started off
> in the panel, and I can't drag it anywhere else.

This is the weirdest thing. The thing is, it shouldn't be possible for the buttons to be anywhere but in the toolbar. I'll check again if I can try to reproduce this at all. OOI, you filed this on Mac, so I presume this is reproducible there? Or was it platform-specific?

Comment 6

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gijs Kruitbosch from comment #5)
> (In reply to :Margaret Leibovic from comment #4)
> > At some point I disabled the social API on my UX build, but when I
> > re-enabled it to test just now, I found that the Facebook button started off
> > in the panel, and I can't drag it anywhere else.
> 
> This is the weirdest thing. The thing is, it shouldn't be possible for the
> buttons to be anywhere but in the toolbar. I'll check again if I can try to
> reproduce this at all. OOI, you filed this on Mac, so I presume this is
> reproducible there? Or was it platform-specific?

There was a time (June/early July?) when you could move the button but that was fixed at some point.  Maybe if it was moved before, it is being placed due to that.  Probably just need to clean the persisted data in the profile being used.

Comment 7

[:Margaret Leibovic]

Yeah, there is something weird going on. I disabled it again after testing for my last comment, and sometimes when I restart the browser the services cloud icon appears in my toolbar, but then when I've clicked on it, it moves into the menu panel.

Let me know if there's any profile data I can give you that might help you figure this out.

Comment 8

[:Gijs (he/him)]

(In reply to :Margaret Leibovic from comment #7)
> Yeah, there is something weird going on. I disabled it again after testing
> for my last comment, and sometimes when I restart the browser the services
> cloud icon appears in my toolbar, but then when I've clicked on it, it moves
> into the menu panel.
> 
> Let me know if there's any profile data I can give you that might help you
> figure this out.

I think that last bit of information did it. I can at least now reproduce it if I mess with my placement variables (browser.uiCustomization.state) locally. Might be WONTFIX though, because on a clean profile you shouldn't be able to move it at all anymore (alternatively, after hitting "restore defaults"), and I don't know if we want to change buildArea to not move items which are put in a non-default area and aren't removable. Seems like something that shouldn't possibly be going wrong anyway. Jared, what do you think?

Comment 9

[:Gijs (he/him)]

Attachment: Fix buildArea to check if a node is removable before moving it,

Actually, this will bite us if we change the removability of anything, like the new tab button or tablist (see bug 878551), so let's just fix this so it can't happen. Small patch, lots of test work.

Comment 10

[:Gijs (he/him)]

Comment on attachment 804426 [details] [diff] [review]
Fix buildArea to check if a node is removable before moving it,

Egh, this is not quite there yet...

Comment 11

[:Gijs (he/him)]

Attachment: Fix buildArea to check if a node is removable before moving it,

Let's try this instead. The unregisterArea fix is necessary because without it, half the tests break because our reset() code likes to loop over gBuildAreas, which still keeps references to unregistered areas, which *have* been removed from gPlacements. Which means exceptions in the cleanup code. Oops.

Comment 12

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

Comment on attachment 804430 [details] [diff] [review]
Fix buildArea to check if a node is removable before moving it,

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

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +370,5 @@
>          continue;
>        }
>  
> +      // If the placements have items in them which are (now) no longer removable,
> +      // we shouldn't be moving them:

Should we also be removing the entry from the placements array, and putting it where it belongs?

::: browser/components/customizableui/test/browser_886323_buildArea_removable_nodes.js
@@ +4,5 @@
> +
> +const kButtonId = "test-886323-removable-moved-node";
> +const kLazyAreaId = "test-886323-lazy-area-for-removability-testing";
> +
> +let navbar = document.getElementById(CustomizableUI.AREA_NAVBAR);

Nit - these globals should be prefixed with g, so gNavBar, gLazyArea.

Comment 13

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) from comment #12)
> Comment on attachment 804430 [details] [diff] [review]
> Fix buildArea to check if a node is removable before moving it,
> 
> Review of attachment 804430 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/customizableui/src/CustomizableUI.jsm
> @@ +370,5 @@
> >          continue;
> >        }
> >  
> > +      // If the placements have items in them which are (now) no longer removable,
> > +      // we shouldn't be moving them:
> 
> Should we also be removing the entry from the placements array, and putting
> it where it belongs?

We could... Generally, we don't mess with missing/"wrong" placements and keep them in case something changes later, so I wasn't sure we'd want to do that. Also, I'd need to doublecheck whether this for...of loop is going to be very sad if I mess with its subject in the loop. :-\

Comment 14

[:Gijs (he/him)]

Attachment: Fix buildArea to check if a node is removable before moving it,

Upon reflection, yes, I think we should do this. Fixed the nits as well. This better, Mike? :-)

Comment 15

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

Comment on attachment 805271 [details] [diff] [review]
Fix buildArea to check if a node is removable before moving it,

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

I like it! Thanks Gijs!

Comment 16

[:Gijs (he/him)]

https://hg.mozilla.org/projects/ux/rev/d4ab67e7dda6

Comment 17

[:Gijs (he/him)]

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