1546451 - [RTL] The sliders in the Fonts panel are overlapping the value input + placement issues

Status: VERIFIED FIXED

(DevTools :: Inspector, defect, P1)

Description

[Itiel]

Attachment: Current state

See attached for how this currently looks vs. how it should look like.

Comment 1

[Itiel]

Attachment: Desired state

Comment 2

[Itiel]

Most of the changes in the desired state screenshot were made with changing e.g. margin-left to margin-inline-start etc.

Comment 3

[Razvan Caliman [:rcaliman]]

Thanks for filing! Indeed, this is an easy fix. A patch will follow shortly.

Comment 4

[Razvan Caliman [:rcaliman]]

Attachment: Bug 1546451 - Fix RTL spacing for inputs in Fonts panel. r=pbro

Minor fixes to the layout of inputs in right-to-left (RTL) language layout. To enable RTL, navigate to about:config and set intl.uidirection to 1.

Comment 5

[Itiel]

(In reply to Razvan Caliman [:rcaliman] from comment #4)

Created attachment 9060324 [details]
Bug 1546451 - Fix RTL spacing for inputs in Fonts panel. r=pbro

Minor fixes to the layout of inputs in right-to-left (RTL) language layout. To enable RTL, navigate to about:config and set intl.uidirection to 1.

With the changes made here applied, the result is much better but the placement of font-value-input and font-value-select are still reversed. Basically they should match LTR here (see Desired state screenshot).

Comment 6

[Razvan Caliman [:rcaliman]]

but the placement of font-value-input and font-value-select are still reversed

You're right. Sorry! I missed that in my first attempt. I will update the patch. Thanks for keeping a close eye ;)

Comment 7

[Razvan Caliman [:rcaliman]]

Patch updated: https://phabricator.services.mozilla.com/D28610
@Itiel, can you please have a look at the latest and see if I missed anything else?

Comment 8

[Itiel]

(In reply to Razvan Caliman [:rcaliman] from comment #7)

Patch updated: https://phabricator.services.mozilla.com/D28610
@Itiel, can you please have a look at the latest and see if I missed anything else?

Sorry for being nitpicky, but I'd add text-align: left to the .font-value-select selector.
Other than that- all's good, thanks!

You may want a re-review from pbro, after these changes (?).

Comment 9

[Itiel]

Hmm, also, the spacing slider is not the same length and thickness (about a pixel less) as the the other two.
See attached.

Comment 10

[Itiel]

Attachment: Screenshot

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/988b90e06a7a
Fix RTL spacing for inputs in Fonts panel. r=pbro

Comment 12

[Razvan Caliman [:rcaliman]]

I'd add text-align: left to the .font-value-select selector.

Fixed

[...] the spacing slider is not the same length

Fixed.

You may want a re-review from pbro, after these changes (?).

He granted me permission to carry over the R+ in a separate chat.

I made the changes and pushed to autoland. Should be in Nightly soon. Thanks for filing and testing!

Comment 13

[Itiel]

(In reply to Itiel from comment #9)

the spacing slider is not the same [...] thickness (about a pixel less) as the the other two.

I guess it's too late for that now, but this is still an issue.
De-selecting height: 3px from the .font-value-slider::-moz-range-track selector doesn't change anything in the spacing slider.

Also the spacer is now a bit longer than the other two, but this is present in LTR as well.

Should I file a new bug for these two issues and CC you?

Comment 14

[Razvan Caliman [:rcaliman]]

I guess it's too late for that now, but this is still an issue.
De-selecting height: 3px from the .font-value-slider::-moz-range-track selector doesn't change anything in the spacing slider.

I'm unable to reproduce that difference in the slider thickness, so this needs a deeper investigation, but not a cause for a P1 bug now.
Do you, by any chance, have a non-HiDPI monitor that may be causing the difference? I have only HiDPI monitors to test with.

Also the spacer is now a bit longer than the other two, but this is present in LTR as well.

That's a slight bug caused by the font-value-label dimensions used on the letter-spacing as a default keyword. On click on it, or when changing the letter-spacing, it gets replaced with an input + select dropdown which seem to align with the other two for line-height and font-size.

Should I file a new bug for these two issues and CC you?

Yes, please file another bug. No need to CC, it shows up on my radar if it's on the Inspector: Fonts component.
Thanks!

Comment 15

[Itiel]

(In reply to Razvan Caliman [:rcaliman] from comment #14)

I'm unable to reproduce that difference in the slider thickness, so this needs a deeper investigation, but not a cause for a P1 bug now.

You may need to look closely for the difference between the sliders thickness, it's hard to discern tbh.
Interestingly (or not), disabling padding-top: 2px from .font-value-label fixes it.

Do you, by any chance, have a non-HiDPI monitor that may be causing the difference? I have only HiDPI monitors to test with.

Umm, not sure? I have Dell P2417H, Windows DPI set to 150%.

Yes, please file another bug. No need to CC, it shows up on my radar if it's on the Inspector: Fonts component.

Will do in a few days.
Sorry for the delay.

Comment 16

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/988b90e06a7a

Comment 17

[Itiel]

Looking great now, thanks!

I've given up on the slider thickness issue, because it apparently is depending on the DevTools height (resizing the DevTools makes this issue visible).
Do you think there's a point in filing a bug about that?

Comment 18

[Razvan Caliman [:rcaliman]]

Do you think there's a point in filing a bug about that?

It's very minor and if it's not consistently reproducing, it will be difficult to isolate and fix. Not worth it, really.