1369407 - Update photonpanelmultiview to not throw JS errors and/or not call showMainView onpopuphidden

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, enhancement, P1)

Description

[:Gijs (he/him)]

When opening pocket and another subview, like the history or devtools one, when their buttons are on the main toolbar and the photon pref is flipped, I noticed we call showSubView onpopuphidden. That's just busy-work, we shouldn't need to do that. It also seems to cause errors:

JavaScript error: resource:///modules/PanelMultiView.jsm, line 66: Error: SlidingPanelViews :: index -1 out of bounds

(for the history/devtools views)

and

15:52:31.915 Pocket panel frame content window is undefined  main.js:699:13

Comment 1

[Mike de Boer [:mikedeboer]]

JS Errors are not nice, indeed. I will take a look at this. (It's kinda difficult not to throw all the issues on one big heap to fix them all at once, but will try not to.)
The Error throwing is a good indication, btw, that we're doing something wrong somewhere in PanelMultiView itself, not the SlidingPanelViews class.

Comment 2

[:Gijs (he/him)]

Attachment: fix-showMainView.txt

I had some WIP changes that seem to fix this earlier today.

Effectively this changes showMainView to only do something if the main view isn't what's currently displayed (which is what it did prior to our changes, I believe) and it ensures we dispatch ViewHiding for the subview if we do do something (which I *think* we might not be doing right now, because the panel is hidden so we'll not run those bits of `showSubView` (though I'm saying that last part from memory and could be wrong...).

The fallbacks for the view's first and last child is because some webextension tests run with panelviews that have no children at the point where we go looking for them (oops!), and DWU doesn't like being passed `null` to get bounds on.

Comment 3

[Mike de Boer [:mikedeboer]]

Sounds and looks like an excellent approach to me!

Comment 4

[:Gijs (he/him)]

Attachment: Bug 1369407 - make showMainView only switch views if the main view isn't current, dispatch ViewHiding, and fix view child assumptions,

Review commit: https://reviewboard.mozilla.org/r/145216/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/145216/

Comment 5

[:Gijs (he/him)]

Stealing given comment #3 :-)

Comment 6

[Mike de Boer [:mikedeboer]]

Comment on attachment 8873796 [details]
Bug 1369407 - make showMainView only switch views if the main view isn't current, dispatch ViewHiding, and fix view child assumptions,

https://reviewboard.mozilla.org/r/145216/#review149182

Even though the comment below makes me doubt this has gone through the appropriate amount of scrutiny, the changes look good to me. :-)
Please land it with that fix, though.

::: browser/components/customizableui/PanelMultiView.jsm:386
(Diff revision 1)
> -    if (this.panelViews) {
> -      this.showSubView(this._mainViewId);
> -    } else {
> -      if (this.showingSubView) {
> +    if (this.showingSubView) {
> -        let viewNode = this._currentSubView;
> +      let viewNode = this._currentSubView;
> -        let evt = new this.window.CustomEvent("ViewHiding", { bubbles: true, cancelable: true });
> +      let evt = new window.CustomEvent("ViewHiding", { bubbles: true, cancelable: true });

`window` is undefined here. Did you test this?
Please change to `this.window`

::: browser/components/customizableui/PanelMultiView.jsm:439
(Diff revision 1)
>            // Here go the measures that have the same caching lifetime as the width
>            // of the main view, i.e. 'forever', during the instance lifetime.
>            if (!this._mainViewWidth) {
>              this._mainViewWidth = previousRect.width;
> -            let top = dwu.getBoundsWithoutFlushing(previousViewNode.firstChild).top;
> -            let bottom = dwu.getBoundsWithoutFlushing(previousViewNode.lastChild).bottom;
> +            let top = dwu.getBoundsWithoutFlushing(previousViewNode.firstChild || previousViewNode).top;
> +            let bottom = dwu.getBoundsWithoutFlushing(previousViewNode.lastChild || previousViewNode).bottom;

Why did you need to make this change?

Comment 7

[:Gijs (he/him)]

Comment on attachment 8873796 [details]
Bug 1369407 - make showMainView only switch views if the main view isn't current, dispatch ViewHiding, and fix view child assumptions,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/145216/diff/1-2/

Comment 8

[:Gijs (he/him)]

Comment on attachment 8873796 [details]
Bug 1369407 - make showMainView only switch views if the main view isn't current, dispatch ViewHiding, and fix view child assumptions,

https://reviewboard.mozilla.org/r/145216/#review149182

> Why did you need to make this change?

Because it's possible for the previous view node to not (or no longer) have children at this point, in which case these references are null, in which case this code throws an exception because getBoundsWithoutFlushing doesn't tolerate a null argument.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/494ddcb16164
make showMainView only switch views if the main view isn't current, dispatch ViewHiding, and fix view child assumptions, r=mikedeboer

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/494ddcb16164