1543828 - [RTL] Messages shown inside the video player are unreadable and in reverse

Status: VERIFIED FIXED

(Toolkit :: Video/Audio Controls, defect, P1)

Description

[Itiel]

Attachment: Screenshot

This is a regression, but as this is reproducible on RTL locales only and Mozregression doesn't provide an option to bisect with a specific language, I can't provide the exact regression window.

STR:

1. Run an RTL'd version of Nightly (Hebrew, Arabic etc). Setting intl.uidirection to 1 won't matter here -- you need an actual RTL build
2. Open http://dl3.webmfiles.org/elephants-dream.webm
3. Right after the video starts playing, reload the page
4. Observe the error message text

AR:
See attached. The letters are reversed and written as:
הקספוה ואדיוו תניעט
Completely unreadable.

ER:
The text should be:
טעינת הווידאו הופסקה

Note that this is different from bug 955983 -- there, the text directionality is LTR so the sentence is messed up a bit if LTR characters are present.
Here, the text itself is written the wrong way.

According to the screenshot, in English it'd be "deppots gnidaol oediV".

Comment 1

[:Gijs (he/him)]

(In reply to Itiel from comment #0)

Created attachment 9057691 [details]
Screenshot

This is a regression, but as this is reproducible on RTL locales only and Mozregression doesn't provide an option to bisect with a specific language, I can't provide the exact regression window.

Any idea about an approximate range? When did it last work?

Comment 2

[Itiel]

Unfortunately no :-(
I know it used to work about a year ago, but this isn't helping.
I'm not seeing these messages frequently enough to know.

Any way you can think of for me to bisect using a specific locale?

Comment 3

[Ian Moody [:Kwan] (UTC+1)]

Thanks to awful hacks to mozregression we end up with a regression range of https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=85b4d2bf888afa32b67638602a3338d0a6935ff9&tochange=9812141ec78239560283181ce610249d181b74b6
which is slightly large, but fortunately bug 1484048 rather stands out, and indeed, on the 2018-09-20 he build, flipping dom.ua_widget.enabled back to false fixes it.

Comment 4

[:Gijs (he/him)]

bug 1507894 enabled this stuff in release w/ Firefox 65.

Brian, do you have cycles to take a look now Tim is gone?

Comment 5

[Brian Grinstead [:bgrins]]

Attachment: Bug 1543828 - WIP - Explicitly set [dir] on strings inside of the video controls UA Widget

This seems to fix it, but setting [dir] only on a container element doesn't
for some reason. Maybe something's not being propagated through the UA widget.

Build / testing instructions for OSX (full build only) based on https://firefox-source-docs.mozilla.org/build/buildsystem/locales.html#instructions-for-single-locale-repacks-for-developers:

./mach build && ./mach package && ./mach build installers-fa && ./objdir-full-opt.noindex/dist/l10n-stage/firefox/Nightly.app/Contents/MacOS/firefox --profile $(mktemp -d) http://dl3.webmfiles.org/elephants-dream.webm

Comment 6

[Brian Grinstead [:bgrins]]

I don't know much about this but I was able to reproduce on a locally-packaged rtl build and found what I think is a fix. But it seems odd - setting the [dir] attribute on the root doesn't seem to have an effect for children within the UA Shadow DOM, I had to set it on each element that had textContent (see https://phabricator.services.mozilla.com/D27589). Olli, do you know if there's any handling for directionality that might be missing in UA Shadow root? And, should we change the platform to assume that directionality within the root is based on the build default?

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

In my testing dir inside shadow DOM gets propagated properly to its descendants
http://mozilla.pettay.fi/moztests/rtl_dir_in_shadow.html

But see https://searchfox.org/mozilla-central/rev/1b2636e8517aa48422ed516affe4d28cb7fa220a/toolkit/themes/shared/media/videocontrols.css#14

Comment 8

[Ian Moody [:Kwan] (UTC+1)]

Some things I noticed:

• the old working builds had the full-stop at the right-end of the text, but I don't that was correct.
• Brian's patch puts the full-stop at the left-end of the text now (I think, based on inspector testing) Itiel is that where it should be? (If so then this is an improvement over the original behaviour as well as a regression fix)
• setting the dir to rtl will fix the text order, and interestingly changing the value again, even to ltr, won't break it again, only move the full-stop.

But this is all testing via inspector altering, so perhaps it behaves differently to initial set values.

I suppose another possibility would be replacing all the error <span>s with <bdi>s.

Comment 9

[Itiel]

(In reply to Ian Moody [:Kwan] (UTC+0) from comment #8)

• the old working builds had the full-stop at the right-end of the text, but I don't that was correct.

Right, that's bug 955983. Would be nice if this bug could fix that one too.

• Brian's patch puts the full-stop at the left-end of the text now (I think, based on inspector testing) Itiel is that where it should be? (If so then this is an improvement over the original behaviour as well as a regression fix)

Yes.

Comment 10

[Ian Moody [:Kwan] (UTC+1)]

Ah, sweet, I hadn't properly looked at bug 955983. That'll probably be duped to here then.

Comment 11

[Brian Grinstead [:bgrins]]

(In reply to Olli Pettay [:smaug] from comment #7)

In my testing dir inside shadow DOM gets propagated properly to its descendants
http://mozilla.pettay.fi/moztests/rtl_dir_in_shadow.html

But see https://searchfox.org/mozilla-central/rev/1b2636e8517aa48422ed516affe4d28cb7fa220a/toolkit/themes/shared/media/videocontrols.css#14

Doh, thank you. When I remove that rule I can set [dir] on the container. I'm still not sure if we should do that vs <bdi> for the strings but we can figure that out.

Comment 12

[Brian Grinstead [:bgrins]]

Attachment: Bug 1543828 - Use <bdi> inside video controls content

Comment 13

[Brian Grinstead [:bgrins]]

Attachment: current-message.png

Without any patch applied

Comment 14

[Brian Grinstead [:bgrins]]

Attachment: with-bdi-patch.png

Comment 15

[Brian Grinstead [:bgrins]]

Attachment: with-bdi.png

Comment 16

[Brian Grinstead [:bgrins]]

I don't know if there are side effects to using <bdi> here instead of <span>, but it does seem to fix the issue as well.

Comment 17

[Brian Grinstead [:bgrins]]

(In reply to Ian Moody [:Kwan] (UTC+0) from comment #8)

But this is all testing via inspector altering, so perhaps it behaves differently to initial set values.

In my experience, messing around with directionality in the inspector (either via attributes or CSS) has been a bit flaky. To be fair I've dealt with it in native anonymous content mostly, so even more could go wrong there and perhaps it works fine for this case.

Comment 18

[Itiel]

(In reply to Brian Grinstead [:bgrins] from comment #16)

I don't know if there are side effects to using <bdi> here instead of <span>, but it does seem to fix the issue as well.

Looking good. Can you please post a screenshot of how attachment 8821802 [details] looks like? To verify this works when LTR characters are involved.

Comment 19

[Brian Grinstead [:bgrins]]

Attachment: screenshot-of-attachment-8821802-with-bdi.png

Comment 20

[Ian Moody [:Kwan] (UTC+1)]

(In reply to Brian Grinstead [:bgrins] from comment #16)

I don't know if there are side effects to using <bdi> here instead of <span>, but it does seem to fix the issue as well.

My reading of https://www.w3.org/International/articles/inline-bidi-markup/#bdi is that there shouldn't be (exactly equivalent to <span dir="auto">) but 🤷. Do we have any bidi/RTL experts who can weigh in?

Comment 21

[:Gijs (he/him)]

(In reply to Ian Moody [:Kwan] (UTC+0) from comment #20)

(In reply to Brian Grinstead [:bgrins] from comment #16)

I don't know if there are side effects to using <bdi> here instead of <span>, but it does seem to fix the issue as well.

My reading of https://www.w3.org/International/articles/inline-bidi-markup/#bdi is that there shouldn't be (exactly equivalent to <span dir="auto">) but 🤷. Do we have any bidi/RTL experts who can weigh in?

Perhaps Ehsan can help?

Comment 22

[Brian Grinstead [:bgrins]]

(In reply to :Gijs (he/him) from comment #21)

(In reply to Ian Moody [:Kwan] (UTC+0) from comment #20)

(In reply to Brian Grinstead [:bgrins] from comment #16)

I don't know if there are side effects to using <bdi> here instead of <span>, but it does seem to fix the issue as well.

My reading of https://www.w3.org/International/articles/inline-bidi-markup/#bdi is that there shouldn't be (exactly equivalent to <span dir="auto">) but 🤷. Do we have any bidi/RTL experts who can weigh in?

Perhaps Ehsan can help?

Ehsan, I have a patch at https://phabricator.services.mozilla.com/D27757 that fixes the direction of RTL messages within the videocontrols UA Widget (see https://bugzilla.mozilla.org/show_bug.cgi?id=1543828#c13 vs https://bugzilla.mozilla.org/show_bug.cgi?id=1543828#c15 with the patch applied). Is <bdi> the right solution here or should we be setting [dir] based on the locale (or both?).

One thing to note is that this whole thing is wrapped into a container with direction: ltr in CSS (presumably to keep the buttons in the same order regardless of locale): https://searchfox.org/mozilla-central/rev/1b2636e8517aa48422ed516affe4d28cb7fa220a/toolkit/themes/shared/media/videocontrols.css#14.

Comment 23

[Itiel]

(In reply to Brian Grinstead [:bgrins] from comment #19)

Looking good.

Comment 24

[(no longer active)]

Hey Brian, you want <bdi> here to ensure the text is isolated from its surroundings and isn't impacted by the directionality of its surrounding when we resolve its directionality (and vice versa).

Comment 25

[:Gijs (he/him)]

[Tracking Requested - why for this release]:
User-visible regression in RTL.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ac2351ce41ef
Use <bdi> inside video controls content r=Gijs

Comment 27

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/ac2351ce41ef

Comment 28

[Pascal Chevrel:pascalc]

The fix looks straightforward, Brian could you request an uplift to beta please? Thanks

Comment 29

[Itiel]

Fixed on latest Nightly.

Comment 31

[Brian Grinstead [:bgrins]]

Comment on attachment 9058685 [details]
Bug 1543828 - Use <bdi> inside video controls content

Beta/Release Uplift Approval Request

• User impact if declined: RTL strings within the video controls UI are displayed incorrectly.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: See https://bugzilla.mozilla.org/show_bug.cgi?id=1543828#c0
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It's a pretty small change (switching from a <span> to a <bdi> elements for the UI) so it's low, but there aren't many automated tests for video controls so we should take care to test this manually.
• String changes made/needed:

Comment 32

[Pascal Chevrel:pascalc]

Comment on attachment 9058685 [details]
Bug 1543828 - Use <bdi> inside video controls content

Low-risk fix, uplift approved for 67 beta 13, thanks.

Comment 33

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6cd36394e2ae

Comment 34

[Adrian Florinescu [:aflorinescu]]

67.0b12 20190418160535
67.0b13 20190422163745

Verified the fix by comparing the "he" localization before (b12) and after the fix (b13) on Windows 10 x64 and Ubuntu 16.04.