1339355 - [webvtt] Remove the strongRTLChars table in vtt.jsm?

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect)

Description

[Benjamin Chen [:bechen]]

In vtt.jsm, there is a strongRTLChars table contains the RTL characters.
Maybe we can remove the table and add a function like http://searchfox.org/mozilla-central/source/intl/unicharutil/util/nsBidiUtils.cpp#85

Hi Ralph:
how do you think about this or do you know who is familiar with RTL?

Comment 1

[Ralph Giles (:rillian)]

Hmm. I'm not very familiar with RTL. Saving the table space would be nice, but it looks like it doesn't do quite the same thing.

The only place the spec uses the strong character table is to determine the base writing direction for implementing `start` or `end` text alignment in the cue box. It looks like we implement this by setting `direction: rtl` or `direction: ltr` on the display <div>.  Cameron, what do you think? Is there a way to implement this in CSS so we can leverage the browser's bidi implementation instead of trying to duplicate it?

Comment 2

[Cameron McCormack (:heycam)]

The script is using that table to implement the "determine paragraph direction automatically" algorithm, although it looks like it is doing it incorrectly.  (It's testing if there are any strong RTL characters, but instead it should look for the first strongly directional character and use that to determine the base direction.)  It's only doing this so that text-align:start and text-align:end resolve to the expected sides.

I think we just want to use unicode-bidi:plaintext directly on the <div>, which means "treat this block isolated from surrounding content, and determine base direction of the block automatically using that algorithm instead of using the direction property".  Since the direction property is ignored when unicode-bidi is plaintext, we don't need to set it.

There seems to be a typo in the script where it's not setting unicode-bidi properly:

  http://searchfox.org/mozilla-central/rev/ac8a72f2d5204ced2004c93a9c193430c63a6a71/dom/media/webvtt/vtt.jsm#761

With that fixed, we should be able to remove the setting of direction, and the determineBidi function and the table.

Comment 3

[Benjamin Chen [:bechen]]

Attachment: Bug 1339355 - Remove the RTL table.

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

Comment 4

[Cameron McCormack (:heycam)]

Comment on attachment 8837952 [details]
Bug 1339355 - Remove the RTL table.

https://reviewboard.mozilla.org/r/112952/#review114454

::: dom/media/webvtt/vtt.jsm
(Diff revision 1)
> -      styles.writingMode = cue.vertical === "" ? "horizontal-tb"
> -                                               : cue.vertical === "lr" ? "vertical-lr"
> -                                                                       : "vertical-rl".
> -      stylesunicodeBidi =  "plaintext";

Don't we need to still set writing-mode, and fix the setting of unicode-bidi?

Please make sure to test a vtt file with vertical cues, and with a cue that has only RTL characters in it, to make sure this keeps working.

Comment 5

[Benjamin Chen [:bechen]]

Comment on attachment 8837952 [details]
Bug 1339355 - Remove the RTL table.

https://reviewboard.mozilla.org/r/112952/#review114458

::: dom/media/webvtt/vtt.jsm
(Diff revision 1)
> -      styles.writingMode = cue.vertical === "" ? "horizontal-tb"
> -                                               : cue.vertical === "lr" ? "vertical-lr"
> -                                                                       : "vertical-rl".
> -      stylesunicodeBidi =  "plaintext";

The styles I delete are apply to the |this.div| which is parent of |this.cueDiv|. At line461~466, we have apply the writingMode and unicodeBidi to this.cueDiv. That's why I move the code block.

Comment 6

[Cameron McCormack (:heycam)]

Comment on attachment 8837952 [details]
Bug 1339355 - Remove the RTL table.

https://reviewboard.mozilla.org/r/112952/#review114458

> The styles I delete are apply to the |this.div| which is parent of |this.cueDiv|. At line461~466, we have apply the writingMode and unicodeBidi to this.cueDiv. That's why I move the code block.

Ah, I see!  Thanks for the explanation.

Comment 7

[Cameron McCormack (:heycam)]

Comment on attachment 8837952 [details]
Bug 1339355 - Remove the RTL table.

https://reviewboard.mozilla.org/r/112952/#review114462

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/698fd6a86e5d
Remove the RTL table. r=heycam

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/698fd6a86e5d