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

RESOLVED FIXED in Firefox 55

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Gijs, Assigned: Gijs)

Tracking

(Blocks 1 bug)

53 Branch
Firefox 55
Points:
---
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [photon-structure])

Attachments

(4 attachments)

(Assignee)

Description

2 years ago
Some of our functionality is being replaced, and the UI tour should continue working. We should update the UI tour code where necessary.
(Assignee)

Updated

2 years ago
Whiteboard: [photon-structure]
Flags: qe-verify?
Priority: -- → P2
(Assignee)

Updated

2 years ago
Flags: qe-verify? → qe-verify+
QA Contact: gwimberly
(Assignee)

Comment 1

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

Comment 7

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

2 years ago
mozreview-review
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 9

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

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

Comment 16

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

Comment 17

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