1385351 - Emphasize the ping selector button in about:telemetry

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P1)

Description

[flyingrub]

It is actually hard to understand that the ping selector button are clickable.

We could use a triangle to indicate drop-down?

Comment 1

[flyingrub]

Attachment: Bug 1385351 - Emphasize the ping selector button in about:telemetry

Add a triangle next to the button.

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

Comment 2

[flyingrub]

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

Comment 3

[flyingrub]

Comment on attachment 8895126 [details]
Bug 1385351 - Emphasize the ping selector button in about:telemetry

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

Comment 4

[flyingrub]

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 5

[Georg Fritzsche [:gfritzsche]]

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 `▼`?

Comment 6

[flyingrub]

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 7

[flyingrub]

Comment on attachment 8895126 [details]
Bug 1385351 - Emphasize the ping selector button in about:telemetry

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

Comment 8

[Georg Fritzsche [:gfritzsche]]

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#

Comment 9

[flyingrub]

Comment on attachment 8895126 [details]
Bug 1385351 - Emphasize the ping selector button in about:telemetry

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

Comment 10

[flyingrub]

Because the unicode character can differ according to the current font, we switched back to the css styling.

Comment 11

[flyingrub]

Comment on attachment 8895126 [details]
Bug 1385351 - Emphasize the ping selector button in about:telemetry

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

Comment 12

[flyingrub]

Comment on attachment 8895126 [details]
Bug 1385351 - Emphasize the ping selector button in about:telemetry

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

Comment 13

[Georg Fritzsche [:gfritzsche]]

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.

Comment 14

[flyingrub]

Attachment: Bug 1385351 - Remove unused old class

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

Comment 15

[flyingrub]

Comment on attachment 8895126 [details]
Bug 1385351 - Emphasize the ping selector button in about:telemetry

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

Comment 16

[Georg Fritzsche [: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?

Comment 17

[Dão Gottwald [:dao]]

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/[e-mail]                 (../../shared/icons/[e-mail])
>    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?

Comment 18

[flyingrub]

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 :)

Comment 19

[flyingrub]

::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 ?

Comment 20

[flyingrub]

Comment on attachment 8895126 [details]
Bug 1385351 - Emphasize the ping selector button in about:telemetry

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

Comment 21

[flyingrub]

Attachment: Bug 1385351 - Remove unused old class

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

Comment 22

[flyingrub]

Comment on attachment 8895126 [details]
Bug 1385351 - Emphasize the ping selector button in about:telemetry

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166234/diff/8-9/

Comment 23

[Dão Gottwald [:dao]]

(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

Comment 24

[flyingrub]

Comment on attachment 8895126 [details]
Bug 1385351 - Emphasize the ping selector button in about:telemetry

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166234/diff/9-10/

Comment 25

[flyingrub]

Comment on attachment 8896495 [details]
Bug 1385351 - Remove unused old class

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

Comment 26

[Dão Gottwald [:dao]]

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;

Comment 27

[flyingrub]

Attachment: sqd

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

Comment 28

[flyingrub]

Comment on attachment 8896495 [details]
Bug 1385351 - Remove unused old class

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

Comment 29

[flyingrub]

Comment on attachment 8895126 [details]
Bug 1385351 - Emphasize the ping selector button in about:telemetry

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166234/diff/10-11/

Comment 30

[flyingrub]

Comment on attachment 8896495 [details]
Bug 1385351 - Remove unused old class

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

Comment 31

[Georg Fritzsche [:gfritzsche]]

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.

Comment 32

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8896495 [details]
Bug 1385351 - Remove unused old class

https://reviewboard.mozilla.org/r/167746/#review173556

Comment 33

[Pulsebot]

Pushed by [e-mail]:
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

Comment 34

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back these out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=123080564&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/1abaf670ca7a882d4f1682d2817241ed4898441e

Comment 35

[flyingrub]

Comment on attachment 8895126 [details]
Bug 1385351 - Emphasize the ping selector button in about:telemetry

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166234/diff/11-12/

Comment 36

[flyingrub]

Comment on attachment 8896495 [details]
Bug 1385351 - Remove unused old class

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

Comment 37

[flyingrub]

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 ?

Comment 38

[Mozilla Autoland]

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 39

[Dão Gottwald [:dao]]

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

Comment 40

[flyingrub]

Comment on attachment 8895126 [details]
Bug 1385351 - Emphasize the ping selector button in about:telemetry

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166234/diff/12-13/

Comment 41

[flyingrub]

Comment on attachment 8896495 [details]
Bug 1385351 - Remove unused old class

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

Comment 42

[Dão Gottwald [:dao]]

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.

Comment 43

[flyingrub]

Comment on attachment 8895126 [details]
Bug 1385351 - Emphasize the ping selector button in about:telemetry

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166234/diff/13-14/

Comment 44

[flyingrub]

Comment on attachment 8896495 [details]
Bug 1385351 - Remove unused old class

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

Comment 45

[flyingrub]

It should be ok now ?

Comment 46

[Dão Gottwald [:dao]]

It looks like urlbar-searchbar.inc.css still needs to be updated.

Comment 47

[flyingrub]

Comment on attachment 8895126 [details]
Bug 1385351 - Emphasize the ping selector button in about:telemetry

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166234/diff/14-15/

Comment 48

[flyingrub]

Comment on attachment 8896495 [details]
Bug 1385351 - Remove unused old class

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

Comment 49

[flyingrub]

I updated it in the second commit... It's fixed.

Comment 50

[Ryan VanderMeulen [:RyanVM]]

This has pending issues in MozReview.

Comment 51

[flyingrub]

:S

Comment 52

[Mozilla Autoland]

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)

Comment 53

[Mozilla Autoland]

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)

Comment 54

[flyingrub]

Comment on attachment 8895126 [details]
Bug 1385351 - Emphasize the ping selector button in about:telemetry

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166234/diff/15-16/

Comment 55

[flyingrub]

Comment on attachment 8896495 [details]
Bug 1385351 - Remove unused old class

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/167746/diff/8-9/

Comment 56

[Pulsebot]

Pushed by [e-mail]:
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

Comment 57

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bc3d6103ff79
https://hg.mozilla.org/mozilla-central/rev/9430ca9565da