Closed Bug 1437512 Opened 6 years ago Closed 6 years ago

Remove the "panelmultiview" binding

Categories

(Firefox :: Toolbars and Customization, task)

Product:
Firefox
Component:
Toolbars and Customization
Version:
59 Branch
Type:
task
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: Paolo, Assigned: Paolo)

References

Details

Attachments

(2 files, 2 obsolete files)

Bug 1437512 - Part 1 - Remove the "panelmultiview" binding construction.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1437512 - Part 2 - Remove the "panelmultiview" binding.
59 bytes, text/x-review-board-request
Gijs
: review+
Details 
In bug 1434883 we unified most of the entry points for the "panelmultiview" element, so we can more easily remove this binding and create the inner elements directly.
For some reason this depends on bug 1434376, in that the page actions panel will not open immediately after I move the construction of the inner elements to JavaScript.
Depends on: 1434376
Work in progress as I still have to remove the binding, but these parts seems to work quite well.

Note that for me it's already the case that the tooltip doesn't appear when hovering the mouse over the back button, I have to figure out if that's expected.
Also I carried over some cleanup from a previous patch, so part 1 may look familiar.
Blocks: 1437811
(In reply to :Paolo Amadini from comment #4)
> Note that for me it's already the case that the tooltip doesn't appear when
> hovering the mouse over the back button, I have to figure out if that's
> expected.

So, this can be traced back to bug 1354141 when the feature was first introduced. The correct attribute name is "tooltiptext" instead of "tooltip", so we never showed any tooltip on the back button.

I think this should continue to be the case for mouse users, but we may still set a proper label for screen readers.
Comment on attachment 8950277 [details]
Bug 1437512 - Part 1 - Support "panelview" elements located anywhere in the document.

https://reviewboard.mozilla.org/r/219536/#review225600
Attachment #8950277 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8950278 [details]
Bug 1437512 - Part 1 - Remove the "panelmultiview" binding construction.

https://reviewboard.mozilla.org/r/219538/#review225606

::: browser/base/content/browser.xul:10
(Diff revision 1)
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  <?xml-stylesheet href="chrome://browser/content/browser.css" type="text/css"?>
> +<?xml-stylesheet href="chrome://browser/content/customizableui/panelUI.css" type="text/css"?>

Can you just move this into browser.css please? :-)

::: browser/base/content/test/performance/browser_appmenu_reflows.js:119
(Diff revision 1)
>          // Unfortunately, I can't find a better accessor to the current
>          // subview, so I have to reach the PanelMultiView instance
>          // here.

Remove this comment while we're here?

::: browser/components/customizableui/PanelMultiView.jsm:281
(Diff revision 1)
> -    this._viewContainer =
> -      document.getAnonymousElementByAttribute(this.node, "anonid", "viewContainer");
> -    this._viewStack =
> +    this._viewContainer = viewContainer;
> +    this._viewStack = viewStack;
> +    this._offscreenViewStack = offscreenViewStack;

Could just assign these when creating them at the top?
Attachment #8950278 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8950547 [details]
Bug 1437512 - Part 2 - Remove the "panelmultiview" binding.

https://reviewboard.mozilla.org/r/219828/#review225634

::: browser/components/customizableui/PanelMultiView.jsm:184
(Diff revision 2)
> +   * Removes the specified <panel> from the document, ensuring that any
> +   * <panelmultiview> node it contains is destroyed properly.
> +   *
> +   * If the panel does not contain a <panelmultiview>, it is removed directly.
> +   * This allows consumers like page actions to accept different panel types.
> +   */

So I mean, this works, but wouldn't it be possible to use a DOM Mutation Observer to figure out when the pmv or its panel gets removed? That would avoid having to update all the callsites and making it harder to start using PMV for anything new (because you have to know that you can't just add/remove it without tidying up).
Attachment #8950547 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs from comment #13)
> So I mean, this works, but wouldn't it be possible to use a DOM Mutation
> Observer to figure out when the pmv or its panel gets removed? That would
> avoid having to update all the callsites and making it harder to start using
> PMV for anything new (because you have to know that you can't just
> add/remove it without tidying up).

So I just tried a simple version of this solution, and it almost works, but not quite. The issue is that the panel nodes may be removed from within ViewHidden and "panelhidden" events, but mutation observers run asynchronously, so we don't notice that something has happened when return from these event handlers. I guess this is why in one place we were explicitly calling "destructor" before removing the panel. If we still have to do this manually, it defeats the point of using a mutation observer to begin with.

We could rewrite the event invocation code so that we explicitly check if someone has removed the node after dispatching each event and tear down synchronously, but I'm not sure it's worth it? I also seem to remember that use of mutation observers in production code is discouraged as they may be expensive to process.

Note that if we wanted to make the observer generic, we would also have to start monitoring a new parent node in case the element is moved instead of removed. There isn't a simpler mutation observer API for monitoring element removal.
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8950547 [details]
Bug 1437512 - Part 2 - Remove the "panelmultiview" binding.

https://reviewboard.mozilla.org/r/219828/#review226054

Hm. Perhaps custom elements will allow us to make this better, then... but I guess for now we will have to live with this.
Attachment #8950547 - Flags: review?(gijskruitbosch+bugs) → review+
Flags: needinfo?(gijskruitbosch+bugs)
No longer blocks: 1437811
Comment on attachment 8950278 [details]
Bug 1437512 - Part 1 - Remove the "panelmultiview" binding construction.

https://reviewboard.mozilla.org/r/219538/#review226460

::: browser/components/customizableui/PanelMultiView.jsm:200
(Diff revision 3)
> +      this._viewContainer.setAttribute("transitioning", "true");
> +      this._viewStack.setAttribute("transitioning", "true");

Turns out this attribute was only relevant on the "panelmultiview" element itself, will remove the other calls before landing.

::: browser/components/customizableui/PanelMultiView.jsm:939
(Diff revision 3)
>          this.node.setAttribute("panelopen", "true");
> +        this._viewContainer.setAttribute("panelopen", "true");

Same for "panelopen" on "this.node".
Attachment #8950277 - Attachment is obsolete: true
Attachment #8950601 - Attachment is obsolete: true
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2acd3d12275
Part 1 - Remove the "panelmultiview" binding construction. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc6edcbe8361
Part 2 - Remove the "panelmultiview" binding. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/b2acd3d12275
https://hg.mozilla.org/mozilla-central/rev/cc6edcbe8361
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: