Closed Bug 1439358 Opened 6 years ago Closed 6 years ago

Avoid redundant PanelMultiView events when the panel is closed during ViewShowing events

Categories

(Firefox :: Toolbars and Customization, enhancement)

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

Tracking

()

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

People

(Reporter: Paolo, Assigned: Paolo)

References

(Blocks 1 open bug)

Details

Attachments

(8 files)

Bug 1439358 - Part 1 - Support "panelview" elements located anywhere in the document.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1439358 - Part 3 - Always raise ViewShowing events and don't update the "current" property.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1439358 - Part 4 - Open views before the transition and close them after it.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1439358 - Part 5 - Handle panel hiding during ViewShowing events.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1439358 - Part 6 - Decouple view events from view visibility.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1439358 - Part 7 - Close the main view synchronously before opening it in a different panel.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1439358 - Part 8 - Change how visibility is controlled so knownViews can be removed.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1439358 - Part 2 - Don't use the "current" property when restoring focus from the blocked downloads subview.
59 bytes, text/x-review-board-request
Gijs
: review+
Details 
Redundant ViewHiding and late ViewShown events are currently dispatched when a PanelMultiView panel is closed during ViewShowing events.

This can be solved by decoupling view events from view visibility, which will then allow the transition and clean up handling to be simplified.
Comment on attachment 8952151 [details]
Bug 1439358 - Part 1 - Support "panelview" elements located anywhere in the document.

This was carried over from bug 1437512.
Attachment #8952151 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8952151 [details]
Bug 1439358 - Part 1 - Support "panelview" elements located anywhere in the document.

https://reviewboard.mozilla.org/r/221390/#review227478
Comment on attachment 8952151 [details]
Bug 1439358 - Part 1 - Support "panelview" elements located anywhere in the document.

https://reviewboard.mozilla.org/r/221390/#review227492
Attachment #8952151 - Flags: review+
Attachment #8952151 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8952152 [details]
Bug 1439358 - Part 3 - Always raise ViewShowing events and don't update the "current" property.

https://reviewboard.mozilla.org/r/221392/#review227572

r=me assuming you can explain the issues below.

::: commit-message-4916e:5
(Diff revision 2)
> +Bug 1439358 - Part 2 - Always raise ViewShowing events and don't update the "current" property. r=Gijs
> +
> +The ViewShowing event is now called earlier and unconditionally, since we don't set the "current" attribute and call showMainView while the panel is closing anymore.
> +
> +It is already the case that the ViewShowing event handlers don't depend on the "current" property, so we don't need to keep track of it before ViewShown events are dispatched.

But this patch also changes the world such that anything else that might interrogate the panel's state between ViewShowing and ViewShown being dispatched now gets a different value for '.current'. Is there nothing that depends on that, either?

In particular, can you explain why this:

https://dxr.mozilla.org/mozilla-central/rev/dc70d241f90df43505ece5ac12261339e9694c50/browser/components/downloads/content/downloads.js#1610-1617

and this:

https://dxr.mozilla.org/mozilla-central/rev/dc70d241f90df43505ece5ac12261339e9694c50/browser/components/customizableui/content/panelUI.js#418-423

are not affected by this change?
Attachment #8952152 - Flags: review?(gijskruitbosch+bugs) → review+
There are no tests here at all. Please can you add unit-style tests for some of these problems that you're fixing? Effectively, whenever we've changed the behaviour of this module in the past, various other tests in webextension land or from other consumers have started failing because they depend on the intricacies of the panel in 'strange' ways. If we have unit-y tests in the CUI directory that check and enforce some of these behaviours, at least we can run those tests and be reasonably sure with future changes that we don't regress things, rather than having to debug the behaviour of the other tests and figuring out which invariant we inadvertently violated.

This would include things like hiding the panel from viewshowing, or from a timeout in viewshowing, or opening the library button while the main panel is open with the library view displayed, etc. etc.
Flags: needinfo?(paolo.mozmail)
(In reply to :Gijs from comment #17)
> In particular, can you explain why this:
> and this:
> are not affected by this change?

The panelUI.js instance handles the case where a view is already open in a different panel, for example clicking on the Library button while the Library subview is open in the main menu. In the case, the "current" property for the view is not changing. This code is removed in a later changeset anyways.

I was looking at the downloads.js instance but it just slipped from my mind, thanks for catching it. It is buggy, if not already in this changeset, then as the result of a later changeset. I'll rewrite it so it doesn't depend on this property.

Note that this also allows us to stop exposing the "current" property, since it's only actually used in these two instances and a couple of tests.
Comment on attachment 8952153 [details]
Bug 1439358 - Part 4 - Open views before the transition and close them after it.

https://reviewboard.mozilla.org/r/221394/#review227586

::: commit-message-c662c:1
(Diff revision 2)
> +Bug 1439358 - Part 3 - Open views before the transition and close them after it. r=Gijs

The long commit message needs to say *why* we're doing this. What bug is this fixing? Why does it make things better to do this?

::: browser/components/customizableui/PanelMultiView.jsm:29
(Diff revision 2)
> + *    All views start "closed" and are considered "open" as soon as they are
> + *    associated to a <panelmultiview> element for displaying. When a view is

This doesn't make sense to me. Some of these views are already children of the pmv at this point. When are they "associated" to this thing for displaying? The diagrams imply they are not 'open' from the beginning, but this text implies the opposite.

::: browser/components/customizableui/PanelMultiView.jsm:601
(Diff revision 2)
> -    this._showView(viewNode, anchor);
> +    let nextPanelView = PanelView.forNode(viewNode);
> +    if (this.openViews.includes(nextPanelView)) {
> +      throw new Error(`Subview ${viewNode.id} is already open.`);
> +    }
> +
> +    this._showView(nextPanelView, anchor);

I'm a little confused. This will fail if anything tries to call showSubView with either the main view or another view that's in the 'history' (ie open views) of that PMV, right? There are probably cases where this can happen, e.g. via some of the UI tour stuff or extensions' force-opening their browser/pageAction popups.

So, why does this need (or why is it useful) to  throw?

::: browser/components/customizableui/PanelMultiView.jsm
(Diff revision 2)
> -    if (!this.openViews.includes(nextPanelView))
> -      this.openViews.push(nextPanelView);
> -

Why was this here before, and why can it go away now?
Attachment #8952153 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs from comment #18)
> There are no tests here at all. Please can you add unit-style tests for some
> of these problems that you're fixing?

This is a good idea, I was considering doing this.

> This would include things like hiding the panel from viewshowing, or from a
> timeout in viewshowing

For these, I'll probably add a unit test file that creates a dedicated panel and panelmultiview element in the browser window, that are not linked to normal CustomizableUI events.
 
> or opening the library button while the main panel
> is open with the library view displayed, etc. etc.

This sounds like a CustomizableUI test instead. I can simulate the clicks and check that the Bookmarks subview can be opened correctly from the view in the Library button when it's opened with the menu already visible.
Comment on attachment 8952154 [details]
Bug 1439358 - Part 5 - Handle panel hiding during ViewShowing events.

https://reviewboard.mozilla.org/r/221396/#review227590

I think we should avoid all the exceptions here, and I think removing some of the checks for whether the panel is open is premature.

::: browser/components/customizableui/PanelMultiView.jsm:595
(Diff revision 2)
>     *        DOM element that triggered the subview, which will be highlighted
>     *        and whose "label" attribute will be used for the title of the
>     *        subview when a "title" attribute is not specified.
>     */
>    showSubView(viewIdOrNode, anchor) {
> +    this._showSubView(viewIdOrNode, anchor).catch(Cu.reportError);

As I commented on the previous cset, throwing seems a bit harsh, and now you take the sting out of it. Maybe just `Cu.reportError` directly, and 'just' return instead of throwing. Are there other exceptions that we're dealing with in this way, or something?

::: browser/components/customizableui/PanelMultiView.jsm:631
(Diff revision 2)
> +                               (anchor && anchor.getAttribute("label"));
> +    // The constrained width of subviews may also vary between panels.
> +    nextPanelView.minMaxWidth = prevPanelView.knownWidth;
> +
> +    if (anchor) {
> +      viewNode.classList.add("PanelUI-subView");

This class should be reset as well, presumably? At least in `showMainView`, maybe also here if `!anchor`.

::: browser/components/customizableui/PanelMultiView.jsm:661
(Diff revision 2)
> +  /**
> +   * Prepares the main view before showing the panel.
> +   */
>    async showMainView() {
> -    if (!this.node || !this._mainViewId)
> +    if (!this.node || !this._mainViewId) {

It's even stranger that this doesn't catch anything like `showSubView` and `goBack`, while doing very similar things.

::: browser/components/customizableui/PanelMultiView.jsm:709
(Diff revision 2)
> -  async _showView(nextPanelView, anchor, reverse) {
> -    try {
> -      let viewNode = nextPanelView.node;
> -      this.knownViews.add(nextPanelView);
> -
> -      viewNode.panelMultiView = this.node;
> +  /**
> +   * Opens the specified PanelView if allowed by the ViewShowing event.
> +   *
> +   * @resolves With true if the view was opened, false otherwise.
> +   */
> +  async _openView(panelView) {

This comment is wrong, because all this does is sending the event. It doesn't do anything after that, and you pushed out any changes to openViews to the callers.

::: browser/components/customizableui/PanelMultiView.jsm:890
(Diff revision 2)
>  
>      details.phase = TRANSITION_PHASES.END;
>  
>      await this._cleanupTransitionPhase(details);
> +
> +    nextPanelView.focusSelectedElement();

Why do we no longer need to check if the panel is open? Seems to me that the await on the previous line is liable to only be resolved once the panel has disappeared.
Attachment #8952154 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs from comment #20)
> The long commit message needs to say *why* we're doing this. What bug is
> this fixing? Why does it make things better to do this?

I had some short text in a previous incarnation, but eventually I thought it would be redundant with the changesets before and after it. I can still try to write something though.

> ::: browser/components/customizableui/PanelMultiView.jsm:29
> (Diff revision 2)
> > + *    All views start "closed" and are considered "open" as soon as they are
> > + *    associated to a <panelmultiview> element for displaying. When a view is
> 
> This doesn't make sense to me. Some of these views are already children of
> the pmv at this point. When are they "associated" to this thing for
> displaying? The diagrams imply they are not 'open' from the beginning, but
> this text implies the opposite.

I can probably clarify by expanding on what "associated for displaying" means.

> ::: browser/components/customizableui/PanelMultiView.jsm:601
> (Diff revision 2)
> > -    this._showView(viewNode, anchor);
> > +    let nextPanelView = PanelView.forNode(viewNode);
> > +    if (this.openViews.includes(nextPanelView)) {
> > +      throw new Error(`Subview ${viewNode.id} is already open.`);
> > +    }
> > +
> > +    this._showView(nextPanelView, anchor);
> 
> I'm a little confused. This will fail if anything tries to call showSubView
> with either the main view or another view that's in the 'history' (ie open
> views) of that PMV, right? There are probably cases where this can happen,
> e.g. via some of the UI tour stuff or extensions' force-opening their
> browser/pageAction popups.

Interesting, if this is the case then I don't think it ever worked, because showSubView assumes a certain order of the view elements to perform the transition properly. Surely it doesn't have tests. I'm thinking I could file a bug to see if this should be supported for UITour, but probably it's such an edge case that it's not worth the additional complexity anyways - and it's better to throw than to go to an inconsistent state silently.

> ::: browser/components/customizableui/PanelMultiView.jsm
> (Diff revision 2)
> > -    if (!this.openViews.includes(nextPanelView))
> > -      this.openViews.push(nextPanelView);
> > -
> 
> Why was this here before, and why can it go away now?

It's been replaced by the other this.openViews.push calls.
Comment on attachment 8952155 [details]
Bug 1439358 - Part 6 - Decouple view events from view visibility.

https://reviewboard.mozilla.org/r/221398/#review227592

::: browser/components/customizableui/PanelMultiView.jsm:1122
(Diff revision 2)
>          this.descriptionHeightWorkaround();
> -        this.dispatchCustomEvent("ViewShown");

Why aren't we moving the descriptionHeightWorkaround into the `_viewShown` helper?
Attachment #8952155 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8952369 [details]
Bug 1439358 - Part 7 - Close the main view synchronously before opening it in a different panel.

https://reviewboard.mozilla.org/r/221628/#review227598

::: browser/components/customizableui/PanelMultiView.jsm:679
(Diff revision 1)
> +    // If the view is already open in another panel, close the panel first.
> +    let oldPanelMultiViewNode = nextPanelView.node.panelMultiView;
> +    if (oldPanelMultiViewNode) {
> +      PanelMultiView.forNode(oldPanelMultiViewNode).hidePopup();
> +    }

This technically doesn't check whether the view is open in the same panel. Which presumably is why you changed some of the callsites. Can't we just check if `oldPanelMultiViewNode` is identical to `this.node` and cope?
Attachment #8952369 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8952370 [details]
Bug 1439358 - Part 8 - Change how visibility is controlled so knownViews can be removed.

https://reviewboard.mozilla.org/r/221630/#review227600

::: browser/components/customizableui/PanelMultiView.jsm:694
(Diff revision 1)
>      nextPanelView.mainview = true;
>      nextPanelView.headerText = "";
>      nextPanelView.minMaxWidth = 0;
>  
>      await this._cleanupTransitionPhase();
> -    this.hideAllViewsExcept(nextPanelView);
> +    nextPanelView.visible = true;

`_transitionViews`, which the 2 other methods that alter the visible view (`goBack` and `showSubView`) use, also marks the previous view as hidden. The previous implementation (`hideAllViewsExcept`) did that too. But you don't do it here anymore. Why?

::: browser/components/customizableui/PanelMultiView.jsm:913
(Diff revision 1)
> +    if (nextPanelView.node.panelMultiView == this.node) {
> +      nextPanelView.visible = true;
> +      prevPanelView.visible = false;
> +    }

This really wants a comment.

::: browser/components/customizableui/PanelMultiView.jsm:1118
(Diff revision 1)
>        if (!this.node.hasAttribute("current")) {
>          this.node.setAttribute("current", true);

Can we rename the attribute? There's less than a handful of uses in CSS (potentially only 1).
Attachment #8952370 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs from comment #22)
> As I commented on the previous cset, throwing seems a bit harsh, and now you
> take the sting out of it. Maybe just `Cu.reportError` directly, and 'just'
> return instead of throwing.

Sounds good, it's effectively equivalent.

> Are there other exceptions that we're dealing
> with in this way, or something?

I don't understand the question, unless it is related to the question below.

> >    async showMainView() {
> 
> It's even stranger that this doesn't catch anything like `showSubView` and
> `goBack`, while doing very similar things.

So if you call an async function from an async function, you have to propagate the resolution or rejection state. But if you call an async function from a standard function, you have to deal with any rejection state yourself, because the standard function always returns sooner. Normally this is done by terminating the Promise chain using "Cu.reportError".

We could make showSubView and goBack asynchronous, but it would simply mean that we would have to move the "Cu.reportError" to each of the callers, because they are not designed to wait and so they would have no use for the returned Promise.

> ::: browser/components/customizableui/PanelMultiView.jsm:631
> > +    if (anchor) {
> > +      viewNode.classList.add("PanelUI-subView");
> 
> This class should be reset as well, presumably?

Well, maybe or maybe not. This particular inconsistency, as well as a number of other issues with the styling, has been there for the entirety of this work, and I'd like to investigate them in a final cleanup. Probably we just need to set the class statically on the "panelview" elements, or event get rid of the class entirely and change how we style the elements. We already have a "mainview" attribute that is redundant with what this was probably trying to accomplish.

For now, I went with just keeping the existing inconsistency.

> ::: browser/components/customizableui/PanelMultiView.jsm:709
> > +  /**
> > +   * Opens the specified PanelView if allowed by the ViewShowing event.
> > +   *
> > +   * @resolves With true if the view was opened, false otherwise.
> > +   */
> > +  async _openView(panelView) {
> 
> This comment is wrong, because all this does is sending the event. It
> doesn't do anything after that, and you pushed out any changes to openViews
> to the callers.

This means "open" in the sense given to it by the comment at the beginning of the file, which doesn't mean "visible". Maybe I should call this state "attached" instead? Would it be clearer?

> ::: browser/components/customizableui/PanelMultiView.jsm:890
> > +    nextPanelView.focusSelectedElement();
> 
> Why do we no longer need to check if the panel is open? Seems to me that the
> await on the previous line is liable to only be resolved once the panel has
> disappeared.

Right, this can still happen, but now the most common case of the panel hiding during the ViewHiding event is handled before we even try to start the transition. For the remaining cases, we'll actually have to test that the view is still attached after each and every "await" statement. This is already an issue and this patch doesn't make things worse.
(In reply to :Gijs from comment #24)
> Why aren't we moving the descriptionHeightWorkaround into the `_viewShown`
> helper?

Because it's related to visibility, however we could move it out to the place where we set the visibility. I was thinking of doing it later but might as well do it sooner.
Flags: needinfo?(paolo.mozmail)
(In reply to :Gijs from comment #25)
> This technically doesn't check whether the view is open in the same panel.
> Which presumably is why you changed some of the callsites. Can't we just
> check if `oldPanelMultiViewNode` is identical to `this.node` and cope?

This can't happen in showMainView because of the re-entrancy checks, so we're always starting with an empty openViews array. It is now the case that the panelMultiView property is set to "this.node" if and only if the view is in openViews (which we could rename to attachedViews or something else according to the previous review comments).

The checks I added in showSubView for the openViews array are effectively handling the case you're mentioning here where the view is open in the same panel.
(In reply to :Gijs from comment #26)
> `_transitionViews`, which the 2 other methods that alter the visible view
> (`goBack` and `showSubView`) use, also marks the previous view as hidden.
> The previous implementation (`hideAllViewsExcept`) did that too. But you
> don't do it here anymore. Why?

Because all the views that were previously open in this panel are now guaranteed to be closed / detached, which implies that they're hidden. We have also closed synchronously any other panel from which we may have moved the view.

> ::: browser/components/customizableui/PanelMultiView.jsm:1118
> (Diff revision 1)
> >        if (!this.node.hasAttribute("current")) {
> >          this.node.setAttribute("current", true);
> 
> Can we rename the attribute? There's less than a handful of uses in CSS
> (potentially only 1).

I was thinking of doing this in a separate changeset, while removing the "in-transition" attribute at the same time.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Paolo Amadini from comment #27)
> (In reply to :Gijs from comment #22)
> > As I commented on the previous cset, throwing seems a bit harsh, and now you
> > take the sting out of it. Maybe just `Cu.reportError` directly, and 'just'
> > return instead of throwing.
> 
> Sounds good, it's effectively equivalent.
> 
> > Are there other exceptions that we're dealing
> > with in this way, or something?
> 
> I don't understand the question, unless it is related to the question below.
> 
> > >    async showMainView() {
> > 
> > It's even stranger that this doesn't catch anything like `showSubView` and
> > `goBack`, while doing very similar things.
> 
> So if you call an async function from an async function, you have to
> propagate the resolution or rejection state. But if you call an async
> function from a standard function, you have to deal with any rejection state
> yourself, because the standard function always returns sooner. Normally this
> is done by terminating the Promise chain using "Cu.reportError".

You don't "have to" if there are no errors...

> We could make showSubView and goBack asynchronous, but it would simply mean
> that we would have to move the "Cu.reportError" to each of the callers,
> because they are not designed to wait and so they would have no use for the
> returned Promise.

I don't really understand the reply. My point was very simple. We catch errors that happen in _showSubView and _goBack. But we don't catch errors for showMainView, even though they do broadly similar things. Why? Are we catching any errors other than the ones that are being explicitly thrown in _showSubView ? Or can we just omit these catches entirely once we remove the explicit error throwing?

> > ::: browser/components/customizableui/PanelMultiView.jsm:631
> > > +    if (anchor) {
> > > +      viewNode.classList.add("PanelUI-subView");
> > 
> > This class should be reset as well, presumably?
> 
> Well, maybe or maybe not. This particular inconsistency, as well as a number
> of other issues with the styling, has been there for the entirety of this
> work, and I'd like to investigate them in a final cleanup. Probably we just
> need to set the class statically on the "panelview" elements, or event get
> rid of the class entirely and change how we style the elements. We already
> have a "mainview" attribute that is redundant with what this was probably
> trying to accomplish.
> 
> For now, I went with just keeping the existing inconsistency.
> 
> > ::: browser/components/customizableui/PanelMultiView.jsm:709
> > > +  /**
> > > +   * Opens the specified PanelView if allowed by the ViewShowing event.
> > > +   *
> > > +   * @resolves With true if the view was opened, false otherwise.
> > > +   */
> > > +  async _openView(panelView) {
> > 
> > This comment is wrong, because all this does is sending the event. It
> > doesn't do anything after that, and you pushed out any changes to openViews
> > to the callers.
> 
> This means "open" in the sense given to it by the comment at the beginning
> of the file, which doesn't mean "visible". Maybe I should call this state
> "attached" instead? Would it be clearer?

No, because 'open' as defined in the beginning of the file maps to 'openViews', and the comment here specifically says that it opens the view (implied: adds it to openViews). But it doesn't. The callers do that. All this method does is fire viewShowing, hook things up to the pmv, and then returns whether or not people complained in ViewShowing. We later make determinations about whether the view is open by specifically checking (or just expecting) that it is/isn't in this.openViews. So the comment is just wrong.

> > ::: browser/components/customizableui/PanelMultiView.jsm:890
> > > +    nextPanelView.focusSelectedElement();
> > 
> > Why do we no longer need to check if the panel is open? Seems to me that the
> > await on the previous line is liable to only be resolved once the panel has
> > disappeared.
> 
> Right, this can still happen, but now the most common case of the panel
> hiding during the ViewHiding event is handled before we even try to start
> the transition. For the remaining cases, we'll actually have to test that
> the view is still attached after each and every "await" statement. This is
> already an issue and this patch doesn't make things worse.

Maybe, but it's trivial to just put this in a `if (this._panel.state == "open")` (which it was before!).


(In reply to :Paolo Amadini from comment #29)
> (In reply to :Gijs from comment #25)
> > This technically doesn't check whether the view is open in the same panel.
> > Which presumably is why you changed some of the callsites. Can't we just
> > check if `oldPanelMultiViewNode` is identical to `this.node` and cope?
> 
> This can't happen in showMainView

Sure it can? showMainView is a public method. Anyone can call it directly at any time. It could be called when the main view is already the current view in the same panelmultiview.

> because of the re-entrancy checks,

Which ones?

  async showMainView() {
    if (!this.node || !this._mainViewId) {
      return false;
    }

    let nextPanelView = PanelView.forNode(this._mainView);

    // If the view is already open in another panel, close the panel first.
    let oldPanelMultiViewNode = nextPanelView.node.panelMultiView;
    if (oldPanelMultiViewNode) {
      PanelMultiView.forNode(oldPanelMultiViewNode).hidePopup();


is all of the code I see. If the panel is open and has a main view id, this code just runs.

> so
> we're always starting with an empty openViews array.

This also doesn't seem to be true to me.

> It is now the case that
> the panelMultiView property is set to "this.node" if and only if the view is
> in openViews (which we could rename to attachedViews or something else
> according to the previous review comments).
> 
> The checks I added in showSubView for the openViews array are effectively
> handling the case you're mentioning here where the view is open in the same
> panel.

Right, but showMainView isn't. Why not?

(In reply to :Paolo Amadini from comment #30)
> (In reply to :Gijs from comment #26)
> > `_transitionViews`, which the 2 other methods that alter the visible view
> > (`goBack` and `showSubView`) use, also marks the previous view as hidden.
> > The previous implementation (`hideAllViewsExcept`) did that too. But you
> > don't do it here anymore. Why?
> 
> Because all the views that were previously open in this panel are now
> guaranteed to be closed / detached, which implies that they're hidden. We
> have also closed synchronously any other panel from which we may have moved
> the view.

Same issue here. I don't see what guarantees that at all. Anyone can call showMainView at any time. You removed the visibility changes from _cleanupTransitionPhase. So what guarantees that all the views are closed?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(paolo.mozmail)
(In reply to :Gijs from comment #31)
> > Normally this is done by terminating the Promise chain using "Cu.reportError".
> 
> You don't "have to" if there are no errors...

If the async function you call guarantees that it doesn't reject, similar to standard functions that guarantee they catch exceptions, then you don't have to. If you're propagating the Promise state to another Promise, then you don't have to. In all other cases, you definitely should, or it would result in unhandled rejections. I also filed bug 1362972 to add an ESLint rule to help with cases like this where we can detect them.

> > We could make showSubView and goBack asynchronous, but it would simply mean
> > that we would have to move the "Cu.reportError" to each of the callers,
> > because they are not designed to wait and so they would have no use for the
> > returned Promise.
> 
> I don't really understand the reply. My point was very simple. We catch
> errors that happen in _showSubView and _goBack. But we don't catch errors
> for showMainView, even though they do broadly similar things. Why?

It's because showMainView propagates all the errors to the caller, which is responsible for handling them. On the other hand, showSubView and goBack (without the underscores) can't propagate any asynchronous errors, because they return before the asynchronous code is executed.

> Are we
> catching any errors other than the ones that are being explicitly thrown in
> _showSubView ?

Yes, this helps with reporting and debugging unexpected situations in the asynchronous function, that would otherwise be left unhandled.

> > This means "open" in the sense given to it by the comment at the beginning
> > of the file, which doesn't mean "visible". Maybe I should call this state
> > "attached" instead? Would it be clearer?
> 
> No, because 'open' as defined in the beginning of the file maps to
> 'openViews', and the comment here specifically says that it opens the view
> (implied: adds it to openViews).

I see what you mean now, I agree this method should be renamed. (An earlier version did indeed add the view to openViews, but I took that bit out so we can check the length of the array in showSubView but not showMainView.)

> > > ::: browser/components/customizableui/PanelMultiView.jsm:890
> > > > +    nextPanelView.focusSelectedElement();
> 
> Maybe, but it's trivial to just put this in a `if (this._panel.state ==
> "open")` (which it was before!).

Ok, though I'd probably check whether nextPanelView is still open instead, to be consistent with the rest of the code. I'll have to add similar checks later and I'll possibly have a helper function for it, but as you said it's trivial to just add the line.

> (In reply to :Paolo Amadini from comment #29)
> > (In reply to :Gijs from comment #25)
> > > This technically doesn't check whether the view is open in the same panel.
> > > Which presumably is why you changed some of the callsites. Can't we just
> > > check if `oldPanelMultiViewNode` is identical to `this.node` and cope?
> > 
> > This can't happen in showMainView
> 
> Sure it can? showMainView is a public method.

Ah, right, it's not a public method anymore as of part 6, so it's effectively protected by the checks in openPopup. I worked under this assumption because I don't think it makes sense to handle potential callers that are just about to be removed. This answers most of the other questions I guess.

Maybe I should add an underscore to the "showMainView" method when it becomes private? I'm not sure we're applying this consistently though.
Flags: needinfo?(paolo.mozmail)
Blocks: 1352792
Note that while in Part 2 I have updated the test for the blocked downloads subview, I have not re-enabled it in automation yet. This is to avoid risk related to the intermittents from bug 1352792, which may have different causes.
Gijs, maybe you should mark Part 1 as reviewed, because if I do it MozReview continues to bring back the review flag.
Blocks: 1440333
Comment on attachment 8952151 [details]
Bug 1439358 - Part 1 - Support "panelview" elements located anywhere in the document.

https://reviewboard.mozilla.org/r/221390/#review228282

Stamping this per bug comment.
Attachment #8952151 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8953068 [details]
Bug 1439358 - Part 2 - Don't use the "current" property when restoring focus from the blocked downloads subview.

https://reviewboard.mozilla.org/r/222344/#review228286
Attachment #8953068 - Flags: review?(gijskruitbosch+bugs) → review+
Fixed the unused variable and the reflow test stack in Part 8.
I'm working on writing new unit tests next. Would you be fine with landing these patches before I've finished the unit tests, given the patches are covered by other tests, or do you strongly prefer having the unit tests at the same time? An advantage of landing these first is that we may uncover non-unit regressions sooner, in other words unit test won't catch any edge case in consumers that rely on the wrong assumptions.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Paolo Amadini from comment #48)
> I'm working on writing new unit tests next. Would you be fine with landing
> these patches before I've finished the unit tests, given the patches are
> covered by other tests, or do you strongly prefer having the unit tests at
> the same time? An advantage of landing these first is that we may uncover
> non-unit regressions sooner, in other words unit test won't catch any edge
> case in consumers that rely on the wrong assumptions.

I think we can live with the unit tests being a follow-up, yeah.
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8952153 [details]
Bug 1439358 - Part 4 - Open views before the transition and close them after it.

https://reviewboard.mozilla.org/r/221394/#review228294
Attachment #8952153 - Flags: review?(gijskruitbosch+bugs) → review+
Blocks: 1440358
Comment on attachment 8952154 [details]
Bug 1439358 - Part 5 - Handle panel hiding during ViewShowing events.

https://reviewboard.mozilla.org/r/221396/#review228298

::: browser/components/customizableui/PanelMultiView.jsm:899
(Diff revision 3)
>        });
>      });
>  
>      details.phase = TRANSITION_PHASES.END;
>  
>      await this._cleanupTransitionPhase(details);

Can you add a comment at both of the callsites of `_cleanup...` in this method? It's pretty confusing that we need to call this twice, from a single transition helper method.
Attachment #8952154 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8952370 [details]
Bug 1439358 - Part 8 - Change how visibility is controlled so knownViews can be removed.

https://reviewboard.mozilla.org/r/221630/#review228302

::: browser/components/customizableui/PanelMultiView.jsm:340
(Diff revision 3)
> +  get showingSubView() {
> +    return this.openViews.length > 1;
> +  }

Why can't we use this in the downloads thingy that's in one of the first csets?

::: browser/components/customizableui/PanelMultiView.jsm:929
(Diff revision 3)
>  
> +    // Apply the final visibility, unless the view was closed in the meantime.
> +    if (nextPanelView.node.panelMultiView == this.node) {
> +      prevPanelView.visible = false;
> +      nextPanelView.visible = true;
> +      nextPanelView.descriptionHeightWorkaround();

So setting `.visible` to true already used to run descriptionHeightWorkaround. All you're doing is moving it to the callsites - but you do this every time you set `.visible` to true, so you might as well leave it in the setter, right? :-\

::: browser/components/customizableui/PanelMultiView.jsm
(Diff revision 3)
> -      if (!this.node.hasAttribute("current")) {
> -        this.node.setAttribute("current", true);
> +      this.node.setAttribute("current", true);

This had a re-entrancy check for running descriptionHeightWorkaround multiple times. Do we not need that anymore? Why (not)?
Attachment #8952370 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Paolo Amadini from comment #32)
> Maybe I should add an underscore to the "showMainView" method when it
> becomes private? I'm not sure we're applying this consistently though.

"We are definitely not applying this consistently" could be a reasonable argument not to update something (like if I were telling you to insert an include/import in sorted order in a list that was clearly not sorted at all), but I don't think that applies here. Please explicitly mark it as internal by using the underscore. If there are other egregious internal/external things that want updating, I will happily review changes in a separate bug. If we're thinking about API surface and re-entrancy, having this type of thing give off the wrong signal is highly frustrating both for reviews and debugging.
(In reply to :Gijs from comment #53)
> Please explicitly mark it as internal by using the underscore.

I did this in Part 7 in fact.
(In reply to :Gijs from comment #52)
> Why can't we use this in the downloads thingy that's in one of the first
> csets?

We definitely can, and I considered using it, but I was also considering removing the showingSubView public property.

The single other consumer of the property seems to have a valid use case for it, that is deciding whether to go back to the previous view or to close the panel. However, this could also be done by having the goBack method close the panel when invoked while the main view is visible, which would be probably a sensible thing to do from an API standpoint.

> So setting `.visible` to true already used to run
> descriptionHeightWorkaround. All you're doing is moving it to the callsites
> - but you do this every time you set `.visible` to true, so you might as
> well leave it in the setter, right? :-\

I think you suggested doing this earlier? In fact, I think it's clearer if it's moved to the callers. The descriptionHeightWorkaround calls should definitely not be linked to setting the visibility, and I will move them to a different point in the flow and make them asynchronous in bug 1420939.

> ::: browser/components/customizableui/PanelMultiView.jsm
> This had a re-entrancy check for running descriptionHeightWorkaround
> multiple times. Do we not need that anymore? Why (not)?

Yes, this is why I didn't move this call out of the attribute earlier. After the changes in this patch, we finally know precisely the previous state of the view, while hideAllViewsExcept didn't have a way to know it.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Paolo Amadini from comment #55)
> (In reply to :Gijs from comment #52)
> > Why can't we use this in the downloads thingy that's in one of the first
> > csets?
> 
> We definitely can, and I considered using it, but I was also considering
> removing the showingSubView public property.
> 
> The single other consumer of the property seems to have a valid use case for
> it, that is deciding whether to go back to the previous view or to close the
> panel. However, this could also be done by having the goBack method close
> the panel when invoked while the main view is visible, which would be
> probably a sensible thing to do from an API standpoint.
> 
> > So setting `.visible` to true already used to run
> > descriptionHeightWorkaround. All you're doing is moving it to the callsites
> > - but you do this every time you set `.visible` to true, so you might as
> > well leave it in the setter, right? :-\
> 
> I think you suggested doing this earlier?

I suggested moving it to another method, which I thought was also called from all the requisite places.

> In fact, I think it's clearer if
> it's moved to the callers. The descriptionHeightWorkaround calls should
> definitely not be linked to setting the visibility, and I will move them to
> a different point in the flow and make them asynchronous in bug 1420939.

OK.


I'm bouncing ni because I'm not sure if you had a question - besides the "did you suggest this" one there wasn't really one in your comment...
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(paolo.mozmail)
(In reply to :Gijs from comment #60)
> I'm bouncing ni because I'm not sure if you had a question - besides the
> "did you suggest this" one there wasn't really one in your comment...

That was in place of requesting review again, which MozReview did for me anyways when I updated the earlier changeset.
Flags: needinfo?(paolo.mozmail)
Comment on attachment 8952370 [details]
Bug 1439358 - Part 8 - Change how visibility is controlled so knownViews can be removed.

https://reviewboard.mozilla.org/r/221630/#review228328
Attachment #8952370 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca3804ac094e
Part 1 - Support "panelview" elements located anywhere in the document. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/28ff0b51399f
Part 2 - Don't use the "current" property when restoring focus from the blocked downloads subview. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/525b36047e16
Part 3 - Always raise ViewShowing events and don't update the "current" property. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/3de93b4c00b2
Part 4 - Open views before the transition and close them after it. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/b817cfb3367f
Part 5 - Handle panel hiding during ViewShowing events. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/98edf86dec13
Part 6 - Decouple view events from view visibility. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/60dd06c598f3
Part 7 - Close the main view synchronously before opening it in a different panel. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/01e3d1408dbe
Part 8 - Change how visibility is controlled so knownViews can be removed. r=Gijs
Depends on: 1440973
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: