Remove the "panelmultiview" binding

RESOLVED FIXED in Firefox 60

Status

()

task
RESOLVED FIXED
Last year
12 days ago

People

(Reporter: Paolo, Assigned: Paolo)

Tracking

(Blocks 1 bug)

59 Branch
Firefox 60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Assignee

Description

Last year
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.
Assignee

Comment 1

Last year
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
Comment hidden (mozreview-request)
Assignee

Comment 4

Last year
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.
Assignee

Comment 5

Last year
Also I carried over some cleanup from a previous patch, so part 1 may look familiar.
Assignee

Updated

Last year
Blocks: 1437811
Comment hidden (mozreview-request)
Assignee

Comment 7

Last year
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 11

Last year
mozreview-review
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 12

Last year
mozreview-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 13

Last year
mozreview-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)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 17

Last year
(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.
Assignee

Updated

Last year
Flags: needinfo?(gijskruitbosch+bugs)

Comment 18

Last year
mozreview-review
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+

Updated

Last year
Flags: needinfo?(gijskruitbosch+bugs)
Assignee

Updated

Last year
No longer blocks: 1437811
Assignee

Comment 19

Last year
mozreview-review
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".
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

Last year
Attachment #8950277 - Attachment is obsolete: true
Assignee

Updated

Last year
Attachment #8950601 - Attachment is obsolete: true

Comment 23

Last year
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: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60

Updated

12 days ago
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.