860646 - Panel isn't resizing reliably when entering sub-views

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Noticed in bug 855803:

If I don't have many widgets in the panel, the panel doesn't resize to fit all the history items in the history subview. But it does for the bookmarks subview, and then going back into the history subview the panel resizes to the size needed for the bookmarks subview.

Perhaps the difference is the history subview is dynamically generated, while the bookmarks subview is currently just a placeholder of static content?

Comment 1

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

The problem with the patch in 855803 is that the history items weren't added before onViewShowing completed. This should be fixed in the next iteration of that patch before landed.

With that being said, there is probably some mutation-observer/some-other-thing that we can do to fix this issue. It will more than likely be encountered again by other widgets (official or 3rd party).

Comment 2

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

(In reply to Jared Wein [:jaws] from comment #1)
> With that being said, there is probably some
> mutation-observer/some-other-thing that we can do to fix this issue. It will
> more than likely be encountered again by other widgets (official or 3rd
> party).

Absolutely - I feel like we're going to get bitten by this again and again unless we can make the panel smarter about subviews.

Comment 3

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

So here's my proposed solution:

1) Replace the subview deck with a box, and just do display: none; on all subviews except the selected one. We'll have to manage this manually, but that shouldn't be too hard. This allows us to get around the deck's "feature" of assuming the size of its smallest child.
2) Use a MutationObserver to listen for changes to the selected subview, and adjusting the panel container accordingly.

Gonna give that a shot right now.

Comment 4

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

Attachment: Patch v1

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

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

Won't give r+ until we actually know this works... but looks like it should do the job nicely.

::: browser/base/content/panelUI.inc
@@ +60,5 @@
>             allows click events to go through it. -->
>        <box id="PanelUI-clickCapturer">
>        </box>
>  
> +      <box id="PanelUI-subViews">

Add a comment as to why this isn't using deck.

::: browser/base/content/panelUI.js
@@ +58,5 @@
> +
> +    // Get a MutationObserver ready to react to subview size changes. We
> +    // only attach this MutationObserver when a subview is being displayed.
> +    let self = this;
> +    this._subViewObserver = new MutationObserver(function(aMutations) {

Nit: Good place to use .bind() here.
  new MutationObserver(this._syncContainerWithSubView.bind(this));

@@ +154,5 @@
>        let evt = document.createEvent("CustomEvent");
>        evt.initCustomEvent("ViewHiding", true, true, viewNode);
>        viewNode.dispatchEvent(evt);
> +
> +      viewNode.removeAttribute("showing");

I wonder about this attribute name - it makes me think it will transition to "shown". Usual attribute name for this kind of thing is "current" or something similar.

@@ +183,5 @@
>        return;
>      }
>  
> +    let oldHeight = this.mainView.clientHeight;
> +    this.viewStack.setAttribute("view", "subview");

The "ViewShowing" event should be fired before this, so the view can do any quick updates before it starts to become visible. Though, the "showing" (or "current") attribute should be set before all that (_currentSubView too) - I can imagine subviews doing a sanity check on that attribute.

::: browser/modules/CustomizableUI.jsm
@@ +48,5 @@
>        LOG("Bookmark view is being shown!");
>        let doc = aEvent.detail.ownerDocument;
>        let vbox = doc.getElementById("PanelUI-bookmarks-tall-maker");
> +
> +      doc.defaultView.setTimeout(function() {

Can we just remove this now? Don't think we need it for testing anymore, given the history subview is about to land.

Comment 6

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

Attachment: Patch v1.1

Comment 7

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

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

Jared has offered to take this, so redirecting.

Comment 8

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

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

Fixing a trailing whitespace nit.

Thanks for the r+!

Comment 9

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

Landed in jamun as https://hg.mozilla.org/projects/jamun/rev/4d73618dacaf

Comment 10

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

Landed in UX as https://hg.mozilla.org/projects/ux/rev/4d73618dacaf

Comment 11

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/95076fe393a8