Closed Bug 1444392 Opened 6 years ago Closed 6 years ago

Simplify PanelUI.show and add test-only helpers to open and close the main menu

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: Paolo, Assigned: Paolo)

References

(Depends on 2 open bugs, Blocks 1 open bug)

Details

Attachments

(3 files)

Bug 1444392 - Part 1 - Add test-only helpers to open and close the main menu.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1444392 - Part 2 - Use test-only helpers for the main menu across the tree.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1444392 - Part 3 - Simplify the PanelUI.show method.
59 bytes, text/x-review-board-request
Gijs
: review+
Details 
See bug 1443189 comment 16.
Blocks: 1444393
Comment on attachment 8957626 [details]
Bug 1444392 - Part 3 - Simplify the PanelUI.show method.

https://reviewboard.mozilla.org/r/226514/#review232410

::: browser/components/customizableui/content/panelUI.js:212
(Diff revision 1)
> -          resolve();
> +      await PanelMultiView.openPopup(this.panel, anchor, {
> -        }, {once: true});
> -
> -        anchor = this._getPanelAnchor(anchor);
> -        PanelMultiView.openPopup(this.panel, anchor, {
> -          triggerEvent: domEvent,
> +        triggerEvent: domEvent,

Do you know what we use the triggerEvent for?
(In reply to :Paolo Amadini from comment #4)
> Comment on attachment 8957626 [details]
> Bug 1444392 - Part 3 - Simplify the PanelUI.show method.
> 
> https://reviewboard.mozilla.org/r/226514/#review232410
> 
> ::: browser/components/customizableui/content/panelUI.js:212
> (Diff revision 1)
> > -          resolve();
> > +      await PanelMultiView.openPopup(this.panel, anchor, {
> > -        }, {once: true});
> > -
> > -        anchor = this._getPanelAnchor(anchor);
> > -        PanelMultiView.openPopup(this.panel, anchor, {
> > -          triggerEvent: domEvent,
> > +        triggerEvent: domEvent,
> 
> Do you know what we use the triggerEvent for?

Determining whether the panel was opened by a touch event, in which case it gets extra padding to be more touch-friendly.
(Similarly I would expect that if we start doing a better job making these menus keyboard-accessible, we might make different focus decisions, or decisions about underlining access keys, based on the triggering event.)
Comment on attachment 8957624 [details]
Bug 1444392 - Part 1 - Add test-only helpers to open and close the main menu.

https://reviewboard.mozilla.org/r/226510/#review232714
Attachment #8957624 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8957625 [details]
Bug 1444392 - Part 2 - Use test-only helpers for the main menu across the tree.

https://reviewboard.mozilla.org/r/226512/#review232716

In `places`, `search`, `performance` and `uitour` test directories, we only use the utility in 1 test file, so personally I'd put the CUI.import in the test file instead of head.js .
Attachment #8957625 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8957626 [details]
Bug 1444392 - Part 3 - Simplify the PanelUI.show method.

https://reviewboard.mozilla.org/r/226514/#review232718
Attachment #8957626 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs from comment #8)
> In `places`, `search`, `performance` and `uitour` test directories, we only
> use the utility in 1 test file, so personally I'd put the CUI.import in the
> test file instead of head.js .

I initially had the import in the individual files, but then I realized that having it in "head.js" makes it easier to use the function when writing a new test in the same folder. If it is in the individual file, the import tends to be copied along with the test instead of being moved to "head.js" when the second test is added, until there is a critical mass of duplicated imports, at which point someone moves it. So I thought it better to put it in "head.js" in the first place for these components that depend on the CustomizableUI interface. Do you disagree?
Flags: needinfo?(gijskruitbosch+bugs)
(Maybe the "performance" folder is made of unique enough tests that it doesn't need to have the import in "head.js".)
(In reply to :Paolo Amadini from comment #10)
> If it is in the individual file, the import
> tends to be copied along with the test instead of being moved to "head.js"
> when the second test is added, until there is a critical mass of duplicated
> imports, at which point someone moves it. 

That sounds fine to me.

> So I thought it better to put it
> in "head.js" in the first place for these components that depend on the
> CustomizableUI interface.

`search` or `places` or `uitour` don't really depend on CUI for most of what they do, so I don't think this is true. They do in some (well, 1) of their tests, and so that test can require the module.

> Do you disagree?

I mean, I can live with it either way, but yes. :-)
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Gijs from comment #12)
> (In reply to :Paolo Amadini from comment #10)
> > If it is in the individual file, the import
> > tends to be copied along with the test instead of being moved to "head.js"
> > when the second test is added, until there is a critical mass of duplicated
> > imports, at which point someone moves it. 
> 
> That sounds fine to me.

Okay, I think I can also live with this :-) I guess some reviewers may have asked for the opposite change, hence I'd prefer we had a guideline like using "head.js" for imports or something, but since it makes no big difference I can move the imports to the individual tests until we write more tests, or add more helper methods to CustomizableUITestUtils that will be used by more than one test in each folder.

> `search` or `places` or `uitour` don't really depend on CUI for most of what
> they do, so I don't think this is true. They do in some (well, 1) of their
> tests, and so that test can require the module.

Actually, in the "uitour" and "places" folders there are already various tests, not just one, that depend on PanelUI or CustomizableUI. This makes sense because the "places" folder contains the PanelView code for history and bookmarks, just without much test coverage, and "uitour" is actually showcasing many elements that are in the CustomizableUI interface. I'm thinking of the interface in general, not just the main menu.
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec2f47133979
Part 1 - Add test-only helpers to open and close the main menu. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8c4122e21bf
Part 2 - Use test-only helpers for the main menu across the tree. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ee2aed56155
Part 3 - Simplify the PanelUI.show method. r=Gijs
Backed out for mochitest clipboard failures on /browser_editcontrols_update.js

Log: https://treeherder.mozilla.org/logviewer.html#?job_id=167708085&repo=mozilla-inbound&lineNumber=4466
Flags: needinfo?(paolo.mozmail)
Backout by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f3705faaa8c
Backed out 3 changesets for mochitest clipboard failures on /browser_editcontrols_update.js. CLOSED TREE
Depends on: 1445324
Depends on: 1445426
Given bug 1445324, I'm considering handling the case where the function is called with an open panel as an early return, like it was before. Anyways, re-landing this may have to wait a few days.
Flags: needinfo?(paolo.mozmail)
Comment on attachment 8957624 [details]
Bug 1444392 - Part 1 - Add test-only helpers to open and close the main menu.

https://reviewboard.mozilla.org/r/226510/#review238752


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/customizableui/test/CustomizableUITestUtils.jsm:30
(Diff revision 2)
> +
> +  /**
> +   * Opens a closed PanelMultiView via the specified function while waiting for
> +   * the main view with the specified ID to become fully interactive.
> +   */
> +  async openPanelMultiView(panel, mainView, openFn) {

Error: Expected to return a value at the end of async method 'openpanelmultiview'. [eslint: consistent-return]
(In reply to :Paolo Amadini from comment #18)
> Given bug 1445324, I'm considering handling the case where the function is
> called with an open panel as an early return.

I did this, let's see what the tryserver build says:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2af33f1b9337eb64d708de4cf9f2d16ceab97764
I didn't see any related intermittent in 20 rebuilds, I'll go ahead and land this.
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/372811caffc6
Part 1 - Add test-only helpers to open and close the main menu. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e4157be6145
Part 2 - Use test-only helpers for the main menu across the tree. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/aafd5de6ce24
Part 3 - Simplify the PanelUI.show method. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/372811caffc6
https://hg.mozilla.org/mozilla-central/rev/9e4157be6145
https://hg.mozilla.org/mozilla-central/rev/aafd5de6ce24
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: