Closed Bug 1689098 Opened 4 years ago Closed 4 years ago

With widget.disable-native-theme-for-content=true, buttons could have some more padding

Categories

(Core :: Widget, enhancement)

enhancement

Tracking

()

RESOLVED FIXED
87 Branch
Tracking Status
firefox87 --- fixed

People

(Reporter: julienw, Assigned: emilio)

References

Details

Attachments

(4 files)

I enabled widget.disable-native-theme-for-content=true today and when testing on the Profiler UI I noticed that the default style for buttons is very different to what I used to see (on Linux at least).

See attachments for the difference for data:text/html,<button>hello world</button> (the same happens for all buttons).

Maybe we could add some padding to the default style for buttons.

Attached image Before (Linux GTK)
Attached image after

I agree, they should at least be as tall as <input>: data:text/html,<input><button>Foo</button> should have the same size.

On the inline direction, not sure, Stephen do you know why you chose 4px padding on each side? Historically we've used 8px, chrome seems to use 6px by default.

Flags: needinfo?(spohl.mozilla.bugs)
Attached file Test-case.

Combobox select has the block-axis padding in the comboboxcontrol frame.

Moving it fixes bug 1560824 and should be better, so will do that there.

Assignee: nobody → emilio
Status: NEW → ASSIGNED

(In reply to Emilio Cobos Álvarez (:emilio) from comment #3)

I agree, they should at least be as tall as <input>: data:text/html,<input><button>Foo</button> should have the same size.

On the inline direction, not sure, Stephen do you know why you chose 4px padding on each side? Historically we've used 8px, chrome seems to use 6px by default.

This is all based on the design doc that we got from :shorlander a while back: https://mozilla.invisionapp.com/share/5CX2MH6EHFB#/screens/415584863_Form_Widgets

See the 5px, minus 1px for border width for buttons.

Flags: needinfo?(spohl.mozilla.bugs)

(In reply to Stephen A Pohl [:spohl] from comment #6)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #3)

I agree, they should at least be as tall as <input>: data:text/html,<input><button>Foo</button> should have the same size.

On the inline direction, not sure, Stephen do you know why you chose 4px padding on each side? Historically we've used 8px, chrome seems to use 6px by default.

This is all based on the design doc that we got from :shorlander a while back: https://mozilla.invisionapp.com/share/5CX2MH6EHFB#/screens/415584863_Form_Widgets

See the 5px, minus 1px for border width for buttons.

Looking at this again, an argument could be made that we were not supposed to deduct the 1px border from the padding and that the dotted lines were just drawn incorrectly in the design doc to include the border width. However, that would still leave us with padding of 5px instead of the 6px in Chrome or the traditional 8px that were mentioned earlier.

Blocks: 1560824

(In reply to Stephen A Pohl [:spohl] from comment #7)

Looking at this again, an argument could be made that we were not supposed to deduct the 1px border from the padding and that the dotted lines were just drawn incorrectly in the design doc to include the border width. However, that would still leave us with padding of 5px instead of the 6px in Chrome or the traditional 8px that were mentioned earlier.

Yeah, I was going to mention that... Would you be fine to change it to 5px? I think that's somewhat more sensible, and probably fine compat-wise.

Though I guess 4px matches our <select> / <option> padding, so I'm on the fence on that one.

Ok, let's fix the block-axis padding for now, since I think that's an obvious bugfix given the comments in our forms.css stylesheet. We might want to change the inline padding afterwards if we hit compat issues or more complaints about this :)

Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/08713a20dea5 Make inputs, selects, and button actually have the same block-axis padding as claimed by our forms.css comments. r=dholbert
Pushed by emilio@crisal.io: https://hg.mozilla.org/integration/autoland/rev/debae927f572 follow-up: Tweak some fuzzy thresholds.
Pushed by emilio@crisal.io: https://hg.mozilla.org/integration/autoland/rev/dffd6eca5ac0 follow-up: Tweak one more fuzzy threshold.
No longer regressions: 1690507
See Also: → 1690507
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: