1476580 - Italic toggle is looking very pixelated on a non-retina screen

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P3)

Description

[Patrick Brosset <:pbro>]

Attachment: image.png

- Make sure your screen is a non-retina screen. If you have a macbook with a retina display, change the resolution to "larger text" so everything is bigger
- Open any page in Firefox
- Open the Font editor

Notice that the italic toggle seems pixelated. The rounded corners aren't smooth like they should be.

Comment 1

[Patrick Brosset <:pbro>]

The code for this toggle is here: https://searchfox.org/mozilla-central/rev/c296d5b2391c8b37374b118180b64cca66c0aa16/devtools/client/themes/fonts.css#273-313

And the relevant part is the following one:

  background-image:
    /* thumb */
    radial-gradient(circle 0.35em, var(--thumb-color) 100%, transparent 0),
    /* left circle */
    radial-gradient(circle 0.5em, var(--bg) 100%, transparent 0),
    /* middle bar */
    linear-gradient(var(--bg) , var(--bg)),
    /* right circle */
    radial-gradient(circle 0.5em, var(--bg) 100%, transparent 0);

It seems that radial-gradients cause this. Using border-radius might lead to better results.
In fact I tried doing the following quick fix and it seemed to work pretty good:

1. Remove the left and right circles as well as the middle bar, leaving just the thumb:

  background-image:
    /* thumb */
    radial-gradient(circle 0.35em, var(--thumb-color) 100%, transparent 0);

2. Adding a background color:

  background-color: var(--bg)

3. Applying border-radius to the element:

  border-radius: 10px;

Now, 10px was just a quick test.

Also, the thumb itself is pixelated. And for this, I think the fix could be to add a ::before pseudo-element with a 50% border-radius, positioned within the input element.

Comment 2

[Patrick Brosset <:pbro>]

Razvan offered to help mentor this good-first-bug if anyone is interested in picking this up.
Before you do, please read through our contribution guidelines here: http://docs.firefox-dev.tools/
Then look at my previous comment, which should give a few pointers for how to solve the issue.

Note that you will need a non high-dpi screen to test and fix this!

Feel free to ask any questions here or on Slack if you need help (info about getting in touch here https://firefox-dev.tools/).

Comment 3

[makmm // v0idifier]

I'm completely new to this but I want to try to fix this. Can I take it?

Comment 4

[Patrick Brosset <:pbro>]

Hi and welcome! Thanks for your interest. I'll assign the bug to you now.

Comment 5

[makmm // v0idifier]

Thanks!
I think I almost got it: https://codepen.io/anon/pen/JBbyqW
The only thing left is the animation; but the problem is that `float` can't be animated.
It works with `margin-left: 1.15em` but I don't know if that is considered dirty or not. Thoughts?

Comment 6

[Patrick Brosset <:pbro>]

Awesome! I'm going to transfer the needinfo to Razvan who is the mentor on this bug. He will be a much better person than me to discuss about the implementation details and help you on the transition.
Thanks for the work so far.

Comment 7

[Martin Balfanz [:mbalfanz]]

Looks great!

My only feedback so far is that the wrong cursor appears (should be a pointer), but I'm not sure if that will be fixed when the control is brought back into devtools context.

Comment 8

[Florens Verschelde :fvsch]

Hi. I took the liberty to suggest a few changes:
https://codepen.io/fvsch/pen/YjpdWG

- base size at 16x32 pixels, as in the current implementation
- optional big size at 24x48 pixels, as in makmm's implementation but using `24px` explicitely; the `font-size:140%` meant a height of 22.4px, which is not ideal to achieve a pixel-perfect look on low-dpi displays.
- animating the `transform` property instead of `margin-left` (best practice for smooth animation is to animate transform and opacity only, though for a small element like this it probably doesn't make much of a difference).
- adjusted the em sizes (circle size, padding) so that they always match a round pixel value.

At `font-size: 16px` the dimensions are:
- full height = 16px
- full width = 32px
- circle size = 12px
- padding = 2px

At `font-size: 24px`, all dimensions are multiplied by 1.5 which gives us integer sizes too.

What's missing currently is focus styles for keyboard navigation, but that's a more general issue for all the controls in this panel.

Comment 9

[Razvan Caliman [:rcaliman]]

This looks good. Thank you both for your contributions!

Since I don't have a non-high-DPI monitor to test on, I will trust that you both checked to ensure this indeed solves the pixelation.

The `font-size:140%` didn't have any purpose other than to scale the decoration in relation to its parent's size. It was a hack at best. Feel free to discard if it's not needed.
 
All of Florens' additions are very welcome, particularly using CSS Transforms and animating X translation of the pseudo-element.

> What's missing currently is focus styles for keyboard navigation, but that's a more general issue for all the controls in this panel.

Indeed, there are no defined focus states for any of the input fields or sliders in the font editor. Adding Victoria for input on that, but that will probably be logged and fixed in a separate bug.

The next step is to integrate this into the stylesheet found in the source code at: `devtools/client/themes/fonts.css`.

The directions on how to get the code and setup the environment are described at: https://docs.firefox-dev.tools/getting-started/
It's a bit of a process, but I can help along the way if you get stuck.

Comment 10

[Florens Verschelde :fvsch]

I updated my version to use a percentage size that gives us 16px precisely (to avoid blur on low-res screens).
And I think the disc should be 10px and not 12px, so I updated that too.

For hover and focus styles, maybe the style used for the "Enable Tracking Protection" toggle in the main Firefox menu (in Nightly) should be used. It adds a white outline (inset shadow I reckon), and for the "active" state it also changes the blue background to a lighter shade. But yes it's probably better to handle in a separate bug.

@makmm, sorry for highjacking your bug. Feel free to do things differently from my forked pen.

Comment 11

[makmm // v0idifier]

The "Enable Tracking Protection" toggle for me looks wrong (maybe open another issue?) https://imgur.com/a/PyhhSSx.

Apart from that, I have changed my pen (https://codepen.io/anon/pen/JBbyqW) to use `transform` instead of `margin` and `font-size: 140%;` to `font-size: 24px;` and it seems to work perfectly. I already integrated it to the source code and it seems to work perfectly fine. The only thing would be the focus states which I don't really know how to do.

BTW, I don't know how to use mercurial (I know git) so even though I have the changes I can't really push them.

Comment 12

[Florens Verschelde :fvsch]

(Indeed this screenshot looks wrong.)

I think the target height is 16px and not 24px. This would make this toggle consistent with the Tracking Protection one (when not buggy) and other elements in the Fonts tab.

Comment 13

[makmm // v0idifier]

That makes sense, you can just change the font-size and it will look alright.

Comment 14

[Razvan Caliman [:rcaliman]]

@Florens

> I updated my version to use a percentage size that gives us 16px precisely (to avoid blur on low-res screens).
> And I think the disc should be 10px and not 12px, so I updated that too.

Be sure to test your code in-context in the Fonts panel to ensure it's not distorted by any ancestor's re-definition of font-size, and that it works well in proportion to the other "thumbs" used in the Font editor UI.

> It adds a white outline (inset shadow I reckon), and for the "active" state it also changes the blue background to a lighter shade. But yes it's probably better to handle in a separate bug.

Good suggestions. You may either follow the focused state of the existing "Enable Tracking Protection" toggle for consistency, or skip it for now. I don't believe that focus state is visible enough to be fit for purpose. I'd welcome a more obvious alternative that still fits well in the browser theme.

@makmm
> The "Enable Tracking Protection" toggle for me looks wrong

It seems ok on the latest Nightly build. It seems you're using a different theme than the default one. Could it be the cause of the issue?


> BTW, I don't know how to use mercurial (I know git) so even though I have the changes I can't really push them.

Some of us at Mozilla also use Git. There is a process for setting up a Git workflow with git-cinnabar [1]. You could set it up manually or you may try out the automated version to set it up with git-cinnabarify [2]. Be warned that the script may not work flawless depending on your system config and you may have to dig into issues and debug. It's not actively maintained.

For this particular patch, you may side-step that and just generate a patch in the right format with Git:

```
git show --binary --find-renames --format="# HG changeset patch%n# User %an <%ae>%n%B" -U8 > ~/Desktop/Bug1476580.patch
```

The last part of the script is where the patch file will be written. You'll have to attach that file to this bug and select the review flag (r?) with my username on it (rcaliman). Take care that your git commit follows the expected format described in the "Commit messages" [3] section of the docs.

This is a workaround. Going through the effort to setup a Mercurial or Git workflow will pay off in the longer term. It will reduce manual work in creating and updating patches, and it will streamline your usage of other helpful Mozilla tools for testing (Treeherder / try) and review (Mozreview).

[1] https://github.com/glandium/git-cinnabar
[2] https://github.com/sole/cinnabarify
[3] https://docs.firefox-dev.tools/contributing/making-prs.html

Comment 15

[Victoria Wang [:victoria]]

> What's missing currently is focus styles for keyboard navigation, but that's a more general issue for all the controls in this panel.
Yes, a focus indicator for sliders would be great, especially because the keyboard controls already work so well :) - we can track that in this visual polish bug https://bugzilla.mozilla.org/show_bug.cgi?id=1474731

Comment 16

[makmm // v0idifier]

Okay. So should I just finish this issue without focus indicator and actually do that work on #1474731 or do it in here?

Thanks for helping me :)

Comment 17

[Razvan Caliman [:rcaliman]]

> Okay. So should I just finish this issue without focus indicator

Yes, you may submit a patch to this bug without the focus indicator. We'll handle that in another bug.

Comment 18

[Razvan Caliman [:rcaliman]]

@makmm, do you need any help with submitting your patch for this?

Comment 19

[makmm // v0idifier]

@rcaliman, I would be needing some help with the git/hg problem. Where should I ask for help? IRC, Slack..?

Comment 20

[Razvan Caliman [:rcaliman]]

Here or on Slack is good. What problems do you have with git/hg? 
Does the suggested workflow mentioned above in comment 14 to generate a patch not work work for you?

Comment 21

[Razvan Caliman [:rcaliman]]

@makmm

The workflow for submitting code for review is changing from mozreview to Phabricator. For this small patch, I still think your easiest option is to generate a small patch file and upload it as an attachment to this bug.

However, if you want to setup to the review system, disregard instructions about mozreview. It will be discontinued as of August 20, 2018 (that's barely 2 weeks away).

Phabricator is the new system and it's easier to work with. Here are the instructions for setup:
https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html#quick-start

If you use git (via git-cinnabar) instead of hg (Mercurial), be sure to make the appropriate change in the install of `arc` mentioned here: https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html#using-git-cinnabar

Let me know if you need any help.

Comment 22

[makmm // v0idifier]

@rcaliman I probably won't be able to do it today (I'm so sorrryy) but can I tag you on the Slack for some help? what are the differences between mozreview and Phabricator? What would be the definition of it? Thanks.

Comment 23

[Razvan Caliman [:rcaliman]]

Phabricator and mozreview are code review interfaces akin to the one on GitHub's to manage pull-requests. Mozreview is an older system used at Mozilla which is being deprecated. Phabricator is the new one. 

Setting up Phabricator[1] makes it easier for contributors and reviewers to comment on code changes and see the patch through to landing in Nightly (which may take a few iterations depending on comments). There are also other checks, like linting, which Phabricator automates.

Feel free to ping me on devtools-html.slack.com Slack either on #inspector or directly (@rcaliman).

[1] https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html#quick-start

Comment 24

[makmm // v0idifier]

Attachment: Bug 1476580 - rewrite italic font toggle using ::before. r=rcaliman

Comment 25

[Razvan Caliman [:rcaliman]]

Comment on attachment 8998830 [details]
Bug 1476580 - rewrite italic font toggle using ::before. r=rcaliman

Razvan Caliman [:rcaliman] has approved the revision.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cf5af792699e
rewrite italic font toggle using ::before. r=rcaliman

Comment 27

[Bogdan Tara[:bogdan_tara | bogdant]]

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