Closed Bug 1410763 Opened 8 years ago Closed 8 years ago

Backout bug 1390055

Categories

(Firefox :: Tours, defect, P1)

Product:
Firefox
Component:
Tours
Version:
58 Branch
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox57 --- verified
firefox58 --- verified

People

(Reporter: gasolin, Assigned: gasolin)

References

Details

(Whiteboard: [photon-onboarding])

Attachments

(1 file)

Bug 1410763 - Backout bug 1390055;
59 bytes, text/x-review-board-request
Fischer
: review+
ritu
: approval-mozilla-beta+
Details
as bug 1406012 comment 8, Jennifer's(researcher) research results confirm that the highlight on the toolbar button is not noticed. Michael (UX) request to backout bug 1390055 so we will always highlight library in appmenu.
Assignee: nobody → gasolin
Status: NEW → ASSIGNED
Depends on: 1390055
Please provide a patch, directly reverting the code from bug 1390055 doesn't work because either the code itself or nearby code has changed. Thank you.
Jdavidson just presented the findings from this study and overall guidance is to fix this for 57. when users clicked the library CTA, they thought that button “didn’t work” because we highlight the little library icon in the toolbar instead of the drop down menu we do for most of the other tour items. recommendation from jdavidson was that it should be fixed for 57 as it contributes to the misunderstanding of library and it makes it look like our tour “doesn’t work”. product also agrees considering we have a low risk patch for this.
Flags: needinfo?(rkothari)
Nicole reached out to me for an opinion on this. I think we should uplift the change based on the *assumption* this is a low-risk patch.
Hi fischer, please compare this PR with https://reviewboard.mozilla.org/r/169450/diff/2#index_header I did not remove the browser.xul change in this patch since it's the correct modification and not effect UI at all. Though we can keep the tests change with just a little modification, as a backout patch I decide to remove all these changes. We can put it back anytime.
Flags: qe-verify+
Priority: -- → P2
Whiteboard: [photon-onboarding]
(In reply to Fred Lin [:gasolin] from comment #5) > Hi fischer, please compare this PR with > https://reviewboard.mozilla.org/r/169450/diff/2#index_header > > I did not remove the browser.xul change in this patch since it's the correct > modification and not effect UI at all. Agree as well > > Though we can keep the tests change with just a little modification, as a > backout patch I decide to remove all these changes. > We can put it back anytime. I think removing all the changes is reasonable too.
Priority: P2 → P1
Backed out for failing browser-chrome's browser/extensions/onboarding/test/browser/browser_onboarding_uitour.js on Windows 7 debug without e10s: https://hg.mozilla.org/integration/autoland/rev/baca0818395316e15c4c59a8382bccb14d9168e7 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=11b79af86747b8e3ac7222b65567b071f1318af0&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=139133698&repo=autoland 09:28:26 INFO - 813 INFO TEST-PASS | browser/extensions/onboarding/test/browser/browser_onboarding_uitour.js | Should show UITour highlight - 09:28:26 INFO - 814 INFO TEST-PASS | browser/extensions/onboarding/test/browser/browser_onboarding_uitour.js | UITour should highlight library - 09:28:26 INFO - Buffered messages finished 09:28:26 ERROR - 815 INFO TEST-UNEXPECTED-FAIL | browser/extensions/onboarding/test/browser/browser_onboarding_uitour.js | Uncaught exception - Should close onboarding overlay - timed out after 30 tries. 09:28:26 INFO - 816 INFO Leaving test bound test_clean_up_uitour_after_closing_overlay
Flags: needinfo?(gasolin)
Thanks for the note Nicole. Hi Fred, could you please fill out an uplift request? If this is indeed low risk, we can uplift to beta57.
Flags: needinfo?(rkothari)
(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #9) > Backed out for failing browser-chrome's > browser/extensions/onboarding/test/browser/browser_onboarding_uitour.js on > Windows 7 debug without e10s: > > https://hg.mozilla.org/integration/autoland/rev/baca0818395316e15c4c59a8382bccb14d9168e7 > > Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=11b79af86747b8e3ac7222b65567b071f1318af0&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable > Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=139133698&repo=autoland > Looks like backed out because of the test case clicking #onboarding-overlay-close-btn. From the try[1], on Win7 debug, it failed at the point that the overlay didn't close after trying to click #onboarding-overlay-close-btn. After remove this test case, from the try[2], all passed. The test case clicking #onboarding-overlay-close-btn is no longer valid and user shouldn't be able to click it when highlighting the library button on the appMenu in real usage. It is fine to remove that test case. @Fred, Would you update the test? Thanks [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e70f596106b80b3e067d45af4414662ed548ad9 [2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb783aa9b996723c298009f1028d49478041aa59
Thanks fischer, updated to remove the clicking #onboarding-overlay-close-btn behavior in test case. Added win7 as test platform, wait for test result before land.
Flags: needinfo?(gasolin)
Backed out in https://hg.mozilla.org/integration/autoland/rev/c12815d7a9fc for failing browser-chrome's browser/extensions/onboarding/test/browser/browser_onboarding_uitour.js on Windows 7 debug without e10s: https://treeherder.mozilla.org/logviewer.html#?job_id=139552350&repo=autoland
Comment on attachment 8921346 [details] Bug 1410763 - Backout bug 1390055; https://reviewboard.mozilla.org/r/192374/#review198436 ::: browser/extensions/onboarding/test/browser/browser_onboarding_uitour.js (Diff revision 2) > await triggerUITourHighlight("library", tab); > await highlightOpenPromise; > is(highlight.state, "open", "Should show UITour highlight"); > is(highlight.getAttribute("targetName"), "library", "UITour should highlight library"); > - > - // Close the overlay by clicking the skip-tour button This is not the test case which should be removed
(In reply to Fischer [:Fischer] from comment #16) > Comment on attachment 8921346 [details] > Bug 1410763 - Backout bug 1390055; > > https://reviewboard.mozilla.org/r/192374/#review198436 > > ::: browser/extensions/onboarding/test/browser/browser_onboarding_uitour.js > (Diff revision 2) > > await triggerUITourHighlight("library", tab); > > await highlightOpenPromise; > > is(highlight.state, "open", "Should show UITour highlight"); > > is(highlight.getAttribute("targetName"), "library", "UITour should highlight library"); > > - > > - // Close the overlay by clicking the skip-tour button > > This is not the test case which should be removed My fault, should close with overlay, not the skip-tour button.
See Also: → 1411296
Not sure is that didn't run the test or didn't see the test failure before check-in. We got a perma failure of bug 1411296. The investigation in comment 11 just focused on the back out reason so didn't run all tests...
Blocks: 1411296
Comment on attachment 8921346 [details] Bug 1410763 - Backout bug 1390055; https://reviewboard.mozilla.org/r/192374/#review198852 ::: browser/extensions/onboarding/test/browser/browser_onboarding_uitour.js:57 (Diff revision 4) > await highlightOpenPromise; > is(highlight.state, "open", "Should show UITour highlight"); > is(highlight.getAttribute("targetName"), "library", "UITour should highlight library"); > > - // Close the overlay by clicking the overlay close button > + // Close the overlay by clicking the overlay > + // Should not click the close button here since the close button is hovered by appmenu and can't be clicked on win7 Adding a comment to explain is good but this test case is being tested above already( start from Line 42) as the comment 11 and the comment 19 what we should do is to remove the invalid test case.
(In reply to Fischer [:Fischer] from comment #21) > Not sure is that didn't run the test or didn't see the test failure before > check-in. We got a perma failure of bug 1411296. The investigation in > comment 11 just focused on the back out reason so didn't run all tests... Correct myself: False alarm, the bug 1411296 test case should be removed but why the autoland still complains about it... https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1411296&startday=2017-10-26&endday=2017-10-26&tree=all
Comment on attachment 8921346 [details] Bug 1410763 - Backout bug 1390055; https://reviewboard.mozilla.org/r/192372/#review198856 ::: browser/components/uitour/test/browser_UITour5.js (Diff revision 4) > - CustomizableUI.removeWidgetFromArea("add-ons-button"); > - }); > -}); > - > add_UITour_task(async function test_highlight_library_and_show_library_subview() { > - CustomizableUI.removeWidgetFromArea("library-button"); we should keep this row so the toolbar library icon will be removed and highlight the right appmenu library item
(In reply to Fred Lin [:gasolin] from comment #24) > Comment on attachment 8921346 [details] > Bug 1410763 - Backout bug 1390055; > > https://reviewboard.mozilla.org/r/192372/#review198856 > > ::: browser/components/uitour/test/browser_UITour5.js > (Diff revision 4) > > - CustomizableUI.removeWidgetFromArea("add-ons-button"); > > - }); > > -}); > > - > > add_UITour_task(async function test_highlight_library_and_show_library_subview() { > > - CustomizableUI.removeWidgetFromArea("library-button"); > > we should keep this row so the toolbar library icon will be removed and > highlight the right appmenu library item Though the test should not care about this non-related id, since "library-button" is removed from the uitour selection query
(In reply to Fred Lin [:gasolin] from comment #24) > Comment on attachment 8921346 [details] > Bug 1410763 - Backout bug 1390055; > > https://reviewboard.mozilla.org/r/192372/#review198856 > > ::: browser/components/uitour/test/browser_UITour5.js > (Diff revision 4) > > - CustomizableUI.removeWidgetFromArea("add-ons-button"); > > - }); > > -}); > > - > > add_UITour_task(async function test_highlight_library_and_show_library_subview() { > > - CustomizableUI.removeWidgetFromArea("library-button"); > > we should keep this row so the toolbar library icon will be removed and > highlight the right appmenu library item OK so this is not a false alarm then but why the test runs didn't complain it in the 1st place... We need to do some specific try run on this test to see what happened
> > > > we should keep this row so the toolbar library icon will be removed and > > highlight the right appmenu library item > OK so this is not a false alarm then but why the test runs didn't complain > it in the 1st place... > We need to do some specific try run on this test to see what happened As comment 25 that should not be the reason cause test fail. Run `browser/components/uitour/test --rebuild 5` in all platform looks green as well https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c11c2a1284084f1e839f94bd53ab42a98889535&selectedJob=140129873
(In reply to Fred Lin [:gasolin] from comment #27) > > > > > > we should keep this row so the toolbar library icon will be removed and > > > highlight the right appmenu library item > > OK so this is not a false alarm then but why the test runs didn't complain > > it in the 1st place... > > We need to do some specific try run on this test to see what happened > > As comment 25 that should not be the reason cause test fail. > > Run `browser/components/uitour/test --rebuild 5` in all platform looks green > as well > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=5c11c2a1284084f1e839f94bd53ab42a98889535&selectedJob=140129873 Thanks for the further investigation. Please still open a follow-up bug to remove the redundant wrong test case as comment 22.
No longer blocks: 1411296
See Also: 1411296 →
https://hg.mozilla.org/mozilla-central/rev/6b5696f6f09c - landed 24 hours ago.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
In summary the test failure's cause is we write a test that assume the library highlight is at toolbar by default, but after backout the behavior is open the app menu by default and the menu will overlap the overlay's close button. So the test code can not hide the overlay with the close button.
Comment on attachment 8921346 [details] Bug 1410763 - Backout bug 1390055; Approval Request Comment [Feature/Bug causing the regression]: Bug 1390055 [User impact if declined]: research shows it comfuse the user when he/she tap onboarding library tour button [Is this code covered by automated tests?]: yes [Has the fix been verified in Nightly?]: N, but covered by test cases [Needs manual test from QE? If yes, steps to reproduce]: switch to update user(browser.onboarding.tour-type = update), Open onboarding and tap library tour button, should open appmenu and highlight the library menu [List of other uplifts needed for the feature/fix]: N [Is the change risky?]: N [Why is the change risky/not risky?]: it's a backout. The reason that cause new testcase fail(fixed) is described in comment 31 [String changes made/needed]: N
Attachment #8921346 - Flags: approval-mozilla-beta?
Did you verify that the patch could apply on Beta with all tests passed? A Try link with that will be helpful. Thanks.
Flags: needinfo?(gasolin)
Applied patch on Beta branch and passed Local test on uitour/onboarding module. Here's the try link https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea37a635a299
Flags: needinfo?(gasolin)
Comment on attachment 8921346 [details] Bug 1410763 - Backout bug 1390055; Backout, Beta57+
Attachment #8921346 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Blocks: 1412761
I verified this issue using Latest Nightly 58.0a1 and Firefox Beta 57.0 with Build ID 20171109220104 on Windows 10 x64. I will mark this as verified fixed.
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
status-firefox58: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: