900442 - Add-on toolbar shouldn't be evicting stuff when we wrap its contents

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[:Gijs (he/him)]

This leads to warnings in the console for long items (eg. springs) and adds bogus stuff to the navbar or palette.

Comment 1

[:Gijs (he/him)]

Attachment: Add-on toolbar shouldn't be evicting stuff when we wrap its contents

Comment 2

[:Gijs (he/him)]

Comment on attachment 784334 [details] [diff] [review]
Add-on toolbar shouldn't be evicting stuff when we wrap its contents

Or perhaps I should pick someone who's not on holiday...

Comment 3

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

Comment on attachment 784334 [details] [diff] [review]
Add-on toolbar shouldn't be evicting stuff when we wrap its contents

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

::: browser/components/customizableui/content/toolbar.xml
@@ +387,5 @@
>                return;
>              }
>              let toolbar = mutations[0].target;
> +            // Can't use our own attribute because we might not have one if we're set to
> +            // collapsed

Why are we even wrapping the add-on bar items?

Comment 4

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) from comment #3)
> Why are we even wrapping the add-on bar items?

We wrap everything in all registered toolbars, I think... Not sure we can fix that. :-\

Comment 5

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

The add-on bar is kind of a special-case. I'm wondering if maybe the better approach is to skip wrapping items in it entirely.

Comment 6

[:Gijs (he/him)]

(In reply to Mike Conley (:mconley) from comment #5)
> The add-on bar is kind of a special-case. I'm wondering if maybe the better
> approach is to skip wrapping items in it entirely.

I would understand that, but that'd involve special-casing it in CustomizeMode, which seems weird...

Comment 7

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

Comment on attachment 784334 [details] [diff] [review]
Add-on toolbar shouldn't be evicting stuff when we wrap its contents

(In reply to :Gijs Kruitbosch from comment #6)
> (In reply to Mike Conley (:mconley) from comment #5)
> > The add-on bar is kind of a special-case. I'm wondering if maybe the better
> > approach is to skip wrapping items in it entirely.
> 
> I would understand that, but that'd involve special-casing it in
> CustomizeMode, which seems weird...

It does. :/ The add-on bar shim is just weird in general. :)

So I guess this added weirdness is acceptable.

Comment 8

[:Gijs (he/him)]

https://hg.mozilla.org/projects/ux/rev/6b7a279be044

Comment 9

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/6b7a279be044