Closed Bug 1385351 Opened 7 years ago Closed 7 years ago

Emphasize the ping selector button in about:telemetry

Categories

(Toolkit :: Telemetry, enhancement, P1)

Product:
Toolkit
Component:
Telemetry
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: flyingrub, Assigned: flyingrub)

References

Details

Attachments

(2 files, 2 obsolete files)

Bug 1385351 - Emphasize the ping selector button in about:telemetry
59 bytes, text/x-review-board-request
gfritzsche
: review+
Details
Bug 1385351 - Remove unused old class
59 bytes, text/x-review-board-request
gfritzsche
: review+
Details 
It is actually hard to understand that the ping selector button are clickable.

We could use a triangle to indicate drop-down?
Assignee: nobody → flyinggrub
Priority: -- → P1
We could move the arrow if the popup is hidden or not ?

Here is some screenshots of the current state.
https://framapic.org/F8TlXVxikZg6/vwN964UskPsN.png
https://framapic.org/IfAaSJUHkTOk/SVTXn1eGMFsW.png


I think that we perhaps need something totaly different. But cannot think of something that would fit all use case :
* Show the current ping details
* Allow to easily switch between ping
* Open the ping picker popup
Attachment #8895126 - Flags: review?(gfritzsche)
Here is a screenshot with the arrow rotating according to the popup state : https://framapic.org/FtkMEjoLObkx/ctYuwzcpMl14.png.
Also you can see the fix of Bug 1388456 (the popup is now next to the button).
Comment on attachment 8895126 [details]
Bug 1385351 - Emphasize the ping selector button in about:telemetry

https://reviewboard.mozilla.org/r/166234/#review171760

I think the triangle indicator is definitely an improvement right now (and we could talk with UX through better options in the future).

- - Why not show the dialog right under the button?
  I believe that is common practice for these kind of popup dialogs.
- I don't think this arrow needs to rotate/change here.
- Can we just use a standard unicode character for the downward triangle?

::: toolkit/content/aboutTelemetry.css:82
(Diff revision 2)
>  
> +#ping-date {
> +  flex-grow: 1;
> +}
> +
> +#ping-date::after {

Why not use a unicode character instead, say `▼`?
Attachment #8895126 - Flags: review?(gfritzsche)
Comment on attachment 8895126 [details]
Bug 1385351 - Emphasize the ping selector button in about:telemetry

https://reviewboard.mozilla.org/r/166234/#review171760

> Why not use a unicode character instead, say `▼`?

I thought about it but the html special char is not interpreted and rendered as is. I tried with the unicode character directly it worked but I prefered the css version design for no particular reason. Let's change it to a unicode char. :)
Comment on attachment 8895126 [details]
Bug 1385351 - Emphasize the ping selector button in about:telemetry

https://reviewboard.mozilla.org/r/166234/#review172300

::: toolkit/content/aboutTelemetry.css:84
(Diff revision 3)
> +  flex-grow: 1;
> +}
> +
> +#ping-date::after {
> +  position: fixed;
> +  content: "▾";

The triangle should be a little bigger (relative to the text size).

We escape non-ASCII characters in our code.
For CSS this is done using `\xxxxxx`:
https://www.w3.org/International/questions/qa-escapes#
Attachment #8895126 - Flags: review?(gfritzsche)
Because the unicode character can differ according to the current font, we switched back to the css styling.
Comment on attachment 8895126 [details]
Bug 1385351 - Emphasize the ping selector button in about:telemetry

https://reviewboard.mozilla.org/r/166234/#review172762

The cleanup of the "hidden" class is unrelated.
You should move that to a separate commit or bug.
Attachment #8895126 - Flags: review?(gfritzsche)
Comment on attachment 8895126 [details]
Bug 1385351 - Emphasize the ping selector button in about:telemetry

Dao, is this way to use the button reasonable?
I guess arrow-dropdown-16.svg should probably be shared properly instead of being copied?
Attachment #8895126 - Flags: feedback?(dao+bmo)
Comment on attachment 8895126 [details]
Bug 1385351 - Emphasize the ping selector button in about:telemetry

https://reviewboard.mozilla.org/r/166234/#review172874

::: toolkit/content/aboutTelemetry.css:39
(Diff revision 7)
>    display: flex;
>    flex-direction: column;
>    font-size: 18px;
>    color: var(--in-content-category-text);
>    pointer-events: none;
> -  padding: 12px 21px 15px 21px;
> +  padding: 12px 8px 12px 8px;

nit:

padding: 12px 8px;

::: toolkit/content/aboutTelemetry.css:76
(Diff revision 7)
>    text-align: center;
>  }
>  
> -.controls {
> +.dropdown {
> +  background-image: url(chrome://global/skin/icons/arrow-dropdown-16.svg);
> +  background-position: right 8px center;

about:telemetry is mirrored in RTL builds, so this will need to take that into account. You can use body[dir=rtl] for that.

::: toolkit/content/aboutTelemetry.css:78
(Diff revision 7)
>  
> -.controls {
> +.dropdown {
> +  background-image: url(chrome://global/skin/icons/arrow-dropdown-16.svg);
> +  background-position: right 8px center;
> +  background-repeat: no-repeat;
> +  background-size: auto 18px;

Hmm, why stretch the image instead of using the native size?

::: toolkit/content/aboutTelemetry.css:80
(Diff revision 7)
> +  background-image: url(chrome://global/skin/icons/arrow-dropdown-16.svg);
> +  background-position: right 8px center;
> +  background-repeat: no-repeat;
> +  background-size: auto 18px;
> +  -moz-context-properties: fill;
> +  fill: var(--in-content-category-text);

Can you use fill: currentColor;?

::: toolkit/themes/shared/jar.inc.mn:41
(Diff revision 7)
>    skin/classic/global/icons/loading.png                    (../../shared/icons/loading.png)
>    skin/classic/global/icons/loading@2x.png                 (../../shared/icons/loading@2x.png)
>    skin/classic/global/icons/spinner-arrow-down.svg         (../../shared/icons/spinner-arrow-down.svg)
>    skin/classic/global/icons/spinner-arrow-up.svg           (../../shared/icons/spinner-arrow-up.svg)
>    skin/classic/global/icons/menubutton-dropmarker.svg      (../../shared/icons/menubutton-dropmarker.svg)
> +  skin/classic/global/icons/arrow-dropdown-16.svg          (../../shared/icons/arrow-dropdown-16.svg)

Why aren't you using menubutton-dropmarker.svg as discussed on IRC?
Attachment #8895126 - Flags: feedback?(dao+bmo)
Comment on attachment 8895126 [details]
Bug 1385351 - Emphasize the ping selector button in about:telemetry

https://reviewboard.mozilla.org/r/166234/#review172932

::: toolkit/content/aboutTelemetry.css:80
(Diff revision 7)
> +  background-image: url(chrome://global/skin/icons/arrow-dropdown-16.svg);
> +  background-position: right 8px center;
> +  background-repeat: no-repeat;
> +  background-size: auto 18px;
> +  -moz-context-properties: fill;
> +  fill: var(--in-content-category-text);

I didn't know about this ! Thanks :)
::dao,

A screenshot with menubutton-dropmaker.svg : https://framapic.org/a8o8tVC49Uia/bXUkscdzx3EL.png (it is not centered and seems blured)
A screenshot without the background resize : https://framapic.org/gYpNostvwpcN/yzeHL2yFpAhL.png
A screenshot of the actual commit : https://framapic.org/5j4xNnXWyIRZ/XOn5iIKB3Fru.png

Do you think I should remove the resize or use the menubutton-dropmaker ?
Flags: needinfo?(dao+bmo)
Attachment #8896352 - Attachment is obsolete: true
Attachment #8896352 - Flags: review?(gfritzsche)
(In reply to flyingrub from comment #19)
> ::dao,
> 
> A screenshot with menubutton-dropmaker.svg :
> https://framapic.org/a8o8tVC49Uia/bXUkscdzx3EL.png (it is not centered and
> seems blured)
> A screenshot without the background resize :
> https://framapic.org/gYpNostvwpcN/yzeHL2yFpAhL.png
> A screenshot of the actual commit :
> https://framapic.org/5j4xNnXWyIRZ/XOn5iIKB3Fru.png
> 
> Do you think I should remove the resize or use the menubutton-dropmaker ?

You shouldn't have to resize and we shouldn't add another dropmarker image. What we could do is:

- move arrow-dropdown-16.svg and arrow-dropdown-12.svg to toolkit
- update browser/ to use these icons from toolkit: http://searchfox.org/mozilla-central/search?q=arrow-dropdown-...svg&case=false&regexp=true&path=css
- update toolkit/themes/shared/popupnotification.inc.css to use arrow-dropdown-16.svg
- remove menubutton-dropmarker.svg
Flags: needinfo?(dao+bmo)
Attachment #8895126 - Flags: feedback?(dao+bmo)
Comment on attachment 8895126 [details]
Bug 1385351 - Emphasize the ping selector button in about:telemetry

https://reviewboard.mozilla.org/r/166234/#review173040

::: toolkit/content/aboutTelemetry.css:83
(Diff revision 10)
> +  -moz-context-properties: fill;
> +  fill: currentColor;
> +}
> +
> +body[dir=rtl] .dropdown {
> +  background-position: left 8px center;

this is simpler:

background-position-x: left 8px;
Attachment #8895126 - Flags: feedback?(dao+bmo) → feedback+
Attachment #8896581 - Attachment is obsolete: true
Comment on attachment 8895126 [details]
Bug 1385351 - Emphasize the ping selector button in about:telemetry

https://reviewboard.mozilla.org/r/166234/#review173554

I gave this a test-run locally and it looks good.
Attachment #8895126 - Flags: review?(gfritzsche) → review+
Comment on attachment 8896495 [details]
Bug 1385351 - Remove unused old class

https://reviewboard.mozilla.org/r/167746/#review173556
Attachment #8896495 - Flags: review?(gfritzsche) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c89766a47689
Emphasize the ping selector button in about:telemetry r=gfritzsche
https://hg.mozilla.org/integration/autoland/rev/84e7b3d7c7cf
Remove unused old class r=gfritzsche
Keywords: checkin-needed
Comment on attachment 8895126 [details]
Bug 1385351 - Emphasize the ping selector button in about:telemetry

This should fix the error. Do you see something else ::dao ?
Flags: needinfo?(flyinggrub)
Attachment #8895126 - Flags: feedback?(dao+bmo)
Keywords: checkin-needed
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 10f3ad337747 -d f17a2e7ae4e0: rebasing 414419:10f3ad337747 "Bug 1385351 - Emphasize the ping selector button in about:telemetry r=gfritzsche"
merging browser/base/content/test/performance/browser_startup_images.js
merging browser/themes/shared/customizableui/customizeMode.inc.css
merging browser/themes/shared/jar.inc.mn
merging browser/themes/shared/sidebar.inc.css
merging browser/themes/shared/tabs.inc.css
merging toolkit/content/aboutTelemetry.css
merging toolkit/content/aboutTelemetry.js
merging toolkit/content/aboutTelemetry.xhtml
merging toolkit/themes/shared/jar.inc.mn
merging toolkit/themes/shared/popupnotification.inc.css
warning: conflicts while merging browser/base/content/test/performance/browser_startup_images.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/themes/shared/jar.inc.mn! (edit, then use 'hg resolve --mark')
warning: conflicts while merging toolkit/content/aboutTelemetry.css! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment on attachment 8895126 [details]
Bug 1385351 - Emphasize the ping selector button in about:telemetry

There's now another place using arrow-dropdown-16.svg: https://hg.mozilla.org/mozilla-central/annotate/7d6962d0a62a/browser/themes/shared/urlbar-searchbar.inc.css#l114
Attachment #8895126 - Flags: feedback?(dao+bmo)
Comment on attachment 8895126 [details]
Bug 1385351 - Emphasize the ping selector button in about:telemetry

https://reviewboard.mozilla.org/r/166234/#review175286

::: browser/base/content/test/performance/browser_startup_images.js:47
(Diff revision 13)
>      platforms: ["linux", "win", "macosx"],
>    },
> +  {
> +    file: "chrome://global/skin/icons/arrow-dropdown-16.svg",
> +    platforms: ["linux", "win", "macosx"],
> +  },

Adding this entry back will make this test fail.
It should be ok now ?
It looks like urlbar-searchbar.inc.css still needs to be updated.
I updated it in the second commit... It's fixed.
Keywords: checkin-needed
This has pending issues in MozReview.
Keywords: checkin-needed
:S
Keywords: checkin-needed
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s be1e818d9af9 -d 4b675a7f0743: rebasing 414786:be1e818d9af9 "Bug 1385351 - Emphasize the ping selector button in about:telemetry r=gfritzsche"
merging browser/themes/shared/customizableui/customizeMode.inc.css
merging browser/themes/shared/jar.inc.mn
merging browser/themes/shared/tabs.inc.css
merging browser/themes/shared/urlbar-searchbar.inc.css
merging toolkit/content/aboutTelemetry.css
merging toolkit/content/aboutTelemetry.js
merging toolkit/content/aboutTelemetry.xhtml
merging browser/themes/shared/icons/arrow-dropdown-16.svg and toolkit/themes/shared/icons/arrow-dropdown-16.svg to toolkit/themes/shared/icons/arrow-dropdown-16.svg
merging toolkit/themes/shared/jar.inc.mn
warning: conflicts while merging browser/themes/shared/urlbar-searchbar.inc.css! (edit, then use 'hg resolve --mark')
warning: conflicts while merging toolkit/themes/shared/jar.inc.mn! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s be1e818d9af9 -d 1ba1737b4878: rebasing 414791:be1e818d9af9 "Bug 1385351 - Emphasize the ping selector button in about:telemetry r=gfritzsche"
merging browser/themes/shared/customizableui/customizeMode.inc.css
merging browser/themes/shared/jar.inc.mn
merging browser/themes/shared/tabs.inc.css
merging browser/themes/shared/urlbar-searchbar.inc.css
merging toolkit/content/aboutTelemetry.css
merging toolkit/content/aboutTelemetry.js
merging toolkit/content/aboutTelemetry.xhtml
merging browser/themes/shared/icons/arrow-dropdown-16.svg and toolkit/themes/shared/icons/arrow-dropdown-16.svg to toolkit/themes/shared/icons/arrow-dropdown-16.svg
merging toolkit/themes/shared/jar.inc.mn
warning: conflicts while merging browser/themes/shared/urlbar-searchbar.inc.css! (edit, then use 'hg resolve --mark')
warning: conflicts while merging toolkit/themes/shared/jar.inc.mn! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Keywords: checkin-needed
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bc3d6103ff79
Emphasize the ping selector button in about:telemetry r=gfritzsche
https://hg.mozilla.org/integration/autoland/rev/9430ca9565da
Remove unused old class r=gfritzsche
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/bc3d6103ff79
https://hg.mozilla.org/mozilla-central/rev/9430ca9565da
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: