1325230 - `document.dir` is not set in about:config, preventing rtl from working

Status: NEW

(GeckoView :: General, defect, P3)

Description

[Itiel]

Attachment: 1

When adding a new pref in about:config, the dialogs does not seem to support RTL.

Some screenshots attached.

Comment 1

[Itiel]

Attachment: 2

Comment 2

[Itiel]

Attachment: 3

Comment 3

[Tim Nguyen :ntim]

Hi Itiel, could you please re-test this after this lands in Firefox Preview Nightly (around next Monday) ?

I think bug 1516577 should have solved this.

Comment 4

[Itiel]

This looks okay, but only because the whole layout is now LTR (including the New Preference dialog).

For example, this rule:
https://searchfox.org/mozilla-central/rev/b04e3a28a2ef4dbf957018dbbdc1840d62fdbc32/mobile/android/themes/geckoview/config.css#120-122
is not applied in RTL [1].

[1] Thank you for thinking about RTL, but this rule should never apply :)
In RTL the search icon should look the same as it is shown in your before/after screenshot in attachment 9119546 [details].
This makes me wonder why in the first place there is so much inconsistency with this icon, across the LTR UI..

Comment 5

[Tim Nguyen :ntim]

(In reply to Itiel from comment #4)

This looks okay, but only because the whole layout is now LTR (including the New Preference dialog).

That's really strange. I thought there was some JS that set the right dir attribute value on the root element...

I'll need to take a closer look later I guess. Thanks for testing!

Comment 6

[Tim Nguyen :ntim]

Itiel, do you have any idea why the rtl mode is not being activated? (You can debug Fenix from about:debugging if you want).

When I manually added dir="rtl", the UI looked properly flipped, but I guess dir="rtl" is not getting applied at all in this case ?

https://searchfox.org/mozilla-central/rev/b04e3a28a2ef4dbf957018dbbdc1840d62fdbc32/mobile/android/chrome/geckoview/config.xhtml#26

That should be doing it, but for some reason it's not ?

Comment 7

[Itiel]

(In reply to Tim Nguyen :ntim (blocking ni? until Jan 22th) from comment #6)

Itiel, do you have any idea why the rtl mode is not being activated? (You can debug Fenix from about:debugging if you want).

Not sure.
Seeing that your patch didn't touch anything that might have regressed this, I'm wondering if this is caused by the recent work on the languages for Fenix [1][2], the timings seem to match.

[1] https://github.com/mozilla-mobile/fenix/issues/220
[2] https://github.com/mozilla-mobile/fenix/releases

Comment 8

[Itiel]

(In reply to Itiel from comment #7)

Seeing that your patch didn't touch anything that might have regressed this, I'm wondering if this is caused by the recent work on the languages for Fenix [1][2], the timings seem to match.

Nope, that's not the case either.
I checked with a Fenix build from 2020-01-01 and the issue is still there, with the old design.

Tim, since you've essentially fixed this bug in bug 1516577 and the issue is really about dir="rtl" not getting added, is it better we close this one out and I'll file a bug in the fenix github repo?

Comment 9

[Tim Nguyen :ntim]

(In reply to Itiel from comment #8)

Nope, that's not the case either.
I checked with a Fenix build from 2020-01-01 and the issue is still there, with the old design.

Thanks for checking!

Tim, since you've essentially fixed this bug in bug 1516577 and the issue is really about dir="rtl" not getting added, is it better we close this one out and I'll file a bug in the fenix github repo?

Not sure if dir='rtl' not being added is Fenix related or GeckoView related, maybe snorp knows (feel free to redirect if not) ?

Comment 10

[James Willcox (:snorp) ([e-mail]) (he/him)]

It looks like document.dir isn't set at all in GeckoView apps. That's bad. I'll investigate some more.

Comment 11

[James Willcox (:snorp) ([e-mail]) (he/him)]

Henri, do you know how document.dir ends up getting set on desktop? I'm not immediately finding it...

Comment 12

[Henri Sivonen (:hsivonen)]

Sorry, I don't how we set document.dir for in-content UI. Needinfoing jfkthame.

Comment 13

[Tim Nguyen :ntim]

Also adding Zibi who might know.

Comment 14

[Zibi Braniecki [:zbraniecki][:gandalf]]

document.dir is set on Desktop via Fluent - https://searchfox.org/mozilla-central/source/dom/l10n/DOMLocalization.cpp#464-482

Comment 15

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Zibi Braniecki [:zbraniecki][:gandalf] from comment #14)

document.dir is set on Desktop via Fluent - https://searchfox.org/mozilla-central/source/dom/l10n/DOMLocalization.cpp#464-482

I had found that, but it appears that we guard running any of it with this[1], which seems to only apply to chrome/privileged pages. I must be missing something else?

[1] https://searchfox.org/mozilla-central/source/dom/base/nsContentUtils.cpp#1735

Comment 16

[Zibi Braniecki [:zbraniecki][:gandalf]]

We run it only on documents that use Fluent - which is triggered by the presence of <link rel="localization"/> in the head or <linkset></linkset> collection.

It should work in all documents from chrome, including unprivileged like about:neterrors and about:mozilla.