Closed Bug 1369407 Opened 2 years ago Closed 2 years ago

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

Categories

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

53 Branch
enhancement

Tracking

()

RESOLVED FIXED
Firefox 55
Iteration:
55.7 - Jun 12
Tracking Status
firefox55 --- fixed

People

(Reporter: Gijs, Assigned: Gijs)

References

Details

(Whiteboard: [photon-structure])

Attachments

(1 file, 1 obsolete file)

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
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.
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 55.7 - Jun 12
Priority: -- → P1
Flags: qe-verify?
Whiteboard: [photon-structure][triage] → [photon-structure]
Flags: qe-verify? → qe-verify-
Attached patch fix-showMainView.txt (obsolete) — Splinter Review
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.
Sounds and looks like an excellent approach to me!
Stealing given comment #3 :-)
Assignee: mdeboer → gijskruitbosch+bugs
Attachment #8873634 - Attachment is obsolete: true
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?
Attachment #8873796 - Flags: review?(mdeboer) → review+
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.
Pushed by gijskruitbosch@gmail.com:
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
https://hg.mozilla.org/mozilla-central/rev/494ddcb16164
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.