1334519 - compact-theme forward button has no border on RTL builds

Status: RESOLVED WORKSFORME

(Firefox :: Theme, defect, P3)

Description

[Tomer Cohen :tomer]

Attachment: Screenshot

The RTL buttons of the compact theme seems to miss a left border on RTL builds of Firefox. I am attaching a screenshot that demonstrate the current state and the purposed fix. 

I was able to produce the expected result by commenting out the #urlbar { border-inline-start: none !important; } from compacttheme.css, but I am unsure if this won't break something else in the process.

Comment 1

[Tomer Cohen :tomer]

Attachment: Screenshot

An addition:

removing the #back-button > .toolbarbutton-icon, #forward-button > .toolbarbutton-icon {
border: 1px solid var(--chrome-nav-bar-controls-border-color); } from compacttheme.css seems to resolve the 2px mid border between the buttons.

Comment 2

[Tomer Cohen :tomer]

Attachment: INCOMPLETE, LINUX ONLY AT THE MOMENT Bug 1334519 - compact-theme forward button has no border on RTL builds

Review commit: https://reviewboard.mozilla.org/r/107800/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/107800/

Comment 3

[:Gijs (he/him)]

This turns out to be really annoying to test, because using force rtl with the English build doesn't produce the same result as in either of your screenshots.

In any case, I don't think the fix you're looking at is right. Something like this seems more likely to be correct:

https://pastebin.mozilla.org/8970169

Note that I'm basically replacing all the padding/margin/border-(left|right) with -inline-(start|end) instead, except for the transition which doesn't seem to work (so I'm specialcasing it based on the direction.

Can you check if this patch works for you? You should be able to download it and then use 'hg import' (though you might need to fix the newlines with dos2unix before it'll work).

Comment 4

[Tomer Cohen :tomer]

I am facing few difficulties, as I am able to fix it on DevTool, but then when I'm build a try build I see no difference.

Comment 5

[Tomer Cohen :tomer]

Comment on attachment 8831232 [details]
INCOMPLETE, LINUX ONLY AT THE MOMENT Bug 1334519 - compact-theme forward button has no border on RTL builds

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107800/diff/1-2/

Comment 6

[Tomer Cohen :tomer]

(In reply to :Gijs from comment #3)
> This turns out to be really annoying to test, because using force rtl with
> the English build doesn't produce the same result as in either of your
> screenshots.

I'm getting the same [bad] results with your patch applied as well.
https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-78d852af09b0852d65d95d7599af586367b4a15c/

Comment 7

[:Gijs (he/him)]

So, I'm confused. Do you see this problem on a clean profile? Does it work correctly on the default theme on a clean profile?

The real bug here is that the forward button in your screenshot is getting a border on the right-hand side instead of the left-hand side. There's already a (lighter, because the back button is disabled) border on the right-hand side of the forward button that is generated by the back button. We shouldn't need to mess with anything except making sure that the current right-hand-side border on the forward button appears on the left-hand-side instead.

Comment 8

[:Gijs (he/him)]

Maybe try changing this:

http://searchfox.org/mozilla-central/source/browser/themes/linux/compacttheme.css#57

which has: border-inline-start: none !important;

to border-left: none !important;

and see if that helps?

Comment 9

[:Gijs (he/him)]

Tomer is still active on bugzilla, refreshing the needinfo.

Comment 10

[:Gijs (he/him)]

I kind of expect this to have gone away in Photon. Tomer, can you re-check? Are you still seeing this?

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.

Comment 12

[Itiel]

(In reply to :Gijs (out, back April 27; he/him) from comment #10)

I kind of expect this to have gone away in Photon. Tomer, can you re-check? Are you still seeing this?

I do not.