Closed Bug 1382579 Opened 6 years ago Closed 6 years ago

UITour should support showMenu, showInfo, showHighlight on the Page action Panel

Categories

(Firefox :: Tours, enhancement, P1)

Product:
Firefox
Component:
Tours
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox57 --- verified

People

(Reporter: Fischer, Assigned: Fischer)

References

Details

(Whiteboard: [photon-onboarding])

Attachments

(3 files)

Bug 1382579 - Part 1: Update the BrowserPageActions API for the UITour's usage,
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1382579 - Part 2: UITour should support showMenu, showInfo, showHighlight on the Page Action Panel,
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Bug 1382579 - Part 3: Tests,
59 bytes, text/x-review-board-request
Gijs
: review+
Details 
      No description provided.
Assignee: nobody → fliu
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [photon-onboarding]
Target Milestone: --- → Firefox 57
Flags: qe-verify+
QA Contact: jwilliams
Component: General → Tours
(In reply to Fischer [:Fischer] from comment #2)
> Created attachment 8889061 [details]
> Bug 1382579 - Part 2: UITour should support showMenu, showInfo,
> showHighlight on the Page Action Panel,
> 
> This commit
> - makes UITour support showMenu, showInfo, showHighlight on the Page Action
> Panel
> 
> - makes UITour support showInfo, showHighlight on the urlbar Page Action
> button
> 
> - fixes Bug 1382700 - "UITour lacks the `hideMenu` api support for the
> single search bar (urlbar) dropdown menu" by the way
> 
> - Add the browser_UITour4.js and the browser_showMenu.js tests
> 
> - Update the browser_UITour_availableTargets.js test because we have more
> targets right now
> 
> - Move the tests in the browser_showMenu_controlCenter.js and the
> browser_showMenu_urlbar.js into the browser_showMenu.js
> 
> Review commit: https://reviewboard.mozilla.org/r/159682/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/159682/
TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f409c0b077bf0f0a62d2d5a071e6ebb7ae20a3b
Comment on attachment 8889061 [details]
Bug 1382579 - Part 2: UITour should support showMenu, showInfo, showHighlight on the Page Action Panel,

https://reviewboard.mozilla.org/r/159682/#review165456

::: commit-message-0278a:14
(Diff revision 1)
> +
> +- Add the browser_UITour4.js and the browser_showMenu.js tests
> +
> +- Update the browser_UITour_availableTargets.js test because we have more targets right now
> +
> +- Move the tests in the browser_showMenu_controlCenter.js and the browser_showMenu_urlbar.js into the browser_showMenu.js

Hi Gijs,

For the browser_showMenu.js, 
- the test_showMenu_controlCenter and the test_hideMenu_controlCenter tests are directly copied from the browser_showMenu_controlCenter.js (only the test function names updated).

- the test_showMenu_hideMenu_urlbarPopup is from the browser_showMenu_urlbar.js and gets some modified for the bug 1382700

- the test_showMenu_hideMenu_pageActionPanel is an new one.

Thanks
Comment on attachment 8889061 [details]
Bug 1382579 - Part 2: UITour should support showMenu, showInfo, showHighlight on the Page Action Panel,

Hi Drew,

That would be great if you could feedback on how UITour utilizes the gPageActionButton APIs.
Below are some points to discuss with you about the UITour.jsm in the part 2 patch:

- Between the line 260~278 are the current Page Action elements and at the line 1030 is a function to check if target element is in the page action panel. After reading the bug 1374477 patch, we think those should be all the things to update after the bug 137447 landed.

- UITour would only call `gPageActionButton.ensureReady` when it found the target is in the panel at the line 1281 and 1357

- UITour would call `gPageActionButton.showPanel` at the line 1512 which is inside `UITour.showMenu` (Other menus like the app menu is opened here too). You will see UITour add `noautohide` on the page action panel here too. UITour will clear `noautohide` inside `onPanelHidden` on the popuphidden event.

- UITour at the line 1537 inside `UITour.hideMenu` calls `gPageActionButton.panel.hidePopup`. Not sure if better to add `gPageActionButton.hidePanel` in case that in the future we want to do something before calling `hidePopup`. 

- The bug 1374477 will introduce the pageAction-urlbar-* buttons directly on the urlbar. This bug doesn’t  support that for reducing the complexity. Maybe we could support that when there is an need and the related bugs land. Things may be simpler then.

Please let us know if any to improve, thank you.
Attachment #8889061 - Flags: feedback?(adw)
Correct myself, sorry.
(In reply to Fischer [:Fischer] from comment #5)
> Comment on attachment 8889061 [details]
> Bug 1382579 - Part 2: UITour should support showMenu, showInfo,
> showHighlight on the Page Action Panel,
> 
> Hi Drew,
> 
> That would be great if you could feedback on how UITour utilizes the
> gPageActionButton APIs.
> Below are some points to discuss with you about the UITour.jsm in the part 2
> patch:
> 
> - Between the line 260~278 are the current Page Action elements and at the
> line 1030 is a function to check if target element is in the page action
> panel. After reading the bug 1374477 patch, we think those should be all the
   things to update after the bug *1374477* landed.
> 
> - UITour would only call `gPageActionButton.ensureReady` when it found the
> target is in the panel at the line 1281 and 1357
> 
> - UITour would call `gPageActionButton.showPanel` at the line 1512 which is
> inside `UITour.showMenu` (Other menus like the app menu is opened here too).
> You will see UITour add `noautohide` on the page action panel here too.
> UITour will clear `noautohide` inside `onPanelHidden` on the popuphidden
> event.
> 
> - UITour at the line 1537 inside `UITour.hideMenu` calls
> `gPageActionButton.panel.hidePopup`. Not sure if better to add
> `gPageActionButton.hidePanel` in case that in the future we want to do
> something before calling `hidePopup`. 
> 
> - The bug 1374477 will introduce the pageAction-urlbar-* buttons directly on
> the urlbar. This bug doesn’t  support that for reducing the complexity.
> Maybe we could support that when there is an need and the related bugs land.
> Things may be simpler then.
> 
> Please let us know if any to improve, thank you.
Comment on attachment 8889061 [details]
Bug 1382579 - Part 2: UITour should support showMenu, showInfo, showHighlight on the Page Action Panel,

https://reviewboard.mozilla.org/r/159682/#review165686

I haven't looked at the tests yet. Please make sure to use `hg mv` to ensure there's a readable diff for at least 1 of the tests you're moving.

::: browser/components/uitour/UITour.jsm:1039
(Diff revision 1)
> +
>    /**
>     * Called before opening or after closing a highlight or info panel to see if
> -   * we need to open or close the appMenu to see the annotation's anchor.
> +   * we need to open or close the appMenu/pageActionPanel to see the annotation's anchor.
>     */
> -  _setAppMenuStateForAnnotation(aWindow, aAnnotationType, aShouldOpenForHighlight, aTarget = null,
> +  async _setMenuStateForAnnotation(aWindow, aAnnotationType, aMenuType, aTarget = null,

FWIW, having read through this method multiple times, I think the repurposing of this method in the way this is doing is not the right decision. It makes the code very difficult to follow, and some of the decisions are very much not obvious (such as explicitly passing `""` for `aMenuType` to mean "close all the menus you think you opened") and also not documented. Additionally, it looks like there are unforeseen side-effects (so for instance, if you now call this method with only 2 arguments, it will close menus, which is not obvious).

::: browser/components/uitour/UITour.jsm:1057
(Diff revision 1)
> +    } else if (!shouldOpenAppMenu && appMenuIsOpen && !this.appMenuOpenForAnnotation.has(aAnnotationType)) {
> +      // This annotation type dosen't exists but the menu is opened.
> +      // That means the menu was opened for another reason (e.g. another annotation type or via showMenu)
> +      // Although we shouldn't open the menu, there is another reason there to open the menu.
> +      // Change `shouldOpenAppMenu` to true.

I don't understand this comment. What does "there is another reason there to open the menu" mean?

::: browser/components/uitour/UITour.jsm:1091
(Diff revision 1)
> -      log.debug("_setAppMenuStateForAnnotation: Opening the menu");
> -      this.showMenu(aWindow, "appMenu", async () => {
> +        let pageActionPanel = aWindow.gPageActionButton.panel;
> +        pageActionPanel.addEventListener("popuphidden", resolve, { once: true });
> +        this.hideMenu(aWindow, "pageActionPanel");
> +      }));
> +    }
> +    // Hiding menu would trigger `hideHighligh` or `hideInfo` during the popup-hdding event.

Spelling

::: browser/components/uitour/UITour.jsm:1095
(Diff revision 1)
> +    }
> +    // Hiding menu would trigger `hideHighligh` or `hideInfo` during the popup-hdding event.
> +    // Make sure proceed after hiding. We don't want to mess with those tasks, such as,
> +    // 1. hide the app menu then
> +    // 2. open the page action panel and highligh a button but
> +    // 3. the `hideHighligh` because of the step 1 comes then tasks mess with each other.

Spelling. I also don't understand what this sentence means.

::: browser/components/uitour/UITour.jsm:1096
(Diff revision 1)
> +    // Hiding menu would trigger `hideHighligh` or `hideInfo` during the popup-hdding event.
> +    // Make sure proceed after hiding. We don't want to mess with those tasks, such as,
> +    // 1. hide the app menu then
> +    // 2. open the page action panel and highligh a button but
> +    // 3. the `hideHighligh` because of the step 1 comes then tasks mess with each other.
> +    await Promise.all(promises);

This will wait a tick even if we didn't do anything above (ie the promise array is empty), after which there's a risk of re-entrancy or the previous states (such as the panel not being closed etc.) not being correct anymore.

There is nothing to do after the promises resolve besides opening the menu, and calling the callback. If we open the menu, there won't be promises in the array for the same popup, because there will only be promises if `shouldOpen...` is false, and we only open the popup if `shouldOpen...` is true.

Can we move the opening code before the `await` ?

::: browser/components/uitour/UITour.jsm:1099
(Diff revision 1)
> +    if (shouldOpenAppMenu && !appMenuIsOpen) {
> +      log.debug("_setMenuStateForAnnotation: Opening App menu");
> +      menuToShow = "appMenu";
> +    } else if (shouldOpenPageActionPanel && !pageActionPanelIsOpen) {
> +      log.debug("_setMenuStateForAnnotation: Opening Page action panel");
> +      menuToShow = "pageActionPanel";
> +    }

This is confusing, because the code supports closing multiple popups but only supports opening 1 at a time. Why?

::: browser/components/uitour/UITour.jsm:1115
(Diff revision 1)
>          // binding it belonged to got destroyed). We work around this by re-querying for
>          // the node and stuffing it into the old target structure.
> -        log.debug("_setAppMenuStateForAnnotation: Refreshing target");
> +        log.debug("_setMenuStateForAnnotation: Refreshing target");
>          let refreshedTarget = await this.getTarget(aWindow, aTarget.targetName);
>          aTarget.node = refreshedTarget.node;
>          aCallback();

In particular, can we maybe just add a promise for showing the menu, await that, and then have a single callsite for `aCallback()` ? That would simplify the code here.

::: browser/components/uitour/UITour.jsm:1369
(Diff revision 1)
> +    } else if (this.targetIsInPageActionPanel(aAnchor)) {
> +      menuType = "pageActionPanel";
> +      // Ensure the page action panel ready so as to ensure the visibility of
> +      // the target element inside the panel otherwise
> +      // we would be rejected in the below `isElementVisible` checking.
> +      aChromeWindow.gPageActionButton.ensureReady();

FWIW, as written this will now fire twice - once here and once from showPanel(). I don't think that's a good idea. If it was just the removal of the hidden attribute, that might still be OK, but it's not...

::: browser/components/uitour/UITour.jsm:1525
(Diff revision 1)
> +      pageActionPanel.setAttribute("noautohide", "true");
> +      pageActionPanel.addEventListener("popuphidden", this.onPanelHidden);
> +      pageActionPanel.addEventListener("ViewShowing", this.hidePageActionPanelAnnotations);
> +      pageActionPanel.addEventListener("popuphiding", this.hidePageActionPanelAnnotations);
> +      if (aOpenCallback) {
> +        pageActionPanel.addEventListener("popupshown", onPopupShown);

This listener is never removed, which should be leaking. Was there a trypush? Are there leaks on that trypush?

::: browser/components/uitour/test/browser_UITour_availableTargets.js
(Diff revision 1)
> -    "backForward",
>      "bookmarks",
> +    "backForward",

Why reorder these? They should be sorted...

::: browser/components/uitour/test/browser_showMenu.js:1
(Diff revision 1)
> +"use strict";

Please use `hg mv` instead of just removing one file and adding another one.
Attachment #8889061 - Flags: review?(gijskruitbosch+bugs) → review-
Comment on attachment 8889060 [details]
Bug 1382579 - Part 1: Update the BrowserPageActions API for the UITour's usage,

https://reviewboard.mozilla.org/r/159680/#review166382

This is OK I guess.  I don't really like having a public method that ensures the panel is ready to be shown.  That seems like something that should be private, if it exists at all.  But, I also don't like the idea of your having to modify or bypass your isElementVisible check when menuType == "pageActionPanel".  Either way, your code needs to do something special when the menu is the page action panel, and I guess calling ensureReady is better than modifying or bypassing a reasonable visibility check.

I see that you have some code comments about renaming some of the page action IDs after bug 1374477 lands.  There are some other instances of IDs that don't have code comments that will need to be updated, too (e.g., gPageActionButton will be renamed BrowserPageActions, panel will be renamed panelNode.)  I suspect your bug will be ready to land before bug 1374477 is ready.  If that's true, I'll update your code as part of bug 1374477.

::: commit-message-e99e3:4
(Diff revision 1)
> +Bug 1382579 - Part 1: Update the gPageActionButton APIs for the UITour's usage, r=adw
> +
> +This commit
> +- adds `ensureReady` like PanelUI (the app menu) did so UITour could ensure page action elements visibility like ensuring PanelUI elements[1].

I think you mean the page action panel, not PanelUI or the app menu?
Attachment #8889060 - Flags: review?(adw) → review+
Comment on attachment 8889061 [details]
Bug 1382579 - Part 2: UITour should support showMenu, showInfo, showHighlight on the Page Action Panel,

f+ -- my comments in comment 8 apply here.
Attachment #8889061 - Flags: feedback?(adw) → feedback+
(In reply to Drew Willcoxon :adw from comment #8)
> Either way, your code needs to do something special when the menu is
> the page action panel

Just for posterity, bug 1366041 has more discussion about that.
Comment on attachment 8889061 [details]
Bug 1382579 - Part 2: UITour should support showMenu, showInfo, showHighlight on the Page Action Panel,

https://reviewboard.mozilla.org/r/159682/#review167430

::: browser/components/uitour/UITour.jsm:1039
(Diff revision 2)
> +
> +  /**
> +   * Called before opening or after closing a highlight or info panel to see if
> +   * we need to open or close the pageActionPanel to see the annotation's anchor.
> +   */
> +  _setPageActionPanelStateForAnnotation(aWindow, aAnnotationType, aShouldOpenForHighlight) {

Thinking maybe we could extract the common parts from `_setAppMenuStateForAnnotation` and `_setPageActionPanelStateForAnnotation` and write a function like `_setMenuStateForAnnotation(aWindow, aAnnotationType, aShouldOpenForHighlight, aMenutype, aMenuOpenForAnnotation)`.
So we could call `_setMenuStateForAnnotation(window, "highlight", true, "appMenu", appMenuOpenForAnnotation)` and `_setMenuStateForAnnotation(window, "highlight", false, "pageActionPanel", pageActionPanelOpenForAnnotation)`.

::: browser/components/uitour/UITour.jsm:1268
(Diff revision 2)
> +    let menu = highlighter.parentElement.getAttribute("menuForTarget");
> +    highlighter.parentElement.removeAttribute("menuForTarget");
>      this._removeAnnotationPanelMutationObserver(highlighter.parentElement);
>      highlighter.parentElement.hidePopup();
>      highlighter.removeAttribute("active");
> -
> +    if (menu == "appMenu") {

Here is to avoid the major issue that call `showHighlight(“addons”)` then `showHighlight(“pageAction-panel-bookmark”)` would close the appMenu and the pageActionPanel altogether and the pageAction-panel-bookmark button wouldn't be highlighted.

::: browser/components/uitour/UITour.jsm:1431
(Diff revision 2)
>      let tooltip = document.getElementById("UITourTooltip");
> +    let menu = tooltip.getAttribute("menuForTarget");
> +    tooltip.removeAttribute("menuForTarget", menu);
>      this._removeAnnotationPanelMutationObserver(tooltip);
>      tooltip.hidePopup();
> +    if (menu == "appMenu") {

The same as the above `hideHighlight` case.
Attachment #8889061 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8889061 [details]
Bug 1382579 - Part 2: UITour should support showMenu, showInfo, showHighlight on the Page Action Panel,

Hi Gijs,

That would be great that you could feedback on what would be a better approach, thanks.

I’ve been thinking how to support more menus and keep the structure good to follow. This time try to separate the appMenu and the pageActionPanel.

And here is the one major issue I try to avoid. Consider the following  case:
1. Call `showHighlight(“addons”)`, then the appMenu is opened and the addons button is highlighted
2. Call `showHighlight(“pageAction-panel-bookmark”)`, then
3. `_setAppMenuStateForAnnotation(aChromeWindow, "highlight", false)` is called
4. Because it shouldn’t open the appMenu, `hideMenu(aWindow, "appMenu”)` called in `_setAppMenuStateForAnnotation`, then
5. The appMenu gets closed and during the closing process `hideAppMenuAnnotations` is called on the appMenu’s popuphiding event.
6. `hideAnnotationsForPanel` is called in `hideAppMenuAnnotations`
7. In `hideAnnotationsForPanel`, it would async get target again then call `hideHighlight` to clean up highlight (but `hideHighlight` isn’t called at this step because of the async operation)
8. Back to the `showHighlight(“pageAction-panel-bookmark”)` call from the step 2, then
9. `_setPageActionPanelStateForAnnotation(aChromeWindow, "highlight", true)` is called, then
10. `showMenu` is called for the pageActionPanel to open it
11. But the async `hideHighlight` from the step 7 is called after that
12 In `hideHighlight`, `_setAppMenuStateForAnnotation(aWindow, "highlight", false)` and `_setPageActionPanelStateForAnnotation(aWindow, "highlight", false)` are called to reset the states.
13. If we didn’t take care of this case, not only the appMenu is closed but also the pageActionPanel is closed.
14. The pageAction-panel-bookmark button, as a result, wouldn’t be highlighted

(In reply to :Gijs from comment #7)
> ::: browser/components/uitour/UITour.jsm:1057
> (Diff revision 1)
> > +    } else if (!shouldOpenAppMenu && appMenuIsOpen && !this.appMenuOpenForAnnotation.has(aAnnotationType)) {
> > +      // This annotation type dosen't exists but the menu is opened.
> > +      // That means the menu was opened for another reason (e.g. another annotation type or via showMenu)
> > +      // Although we shouldn't open the menu, there is another reason there to open the menu.
> > +      // Change `shouldOpenAppMenu` to true.
> 
> I don't understand this comment. What does "there is another reason there to open the menu" mean?
> 
UITour uses `appMenuOpenForAnnotation` to track what opened the menu.
For example,
1. Call `showInfo` to show info on the addons button, then `appMenuOpenForAnnotation` would have "info" annotation.
2. Call `showHighlight` to highlight the backForward button.
3. Then when setting the menu state for the "highlight" annotation, it would reach the case that shouldOpenAppMenu is false but appMenuIsOpen is true. But if, at this moment, it couldn't find "highlight" annotation, which is our case, it would know the appMenu is opened for another annotation and shouldn't close it.
The same case is applied to `showMenu` call as well.

> ::: browser/components/uitour/UITour.jsm:1095
> (Diff revision 1)
> > +    }
> > +    // Hiding menu would trigger `hideHighligh` or `hideInfo` during the popup-hdding event.
> > +    // Make sure proceed after hiding. We don't want to mess with those tasks, such as,
> > +    // 1. hide the app menu then
> > +    // 2. open the page action panel and highligh a button but
> > +    // 3. the `hideHighligh` because of the step 1 comes then tasks mess with each other.
> 
> Spelling. I also don't understand what this sentence means.
> 
This is that major issue I try to avoid.

> ::: browser/components/uitour/UITour.jsm:1369
> (Diff revision 1)
> > +    } else if (this.targetIsInPageActionPanel(aAnchor)) {
> > +      menuType = "pageActionPanel";
> > +      // Ensure the page action panel ready so as to ensure the visibility of
> > +      // the target element inside the panel otherwise
> > +      // we would be rejected in the below `isElementVisible` checking.
> > +      aChromeWindow.gPageActionButton.ensureReady();
> 
> FWIW, as written this will now fire twice - once here and once from
> showPanel(). I don't think that's a good idea. If it was just the removal of
> the hidden attribute, that might still be OK, but it's not...
> 
After reading your and Drew's comments, I think maybe better just go for `gPageActionButton.panel.hidden = false`, which is simpler and clearer.

> ::: browser/components/uitour/UITour.jsm:1525
> (Diff revision 1)
> > +      pageActionPanel.setAttribute("noautohide", "true");
> > +      pageActionPanel.addEventListener("popuphidden", this.onPanelHidden);
> > +      pageActionPanel.addEventListener("ViewShowing", this.hidePageActionPanelAnnotations);
> > +      pageActionPanel.addEventListener("popuphiding", this.hidePageActionPanelAnnotations);
> > +      if (aOpenCallback) {
> > +        pageActionPanel.addEventListener("popupshown", onPopupShown);
> 
> This listener is never removed, which should be leaking. Was there a rypush? Are there leaks on that trypush?
> 
The `onPopupShown` is a shared function that will remove itself from event listeners on calling.
TBH, I got the same question at the 1st sight of this but didn't think too much back then so simply reused it.
Maybe `{ once: true }` is clearer and simpler. Updated

> ::: browser/components/uitour/test/browser_UITour_availableTargets.js
> (Diff revision 1)
> > -    "backForward",
> >      "bookmarks",
> > +    "backForward",
> 
> Why reorder these? They should be sorted...
> 
Sorry. This is just an mistake.
Attachment #8889061 - Flags: feedback?(gijskruitbosch+bugs)
(In reply to Drew Willcoxon :adw from comment #8)
> Comment on attachment 8889060 [details]
> Bug 1382579 - Part 1: Update the gPageActionButton APIs for the UITour's
> usage,
> 
> https://reviewboard.mozilla.org/r/159680/#review166382
> 
> This is OK I guess.  I don't really like having a public method that ensures
> the panel is ready to be shown.  That seems like something that should be
> private, if it exists at all.  But, I also don't like the idea of your
> having to modify or bypass your isElementVisible check when menuType ==
> "pageActionPanel".  Either way, your code needs to do something special when
> the menu is the page action panel, and I guess calling ensureReady is better
> than modifying or bypassing a reasonable visibility check.
> 
After reading the comments from you and Gijs, I removed `ensureReady` and simply call `gPageActionButton.panel.hidden = false`

> I see that you have some code comments about renaming some of the page
> action IDs after bug 1374477 lands.  There are some other instances of IDs
> that don't have code comments that will need to be updated, too (e.g.,
> gPageActionButton will be renamed BrowserPageActions, panel will be renamed
> panelNode.)  I suspect your bug will be ready to land before bug 1374477 is
> ready.  If that's true, I'll update your code as part of bug 1374477.
> 
Thanks, I will write these notes down.
Comment on attachment 8889061 [details]
Bug 1382579 - Part 2: UITour should support showMenu, showInfo, showHighlight on the Page Action Panel,

I have to admit I'm very confused by the patch and the comments on this bug.

It seems to me only one highlight/info thing should be used at any one point, and likewise only 1 panel should be open. We should `await` or equivalent for any other panels to close before opening a new one, and likewise we should hide previous info and highlights before showing new ones. If we need to be able to show 1 info and 1 highlight simultaneously, we should still close the first popup once we're showing a second one (and hide the info/highlight if it was open for that popup).

We should structure the code so that we use async functions and `await` the closing/hiding before opening new things. This should avoid all the workarounds we're employing here to deal with reentrancy and concurrency, and clarify the code, too. We should name (either with DOM IDs or with another naming scheme) the popups we support opening, and keep track of which one is open, and what its anchor is. If we put those things in properties on UITour, we should be able to have 1 method that closes the current popup, if any (and any info/highlight panes attached to it) and 1 method that opens a newly desired popup only *after* first calling the earlier method to clear any present popups if they don't match the new one. Once popups close (even if it's not because of UITour but by user action), we should remove the highlight/info and clear the local anchor/popup properties (so we don't try to hide/close the popup again.

Does that make sense?

Finally, can we split the tests out into a separate cset (and use hg mv as suggested previously) to make it easier to review things?
Attachment #8889061 - Flags: feedback?(gijskruitbosch+bugs)
(In reply to :Gijs from comment #16)
> Comment on attachment 8889061 [details]
> Bug 1382579 - Part 2: UITour should support showMenu, showInfo,
> showHighlight on the Page Action Panel,
> 
> I have to admit I'm very confused by the patch and the comments on this bug.
> 
> It seems to me only one highlight/info thing should be used at any one
> point, and likewise only 1 panel should be open. We should `await` or
> equivalent for any other panels to close before opening a new one, and
> likewise we should hide previous info and highlights before showing new
> ones. If we need to be able to show 1 info and 1 highlight simultaneously,
> we should still close the first popup once we're showing a second one (and
> hide the info/highlight if it was open for that popup).
> 
Yeah, from the current code, UITour supports showing 1 info and 1 highlight simultaneously (even on the same one menu).

> We should structure the code so that we use async functions and `await` the
> closing/hiding before opening new things. This should avoid all the
> workarounds we're employing here to deal with reentrancy and concurrency,
> and clarify the code, too. We should name (either with DOM IDs or with
> another naming scheme) the popups we support opening, and keep track of
> which one is open, and what its anchor is. If we put those things in
> properties on UITour, we should be able to have 1 method that closes the
> current popup, if any (and any info/highlight panes attached to it) and 1
> method that opens a newly desired popup only *after* first calling the
> earlier method to clear any present popups if they don't match the new one.
> Once popups close (even if it's not because of UITour but by user action),
> we should remove the highlight/info and clear the local anchor/popup
> properties (so we don't try to hide/close the popup again.
> 
> Does that make sense?
> 
Thanks for that. Will try another update.

> Finally, can we split the tests out into a separate cset (and use hg mv as suggested previously) to make it easier to review things?
Definitely will do this when sending out the r?.
Comment on attachment 8889060 [details]
Bug 1382579 - Part 1: Update the BrowserPageActions API for the UITour's usage,

https://reviewboard.mozilla.org/r/159680/#review169120

::: browser/base/content/browser-pageActions.js:497
(Diff revision 3)
> +   *
> +   * @param  event (DOM event, optional)
> +   *         The event that triggers showing the panel. (such as a mouse click,
> +   *         if the user clicked something to open the panel)
> +   */
> +  showPanel(event = null) {

It is fine that the event is optional.
Checked out the `openPopup` doc[1], the `triggerEvent` arg is expected if there was a user-triggered event.
Also checked out the underlying xul implementation as well.
During the call path: `PopupBoxObject::OpenPopup` -> `nsXULPopupManager::ShowPopup`:
  - `nsXULPopupManager::InitTriggerEvent`[2] would consider the case of an null aEvent
  - `nsXULPopupManager::FirePopupShowingEvent`[3] also considers the case of an null triggerEvent

The `mainButtonClicked` above should provide the event since it is triggered by the button clicking.
For other cases, like UITour, the event could be null and directly be called as `showPanel()`.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Method/openPopup
[2] https://dxr.mozilla.org/mozilla-central/rev/ef9a0f01e4f68214f0ff8f4631783b8a0e075a82/layout/xul/nsXULPopupManager.cpp#608
[3] https://dxr.mozilla.org/mozilla-central/rev/ef9a0f01e4f68214f0ff8f4631783b8a0e075a82/layout/xul/nsXULPopupManager.cpp#1475
Comment on attachment 8889061 [details]
Bug 1382579 - Part 2: UITour should support showMenu, showInfo, showHighlight on the Page Action Panel,

https://reviewboard.mozilla.org/r/159682/#review169122

::: browser/components/uitour/UITour.jsm:1543
(Diff revision 3)
>        let menuBtn = aWindow.document.getElementById("bookmarks-menu-button");
>        closeMenuButton(menuBtn);
>      } else if (aMenuName == "controlCenter") {
>        let panel = aWindow.gIdentityHandler._identityPopup;
>        panel.hidePopup();
> +    } else if (aMenuName == "urlbar") {

This part would fix Bug 1382700.

::: browser/components/uitour/UITour.jsm:1557
(Diff revision 3)
>  
>    showNewTab(aWindow, aBrowser) {
>      aWindow.openLinkIn("about:newtab", "current", {targetBrowser: aBrowser});
>    },
>  
> -  hideAnnotationsForPanel(aEvent, aTargetPositionCallback) {
> +  _hideAnnotationsForPanel(aEvent, aTargetPositionCallback) {

`hideAnnotationsForPanel` is more like an internal helper function, only called by `hideAppMenuAnnotations`, `hidePageActionPanelAnnotations` and `hideControlCenterAnnotations`. So change it to `_hideAnnotationsForPanel`. Make it clearer that should call `hideAppMenuAnnotations` etc from the outside to do the job instead of calling `_hideAnnotationsForPanel` directly.

::: browser/components/uitour/UITour.jsm
(Diff revision 3)
>            }
>            hideMethod(win);
>          }).catch(log.error);
>        }
>      });
> -    UITour.appMenuOpenForAnnotation.clear();

To move this line out would fix Bug 1386201.
(In reply to Fischer [:Fischer] from comment #20)
> Comment on attachment 8889060 [details]
> Bug 1382579 - Part 1: Update the BrowserPageActions API for the UITour's
> usage,
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/159680/diff/2-3/
Hi Gijs,

The patches have been reabsed on the bug 1374477.

The UITour.jsm has been updated to extract shared logics into functions as well as to use async functions and `await` the closing before opening new things as the comment 16. After reading the original codes again, we still follow the "annotaion" approach because that approach is used on many places. It seems following that approach should make less changes.

Thanks
Comment on attachment 8889060 [details]
Bug 1382579 - Part 1: Update the BrowserPageActions API for the UITour's usage,

https://reviewboard.mozilla.org/r/159680/#review169212

::: browser/base/content/browser-pageActions.js:483
(Diff revision 3)
> +    if (this.panelNode.state == "open") {
> +      this.panelNode.hidePopup();
> +      return;
> +    }

Why is this now necessary? Do we need to do the same for state == "showing" ?
Attachment #8889060 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8889061 [details]
Bug 1382579 - Part 2: UITour should support showMenu, showInfo, showHighlight on the Page Action Panel,

https://reviewboard.mozilla.org/r/159682/#review169214

::: browser/components/uitour/UITour.jsm:1037
(Diff revision 3)
> +   *
> +   * @param {ChromeWindow} aWindow the chrome window
> +   * @param {String} aAnnotationType "highlight" or "info"
> +   * @param {bool} aShouldOpenForHighlight true means we should open the menu, otherwise false
> +   * @param {String} aMenuName "appMenu" or "pageActionPanel"
> +   * @param {Node} aMenu the DOM node of the appMenu or the pageActionPanel

You can and should just get this node based on `aWindow` and `aMenuName`.

::: browser/components/uitour/UITour.jsm:1038
(Diff revision 3)
> +   * @param {ChromeWindow} aWindow the chrome window
> +   * @param {String} aAnnotationType "highlight" or "info"
> +   * @param {bool} aShouldOpenForHighlight true means we should open the menu, otherwise false
> +   * @param {String} aMenuName "appMenu" or "pageActionPanel"
> +   * @param {Node} aMenu the DOM node of the appMenu or the pageActionPanel
> +   * @param {Set} aMenuOpenForAnnotation `this.appMenuOpenForAnnotation` or `this.pageActionPanelOpenForAnnotation`

You can (and should) fetch this based on aMenuName, rather than passing in redundant information.

::: browser/components/uitour/UITour.jsm:1099
(Diff revision 3)
> +    } else if (this.targetIsInPageActionPanel(aTarget)) {
> +      shouldOpenPageActionPanel = true;
> +      // Ensure the panel visibility so as to ensure the visibility of
> +      // the target element inside the panel otherwise
> +      // we would be rejected in the below `isElementVisible` checking.
> +      aChromeWindow.BrowserPageActions.panelNode.hidden = false;

Could and probably should just do this inside the `showPanel` method you added to BrowserPageActions.

::: browser/components/uitour/UITour.jsm:1112
(Diff revision 3)
> +      menuToOpen = {
> +        name: "appMenu",
> +        node: appMenu,
> +        annotation: this.appMenuOpenForAnnotation
> +      };

This will be simpler after removing the redundant parameters from `_setMenuStateForAnnotation`

::: browser/components/uitour/UITour.jsm:1135
(Diff revision 3)
> +      menuClosePromises.push(this._setMenuStateForAnnotation(aChromeWindow, aAnnotationType, false,
> +        "pageActionPanel", pageActionPanel, this.pageActionPanelOpenForAnnotation));
> +    }
> +
> +    if (menuToOpen) {
> +      await Promise.all(menuClosePromises);

To reduce duplication, we can just await this promise before the if statement given this is an async function and otherwise you return the same set of promises to wait for.

::: browser/components/uitour/UITour.jsm
(Diff revision 3)
> -    if (!this.isElementVisible(aAnchor.node)) {
> -      log.warn("showInfo: Not showing since the target isn't visible", aAnchor);
> -      return;
> -    }

Can you explain why we no longer need to check this?

::: browser/components/uitour/UITour.jsm:1544
(Diff revision 3)
> +      this.getTarget(aWindow, "urlbar").then(target => {
> +        let urlbar = target.node;
> +        urlbar.popup.closePopup();
> +      });

This is async and the rest of the panel closings are sync. Why not just:

```js
aWindow.gURLBar.popup.closePopup()
```

? Also, why `closePopup` and not `hidePopup` - what's the difference?
Attachment #8889061 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8889061 [details]
Bug 1382579 - Part 2: UITour should support showMenu, showInfo, showHighlight on the Page Action Panel,

https://reviewboard.mozilla.org/r/159682/#review169240

::: browser/components/uitour/UITour-lib.js:126
(Diff revision 3)
>     * <li>selectedTabIcon
>     * <li>trackingProtection
>     * <li>urlbar
>     * <li>webide
> +   * <li> pageActionButton
> +   * <li> pageAction-panel-bookmark

Do we not want to highlight this in the URL bar?
Comment on attachment 8892759 [details]
Bug 1382579 - Part 3: Tests,

https://reviewboard.mozilla.org/r/163718/#review169220

(In reply to Fischer [:Fischer] from comment #17)
> (In reply to :Gijs from comment #16)
> > If we need to be able to show 1 info and 1 highlight simultaneously,
> > we should still close the first popup once we're showing a second one (and
> > hide the info/highlight if it was open for that popup).
> > 
> Yeah, from the current code, UITour supports showing 1 info and 1 highlight
> simultaneously (even on the same one menu).

So, tbh, I still think that we should just hide the previous menu and info/highlight, even if the first thing we opened a menu for was 'info', and now we want a 'highlight', or vice versa. Is there actually a hard requirement from UX we need to be able to have both menus open at the same time? I kind of doubt that... and if we can simply ensure we only open 1 menu at a time, that can simplify the implementation and the test further.

::: browser/components/uitour/test/browser_UITour4.js:7
(Diff revision 1)
> +
> +var gTestTab;
> +var gContentAPI;
> +var gContentWindow;
> +
> +requestLongerTimeout(2);

Is this actually required for this test?

::: browser/components/uitour/test/browser_UITour4.js:181
(Diff revision 1)
> +  await pageActionPanelShownPromise;
> +  await highlightVisiblePromise;
> +  is(appMenu.state, "open", "Shouldn't close the app menu");

Is this still right? I'm confused why this wouldn't hide the app menu.
Attachment #8892759 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8889060 [details]
Bug 1382579 - Part 1: Update the BrowserPageActions API for the UITour's usage,

https://reviewboard.mozilla.org/r/159680/#review169212

> Why is this now necessary? Do we need to do the same for state == "showing" ?

When UITour highlights a button in an menu(which is the appMenu or the pageActionPanel), it sets the `noautohide` to true so that the menu wouldn't close while clicking outside the menu. AFAIK, this is to avoid the case that user accidentally clicks outside the menu and the menu closes before user got a chance to see the highlight. However, we still need another way to let user close the menu besides clicking the highlight. That another way is the menu button and the appMenu is doing  too[1].

And yes, better to show/hide in the exactly right state so updated to show in the `closed` state and hide in the `open` state.

[1] https://dxr.mozilla.org/mozilla-central/rev/1b065ffd8a535a0ad4c39a912af18e948e6a42c1/browser/components/customizableui/content/panelUI.js#246
Comment on attachment 8889061 [details]
Bug 1382579 - Part 2: UITour should support showMenu, showInfo, showHighlight on the Page Action Panel,

https://reviewboard.mozilla.org/r/159682/#review169214

> Could and probably should just do this inside the `showPanel` method you added to BrowserPageActions.

Need this for the following `isElementVisible`.

> Can you explain why we no longer need to check this?

We still check this. It is moved into the `_ensureTarget` method.

> This is async and the rest of the panel closings are sync. Why not just:
> 
> ```js
> aWindow.gURLBar.popup.closePopup()
> ```
> 
> ? Also, why `closePopup` and not `hidePopup` - what's the difference?

Why `closePopup` is due to the gURLBar.popup’s binding inheritance as below:
urlbarBindings.xml#urlbar-rich-result-popup
-> autocomplete.xml#autocomplete-rich-result-popup
-> autocomplete.xml#autocomplete-base-popup

And autocomplete.xml#autocomplete-base-popup implements `closePopup`[1] which will do `hidePopup` and some cleanup.
[1] https://dxr.mozilla.org/mozilla-central/rev/52285ea5e54c73d3ed824544cef2ee3f195f05e6/toolkit/content/widgets/autocomplete.xml#931

Thanks for reminding this. We could even call `gURLBar.closePopup`. See its binding:
urlbarBindings.xml#urlbar
-> autocomplete.xml#autocomplete
The autocomplete.xml#autocomplete got a `closePopup`[2] which will do `gURLBar.popup.closePopup` for us.
[2] https://dxr.mozilla.org/mozilla-central/rev/52285ea5e54c73d3ed824544cef2ee3f195f05e6/toolkit/content/widgets/autocomplete.xml#396
Comment on attachment 8889061 [details]
Bug 1382579 - Part 2: UITour should support showMenu, showInfo, showHighlight on the Page Action Panel,

https://reviewboard.mozilla.org/r/159682/#review169240

> Do we not want to highlight this in the URL bar?

No request currently. Probably will have since it & the page action button are 2 prominent buttons on the urlbar. Adding this bookmark star button is simple. Only need the id. So add it btw.
Comment on attachment 8892759 [details]
Bug 1382579 - Part 3: Tests,

https://reviewboard.mozilla.org/r/163718/#review169220

Checked with the UX: Verdi, no this is not really required. We could do `showHighligh` on the app menu and close pervious highligh/info on the page action panel and vice versa.

> Is this actually required for this test?

Removed it. Ran multiple try runs on the slow debug enviroment. Looks good. No need for this.

> Is this still right? I'm confused why this wouldn't hide the app menu.

Updated the comments to explain more on this behavior.
If the page action panel or the app menu was opened by api user through `Mozilla.UITour.showMenu` explictly, UITour should close the menu through the explict `Mozilla.UITour.hideMenu` call from api user as well.
So, for example,
1. API user requests to open the app Menu with `Mozilla.UITour.showMenu("appMenu")`
2. API user requests to highlight the page action panel's copyURL button with `Mozilla.UITour.showHighlgiht("pageAction-panel-copyURL")`
After the step 2, the app menu should remain open because user explictly requested to open it and not yet request to close it.
Comment on attachment 8889060 [details]
Bug 1382579 - Part 1: Update the BrowserPageActions API for the UITour's usage,

https://reviewboard.mozilla.org/r/159680/#review170524
Attachment #8889060 - Flags: review?(gijskruitbosch+bugs) → review+
Blocks: 1387697
Comment on attachment 8889061 [details]
Bug 1382579 - Part 2: UITour should support showMenu, showInfo, showHighlight on the Page Action Panel,

https://reviewboard.mozilla.org/r/159682/#review170526

Almost. :-)

You probably want to rebase against central for this patchset...

::: browser/components/uitour/UITour.jsm:90
(Diff revision 4)
>    pageIDsForSession: new Map(),
>    pageIDSourceBrowsers: new WeakMap(),
>    /* Map from browser chrome windows to a Set of <browser>s in which a tour is open (both visible and hidden) */
>    tourBrowsersByWindow: new WeakMap(),
> -  appMenuOpenForAnnotation: new Set(),
> +  /* Menus opened by api users explictly through `Mozilla.UITour.showMenu` call */
> +  userRequestedMenus: new Set(),

I think `user` is confusing here, because it sounds like the user opened it by clicking the anchor like normal - maybe call it `noautohideMenus` or something like this.

::: browser/components/uitour/UITour.jsm:888
(Diff revision 4)
> -    aWindow.PanelUI.panel.removeEventListener("popuphiding", this.hideAppMenuAnnotations);
> -    aWindow.PanelUI.panel.removeEventListener("ViewShowing", this.hideAppMenuAnnotations);
> -    aWindow.PanelUI.panel.removeEventListener("popuphidden", this.onPanelHidden);
> +    let appMenu = aWindow.PanelUI.panel;
> +    appMenu.removeEventListener("popuphiding", this.hideAppMenuAnnotations);
> +    appMenu.removeEventListener("ViewShowing", this.hideAppMenuAnnotations);
> +    appMenu.removeEventListener("popuphidden", this.onPanelHidden);
>      let controlCenterPanel = aWindow.gIdentityHandler._identityPopup;
>      controlCenterPanel.removeEventListener("popuphidden", this.onPanelHidden);
>      controlCenterPanel.removeEventListener("popuphiding", this.hideControlCenterAnnotations);
> +    let pageActionPanel = aWindow.BrowserPageActions.panelNode;
> +    pageActionPanel.removeEventListener("popuphidden", this.onPanelHidden);
> +    pageActionPanel.removeEventListener("ViewShowing", this.hidePageActionPanelAnnotations);
> +    pageActionPanel.removeEventListener("popuphiding", this.hidePageActionPanelAnnotations);

```js

let panels [
  aWindow.PanelUI.panel,
  aWindow.gIdentityHandler._identityPopup,
  aWindow.BrowserPageActions.panelNode
];
for (let panel of panels) {
  panel.removeEventListener(...); // x3
}

```

::: browser/components/uitour/UITour.jsm:891
(Diff revision 4)
>  
>      // Clean up panel listeners after calling hideMenu above.
> -    aWindow.PanelUI.panel.removeEventListener("popuphiding", this.hideAppMenuAnnotations);
> -    aWindow.PanelUI.panel.removeEventListener("ViewShowing", this.hideAppMenuAnnotations);
> -    aWindow.PanelUI.panel.removeEventListener("popuphidden", this.onPanelHidden);
> +    let appMenu = aWindow.PanelUI.panel;
> +    appMenu.removeEventListener("popuphiding", this.hideAppMenuAnnotations);
> +    appMenu.removeEventListener("ViewShowing", this.hideAppMenuAnnotations);
> +    appMenu.removeEventListener("popuphidden", this.onPanelHidden);

Nit: put the popuphiding and popuphidden listener additions/removals together. Same below for the page action menu.

::: browser/components/uitour/UITour.jsm:1062
(Diff revision 4)
> +    } else if (!this.userRequestedMenus.has(aMenuName)) {
> +      // If the menu was opened explictly by api user through `Mozilla.UITour.showMenu`,
> +      // it should be closed explictly by api user through `Mozilla.UITour.hideMenu`.
> +      // So we shouldn't get to here to close it for the highlight/info annotation.
> +      log.debug("_setMenuStateForAnnotation: Closing the menu");
> +      promise = new Promise(resolve => {
> +        menu.addEventListener("popuphidden", resolve, { once: true });
> +        this.hideMenu(aWindow, aMenuName);
> +      });

We can enter this block when the menu isn't actually open, in which case I would not expect `popuphidden` to fire and `hideMenu` shouldn't be called, right?

::: browser/components/uitour/UITour.jsm:1079
(Diff revision 4)
>  
> -    if (aShouldOpenForHighlight) {
> -      this.appMenuOpenForAnnotation.add(aAnnotationType);
> -    } else {
> -      this.appMenuOpenForAnnotation.delete(aAnnotationType);
> +  /**
> +   * Ensure the target's visibility and the open/close states of menus for the target.
> +   *
> +   * @param {ChromeWindow} aChromeWindow The chrome window
> +   * @param {Object} aTarget The target on which show highligh or show info.

nit: on which we showHighlight or showInfo

::: browser/components/uitour/UITour.jsm:1112
(Diff revision 4)
> +    let promise = Promise.all(menuClosePromises);
> +    await promise;

Nit: just await the result, await the `menuToOpen` promise as well, and remove the return statement.

::: browser/components/uitour/UITour.jsm:1137
(Diff revision 4)
>     * The node to which a highlight or notification(-popup) is anchored is sometimes
>     * obscured because it may be inside an overflow menu. This function should figure
>     * that out and offer the overflow chevron as an alternative.
>     *
> -   * @param {Node} aAnchor The element that's supposed to be the anchor
> +   * @param {ChromeWindow} aChromeWindow The chrome window
> +   * @param {Object} aTarget The target object which its node is supposed to be the anchor

Nit: "The target object whose node is supposed to be the anchor"

::: browser/components/uitour/UITour.jsm:1148
(Diff revision 4)
>      // If the target is in the overflow panel, just return the overflow button.
> -    if (aAnchor.getAttribute("overflowedItem")) {
> -      let doc = aAnchor.ownerDocument;
> -      let placement = CustomizableUI.getPlacementOfWidget(aAnchor.id);
> +    if (node.getAttribute("overflowedItem")) {
> +      let doc = node.ownerDocument;
> +      let placement = CustomizableUI.getPlacementOfWidget(node.id);
>        let areaNode = doc.getElementById(placement.area);
>        return areaNode.overflowable._chevron;
>      }

This won't cover items in the permanent part of the overflow menu. We could fix this here or fix it in a followup bug?

More generally, I suspect this could be simplified by just calling `CustomizableUI.getWidget(node.id).forWindow(aChromeWindow).anchor`.

::: browser/components/uitour/UITour.jsm:1489
(Diff revision 4)
> +    } else if (aMenuName == "pageActionPanel") {
> +      let pageActionPanel = aWindow.BrowserPageActions.panelNode;
> +      pageActionPanel.setAttribute("noautohide", "true");
> +      pageActionPanel.addEventListener("popuphidden", this.onPanelHidden);
> +      pageActionPanel.addEventListener("ViewShowing", this.hidePageActionPanelAnnotations);
> +      pageActionPanel.addEventListener("popuphiding", this.hidePageActionPanelAnnotations);
> +      if (aOpenCallback) {
> +        pageActionPanel.addEventListener("popupshown", aOpenCallback, { once: true });
> +      }
> +      aWindow.BrowserPageActions.showPanel();

This is basically the same as the app menu code higher up. Can you de-duplicate it?
Attachment #8889061 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8892759 [details]
Bug 1382579 - Part 3: Tests,

https://reviewboard.mozilla.org/r/163718/#review170530

::: browser/components/uitour/test/browser_UITour4.js:117
(Diff revision 2)
> +  await tooltipHiddenPromise;
> +  is(appMenu.state, "closed", "Should close the app menu after hiding info");
> +  is(pageActionPanel.state, "closed", "Shouldn't open the page action panel after hiding info");
> +});
> +
> +add_UITour_task(async function test_highlight_buttonOnPageActionPanel_and_showInfo_buttonOnAppMenu() {

Can you add an opposite test as well (ie show info after show highlight to see that the panel closes in that case, too?

::: browser/components/uitour/test/browser_UITour4.js:126
(Diff revision 2)
> +  is_element_hidden(tooltip, "Tooltip should initially be hidden");
> +
> +  let appMenu = window.PanelUI.panel;
> +  let pageActionPanel = BrowserPageActions.panelNode;
> +
> +  // Test highlightiong the sendToDevice button on the page action panel

Nit: highlighting

::: browser/components/uitour/test/browser_UITour_availableTargets.js:8
(Diff revision 2)
>  var gTestTab;
>  var gContentAPI;
>  var gContentWindow;
>  
>  var hasPocket = Services.prefs.getBoolPref("extensions.pocket.enabled");
>  var isPhoton = Services.prefs.getBoolPref("browser.photon.structure.enabled");

This is gone now on current central. :-)

::: browser/components/uitour/test/browser_UITour_availableTargets.js
(Diff revision 2)
>    let data = await getConfigurationPromise("availableTargets");
> +  let expecteds = getExpectedTargets();
>    // Default minus "search" and "searchIcon"
> -  ok_targets(data, [
> -    "accountStatus",
> +  expecteds = expecteds.filter(target => target != "search" && target != "searchIcon");
> +  ok_targets(data, expecteds);
> -    "addons",

Thank you for deduplicating this!
Attachment #8892759 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8889061 [details]
Bug 1382579 - Part 2: UITour should support showMenu, showInfo, showHighlight on the Page Action Panel,

https://reviewboard.mozilla.org/r/159682/#review170526

> I think `user` is confusing here, because it sounds like the user opened it by clicking the anchor like normal - maybe call it `noautohideMenus` or something like this.

Thanks updated

> ```js
> 
> let panels [
>   aWindow.PanelUI.panel,
>   aWindow.gIdentityHandler._identityPopup,
>   aWindow.BrowserPageActions.panelNode
> ];
> for (let panel of panels) {
>   panel.removeEventListener(...); // x3
> }
> 
> ```

Thanks updated

> Nit: put the popuphiding and popuphidden listener additions/removals together. Same below for the page action menu.

Thanks updated

> We can enter this block when the menu isn't actually open, in which case I would not expect `popuphidden` to fire and `hideMenu` shouldn't be called, right?

No, looked into codes and from my tests, this wouldn’t happen.
Here is my test steps:
1. Call `showHighlight("addons”)` so the appMenu opens and the highlight shows.
2. Call `showInfo("pageAction-panel-bookmark”)`
3. Because of the step 2, we enters this block to close the appMenu.
4. The “popupHidding” event is fired at [1]. At this moment the popup state is “hiding”
5. In the “popupHidding” event, UITour does `hideAppMenuAnnotations` which calls `_hideAnnotationsForPanel` in turn.
6. The `_hideAnnotationsForPanel` would call `hideHihglight` but not immediately instead later after getting the target[2].
7. We don’t stop the event default behavior so the job of hiding popup continues to `HidePopupCallback`[3][4]. 
8. The popup state changes to “closed”[5] and soon the “popupHidden” event is fired[6].
9. Back to the `hideHihglight` call from the step 6, we would enter `_setMenuStateForAnnotation` again. Because the appMenu is in in the desired closed state, we would just leave.

[1] https://dxr.mozilla.org/mozilla-central/rev/52285ea5e54c73d3ed824544cef2ee3f195f05e6/layout/xul/nsXULPopupManager.cpp#1094
[2] https://dxr.mozilla.org/mozilla-central/rev/52285ea5e54c73d3ed824544cef2ee3f195f05e6/browser/components/uitour/UITour.jsm#1470
[3] https://dxr.mozilla.org/mozilla-central/rev/52285ea5e54c73d3ed824544cef2ee3f195f05e6/layout/xul/nsXULPopupManager.cpp#1591
[4] https://dxr.mozilla.org/mozilla-central/rev/52285ea5e54c73d3ed824544cef2ee3f195f05e6/layout/xul/nsXULPopupManager.cpp#1634
[5] https://dxr.mozilla.org/mozilla-central/rev/52285ea5e54c73d3ed824544cef2ee3f195f05e6/layout/xul/nsXULPopupManager.cpp#1180
[6] https://dxr.mozilla.org/mozilla-central/rev/52285ea5e54c73d3ed824544cef2ee3f195f05e6/layout/xul/nsXULPopupManager.cpp#1188

> nit: on which we showHighlight or showInfo

Thanks updated

> Nit: just await the result, await the `menuToOpen` promise as well, and remove the return statement.

Above there is a possible case we return a rejected promise when doing element visibility check. We cannot return nothing here otherwise the lint would complain about the inconsistent-return issue.

> Nit: "The target object whose node is supposed to be the anchor"

Thanks updated

> This won't cover items in the permanent part of the overflow menu. We could fix this here or fix it in a followup bug?
> 
> More generally, I suspect this could be simplified by just calling `CustomizableUI.getWidget(node.id).forWindow(aChromeWindow).anchor`.

Thanks for this. Yes, we could do `CustomizableUI.getWidget(node.id).forWindow(aChromeWindow).anchor`.

Looked into the CustomizableUI.getWidget.jsm. The major difference between the old and the new code is the `aOnlyRegistered` arg:
- The old code does `CustomizableUI.getPlacementOfWidget(node.id)`[1], which passes `aOnlyRegistered` as true.
- The new code does `CustomizableUIInternal.getPlacementOfWidget(aWidgetId)`[2], which passes `aOnlyRegistered` as undefined(false).

With a true `aOnlyRegistered`, we explicitly demand one existed widget[3][4].
If an node could match `node.getAttribute("overflowedItem”)` condition, I think it should exist in the `gPalette`, otherwise the CustomizableUI was handling something it didn’t know. On the other hand, for our case here `aOnlyRegistered == true` is unnecessary.

Please correct me if I missed something.

[1] https://dxr.mozilla.org/mozilla-central/rev/52285ea5e54c73d3ed824544cef2ee3f195f05e6/browser/components/customizableui/CustomizableUI.jsm#3620
[2] https://dxr.mozilla.org/mozilla-central/rev/52285ea5e54c73d3ed824544cef2ee3f195f05e6/browser/components/customizableui/CustomizableUI.jsm#4084
[3] https://dxr.mozilla.org/mozilla-central/rev/52285ea5e54c73d3ed824544cef2ee3f195f05e6/browser/components/customizableui/CustomizableUI.jsm#1806
[4] https://dxr.mozilla.org/mozilla-central/rev/52285ea5e54c73d3ed824544cef2ee3f195f05e6/browser/components/customizableui/CustomizableUI.jsm#1823

> This is basically the same as the app menu code higher up. Can you de-duplicate it?

Thanks updated
Comment on attachment 8892759 [details]
Bug 1382579 - Part 3: Tests,

https://reviewboard.mozilla.org/r/163718/#review170530

> Can you add an opposite test as well (ie show info after show highlight to see that the panel closes in that case, too?

Sure, added.

> Nit: highlighting

Thanks updated.

> This is gone now on current central. :-)

Thanks. Yes after rebasing this is gone.
Comment on attachment 8889061 [details]
Bug 1382579 - Part 2: UITour should support showMenu, showInfo, showHighlight on the Page Action Panel,

https://reviewboard.mozilla.org/r/159682/#review170682

::: browser/components/uitour/UITour.jsm:864
(Diff revision 5)
> +    ];
> +    for (let panel of panels) {
> +      // Ensure the menu panel is hidden and clean up panel listeners after calling hideMenu.
> +      if (panel.node.state != "closed") {
> +        await new Promise(resolve => {
> +          panel.node.addEventListener("popuphidden", resolve, { once: true });

Although the original codes work well, theoretically better to clean listeners after popup did close.
(In reply to :Gijs from comment #37)
> ::: browser/components/uitour/UITour.jsm:1148
> (Diff revision 4)
> >      // If the target is in the overflow panel, just return the overflow button.
> > -    if (aAnchor.getAttribute("overflowedItem")) {
> > -      let doc = aAnchor.ownerDocument;
> > -      let placement = CustomizableUI.getPlacementOfWidget(aAnchor.id);
> > +    if (node.getAttribute("overflowedItem")) {
> > +      let doc = node.ownerDocument;
> > +      let placement = CustomizableUI.getPlacementOfWidget(node.id);
> >        let areaNode = doc.getElementById(placement.area);
> >        return areaNode.overflowable._chevron;
> >      }
> 
> This won't cover items in the permanent part of the overflow menu. We could fix this here or fix it in a followup bug?
> 
OK, I will file a bug for it.
That would be great that Gijs, you could talk more about what is “items in the permanent part of the overflow menu”.
I mean in my daily usage the overflow chevron button is hidden. When the window is shrunk and no enough space, the chevron button would then appear to hold some items. Not sure what it is, thank you.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Fischer [:Fischer] from comment #45)
> (In reply to :Gijs from comment #37)
> > This won't cover items in the permanent part of the overflow menu. We could fix this here or fix it in a followup bug?
> > 
> OK, I will file a bug for it.
> That would be great that Gijs, you could talk more about what is “items in
> the permanent part of the overflow menu”.

You can right click items in the toolbar and click "pin to overflow menu".
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8889061 [details]
Bug 1382579 - Part 2: UITour should support showMenu, showInfo, showHighlight on the Page Action Panel,

https://reviewboard.mozilla.org/r/159682/#review170728

::: browser/components/uitour/UITour.jsm:1044
(Diff revision 5)
>  
> -    if (aShouldOpenForHighlight) {
> -      this.appMenuOpenForAnnotation.add(aAnnotationType);
> -    } else {
> -      this.appMenuOpenForAnnotation.delete(aAnnotationType);
> +  /**
> +   * Ensure the target's visibility and the open/close states of menus for the target.
> +   *
> +   * @param {ChromeWindow} aChromeWindow The chrome window
> +   * @param {Object} aTarget The target on which we show highligh or show info.

highlight

::: browser/components/uitour/UITour.jsm:1114
(Diff revision 5)
> +    // the node and stuffing it into the old anchor structure.
> +    let refreshedTarget = await this.getTarget(aChromeWindow, aTarget.targetName);
> +    let node = aTarget.node = refreshedTarget.node;
> +
>      // If the target is in the overflow panel, just return the overflow button.
> -    if (aAnchor.getAttribute("overflowedItem")) {
> +    if (node.getAttribute("overflowedItem")) {

Swap this out with: node.closest("#widget-overflow-scroller") and it should also work for items in the permanent part of the overflow panel.
Attachment #8889061 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs from comment #47)
> Comment on attachment 8889061 [details]
> Bug 1382579 - Part 2: UITour should support showMenu, showInfo, showHighlight on the Page Action Panel,
> 
> https://reviewboard.mozilla.org/r/159682/#review170728
> 
> ::: browser/components/uitour/UITour.jsm:1044 (Diff revision 5)
> > +   * @param {Object} aTarget The target on which we show highligh or show info.
> 
> highlight
> 
Thanks, will fix that.

> ::: browser/components/uitour/UITour.jsm:1114 (Diff revision 5)
> >      // If the target is in the overflow panel, just return the overflow button.
> > -    if (aAnchor.getAttribute("overflowedItem")) {
> > +    if (node.getAttribute("overflowedItem")) {
> 
> Swap this out with: node.closest("#widget-overflow-scroller") and it should also work for items in the permanent part of the overflow panel.
Thanks, got your point.
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2ad54451c5fb
Part 1: Update the BrowserPageActions API for the UITour's usage, r=adw,Gijs
https://hg.mozilla.org/integration/autoland/rev/fbf33c0d12d9
Part 2: UITour should support showMenu, showInfo, showHighlight on the Page Action Panel, r=Gijs
https://hg.mozilla.org/integration/autoland/rev/46706e14de5b
Part 3: Tests, r=Gijs
Keywords: checkin-needed
https://hg.mozilla.org/projects/date/rev/2ad54451c5fb657405e1b100cbdc981bb5dcb453
Bug 1382579 - Part 1: Update the BrowserPageActions API for the UITour's usage, r=adw,Gijs

https://hg.mozilla.org/projects/date/rev/fbf33c0d12d9d39be0dba9b2108c25154ab38a21
Bug 1382579 - Part 2: UITour should support showMenu, showInfo, showHighlight on the Page Action Panel, r=Gijs

https://hg.mozilla.org/projects/date/rev/46706e14de5bd8f36ae1671932d6c4f0054bedbe
Bug 1382579 - Part 3: Tests, r=Gijs
Hey Fischer, Could you explain what this change is please?
Flags: needinfo?(fliu)
(In reply to Justin [:JW_SoftvisionQA] from comment #56)
> Hey Fischer, Could you explain what this change is please?
HI Justin,
This is to enhance our library api for the external user. It made the external user able to highlight and show info on more Firefox features.
Guess probably you could do the real usage verification in the bug 1371538. The onboarding tour will use the function this bug added.
Flags: qe-verify+
Flags: needinfo?(jwilliams)
Flags: needinfo?(fliu)
No longer blocks: 1391071
Flags: needinfo?(jwilliams)
Flags: qe-verify+
I have verified that this bug has been fixed on today's nightly.
Status: RESOLVED → VERIFIED
I can confirm this fix on Fx 57.0b7 as well.
status-firefox57: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.