Closed Bug 1359137 Opened 8 years ago Closed 7 years ago

Update UI Tour highlighting and other secondary consumers to know about new hamburger panel

Categories

(Firefox :: Tours, enhancement, P1)

Product:
Firefox
Component:
Tours
Version:
53 Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Iteration:
55.7 - Jun 12
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: Gijs, Assigned: Gijs)

References

Details

(Whiteboard: [photon-structure])

Attachments

(4 files)

Bug 1359137 - update UITour to highlight in new panel where appropriate,
59 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1359137 - fix BrowserUITelemetry's reliance on the default state of things in CustomizableUI,
59 bytes, text/x-review-board-request
mikedeboer
: review+
Details
Bug 1359137 - move webcompat button into toolbar for tests,
59 bytes, text/x-review-board-request
mikedeboer
: review+
Details
Bug 1359137 - fix bookmarks menu context test to not rely on AREA_PANEL,
59 bytes, text/x-review-board-request
mikedeboer
: review+
Details
Some of our functionality is being replaced, and the UI tour should continue working. We should update the UI tour code where necessary.
Whiteboard: [photon-structure]
Flags: qe-verify?
Priority: -- → P2
Flags: qe-verify? → qe-verify+
QA Contact: gwimberly
Taking, morphing slightly, and setting qe- given that this has so many automated tests.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: qe-verify+ → qe-verify-
Priority: P2 → P1
Summary: Update UI Tour highlighting to be able to deal with new hamburger panel and/or new overflow panel → Update UI Tour highlighting and other secondary consumers to know about new hamburger panel
Iteration: --- → 55.7 - Jun 12
QA Contact: gwimberly
Comment on attachment 8873111 [details] Bug 1359137 - update UITour to highlight in new panel where appropriate, https://reviewboard.mozilla.org/r/144568/#review148514 The approach LGTM (I didn't test it). ::: browser/components/uitour/test/browser_UITour.js:120 (Diff revision 1) > let highlight = document.getElementById("UITourHighlight"); > is_element_hidden(highlight, "Highlight should initially be hidden"); > > - gContentAPI.showHighlight("addons"); > + gContentAPI.showHighlight("home"); > waitForElementToBeVisible(highlight, check_highlight_size, "Highlight should be shown after showHighlight()"); > }, OOC, what's this for?
Attachment #8873111 - Flags: review?(MattN+bmo) → review+
(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #6) > Comment on attachment 8873111 [details] > Bug 1359137 - update UITour to highlight in new panel where appropriate, > > https://reviewboard.mozilla.org/r/144568/#review148514 > > The approach LGTM (I didn't test it). > > ::: browser/components/uitour/test/browser_UITour.js:120 > (Diff revision 1) > > let highlight = document.getElementById("UITourHighlight"); > > is_element_hidden(highlight, "Highlight should initially be hidden"); > > > > - gContentAPI.showHighlight("addons"); > > + gContentAPI.showHighlight("home"); > > waitForElementToBeVisible(highlight, check_highlight_size, "Highlight should be shown after showHighlight()"); > > }, > > OOC, what's this for? Ah, yes, I meant to ask, sorry... this bit of the browser_UITour.js test checks that the highlight is round. But it seems to only be round by virtue of the fact that it's highlighting a square element. A lot of these elements are no longer square, because the hamburger menu is now vertically laid out so all the elements are long and thin, that is, quite wide and not very tall. So test assertions were failing as a result of the highlight being basically a rounded rectangle rather than a huge circle. I swapped this test over to a different button that is still square, and then it doesn't fail. This is, perhaps, a general issue - the highlights will now, in some circumstances, highlight entire rows in the menu. I think this is fine, but if you already know that we should be highlighting only the small icon in front of the row (like we did/do for customize mode in the pre-photon case) for each of these, a number of these highlights probably want updating. Thoughts? :-)
Flags: needinfo?(MattN+bmo)
Comment on attachment 8873112 [details] Bug 1359137 - fix BrowserUITelemetry's reliance on the default state of things in CustomizableUI, https://reviewboard.mozilla.org/r/144570/#review148954 s/OLD/LEGACY/ or DEPRECATED, perhaps?
Attachment #8873112 - Flags: review?(mdeboer) → review+
Comment on attachment 8873113 [details] Bug 1359137 - move webcompat button into toolbar for tests, https://reviewboard.mozilla.org/r/144572/#review148956
Attachment #8873113 - Flags: review?(mdeboer) → review+
Comment on attachment 8873114 [details] Bug 1359137 - fix bookmarks menu context test to not rely on AREA_PANEL, https://reviewboard.mozilla.org/r/144574/#review148958
Attachment #8873114 - Flags: review?(mdeboer) → review+
(In reply to :Gijs from comment #7) > (In reply to Matthew N. [:MattN] (PM if requests are blocking you) from > comment #6) > > Comment on attachment 8873111 [details] > > Bug 1359137 - update UITour to highlight in new panel where appropriate, > > > > https://reviewboard.mozilla.org/r/144568/#review148514 > > > > The approach LGTM (I didn't test it). > > > > ::: browser/components/uitour/test/browser_UITour.js:120 > > (Diff revision 1) > > > let highlight = document.getElementById("UITourHighlight"); > > > is_element_hidden(highlight, "Highlight should initially be hidden"); > > > > > > - gContentAPI.showHighlight("addons"); > > > + gContentAPI.showHighlight("home"); > > > waitForElementToBeVisible(highlight, check_highlight_size, "Highlight should be shown after showHighlight()"); > > > }, > > > > OOC, what's this for? > > Ah, yes, I meant to ask, sorry... this bit of the browser_UITour.js test > checks that the highlight is round. But it seems to only be round by virtue > of the fact that it's highlighting a square element. A lot of these elements > are no longer square, because the hamburger menu is now vertically laid out > so all the elements are long and thin, that is, quite wide and not very > tall. So test assertions were failing as a result of the highlight being > basically a rounded rectangle rather than a huge circle. I swapped this test > over to a different button that is still square, and then it doesn't fail. I see, makes sense. > This is, perhaps, a general issue - the highlights will now, in some > circumstances, highlight entire rows in the menu. I think this is fine, but > if you already know that we should be highlighting only the small icon in > front of the row (like we did/do for customize mode in the pre-photon case) > for each of these, a number of these highlights probably want updating. > Thoughts? :-) I don't know about the plans for how highlights will look since the only onboarding spec image I see is which the old menu (since onboarding may ship before Photon): https://mozilla.invisionapp.com/share/4MBDUMS5W#/screens/228511972_Overview I guess you would have to ask Verdi or a visual designer on how it's supposed to look with the new menu but I'm fine landing without knowing that.
Flags: needinfo?(MattN+bmo)
Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/5ec621ced42e update UITour to highlight in new panel where appropriate, r=MattN https://hg.mozilla.org/integration/autoland/rev/4f3980aa447f fix BrowserUITelemetry's reliance on the default state of things in CustomizableUI, r=mikedeboer https://hg.mozilla.org/integration/autoland/rev/a0899380ac24 move webcompat button into toolbar for tests, r=mikedeboer https://hg.mozilla.org/integration/autoland/rev/ca3a6a1ab24f fix bookmarks menu context test to not rely on AREA_PANEL, r=mikedeboer
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/679c049063f1 Fix rest-spread-spacing ESLint failure in BrowserUITelemetry.jsm.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: