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

VERIFIED FIXED in Firefox 68

Status

defect
P1
normal
VERIFIED FIXED
2 months ago
2 months ago

People

(Reporter: itiel_yn8, Assigned: rcaliman)

Tracking

({rtl})

unspecified
Firefox 68

Firefox Tracking Flags

(firefox68 verified)

Details

Attachments

(4 attachments)

Reporter

Description

2 months ago
Posted image Current state

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

Reporter

Comment 1

2 months ago
Posted image Desired state
Reporter

Comment 2

2 months ago

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

Assignee

Comment 3

2 months ago

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

Assignee: nobody → rcaliman
Status: NEW → ASSIGNED
Priority: -- → P1
Assignee

Comment 4

2 months ago

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.

Reporter

Comment 5

2 months ago

(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).

Flags: needinfo?(rcaliman)
Assignee

Comment 6

2 months ago

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 ;)

Flags: needinfo?(rcaliman)
Assignee

Comment 7

2 months ago

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?

Flags: needinfo?(itiel_yn8)
Reporter

Comment 8

2 months ago

(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 (?).

Flags: needinfo?(itiel_yn8)
Reporter

Comment 9

2 months ago

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

Reporter

Comment 10

2 months ago
Posted image Screenshot

Comment 11

2 months ago
Pushed by rcaliman@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/988b90e06a7a
Fix RTL spacing for inputs in Fonts panel. r=pbro
Assignee

Comment 12

2 months ago

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!

Reporter

Comment 13

2 months ago

(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?

Flags: needinfo?(rcaliman)
Assignee

Comment 14

2 months ago

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!

Flags: needinfo?(rcaliman)
Reporter

Comment 15

2 months ago

(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

2 months ago
bugherder
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68
Reporter

Comment 17

2 months ago

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?

Status: RESOLVED → VERIFIED
Flags: needinfo?(rcaliman)
Assignee

Comment 18

2 months ago

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.

Flags: needinfo?(rcaliman)
You need to log in before you can comment on or make changes to this bug.