Closed Bug 1513922 Opened 10 months ago Closed 9 months ago

Use -moz-appearance: textfield for the Weight number input

Categories

(DevTools :: Inspector: Fonts, enhancement, P3)

enhancement

Tracking

(firefox67 fixed)

RESOLVED FIXED
Firefox 67
Tracking Status
firefox67 --- fixed

People

(Reporter: fvsch, Assigned: garvitdelhi, Mentored)

References

Details

(Keywords: good-first-bug, Whiteboard: [qa-67b-p2])

Attachments

(4 files)

Attached image linux-screenshot.png
The Weight <input type="number"> looks rather ugly on Linux (see screenshot).

We should perhaps use `-moz-appearance: textfield` for this field, as we already do for Size and Line Height, to be consistent and more visually pleasing.

If it looks and works great on Windows and macOS, we can consider setting that for Linux only, but I would rather use the same style for all platforms.
I'm in favor of hiding the increment/decrement arrows for those number inputs. The functionality is preserved when using up/down keys on the keyboard, similar to the inputs in the Rule view.

The number input is an ugly duckling on all platforms. A bit less on macOS by virtue of the Fonts panel being developed on that platform, but still not *really* integrated into the design. Some hacks were needed for the steppers to look better in the dark theme. They never vertically aligned properly either.

Those stepper buttons look bad on Windows and are downright inaccessible in High Contrast mode due to another Firefox bug. 
See: https://bugzilla.mozilla.org/show_bug.cgi?id=1496436

With Victoria and Martin's permissions, I'd be happy to see Florens' suggestion applied to all platforms.
Flags: needinfo?(victoria)
Flags: needinfo?(mbalfanz)
Agreed.  The windows version doesn't look as bad, but is still unusable.  Changing -moz-appearance works for me.
Flags: needinfo?(mbalfanz)
Duplicate of this bug: 1514476
Priority: -- → P3
Ah, that's too bad - I'm not sure many people are used to the increment/decrement keyboard behavior. From the Windows screenshots (non-high-contrast mode) I felt it's not so ugly that it distracts from the whole panel (like this linux example :)). 

I can see how this change would simplify things though, so I think it's fine to move forward on this.
Flags: needinfo?(victoria)
Attached image arrows-windows.png
For reference I added a Windows screenshot.
Thank you both for confirmations!

This is an easy fix. I'll mark this as a good first bug for a newcomer to try and fix and set myself as a mentor.
Learn here how to get started with contributing to Firefox DevTools: 
https://docs.firefox-dev.tools/getting-started/
and
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch

The fix for this issue would have to be made in devtools/client/themes/fonts.css 
As Florens suggested, it's something along the lines of:
```
.font-value-input[type=number] {
  -moz-appearance: textfield;
}
```
This would target all number inputs in the Font Editor.

Also, please take care to remove the hacks made for positioning the number spin-box and adapting it for dark theme:
https://searchfox.org/mozilla-central/source/devtools/client/themes/fonts.css#268-276
Mentor: rcaliman
Keywords: good-first-bug
Re: Windows screenshot, interestingly it looked less bad in the MDN screenshot which I assumed was Windows: https://mdn.mozillademos.org/files/16162/font-weight_cropped.png but 

I can see how here in Martin's screenshot, it does look basically unusable due to being so tiny/short. 

Sounds great to make this a good first bug!

Hi Razvan, I am looking for a first bug to start with, would it be possible for me to contribute to this? Thanks!

Flags: needinfo?(rcaliman)

@Garvit, usual etiquette on Bugzilla is that you first request to be assigned to a bug (or assign yourself if you can), then you work on it. This signals to others that may be interested in working on the same bug, as with Sakshaat above.

But since this among your first contributions and you already did the work to setup the dev environment (which is by no means trivial) and to fix the bug, I'll accept your patch on this bug. Thank you for your work on it!

@Sakshaat, I'm sorry for the situation on this bug. But if you're still interested in contributing to DevTools (we're very thankful for that), here's a list of other bugs marked as good-first-bug: https://mzl.la/2CNH1lB

Perhaps you can find another one there that interests you.

Even if bugs are assigned to someone, if you see inactivity for a long time since the last comment, you can ask to get the bug reassigned to you.

Flags: needinfo?(rcaliman)
Assignee: nobody → garvitdelhi
Status: NEW → ASSIGNED

@Razvan, I do realise it was indeed a mistake. I would be more aware in future before starting to work upon a bug.

@Sakshaat, I would like to apologize I didn't notice your comment and decided to work upon it after seeing it unassigned. I hope you will find a new bug to work upon soon.

Keywords: checkin-needed

Unable to land this bug, the revision is blocked because it doesn't have the proper author information in Phabricator

Flags: needinfo?(garvitdelhi)

@Noemi what all information is needed/missing from Phabricator?

Flags: needinfo?(garvitdelhi)

I am only prompted with that message while trying to land from Phabricator, and there is a FAQ link about why this could be, maybe this can help you: https://wiki.mozilla.org/Phabricator/FAQ#Lando

Flags: needinfo?(garvitdelhi)

@Garvit, perhaps it is this issue from the FAQ linked above?

The revision was created via arcanist or moz-phab, but the error still appears.
This can happen if you have not set an author email in your .hgrc file (or git config).
Set your author email in your .hgrc to your username, Firstname Lastname <yourldapemail@mozilla.com>.
Update your commit so it contains the new author data. Re-run arc diff so that the new commit+data is uploaded to Phabricator.

@Garvit @Razvan no problem, I'll find another bug. Thanks.

@Razvan Thanks I hope this would solve the problem. @Noemi I have updated and marking it again as checkin-needed. I hope it would work this time.

Flags: needinfo?(garvitdelhi)
Keywords: checkin-needed

@Garvit Khatri tried to land the patch via Phabricator but it just hangs on the Preview Landing window and does not finalize the landing.

Flags: needinfo?(garvitdelhi)

Pushed by rcaliman@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f42ed4eb7274
Use -moz-appearance: textfield for the Weight number input r=rcaliman

Keywords: checkin-needed

I tried the Phabricator route and it seems to have worked.
This should land, unless there's a backout, which is not expected.

Flags: needinfo?(garvitdelhi)

@Razvan Thanks!

Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67
Whiteboard: [qa-67b-p2]
You need to log in before you can comment on or make changes to this bug.