Match the style of the UITour highlights with Photon style guide

VERIFIED FIXED in Firefox 57

Status

()

Firefox
Tours
P1
normal
VERIFIED FIXED
5 months ago
2 months ago

People

(Reporter: bryantmao, Assigned: rexboy)

Tracking

unspecified
Firefox 57
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify +

Firefox Tracking Flags

(firefox57 verified)

Details

(Whiteboard: [photon-onboarding])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(7 attachments, 1 obsolete attachment)

(Reporter)

Description

5 months ago
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

Updated

5 months ago
Assignee: nobody → fliu
Status: NEW → ASSIGNED
Component: General → Tours
Whiteboard: [photon-onboarding][triage] → [photon-onboarding]

Updated

5 months ago
Assignee: fliu → rexboy
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.
See Also: → bug 1049130

Comment 2

5 months ago
Bryant, is there a spec or a good reference for the proper style?
Flags: needinfo?(bmao)

Updated

5 months ago
Whiteboard: [photon-onboarding] → [photon-onboarding][triage]
(Reporter)

Comment 3

5 months ago
Created attachment 8891175 [details]
Tour CTA Highlights 0728.png

Here is the spec for the CTA highlights.
Flags: needinfo?(bmao)

Updated

5 months ago
Flags: qe-verify+
QA Contact: jwilliams
Whiteboard: [photon-onboarding][triage] → [photon-onboarding]
Created attachment 8891953 [details]
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.
Created attachment 8891955 [details]
preview_dark

... and on dark theme.

Updated

5 months ago
Target Milestone: --- → Firefox 57
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
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

5 months ago
mozreview-review
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.
Attachment #8892780 - Flags: review?(gijskruitbosch+bugs) → review-
(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.
Created attachment 8893240 [details]
keep highlight square on toolbar button
Created attachment 8893243 [details]
preview-square
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!
Attachment #8893240 - Flags: ui-review?(bmao)
Comment on attachment 8893243 [details]
preview-square

Sorry, let's use this for more cases.
Attachment #8893243 - Flags: ui-review?(bmao)
Attachment #8893240 - Flags: ui-review?(bmao)
Comment hidden (mozreview-request)

Comment 17

5 months ago
(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

5 months ago
mozreview-review
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.
Attachment #8892780 - Flags: review?(gijskruitbosch+bugs)
Created attachment 8893749 [details]
Preview - exact size
Attachment #8893243 - Attachment is obsolete: true
Attachment #8893243 - Flags: ui-review?(bmao)
Created attachment 8893754 [details]
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 hidden (mozreview-request)
The suggestions are addressed in the patch. Gijs may you take a look again?
Thank you!

Comment 23

4 months ago
mozreview-review
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.
Attachment #8892780 - Flags: review?(gijskruitbosch+bugs)

Updated

4 months ago
Summary: Match the style of the onboarding tour highlights with Photon style guide → Match the style of the UITour highlights with Photon style guide

Comment 24

4 months ago
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.
(Assignee)

Comment 25

4 months ago
mozreview-review-reply
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 hidden (mozreview-request)

Comment 27

4 months ago
mozreview-review
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?
Attachment #8892780 - Flags: review?(gijskruitbosch+bugs) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 29

4 months ago
mozreview-review-reply
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.
looks quite good on try.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc76fe216349
Keywords: checkin-needed

Comment 31

4 months ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7c3e4a58c61c
Match the style of the UITour highlights with Photon style guide. r=Gijs
Keywords: checkin-needed
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
Flags: needinfo?(rexboy)
Comment hidden (mozreview-request)

Comment 34

4 months ago
'ascii' codec can't encode character u'\u2014' in position 74: ordinal not in range(128)
Looks like there are some test items that didn't run on linux.
I've addressed them in the revised patch.
Flags: needinfo?(rexboy)

Comment 36

4 months ago
(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.
Flags: needinfo?(rexboy)
Thanks for reminding. there was a wrong character in my try command. It should be okay now.
Flags: needinfo?(rexboy)
Keywords: checkin-needed

Comment 38

4 months ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cc5b627abbdd
Match the style of the UITour highlights with Photon style guide. r=Gijs
Keywords: checkin-needed

Comment 39

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cc5b627abbdd
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox57: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED

Comment 40

4 months ago
Verified as fixed.
Status: RESOLVED → VERIFIED
Blocks: 1389416

Updated

4 months ago
Depends on: 1390767
I can confirm the happy flow is respected in Fx 57.0b7 as well.
status-firefox57: fixed → verified
You need to log in before you can comment on or make changes to this bug.