1177175 - Add a UITour target inside the TP panel

Status: RESOLVED FIXED

(Firefox :: Tours, defect, P1)

Description

[Matthew N. [:MattN]]

We may want to anchor a tour panel on the Tracking Protection panel for the first run experience.

The anchor should be on the side so we should use the infoPanelPosition property and maybe infoPanelOffsetY too.

Comment 1

[:Paolo Amadini]

Attachment: MozReview Request: Bug 1177175 - Add a UITour target inside the TP panel. r=MattN

Bug 1177175 - Add a UITour target inside the TP panel. r=MattN

Comment 2

[:Paolo Amadini]

Work in progress, mostly works but the tests and the panel close logic need adjustments. Also, the arrow of the UITour "tooltip" panel is hidden behind the Control Center panel.

Comment 3

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/13527/#review12195

::: browser/components/uitour/UITour.jsm:125
(Diff revision 1)
> +    ["controlCenter-tracking-action-unblock", {

Nit: "controlCenter-trackingUnblock"

::: browser/components/uitour/UITour.jsm:1833
(Diff revision 1)
>    getAvailableTargets: function(aMessageManager, aChromeWindow, aCallbackID) {

Could you add a test that your new target isn't in available targets when the menu is closed but is when the menu opens. I updated my patch to invalidate the available targets cache.

::: browser/components/uitour/UITour.jsm:127
(Diff revision 1)
> +      query: (aDocument) => {

Nit: feel free to use newer method syntax for new lines since you're not using `this` anyways:

`query(aDocument) {`

::: browser/components/uitour/test/browser.ini:54
(Diff revision 1)
> +[browser_UITour_trackingprotection.js]

Nit: The _UITour_ in the file name is legacy cruft from before the tests moved to their own component.

You can add this to browser_trackingProtection.js

::: browser/components/uitour/test/head.js:179
(Diff revision 1)
> -function loadUITourTestPage(callback, host = "https://example.com/") {
> +function loadUITourTestPage(callback, host = "https://tracking.example.org/") {

Try to leave "tracking." out of the domain as we discussed.

Comment 4

[:Paolo Amadini]

Somewhat unrelated, I've noticed that the browser_UITour_availableTargets.js test fails when run on its own because of a missing "accountStatus" target. I'm testing all the directory at once for now.

Comment 5

[:Paolo Amadini]

(In reply to Matthew N. [:MattN] from comment #3)
> You can add this to browser_trackingProtection.js

I actually had to create a separate browser_trackingProtection_tour.js file in order to use the test runner that load the content API (like browser_showMenu_controlCenter does).

Comment 6

[:Paolo Amadini]

Comment on attachment 8635296 [details]
MozReview Request: Bug 1177175 - Add a UITour target inside the TP panel. r=MattN

Bug 1177175 - Add a UITour target inside the TP panel. r=MattN

Comment 7

[:Paolo Amadini]

Comment on attachment 8635296 [details]
MozReview Request: Bug 1177175 - Add a UITour target inside the TP panel. r=MattN

Bug 1177175 - Add a UITour target inside the TP panel. r=MattN

Comment 8

[:Paolo Amadini]

So, the tooltip appeared behind the panel since the latter had level="top". The only way to solve that without changing the level of the Control Center is to open the tooltip with level="top" as well, which can be done by setting the attribute on the element just before opening the popup.

I think the patch in ready for a full review now.

Comment 9

[:Paolo Amadini]

Comment on attachment 8635296 [details]
MozReview Request: Bug 1177175 - Add a UITour target inside the TP panel. r=MattN

Bug 1177175 - Add a UITour target inside the TP panel. r=MattN

Comment 10

[:Paolo Amadini]

Fixed an issue highlighted by a previous try run, caused by the fact that the first item in server-locations.txt is used to generate the CN of the Issued To field of the certificate. I still reordered the example.org entry to match the order in the HTTP section for clarity.

Comment 11

[Matthew N. [:MattN]]

(In reply to :Paolo Amadini from comment #8)
> So, the tooltip appeared behind the panel since the latter had level="top".
> The only way to solve that without changing the level of the Control Center
> is to open the tooltip with level="top" as well, which can be done by
> setting the attribute on the element just before opening the popup.

level=top is almost always a bad thing in my experience (especially when combined with noautohide=true). I didn't realize the control center had that set to "top". It was added in http://hg.mozilla.org/mozilla-central/diff/fcc32ef59156/browser/base/content/browser.xul but the rationale isn't clear to me. While working on Australis, we removed level="top" from a few panels (most notably the new menu panel) because it caused problems and I think we'll those here too.

According to the definition from https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Attribute/panel.level, we wouldn't want level="top" since the identity panel shouldn't appear above other applications.

Masayuki, can you explain the difference in UX if we remove level="top" from the identity panel (aka. control center)? Note that during the UITour we will set @noautohide=true.

Comment 12

[Matthew N. [:MattN]]

Comment on attachment 8635296 [details]
MozReview Request: Bug 1177175 - Add a UITour target inside the TP panel. r=MattN

https://reviewboard.mozilla.org/r/13527/#review12207

::: browser/components/uitour/UITour.jsm:1750
(Diff revision 4)
>    onPanelHidden: function(aEvent) {
>      aEvent.target.removeAttribute("noautohide");
>      UITour.recreatePopup(aEvent.target);
>      this.availableTargetsCache.clear();

Would you mind fixing a mistake in my patch. This bottom line should be:
UITour.availableTargetsCache.clear();

::: browser/components/uitour/UITour.jsm:1505
(Diff revision 4)
> +      if (aAnchor.infoPanelOnTop) {
> +        tooltip.setAttribute("level", "top");
> +      }
>        tooltip.openPopup(aAnchorEl, alignment, xOffset || 0, yOffset || 0);
> +      if (aAnchor.infoPanelOnTop) {
> +        tooltip.removeAttribute("level");
> +      }

I really think we should remove @level=top from teh control center instead. To see the problem, switch to another application while the control center is opened by showMenu. It will appear above the other application.

::: browser/components/uitour/UITour.jsm:132
(Diff revision 4)
> +        if (popup.state != "open") {
> +          return null;
> +        }

You could use `UITour.isElementVisible` on the element instead which also handles other reasons why it may not be visible.

::: browser/components/uitour/test/browser_trackingProtection_tour.js:42
(Diff revision 4)
> +    let available = (data.targets.indexOf(currentTarget) != -1);
> +    ok(!available, "Control Center targets should not be available by default");

Once you fix the place I made a mistake with above, can you add a test that the target is no longer available after closing the control center.

::: browser/components/uitour/test/browser_trackingProtection_tour.js:56
(Diff revision 4)
> +    yield ContentTask.spawn(gBrowser.selectedBrowser, {}, function () {
> +      content.document.getElementById("tracking-element").remove();
> +    });
> +  }),
> +];

Can you also test that `hideMenu("controlCenter")` causes the info panel to automatically hide?

Comment 13

[:Paolo Amadini]

Comment on attachment 8635296 [details]
MozReview Request: Bug 1177175 - Add a UITour target inside the TP panel. r=MattN

Bug 1177175 - Add a UITour target inside the TP panel. r=MattN

Comment 14

[:Paolo Amadini]

(In reply to Matthew N. [:MattN] from comment #12)
> You could use `UITour.isElementVisible` on the element instead which also
> handles other reasons why it may not be visible.

Note that in addition to isElementVisible I also had to retain the check on the visibility of the panel, otherwise the test wouldn't pass. Maybe the state that causes isElementVisible to return false is not updated fast enough, despite the identity panel itself is being hidden.

Comment 15

[Matthew N. [:MattN]]

Comment on attachment 8635296 [details]
MozReview Request: Bug 1177175 - Add a UITour target inside the TP panel. r=MattN

https://reviewboard.mozilla.org/r/13527/#review12247

Thanks for the test fixes!

Comment 16

[:Paolo Amadini]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d2e8e75e8ec

https://hg.mozilla.org/integration/fx-team/rev/15fb177845e9

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/15fb177845e9

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Matthew N. [:MattN] from comment #11)
> (In reply to :Paolo Amadini from comment #8)
> According to the definition from
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Attribute/panel.
> level, we wouldn't want level="top" since the identity panel shouldn't
> appear above other applications.
> 
> Masayuki, can you explain the difference in UX if we remove level="top" from
> the identity panel (aka. control center)? Note that during the UITour we
> will set @noautohide=true.

It's better not to use front-most window since as you mentioned, it can appear other application's window too. If level="top" not specified, the window level depends on the platform. Unfortunately, Linux's default window level is "top". So, you should check if the issue isn't reproduced on Linux.