1562425 - [RTL][about:addons] Some icons appear on the wrong side of their container

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect, P3)

Description

[Itiel]

Attachment: Screenshot

See attached.
These icons should appear on the right side of the text.

Comment 1

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

The priority flag is not set for this bug.
:jimm, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 3

[Itiel]

I'm guessing this:
https://searchfox.org/mozilla-central/rev/c7e8bc4996f979e5876b33afae3de3b1ab4f3ae1/toolkit/mozapps/extensions/content/panel-item.css#24
should change to button:-moz-locale-dir(rtl).

Comment 4

[Mark Striemer [:mstriemer]]

Attachment: Screenshot 2019-08-13 17.01.55.png

The contribute button has a similar issue in RTL.

Comment 5

[:Gijs (he/him)]

In fact, there is already an RTL rule here ( https://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/content/panel-item.css#25 ) but it never applies.

If I switch to hebrew on beta, and use the inspector in about:addons to inspect the relevant button, then:

> $0.matches(":dir(rtl)") // button
< false
> $0.parentNode.matches(":dir(rtl)") // { ShadowRoot }
< an error, no "matches" on parentNode.
> $0.parentNode.host.matches(":dir(rtl)") // <panel-item>
< false
> $0.parentNode.host.parentNode.matches(":dir(rtl)") // <panel-list>
< true

This... doesn't make a lot of sense to me. Emilio, can you explain what's happening here? Why isn't the dir pseudoselector being inherited, and even besides that, given that the text in the <button> is RTL, why isn't it being treated as matching :dir(rtl) itself? And what's the correct way to work around this?

(In reply to Itiel from comment #3)

I'm guessing this:
https://searchfox.org/mozilla-central/rev/c7e8bc4996f979e5876b33afae3de3b1ab4f3ae1/toolkit/mozapps/extensions/content/panel-item.css#24
should change to button:-moz-locale-dir(rtl).

That also doesn't work.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Sorry for the lag here. Directionality should inherit from host to shadow children. Not sure why the host is not rtl there. Will take a look.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

So there's something that's going awry. You can see that if you add and remove the dir attribute on the bogus element how it starts matching a different directionality.

Maybe toggling it on the root is a workaround worth uplifting to beta (so that it recomputes all elements...).

Is there any chance to try to get a reduced test-case for this? There is clearly an underlying DOM bug to fix.

I'm happy to take a shot at reducing it. I found the panel-item code, but not the code that sets dir="rtl" on the root element...

Comment 8

[:Gijs (he/him)]

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

So there's something that's going awry. You can see that if you add and remove the dir attribute on the bogus element how it starts matching a different directionality.

Maybe toggling it on the root is a workaround worth uplifting to beta (so that it recomputes all elements...).

Is there any chance to try to get a reduced test-case for this? There is clearly an underlying DOM bug to fix.

I'm afraid I don't really have bandwidth to take this on, and I'm not really an expert on the custom element / shadow DOM stuff myself... :-(

I'm happy to take a shot at reducing it. I found the panel-item code, but not the code that sets dir="rtl" on the root element...

I think it's done from fluent, at https://searchfox.org/mozilla-central/rev/3366c3d24f1c3818df37ec0818833bf085e41a53/dom/l10n/DOMLocalization.cpp#481 .

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Ok, cool, that's helpful. I'll try to poke at this with rr.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Attachment: Reduced test-case.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1562425 - Ensure to bind ShadowRoot before explicit kids, so that slot directionality is up-to-date. r=smaug

When we're slotted, we inherit our directionality from the slot element. But
when binding an ltr subtree into an rtl subtree, we used to bind the explicit
kids first, then the shadow root kids (which includes the slots to which
explicit kids may have been assigned).

So the explicit kids looked at the slot directionality before it was recomputed,
and thus ended up with ltr rather than rtl directionality.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2b1d674933e9
Ensure to bind ShadowRoot before explicit kids, so that slot directionality is up-to-date. r=smaug

Comment 13

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/2b1d674933e9

Comment 14

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 15

[Itiel]

Fixed on latest Nightly. Thanks!