1394730 - Should clean up UITour highlight while navigating to another tour

Status: VERIFIED FIXED

(Firefox :: Tours, defect, P2)

Description

[Fischer [:Fischer]]

STR:
1. Open the Onboarding tour overlay
2. Go to the Customize tour by mouse click
3. Click the Customize tour action button to highlight the Customize button
4. Go to another tour by mouse click
p.s in the step 2 and 3, we could go to other tour which would highlight UI button such as the Library tour

Expected:
The the Customize button highlight disappears

Actual:
The the Customize button highlight doesn't disappear

Root Cause:
The .onboarding-tour-item elements have been set with the rule of `pointer-events: none;`[1] so that it won't get focus style during mouse navigation[2]. However, the .onboarding-tour-item elements won't react to the mouse event any more with `pointer-events: none;`[3]. As a result, we won't hide the previous UITour highlight while navigating to another tour.


[1] http://searchfox.org/mozilla-central/rev/cd82cacec2cf734768827ff85ba2dba90a534c5e/browser/extensions/onboarding/content/onboarding.css#163
[2] http://searchfox.org/mozilla-central/rev/cd82cacec2cf734768827ff85ba2dba90a534c5e/browser/extensions/onboarding/content/onboarding.css#333
[3] http://searchfox.org/mozilla-central/rev/cd82cacec2cf734768827ff85ba2dba90a534c5e/browser/extensions/onboarding/content/onboarding-tour-agent.js#67

Comment 1

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked for 57, this bug (among others) will track the "Onboarding spec change" work planned for early Beta57

Comment 2

[Fischer [:Fischer]]

Attachment: Bug 1394730 - Should clean up UITour highlight while navigating to another tour,

This patch:
- cleans up UITour while navigating to another tour by mouse
- cleans up UITour while navigating to another tour by keyboard
- fixes Bug 1397701 - Should clean up UITour after closing the onboarding overlay by clicking the Skip-Tour button together
- fixes Bug 1394731 - The onboarding tour nav item's icon doesn't change to the proper colored icon while mouse hovering together

Review commit: https://reviewboard.mozilla.org/r/177232/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/177232/

Comment 3

[Fischer [:Fischer]]

Comment on attachment 8905824 [details]
Bug 1394730 - Should clean up UITour highlight while navigating to another tour,

https://reviewboard.mozilla.org/r/177232/#review182684

::: browser/extensions/onboarding/content/onboarding-tour-agent.js:78
(Diff revision 1)
> -document.getElementById("onboarding-overlay-button").addEventListener("Agent:Destroy", () => {
> -  Mozilla.UITour.hideHighlight();
> -  Mozilla.UITour.hideMenu("urlbar");
> +let overlay = document.getElementById("onboarding-overlay");
> +overlay.addEventListener("click", onClick);
> +overlay.addEventListener("keypress", e => {
> +  let { target, key } = e;
> +  let classList = target.classList;
> +  if ((key == " " || key == "Enter") &&

Currently we didn't handle the keyboard case for cleaning up UITour. This part is for that.

::: browser/extensions/onboarding/content/onboarding-tour-agent.js:84
(Diff revision 1)
> +      (classList.contains("onboarding-tour-item") || classList.contains("onboarding-tour-item-container"))) {
> +    // Clean up UITour if a user tries to change to other tours.
> +    cleanupUITour();
> +  }
>  });
> +let overlayObserver = new MutationObserver(mutations => {

Here is to fix bug 1397701. We switched to a more general way please see https://bugzilla.mozilla.org/show_bug.cgi?id=1397701#c0 for the reason.

::: browser/extensions/onboarding/content/onboarding.css:388
(Diff revision 1)
>  #onboarding-notification-bar[data-target-tour-id=onboarding-tour-singlesearch] #onboarding-notification-tour-title::before {
>    background-image: url("img/icons_singlesearch.svg");
>  }
>  
>  #onboarding-tour-singlesearch.onboarding-active,
> -#onboarding-tour-singlesearch:hover {
> +.onboarding-tour-item-container:hover #onboarding-tour-singlesearch {

This CSS change is to fix bug 1394731.

::: browser/extensions/onboarding/test/browser/browser.ini:18
(Diff revision 1)
>  [browser_onboarding_notification_click_auto_complete_tour.js]
>  [browser_onboarding_select_default_tour.js]
>  [browser_onboarding_skip_tour.js]
>  [browser_onboarding_tours.js]
>  [browser_onboarding_tourset.js]
> +[browser_onboarding_uitour.js]

Created an new test file for the uitour test cases

::: browser/extensions/onboarding/test/browser/browser_onboarding_tours.js
(Diff revision 1)
>      }
>      await BrowserTestUtils.removeTab(tab);
>    }
>  });
> -
> -add_task(async function test_clean_up_uitour_on_page_unload() {

promisePopupChange, test_clean_up_uitour_on_page_unload & test_clean_up_uitour_on_window_resize have been moved to and modified in browser_onboarding_uitour.js

Comment 4

[Fischer [:Fischer]]

Attachment: correct_nav_icon_hovering_state.gif

Comment 5

[Fischer [:Fischer]]

(In reply to Fischer [:Fischer] from comment #2)
> Created attachment 8905824 [details]
> Bug 1394730 - Should clean up UITour while navigating to another tour,
> 
> This patch:
> - cleans up UITour while navigating to another tour by mouse
> - cleans up UITour while navigating to another tour by keyboard
> - fixes Bug 1397701 - Should clean up UITour after closing the onboarding
> overlay by clicking the Skip-Tour button together
> - fixes Bug 1394731 - The onboarding tour nav item's icon doesn't change to
> the proper colored icon while mouse hovering together
> 
> Review commit: https://reviewboard.mozilla.org/r/177232/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/177232/
Hi Rex,

Please see attachment 8905826 [details]: correct_nav_icon_hovering_state.gif for the CSS fix in action
TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5e6e3f995d055bdc2497bb3de95e18aff8afdd1

Thanks

Comment 6

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8905824 [details]
Bug 1394730 - Should clean up UITour highlight while navigating to another tour,

https://reviewboard.mozilla.org/r/177232/#review182742

::: browser/extensions/onboarding/content/onboarding-tour-agent.js:84
(Diff revision 1)
> +      (classList.contains("onboarding-tour-item") || classList.contains("onboarding-tour-item-container"))) {
> +    // Clean up UITour if a user tries to change to other tours.
> +    cleanupUITour();
> +  }
>  });
> +let overlayObserver = new MutationObserver(mutations => {

Considering the size of the patch I think put these 3 fixes together is bad for maintaince and tracing... could you try to separate them?

Comment 7

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8905824 [details]
Bug 1394730 - Should clean up UITour highlight while navigating to another tour,

https://reviewboard.mozilla.org/r/177232/#review182746

::: browser/extensions/onboarding/content/onboarding-tour-agent.js:12
(Diff revision 1)
> +(function() {
>  "use strict";
>  
> -document.addEventListener("Agent:CanSetDefaultBrowserInBackground", () => {
> +let cleanupUITour = () => {
> +  Mozilla.UITour.hideHighlight();
> +  Mozilla.UITour.hideMenu("urlbar");

I thought urlbar disappears automatically when losing focus... So I'm not quite sure why this line is necessary.

::: browser/extensions/onboarding/content/onboarding-tour-agent.js:67
(Diff revision 1)
> -      // Dismiss any highlights if a user tries to close the dialog.
> -      Mozilla.UITour.hideHighlight();
> -      break;
>    }
> -  // Dismiss any highlights if a user tries to change to other tours.
> -  if (evt.target.classList.contains("onboarding-tour-item")) {
> +  let classList = evt.target.classList;
> +  if (classList.contains("onboarding-tour-item") || classList.contains("onboarding-tour-item-container")) {

I think just `onboarding-tour-item-container` should be enough because onboarding-tour-item doesn't receive mouse events?

Comment 8

[Fischer [:Fischer]]

Comment on attachment 8905824 [details]
Bug 1394730 - Should clean up UITour highlight while navigating to another tour,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/177232/diff/1-2/

Comment 9

[Fischer [:Fischer]]

(In reply to KM Lee [:rexboy] from comment #6)
> 
> Considering the size of the patch I think put these 3 fixes together is bad
> for maintaince and tracing... could you try to separate them?
Sure, split up the patches.

Comment 10

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8905824 [details]
Bug 1394730 - Should clean up UITour highlight while navigating to another tour,

https://reviewboard.mozilla.org/r/177232/#review183188

::: browser/extensions/onboarding/content/onboarding-tour-agent.js:10
(Diff revision 2)
> - /* globals Mozilla */
> +/* globals Mozilla */
>  
> +(function() {
>  "use strict";
>  
> -document.addEventListener("Agent:CanSetDefaultBrowserInBackground", () => {
> +let cleanupUITour = () => {

Can we just remove this function?

Comment 11

[Fischer [:Fischer]]

Comment on attachment 8905824 [details]
Bug 1394730 - Should clean up UITour highlight while navigating to another tour,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/177232/diff/2-3/

Comment 12

[Fischer [:Fischer]]

Comment on attachment 8905824 [details]
Bug 1394730 - Should clean up UITour highlight while navigating to another tour,

https://reviewboard.mozilla.org/r/177232/#review183188

> Can we just remove this function?

OK, updated

Comment 13

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Maybe 56 beta is affected by this bug if accessibility patches has been uplifted to beta.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/67fb712adac1
Should clean up UITour highlight while navigating to another tour, r=rexboy

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/67fb712adac1

Comment 16

[Justin [:JW_SoftvisionQA]]

I have verified that this bug has been fixed on today's nightly.

Comment 17

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I can confirm the intended behavior is respected on beta. I verified using Fx 57.0b7 on Windows 10 x64, Ubuntu 14.04 LTS and macOS X 10.12.6.