1384841 - Match the style of the UITour highlights with Photon style guide

Status: VERIFIED FIXED

(Firefox :: Tours, enhancement, P1)

Description

[Bryant Mao [:bryantmao]]

Current highlights style on toolbar and menu (those triggered by clicking the CTA on the onboarding tour) are not aligned well with the Photon style guideline. We'll need to refine them to provide a consistent visual experience in 57

Comment 1

[KM Lee [:rexboy] (inactivate after Sep. 22)]

We may consider if we want to include the first highlight problem into our backlog. I've sent a patch for it per earlier investigate.

Comment 2

[[e-mail]]

Bryant, is there a spec or a good reference for the proper style?

Comment 3

[Bryant Mao [:bryantmao]]

Attachment: Tour CTA Highlights 0728.png

Here is the spec for the CTA highlights.

Comment 4

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: preview

Tried to highlight most of the things I can using the new style.

UX needs to investigate if it passes contrast checker first, and maybe refine the specification.

Comment 5

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: preview_dark

... and on dark theme.

Comment 6

[KM Lee [:rexboy] (inactivate after Sep. 22)]

After some discussion with UX there will be some change on the spec:
1. The fill color is now in 30% opacity.
2. Seems highlighting on the toolbar widget need to be a tall rectangle because of containers' shape. The structure inside each widget differs. I'm not sure changing the query sentence to query the "square-shaped" childs is a good idea.

Comment 7

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: Bug 1384841 - Match the style of the UITour highlights with Photon style guide.

Review commit: https://reviewboard.mozilla.org/r/163766/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/163766/

Comment 8

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8892780 [details]
Bug 1384841 - Match the style of the UITour highlights with Photon style guide.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/163766/diff/1-2/

Comment 9

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Hello Gijs, can you help review this patch? It changes the style of the UITour's highlight; and there's a little change on categorizing its styles. Now the style is determined by whether the object to be highlighted is inside Photon's hamburger menu.

Thank you!

Comment 10

[:Gijs (he/him)]

Comment on attachment 8892780 [details]
Bug 1384841 - Match the style of the UITour highlights with Photon style guide.

https://reviewboard.mozilla.org/r/163766/#review169208

It looks like the height now always matches the toolbar height instead of being a square inside the toolbar, so this doesn't look right to me. Additionally, the styling looks bad on the back button because it is left-aligned but doesn't encompass the right border and is asymmetrical.

Comment 11

[KM Lee [:rexboy] (inactivate after Sep. 22)]

(In reply to :Gijs from comment #10)
> Comment on attachment 8892780 [details]
> Bug 1384841 - Match the style of the onboarding tour highlights with Photon
> style guide.
> 
> https://reviewboard.mozilla.org/r/163766/#review169208
> 
> It looks like the height now always matches the toolbar height instead of
> being a square inside the toolbar, so this doesn't look right to me.
I may just put back the dimensions ratio check to make it a square; But That would still be a slightly bigger square than the spec. I'll double confirm with UX on it.
The area that really changes color on hover is implemented inside the button's XBL binding. I think it would be better to avoid querying anonymous nodes and relying on those encapsulated details.

> Additionally, the styling looks bad on the back button because it is
> left-aligned but doesn't encompass the right border and is asymmetrical.
This is the same as above. the element that really changes color is just inside its binding, so that would need another query into its anonymous nodes.

Comment 12

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: keep highlight square on toolbar button

Comment 13

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: preview-square

Comment 14

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8893240 [details]
keep highlight square on toolbar button

based on our discussion, Bryant could you confirm if this looks fine to you?
Thanks!

Comment 15

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8893243 [details]
preview-square

Sorry, let's use this for more cases.

Comment 16

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8892780 [details]
Bug 1384841 - Match the style of the UITour highlights with Photon style guide.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/163766/diff/2-3/

Comment 17

[:Gijs (he/him)]

(In reply to KM Lee [:rexboy] from comment #11)
> (In reply to :Gijs from comment #10)
> > Comment on attachment 8892780 [details]
> > Bug 1384841 - Match the style of the onboarding tour highlights with Photon
> > style guide.
> > 
> > https://reviewboard.mozilla.org/r/163766/#review169208
> > 
> > It looks like the height now always matches the toolbar height instead of
> > being a square inside the toolbar, so this doesn't look right to me.
> I may just put back the dimensions ratio check to make it a square; But That
> would still be a slightly bigger square than the spec. I'll double confirm
> with UX on it.
> The area that really changes color on hover is implemented inside the
> button's XBL binding. I think it would be better to avoid querying anonymous
> nodes and relying on those encapsulated details.
> 
> > Additionally, the styling looks bad on the back button because it is
> > left-aligned but doesn't encompass the right border and is asymmetrical.
> This is the same as above. the element that really changes color is just
> inside its binding, so that would need another query into its anonymous
> nodes.

I think we can use .toolbarbutton-icon iff the target is a toolbarbutton and it is a descendant of a toolbar. Then we can just use a pre-specified size because we know the icon is 16x16, and we can center the highlight over the icon. That would achieve the spec, and I don't see any reason why it would be broken - we've always used .toolbarbutton-icon (since pre-Firefox-1) and so it's unlikely to ever change as too many things would break. The only case I can think of is badged buttons, or something like the back button where the overall size needs to be bigger. That, too, should be solvable by specialcasing badged buttons and the back button.

Comment 18

[:Gijs (he/him)]

Comment on attachment 8892780 [details]
Bug 1384841 - Match the style of the UITour highlights with Photon style guide.

https://reviewboard.mozilla.org/r/163766/#review169720

Clearing review given my comment on bugzilla.

Comment 19

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: Preview - exact size

Comment 20

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: Preview - Make square by button's width

After some investigate I can get the exact size of the clickable area's size by using "var(--toolbarbutton-inner-padding) * 2 + 16" (found in toolbarbuttons.inc.css); But that'd be just 28px.

Since the spec specify the size should be 32px, which is just the width of toolbar buttons, it turns out that I can make a square by just using the width of toolbar button, and align it vertically center to achieve the requirement.

Comment 21

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8892780 [details]
Bug 1384841 - Match the style of the UITour highlights with Photon style guide.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/163766/diff/3-4/

Comment 22

[KM Lee [:rexboy] (inactivate after Sep. 22)]

The suggestions are addressed in the patch. Gijs may you take a look again?
Thank you!

Comment 23

[:Gijs (he/him)]

Comment on attachment 8892780 [details]
Bug 1384841 - Match the style of the UITour highlights with Photon style guide.

https://reviewboard.mozilla.org/r/163766/#review170378

::: browser/components/uitour/UITour.jsm:1121
(Diff revision 4)
>        let highlightHeight = targetRect.height;
>        let highlightWidth = targetRect.width;
> -      let minDimension = Math.min(highlightHeight, highlightWidth);
> -      let maxDimension = Math.max(highlightHeight, highlightWidth);
>  
> -      // If the dimensions are within 200% of each other (to include the bookmarks button),
> +      if (gPhotonStructure && this.targetIsInAppMenu(aTarget)) {

I'm removing gPhotonStructure in bug 1354117 so please rebase and don't depend on this.

::: browser/components/uitour/UITour.jsm:1126
(Diff revision 4)
> +      if (highlightAnchor.classList.contains("toolbarbutton-1") &&
> +        highlightAnchor.getAttribute("cui-areatype") === "toolbar") {
> +          // A toolbar button in navbar has its clickable area an
> +          // inner-contained square while the button component itself is a tall
> +          // rectangle. We adjust the highlight area to a square as well.
> +          highlightHeight = highlightWidth;
>        }

This will be wrong if the item is in the dynamic (not pinned) part of the overflow panel.

::: browser/components/uitour/UITour.jsm:1148
(Diff revision 4)
> -      let offsetX = -(Math.max(0, highlightWidthWithMin - targetRect.width) / 2);
> -      let offsetY = -(Math.max(0, highlightHeightWithMin - targetRect.height) / 2);
> +      let offsetX = (targetRect.width - highlightWidthWithMin) / 2;
> +      let offsetY = (targetRect.height - highlightHeightWithMin) / 2;

This only works when the highlight is smaller than the item. What guarantees that that is the case? Have you tested with items in the url bar, for instance, which I expect are < than 24px?

::: browser/themes/shared/UITour.inc.css:22
(Diff revision 4)
>    /* Compensate the displacement caused by padding. */
>    margin: -4px;
>  }
>  
>  #UITourHighlight {
> -  background-image: radial-gradient(50% 100%, rgba(0,149,220,0.4) 50%, rgba(0,149,220,0.6) 100%);
> +  background-color: RGBA(0, 200, 215, 0.3);

Nit: lowercase rgba

::: browser/themes/shared/UITour.inc.css:27
(Diff revision 4)
> -  box-shadow: 0 0 3px 0 rgba(0,0,0,0.49);
> -  min-height: 32px;
> +#UITourHighlight:not(.photon-appmenu-highlight) {
> +  border-radius: 4px;

This is confusing - there's no style rule that does anything for `.photon-appmenu-highlight`, so why do we need this? If we need a class to toggle the border radius because we don't want a border-radius in the app meu, then set a class when we're *not* highlighting in the app menu, instead of using a :not() selector.

Comment 24

[Fischer [:Fischer]]

A drive-by:
In fact this bug is to modify UITour's highlight style for the photon design so update the bug title to reflect what it is doing better.

Comment 25

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8892780 [details]
Bug 1384841 - Match the style of the UITour highlights with Photon style guide.

https://reviewboard.mozilla.org/r/163766/#review170378

> This will be wrong if the item is in the dynamic (not pinned) part of the overflow panel.

Thanks for picking up this. I'll try to add a condition for `overflowedItem=true`.

> This only works when the highlight is smaller than the item. What guarantees that that is the case? Have you tested with items in the url bar, for instance, which I expect are < than 24px?

I've tested for the search icon which is 20x20. It works well. It turns out that the Max() should be just unnecessary in all cases. The Max() just reads like, “don’t put on any offset when the final highlight size is smaller than element itself”, which should be a case not even exist before.
for a `< 24` element, like 20px, the subtraction is always positive. `Math.max(0, 24 - 20)` greater than 0.
for a `>= 24` element, like 30px, the subtraction is always 0. `-Math.max(0, 30 - 30)` equals 0. Note that in this case `highlightHeightWithMin` picks an element's original size.

So it looks like some leftover or just miscoding to me..

I made a step-by-step trace with code fragments if you'd like to take a look:
https://gist.github.com/rexboy7/8ab6c315b64706ca5b98fbdecce01127

Comment 26

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8892780 [details]
Bug 1384841 - Match the style of the UITour highlights with Photon style guide.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/163766/diff/4-5/

Comment 27

[:Gijs (he/him)]

Comment on attachment 8892780 [details]
Bug 1384841 - Match the style of the UITour highlights with Photon style guide.

https://reviewboard.mozilla.org/r/163766/#review170730

::: browser/components/uitour/UITour.jsm:1064
(Diff revision 5)
> -      // make the highlight a circle with the largest dimension as the diameter.
> +        highlighter.classList.remove("photon-non-appmenu-highlight");
> -      if (maxDimension / minDimension <= 3.0) {
> -        highlightHeight = highlightWidth = maxDimension;
> -        highlighter.style.borderRadius = "100%";
>        } else {
> -        highlighter.style.borderRadius = "";
> +        highlighter.classList.add("photon-non-appmenu-highlight");

Use a descriptive class name, like "rounded-highlight".

::: browser/components/uitour/UITour.jsm:1067
(Diff revision 5)
> +        highlightAnchor.getAttribute("cui-areatype") === "toolbar" &&
> +        highlightAnchor.getAttribute("overflowedItem") !== "true") {

Indenting is wrong here.

::: browser/components/uitour/UITour.jsm:1072
(Diff revision 5)
> +        highlightAnchor.getAttribute("cui-areatype") === "toolbar" &&
> +        highlightAnchor.getAttribute("overflowedItem") !== "true") {
> +          // A toolbar button in navbar has its clickable area an
> +          // inner-contained square while the button component itself is a tall
> +          // rectangle. We adjust the highlight area to a square as well.
> +          highlightHeight = highlightWidth;

And here.

The if statement condition contents should line up together, and the contents of the block should be 2-space indented because that's what the rest of the file does.

Did you run eslint?

Comment 28

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8892780 [details]
Bug 1384841 - Match the style of the UITour highlights with Photon style guide.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/163766/diff/5-6/

Comment 29

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8892780 [details]
Bug 1384841 - Match the style of the UITour highlights with Photon style guide.

https://reviewboard.mozilla.org/r/163766/#review170730

> And here.
> 
> The if statement condition contents should line up together, and the contents of the block should be 2-space indented because that's what the rest of the file does.
> 
> Did you run eslint?

Yes. the before-commit hook you told me is very handy, thanks!
Let me correct the indent before checking-in.

Comment 30

[KM Lee [:rexboy] (inactivate after Sep. 22)]

looks quite good on try.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc76fe216349

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7c3e4a58c61c
Match the style of the UITour highlights with Photon style guide. r=Gijs

Comment 32

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing browser_UITour.js, at least on OS X:

https://hg.mozilla.org/integration/autoland/rev/8c7788eda00e4cb4445638bdb7500009cd4cbe59

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=7c3e4a58c61c83e6aff0efd886f0f2bcaaac6b78&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=121579256&repo=autoland

00:28:17     INFO - Starting test_highlight_circle
00:28:17     INFO - Buffered messages logged at 00:28:17
00:28:17     INFO - TEST-PASS | browser/components/uitour/test/browser_UITour.js | Element should not be null, when checking visibility - 
00:28:17     INFO - TEST-PASS | browser/components/uitour/test/browser_UITour.js | Highlight should initially be hidden - 
00:28:17     INFO - TEST-PASS | browser/components/uitour/test/browser_UITour.js | Highlight should be shown after showHighlight() - 
00:28:17     INFO - addons target: width: 32 height: 38
00:28:17     INFO - highlight: width: 32 height: 32
00:28:17     INFO - Buffered messages finished
00:28:17     INFO - TEST-UNEXPECTED-FAIL | browser/components/uitour/test/browser_UITour.js | The width of the highlight should be equal to the largest dimension of the target - Got 32, expected 38
00:28:17     INFO - Stack trace:
00:28:17     INFO - chrome://mochikit/content/browser-test.js:test_is:1002
00:28:17     INFO - chrome://mochitests/content/browser/browser/components/uitour/test/browser_UITour.js:check_highlight_size:116
00:28:17     INFO - chrome://mochitests/content/browser/browser/components/uitour/test/h

Comment 33

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8892780 [details]
Bug 1384841 - Match the style of the UITour highlights with Photon style guide.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/163766/diff/6-7/

Comment 34

[Mozilla Autoland]

'ascii' codec can't encode character u'\u2014' in position 74: ordinal not in range(128)

Comment 35

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Looks like there are some test items that didn't run on linux.
I've addressed them in the revised patch.

Comment 36

[:Gijs (he/him)]

(In reply to KM Lee [:rexboy] from comment #35)
> Looks like there are some test items that didn't run on linux.
> I've addressed them in the revised patch.

Autoland failed, see comment 34.

Comment 37

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Thanks for reminding. there was a wrong character in my try command. It should be okay now.

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cc5b627abbdd
Match the style of the UITour highlights with Photon style guide. r=Gijs

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cc5b627abbdd

Comment 40

[Abe - QA (:Abe_LV)]

Verified as fixed.

Comment 41

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I can confirm the happy flow is respected in Fx 57.0b7 as well.