Closed Bug 1394730 Opened 7 years ago Closed 7 years ago

Should clean up UITour highlight while navigating to another tour

Categories

(Firefox :: Tours, defect, P2)

defect

Tracking

()

VERIFIED FIXED
Firefox 57
Tracking Status
firefox57 + verified

People

(Reporter: Fischer, Assigned: Fischer)

References

Details

(Whiteboard: [photon-onboarding])

Attachments

(2 files)

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
Assignee: nobody → fliu
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [photon-onboarding]
Flags: qe-verify+
QA Contact: jwilliams
Tracked for 57, this bug (among others) will track the "Onboarding spec change" work planned for early Beta57
See Also: → 1394731
See Also: → 1397701
Summary: Should hide the previous UITour highlight while navigating to another tour → Should clean up UITour while navigating to another tour
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
(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 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?
Attachment #8905824 - Flags: review?(rexboy)
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?
Summary: Should clean up UITour while navigating to another tour → Should clean up UITour highlight while navigating to another tour
(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.
Blocks: 1397701
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?
Attachment #8905824 - Flags: review?(rexboy) → review+
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
Maybe 56 beta is affected by this bug if accessibility patches has been uplifted to beta.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/67fb712adac1
Should clean up UITour highlight while navigating to another tour, r=rexboy
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/67fb712adac1
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
I have verified that this bug has been fixed on today's nightly.
Status: RESOLVED → VERIFIED
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.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.