1410498 - Visual appearance of autoscroll indicator has regressed, doesn't match photon specs

Status: VERIFIED FIXED

(Toolkit :: Themes, defect, P2)

Description

[Botond Ballo [:botond]]

In recent nightlies, the visual appearance of the autoscroll indicator has changed on Linux, and it looks pretty bad.

Comment 1

[Botond Ballo [:botond]]

Attachment: Autoscroll indicator before

Comment 2

[Botond Ballo [:botond]]

Attachment: Autoscroll indicator after

Comment 3

[Botond Ballo [:botond]]

Regression range points to bug 1389462.

Comment 4

[Botond Ballo [:botond]]

Based on bug 1389462, it looks like it was an intentional change. I think that's unfortunate - to me, the previous one looks polished, while the new one looks cartoonish - but ok.

Comment 5

[Zibi Braniecki [:zbraniecki][:gandalf]]

I don't think you have to close it as invalid. You can ask UX team for review of the result of that work if you think the visual style of the new one is an actual regression.

Comment 6

[Botond Ballo [:botond]]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #5)
> You can ask UX team for
> review of the result of that work if you think the visual style of the new
> one is an actual regression.

It looks to me like the new design got UX approval in bug 1389462 comment 3.

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

Correct, and it looks really good on the SVG file, but maybe not as great in the miniature like you presented on the screenshot.

The fact that our UX team accepted a proposal doesn't mean it cannot be revised based on the result :)

Comment 8

[Botond Ballo [:botond]]

Ok, fair enough. Stephen, would it be possible for UX to review the result of the changes made in bug 1389462, as suggested in comment 7?

Comment 9

[gwarser]

I think it looks like it should for iOS:

>On iOS, use 3px strokes for the foundation and 2px strokes for the details

And should look like this :

>When lines principally shape your icon, use a 2px stroke for the foundation and a 1px line for extra details.


http://design.firefox.com/photon/visuals/iconography.html#shape


Compared to, for example "Reload" button, this looks bad.

Comment 11

[Stephen Horlander [:shorlander]]

The change with regards to the styling is the direction we want to go. Something seems off with the scaling of the SVG thought.

Comment 12

[Dão Gottwald [:dao]]

(In reply to Stephen Horlander [:shorlander] from comment #11)
> The change with regards to the styling is the direction we want to go.
> Something seems off with the scaling of the SVG thought.

Could you please elaborate how the scaling is off / what we should fix here?

Comment 13

[Stephen Horlander [:shorlander]]

Attachment: Line Width Comparison

(In reply to Dão Gottwald [::dao] from comment #12)
> (In reply to Stephen Horlander [:shorlander] from comment #11)
> > The change with regards to the styling is the direction we want to go.
> > Something seems off with the scaling of the SVG thought.
> 
> Could you please elaborate how the scaling is off / what we should fix here?

I still need to dig into exactly what's going on, but the autoscroll image should be following the same 2px and 1px line rule as the rest of the icons (http://design.firefox.com/photon/visuals/iconography.html#shape). It looks like it's scaled too large though.

Comment 14

[Mike Taylor [:miketaylr]]

(In reply to Stephen Horlander [:shorlander] from comment #13)
> I still need to dig into exactly what's going on, but the autoscroll image
> should be following the same 2px and 1px line rule as the rest of the icons
> (http://design.firefox.com/photon/visuals/iconography.html#shape). It looks
> like it's scaled too large though.

Any updates here?

Comment 15

[Ian Moody [:Kwan] (UTC+1)]

As far as I can tell this is just because the icon is drawn on a 16x16 canvas like the toolbar icons, but displayed at 28x28.  If the SVG is modified to use a 28x28 canvas it looks a lot better.

Comment 16

[Ian Moody [:Kwan] (UTC+1)]

Attachment: Bug 1410498 - Draw autoscroll icon on a 28x28 canvas.

The icon is currently drawn with lines too thick because it's drawn on a 16x16
canvas like toolbar icons, but then blown up to 28x28.
Also simplify by removing some redundant attributes, and using just stroke for
the border rather than a separate circle.

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

Comment 17

[Ian Moody [:Kwan] (UTC+1)]

Oops, forgot to comment.  So the above patch is just converting the icon to being drawn in a 28x28 canvas as I mentioned in comment #15.  Discussed with shorlander on IRC and the arrows might need adjusting slightly since the diagonal angle they are drawn at means that perceptually they are slightly thinner than their specified 1px.

Comment 18

[Ian Moody [:Kwan] (UTC+1)]

Update description & platforms since this isn't exclusive to Linux (tested on Windows).

Comment 19

[Stephen Horlander [:shorlander]]

Attachment: fix-autoScroll-image.patch

I looked into this a bit. I decided to flip the visual emphasis from the circle to the arrows and made it 32 x 32 to conform to our icon guidelines.

Comment 20

[Stephen Horlander [:shorlander]]

Attachment: Screenshot

Comment 21

[Tim Nguyen :ntim]

Would be nice to uplift this to beta.

Comment 22

[Tim Nguyen :ntim]

Dão, does the .autoscroll CSS in the global.css files need to be updated as well to reflect the new dimensions ?

Comment 23

[Tim Nguyen :ntim]

Nevermind, I was looking at an old version of the CSS.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9d9579b0629
Fix autoscroll image. r=dao

Comment 25

[Tim Nguyen :ntim]

Comment on attachment 8954494 [details] [diff] [review]
fix-autoScroll-image.patch

Approval Request Comment
[Feature/Bug causing the regression]: bug 1389462
[User impact if declined]: autoscroll image looks bad
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: simple image swap
[String changes made/needed]: none

Comment 26

[Cristina Coroiu [:ccoroiu]]

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

Comment 27

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8954494 [details] [diff] [review]
fix-autoScroll-image.patch

Since the beta to release merge was today, just moving the flag to m-r.

Comment 28

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8954494 [details] [diff] [review]
fix-autoScroll-image.patch

Though this seems like a small change, I'd rather let it ride the trains with 60, unless you have strong objections. We are about to build the release candidate on Monday so this is pretty last minute.

Comment 29

[Oana Botisan, Desktop QA]

I managed to reproduce the bug using an older version of Nightly (2017-10-20) on Windows 10 x64. 
I retested everything on latest Nightly 61.0a1 and beta 60.0b5 on Windows 10 x 64, macOS 10.13 and Ubuntu 16.04 x64 and the bug is not reproducing anymore.