Closed Bug 1440358 Opened 6 years ago Closed 6 years ago

Add unit tests for PanelMultiView

Categories

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

Product:
Firefox
Component:
Toolbars and Customization
Version:
59 Branch
Type:
enhancement

Tracking

()

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

People

(Reporter: Paolo, Assigned: Paolo)

References

(Blocks 2 open bugs)

Details

Attachments

(3 files)

Bug 1440358 - Part 1 - Add unit tests for PanelMultiView.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1440358 - Part 2 - Add tests for cancellation while opening a view.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1440358 - Part 3 - Add tests for reopening a visible Library view.
59 bytes, text/x-review-board-request
Gijs
: review+
Details 
The PanelMultiView module is tested through CustomizableUI and other consumers, but doesn't have unit tests at present.

These tests can now be added starting with the new event ordering in bug 1439358. With the occasion, we can also add some tests for cases that may be triggered by CustomizableUI and are not currently exercised.
Note that the layout issue I found here may be related to the comment noting that "putting these properties in PanelUI.css doesn't work for newly shown nodes in a XUL parent node". It's possible that this is now fixed and we'll be able to move the animation properties to CSS. I'll also investigate this later.
(In reply to :Paolo Amadini from comment #2)
> Note that the layout issue I found here may be related to the comment noting
> that "putting these properties in PanelUI.css doesn't work for newly shown
> nodes in a XUL parent node". It's possible that this is now fixed and we'll
> be able to move the animation properties to CSS. I'll also investigate this
> later.

Actually, this fixes an issue that prevents the "in-transition" attribute from being removed. I'll move this bit to bug 1440333 and I'll work on it first. This will also allow me to use the ViewShown event for one of the tests here.
Depends on: 1440333
Comment on attachment 8953436 [details]
Bug 1440358 - Part 1 - Add unit tests for PanelMultiView.

https://reviewboard.mozilla.org/r/222698/#review228656

Thanks for doing this. I'm super happy with how this is immediately pointing to concrete small improvements we can make to the code under test. Can we add tests for when the viewshowing blocks showing the view and/or popup?

::: browser/components/customizableui/PanelMultiView.jsm:675
(Diff revision 1)
> +      // Wait for a layout flush after hiding the popup, otherwise the view may
> +      // not be displayed correctly for some time after the new panel is opened.
> +      await BrowserUtils.promiseLayoutFlushed(this.document, "layout",
> +                                              () => {});

What *is* the layout issue here? Also, bug 1434376 hasn't landed yet so I'm worried about landing this as-is (see also bug 1424823).

::: browser/components/customizableui/test/browser_PanelMultiView.js:25
(Diff revision 1)
> +function is_visible(element) {
> +  var style = element.ownerGlobal.getComputedStyle(element);

This forces a layout flush, so in a test that's meant to test the behaviour of code that is dependent on when layout does certain things, that doesn't seem wise. Can we use lazy bounds getters and just check that the node has non-0 bounds?

::: browser/components/customizableui/test/browser_PanelMultiView.js:150
(Diff revision 1)
> +    gPanelAnchors[i].classList.add("toolbarbutton-1",
> +                              "chromeclass-toolbar-additional");

This indenting is weird. Either align or use 2-space. Also, I would just use `.className`, but meh.

::: browser/components/customizableui/test/browser_PanelMultiView.js:155
(Diff revision 1)
> +    gPanelAnchors[i].classList.add("toolbarbutton-1",
> +                              "chromeclass-toolbar-additional");
> +    navBar.appendChild(gPanelAnchors[i]);
> +
> +    gPanels[i] = document.createElement("panel");
> +    gPanels[i].setAttribute("id", "panel-" + i);

Just assign to `.id`. Here and in the other places where you set an id for an element.

::: browser/components/customizableui/test/browser_PanelMultiView.js:157
(Diff revision 1)
> +    navBar.appendChild(gPanelAnchors[i]);
> +
> +    gPanels[i] = document.createElement("panel");
> +    gPanels[i].setAttribute("id", "panel-" + i);
> +    gPanels[i].setAttribute("type", "arrow");
> +    gPanels[i].setAttribute("photon", true);

Please file a bug on removing this attribute. It's only used for 2 rules in panelUI.inc.css and should just be dumped.
Attachment #8953436 - Flags: review?(gijskruitbosch+bugs)
Blocks: 1441015
(In reply to :Gijs from comment #4)
> What *is* the layout issue here?

I filed bug 1441015 and added the bug number to the code. I went through the following lists to see if there was already a similar bug on file, but the most similar seems to be bug 692348 which is likely unrelated:

https://bugzilla.mozilla.org/buglist.cgi?quicksearch=XUL%20panel
https://bugzilla.mozilla.org/buglist.cgi?quicksearch=XUL%20popup

> Also, bug 1434376 hasn't landed yet so I'm
> worried about landing this as-is (see also bug 1424823).

I'll rebase these patches on top of bug 1434376.

> This forces a layout flush, so in a test that's meant to test the behaviour
> of code that is dependent on when layout does certain things, that doesn't
> seem wise. Can we use lazy bounds getters and just check that the node has
> non-0 bounds?

We cannot use lazy bound getters because the API does not guarantee that no reflows are pending when the events are called, so we might be getting wrong values when actually the code under testing is correct.

I've added a comment to that effect. Note that we could make the function asynchronous, wait for the document to be flushed, and then use the lazy bound getter, but I think the current approach works fine and actually helped detect bug 1441015.

> ::: browser/components/customizableui/test/browser_PanelMultiView.js:150
> (Diff revision 1)
> > +    gPanelAnchors[i].classList.add("toolbarbutton-1",
> > +                              "chromeclass-toolbar-additional");
> 
> This indenting is weird. Either align or use 2-space. Also, I would just use
> `.className`, but meh.

For what it's worth, this got misaligned after a rename, thanks for catching it.

> Please file a bug on removing this attribute. It's only used for 2 rules in
> panelUI.inc.css and should just be dumped.

Filed bug 1441017.
Comment on attachment 8953436 [details]
Bug 1440358 - Part 1 - Add unit tests for PanelMultiView.

https://reviewboard.mozilla.org/r/222698/#review229012
Attachment #8953436 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8953856 [details]
Bug 1440358 - Part 2 - Add tests for cancellation while opening a view.

https://reviewboard.mozilla.org/r/223042/#review229014

::: browser/components/customizableui/test/browser_PanelMultiView.js:375
(Diff revision 1)
> +  stopRecordingEvents(gPanels[0]);
> +
> +  Assert.deepEqual(recordArray, [
> +    "panelview-0: ViewShowing",
> +    "panelview-0: ViewHiding",
> +    "panelmultiview-0: popuphidden",

Why doesn't the `popuphidden` event fire on the panel/popup, like it does elsewhere?

Also, why doesn't the `PanelMultiViewHidden` event fire? Could any consumers be confused by this?

::: browser/components/customizableui/test/browser_PanelMultiView.js:444
(Diff revision 1)
> +    "panelview-0: ViewShowing > panelmultiview-0: popuphidden",
> +    "panelview-0: ViewShowing > panelview-0: ViewHiding",

Why does the `popuphidden` event happen *before* the `ViewHiding` event, instead of vice versa, like elsewhere?
Attachment #8953856 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8953857 [details]
Bug 1440358 - Part 3 - Add tests for reopening a visible Library view.

https://reviewboard.mozilla.org/r/223044/#review229046

::: browser/components/customizableui/test/browser_library_after_appMenu.js:1
(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/. */

Public domain for tests please.

::: browser/components/customizableui/test/browser_library_after_appMenu.js:5
(Diff revision 1)
> +/**
> + * Checks that opening the Library view using the default toolbar button works
> + * also while the view is displayed in the main menu.
> + */

Please put this after `use strict`.
Attachment #8953857 - Flags: review?(gijskruitbosch+bugs) → review+
Blocks: 1438507
Comment on attachment 8953856 [details]
Bug 1440358 - Part 2 - Add tests for cancellation while opening a view.

Thanks for looking into this, I've added these issues to bug 1438507.
Attachment #8953856 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs from comment #11)
> Why doesn't the `popuphidden` event fire on the panel/popup, like it does
> elsewhere?
> Also, why doesn't the `PanelMultiViewHidden` event fire? Could any consumers
> be confused by this?
> Why does the `popuphidden` event happen *before* the `ViewHiding` event,
> instead of vice versa, like elsewhere?

And to answer these questions, these are because of the synthetic events we generate during canellation. I think we should probably clean these up at some point, but the situation is already much better than it was before these workarounds were introduced.
(In reply to :Gijs from comment #12)
> Please put this after `use strict`.

I was going to say that this had to be put before in order for DXR to index it as the file description:

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/src

However, I found at least one counterexample, and it seems that the Public Domain header confuses the parser anyways. So I guess it's fine to put this after, so that it looks like a test function description rather than a file description.
Comment on attachment 8953856 [details]
Bug 1440358 - Part 2 - Add tests for cancellation while opening a view.

https://reviewboard.mozilla.org/r/223042/#review229086
Attachment #8953856 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/209f0a2bb451
Part 1 - Add unit tests for PanelMultiView. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/bec44aee29ae
Part 2 - Add tests for cancellation while opening a view. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ba9e23a9735
Part 3 - Add tests for reopening a visible Library view. r=Gijs
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: