Closed Bug 860646 Opened 10 years ago Closed 9 years ago

Panel isn't resizing reliably when entering sub-views

Categories

(Firefox :: Toolbars and Customization, defect)

x86_64
Windows 8
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: Unfocused, Assigned: mconley)

References

Details

Attachments

(1 file, 2 obsolete files)

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?
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).
(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.
Status: NEW → ASSIGNED
QA Contact: 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.
Attached patch Patch v1 (obsolete) — Splinter Review
Assignee: nobody → mconley
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.
Attachment #736478 - Flags: feedback+
Attached patch Patch v1.1 (obsolete) — Splinter Review
Attachment #736478 - Attachment is obsolete: true
Attachment #736592 - Flags: review?(bmcbride)
Comment on attachment 736592 [details] [diff] [review]
Patch v1.1

Jared has offered to take this, so redirecting.
Attachment #736592 - Flags: review?(bmcbride) → review?(jaws)
Attachment #736592 - Flags: review?(jaws) → review+
Fixing a trailing whitespace nit.

Thanks for the r+!
Attachment #736592 - Attachment is obsolete: true
Attachment #736700 - Flags: review+
Landed in jamun as https://hg.mozilla.org/projects/jamun/rev/4d73618dacaf
Whiteboard: [fixed in jamun]
Blocks: 861087
Blocks: 861088
Landed in UX as https://hg.mozilla.org/projects/ux/rev/4d73618dacaf
Whiteboard: [fixed in jamun] → [fixed in jamun][fixed in ux]
https://hg.mozilla.org/mozilla-central/rev/95076fe393a8
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed in jamun][fixed in ux]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.