Should clean up UITour highlight while navigating to another tour

VERIFIED FIXED in Firefox 57

Status

()

P2
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: Fischer, Assigned: Fischer)

Tracking

unspecified
Firefox 57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57+ verified)

Details

(Whiteboard: [photon-onboarding])

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
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)

Updated

2 years ago
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
status-firefox57: --- → affected
tracking-firefox57: --- → +
(Assignee)

Updated

2 years ago
See Also: → bug 1394731
(Assignee)

Updated

2 years ago
See Also: → bug 1397701
(Assignee)

Updated

2 years ago
Summary: Should hide the previous UITour highlight while navigating to another tour → Should clean up UITour while navigating to another tour
Comment hidden (mozreview-request)
(Assignee)

Comment 3

2 years ago
mozreview-review
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
(Assignee)

Comment 5

2 years ago
(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?
(Assignee)

Updated

2 years ago
Summary: Should clean up UITour while navigating to another tour → Should clean up UITour highlight while navigating to another tour
Comment hidden (mozreview-request)
(Assignee)

Comment 9

2 years ago
(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.
(Assignee)

Updated

2 years ago
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 hidden (mozreview-request)
(Assignee)

Comment 12

2 years ago
mozreview-review-reply
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.
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 14

2 years ago
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

Comment 15

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/67fb712adac1
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox57: affected → fixed
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.
status-firefox57: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.