880701 - Add capability for PanelUI to ignore mutations

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Blake Winton (:bwinton) (:☕️)]

And that makes the first call to wrapToolbarItem take 60ms, which causes us to skip four frames, if my math works out…

Thoughts on how to fix it:
1) Have the PanelUI not resize itself on mutation if we're entering/leaving customize mode.
2) Use a document fragment to update all the toolbar items with their wrapped versions in one big swoop.


As a side note, I wonder if this is the cause for question #1 in bug 873060…

Comment 1

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

I think we should try to slip a fix for this into the UR build if possible.

Comment 2

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

Hey Mike - can you maybe take a look at this?

Comment 3

[Mike de Boer [:mikedeboer]]

Attachment: WIP: add transitioning property to CustomizeMode

Comment 4

[Blake Winton (:bwinton) (:☕️)]

(We probably also want to do something similar on the transition out of Customize mode, when we unwrap the items…)

Comment 5

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

Comment on attachment 761027 [details] [diff] [review]
WIP: add transitioning property to CustomizeMode

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

Just a drive-by - I don't think I like panelmultiview reaching into CustomizeMode like this.

Lots of the UI "prepares" itself before we go into customization mode. You can see that happening in browser/base/content/browser-customization.js. I think we might want to do something similar here. Alter panelUI.js so that it has customizationStart and customizationEnd functions, and have those called within browser-customization.js.

Those functions should do the manipulation of the panelmultiview - maybe calling a method on the minding, or (more likely) setting a property. Does that make sense?

Comment 6

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

Attachment: Patch v1

Hey Mike,

I needed this bug in order to work on bug 881909, and I did some fiddling with it. In the end, I think this is what I need. I call startBatchUpdate and endBatchUpdate in my (soon to be posted) patch in bug 881909.

Hope it's cool if I yank this one from you!

-Mike

Comment 7

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

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

How does this look, Jared? I'll actually be calling beginBatchUpdate and endBatchUpdate in bug 881909.

Comment 8

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

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

I've just realized that both this and my patch in bug 881909 don't apply cleanly to trunk. Let me fix that...

Comment 9

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

Attachment: Patch 1.1

Comment 10

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

Attachment: Patch v1.2

Comment 11

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

Updated the summary, since bug 881909 takes care of making sure we don't reflow when wrapping the items.

Comment 12

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

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

What do you think of this, Jared?

Comment 13

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

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

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

::: browser/components/customizableui/content/panelUI.xml
@@ +88,5 @@
>        <field name="_anchorElement">null</field>
>        <field name="_mainViewHeight">0</field>
>        <field name="_subViewObserver">null</field>
>        <field name="_transitioning">false</field>
> +      <field name="ignoreMutations">false</field>

This can be a property that calls disconnect or observe when it is set. That would also allow us to keep syncContainerWith*View private. It could also kick off the sync when ignoreMutations becomes true.

Comment 14

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

Attachment: Patch v1.3

Having ignoreMutations take care of connecting / disconnecting observers is kinda complicated...this is what it looks like.

Maybe I've been looking at this performance stuff for too long, but this looks pretty complicated. :/

Perhaps we can have the mutation observer functions check ignoreMutations instead?

Comment 15

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

Ok let's just go with Patch v1.2. Thanks for trying my recommendation.

Comment 16

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

Attachment: Patch v1.4

I liked your suggestion about re-hiding the _sync functions, and I think I've found a happy medium here.

Comment 17

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

Comment on attachment 762213 [details] [diff] [review]
Patch v1.4

I *think* this is the least complicated solution. This way, in bug 881909, I can call ensureRegistered(true), and we don't call the _sync functions until CustomizeMode.enter/exit is ready.

Sorry to make you review this thing yet again. :/

Comment 18

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

Comment on attachment 762213 [details] [diff] [review]
Patch v1.4

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

Yeah, this looks nice.

::: browser/components/customizableui/content/panelUI.js
@@ +200,5 @@
>        tempPanel.openPopup(iconAnchor || aAnchor, "bottomcenter topright");
>      }
>    },
>  
> +  /** 

nit, trailing whitespace

Comment 19

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

https://hg.mozilla.org/projects/ux/rev/91e3867710d7

Comment 20

[:Gijs (he/him)]

This seems to have broken sizing the menupanel to its contents. :-(

Comment 21

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

(In reply to :Gijs Kruitbosch from comment #20)
> This seems to have broken sizing the menupanel to its contents. :-(

We should back this out until we have a fix for the menupanel sizing.

Comment 22

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

Yep, backed out as:

https://hg.mozilla.org/projects/ux/rev/0965605b441e

Comment 23

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

Attachment: Patch v1.5 (r+'d by jaws)

I think this bug was caused because we were trying to sync the panelview container heights too early, before the panel had even opened. We should only sync the container after ignoreMutations is set to false if the panel is open. If the panel is closed, the sync will happen automatically once the onpopupshowing event is fired.

Comment 24

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

Attachment: Patch v1.6 (r+'d by jaws)

Fixing some documentation. Going to re-land now.

Comment 25

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

Relanded as https://hg.mozilla.org/projects/ux/rev/cd6b6424d334

Comment 26

[:Gijs (he/him)]

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