1151496 - [RTL][Systems] Parentheses in song title are not displayed correctly on ringtone-related screens

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Ringtones, defect, P2)

Description

[Gregor Wagner [:gwagner]]

Creating a new bug for the ringtone part.

+++ This bug was initially created as a clone of Bug #1150749 +++

Description:
Song title that contains parentheses is displayed incorrectly on lockscreen and notification tray. This issue also reproduces on Create Ringtone screen and Settings > Sound screen.

Pre-requisite: Have at least 1 song containing parentheses in the title on the device.

Repro Steps:
1) Update a Flame to 20150402063750.
2) Set the device language in Arabic under Settings > Language.
3) Open Music app.
4) Play the song with the parentheses in the title.
5) Pull down the notification tray, or bring the lockscreen by pressing the power button twice.
6) Observe the parentheses in the song title.

Actual:
The parentheses are not located correctly.

Expected:
The parentheses are located correctly.

Note:
- The parentheses are also mis-located on "Create Ringtone" page, and on Settings >  Sound page when the song is set as ringtone.
- The parentheses are displayed correctly within the Music app. See bug 1140154.

Environmental Variables:
Device: Flame Master (KK, 319mb, full flash)
Build ID: 20150402063750
Gaia: f37be8b44cb7c3a147b9615ab76743b760f08eeb
Gecko: 35046df9df1f
Gonk: b83fc73de7b64594cd74b33e498bf08332b5d87b
Version: 40.0a1 (Master)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:40.0) Gecko/40.0 Firefox/40.0

Repro frequency: 5/5
See attached: screenshot

Comment 1

[Hema Koka [:hema]]

Jim,

Could you please take a look at this. 

Thanks
Hema

Comment 2

[Lancy(Leave from Mozilla)]

Test cases has been added in moztrap:
https://moztrap.mozilla.org/manage/case/15924/
https://moztrap.mozilla.org/manage/case/15520/
https://moztrap.mozilla.org/manage/case/15576/

Comment 3

[Josh Cheng [:josh]]

Help NI Jim for reminder. 

Thank you Jim for helping the issue.

Comment 4

[Hema Koka [:hema]]

Jim,

I know you were planning on an interim fix for this (until gecko support unicode 6.2). Any update on the fix?

Thanks
Hema

Comment 5

[David Flanagan [:djf]]

The fix for bug 1150749 was just to put the song title in <bdi> tags, and I assume that the fix here is to do the same. Is there anything that makes this bug more complicated that that one, Jim?  Please either provide an ETA for your fix, or if you can't fix it in the next couple of days, please let me know and I'll take it.

Comment 6

[Jim Porter (:squib)]

(In reply to David Flanagan [:djf] from comment #5)
> The fix for bug 1150749 was just to put the song title in <bdi> tags, and I
> assume that the fix here is to do the same. Is there anything that makes
> this bug more complicated that that one, Jim?

<bdi> elements don't work here, and actually make things worse. As I hypothesized in bug 1140154:

> I think this will still break for more complex cases (e.g. "ARABIC TEXT
> (English text)"), but that probably requires the Unicode paren-matching code
> I mentioned. If you'd like me to test the patch out with cases like that,
> just let me know.

Arguably, bug 1140154 is still an improvement, since we can assume that many cases of parenthesis are all in one language, but we can't assume that in the ringtones app, since we have a bunch of code to append parenthetical numbers to the end of duplicate ringtone names. The current code *should* actually work correctly for "ARABIC TEXT (<nn>)" when the locale is set to Arabic, but as comment 0 notes, fails for for "ENGLISH TEXT (<nn>)" in an Arabic locale.

I imagine that it'll be somewhat more common to see parens in a ringtone of the form "ARABIC TEXT (<nn>)" in this case, so the status quo is probably *better* than just wrapping the text in a <bdi> element. Unless we'd like to write our very own implementation of Unicode 6.3's paren-balancing algorithm for the ringtone app, I don't think we can actually display the parens correctly in all cases.

That said, I'm no expert on Unicode's RTL support, so it's possible there's another way to handle this. However, my investigations at the time of bug 1140154 suggested that the only complete solution is the paren-balancing algorithm. I think a change of that size is probably too risky for 2.2 at this stage (and again, I'd rather we implement the algorithm in Gecko so that everyone benefits from it).

I've tried to come up with a more-limited solution that masks the bug, but so far all I can think of is to add the " (<nn>)" string in a later step. However, that's still a significant change that would break many of the abstractions in the ringtones app. (It'd probably be cleaner to write the paren-balancing algorithm than to do this.)

Comment 7

[GitHub Autolander Bot]

Attachment: [gaia] jimporter:ringtones-rtl > mozilla-b2g:master

Comment 8

[Jim Porter (:squib)]

Comment on attachment 8595496 [details] [review]
[gaia] jimporter:ringtones-rtl > mozilla-b2g:master

Ok, as discussed on Vidyo, here's the clever workaround for the LTR parens in duplicate filenames.

Comment 9

[David Flanagan [:djf]]

The patch causes test failures, but I'm going to go ahead and review it anyway.

Comment 10

[David Flanagan [:djf]]

Comment on attachment 8595496 [details] [review]
[gaia] jimporter:ringtones-rtl > mozilla-b2g:master

So there are two cases where we have paren issues.

a) Some songs have parens in their song name.
b) If the user creates two ringtones from the same song, the second one will get a number in parens "(1)" to disambiguate it.

When I test without this patch, here are the places where I see those parens appear incorrectly:

1) In the "Create Ringtone" screen when the user does a share activity 

2) In the "Select Sound" list of ringtones accessed from the settings app

3) In the "Manage Ringtones" list of ringtones accessed from the settings app

4) In the delete confirmation dialog displayed from the manage ringtones panel (there are two variants depending on whether it is the default ringtone or not)

5) In the Settings app itself where the default ringtone name is displayed

6) In the player view of the music app before the user shares with the Ringtones app

Note that I am only testing the case of LTR english song names in the RTL mirrored english locale. I am treating RTL song names in an LTR locale to be out of scope for this bug and haven't even tried to test that. (Does anyone have an audio file where the song title is RTL and has parens?)

Here is what I find with this patch applied:

1) The "Create Ringtone" screen is fixed. Only case a applies here, since we never have a disambiguating number appended to the ringtone name at this point

2) The "Select Sound" list is fixed for both a and b.

3) The "Manage Ringtones" list is fixed for both a and b.

4) The delete confirmation dialog is not fixed for either case.

5) The default ringtone name in the Settings app is fixed for case b but not for case a. Apparently the Unicode LTR character that this patch appends fixes it in the settings app.

6) The Music app player title bar is not fixed by this patch. That should probably be considered a separate bug.

I also tested with a variant of this patch where I removed the \u200E character. My results were the same except for:

5) Without the \u200E character, neither case a nor b displayed correctly in the settings app.

As far as I can tell from these tests, it is not necessary to use both a bdi element and the \u200E character. The only gain we get from the \u200E character right now is fixing 5b. But the settings app needs to be modified to use a bdi element in order to fix 5a, and if we do that, then there is arguably no reason to use \u200E at all anymore. (Or, perhaps I've just not considered complicated enough cases yet. If \u200E is needed when we have an Arabic song title in an English locale, then that would be a reason to keep using it.)

Here are my recommendations:

1) Remove the \u200E part of this patch

2) Fix the tests (they may not be broken anymore with the \u200E removed)

3) Land this patch to resolve issues 1, 2 and 3.

4) File followup bugs (and assign them to yourself) for changes to the Settings and Music apps to resolve issues 5 and 6.

5) File a followup bug to fix the delete confirmation dialogs in the ringtones manager. I suspect this may require a change to the shared confirmation dialogs component. It is not clear to me whether that is something that we'd want to block on or not.

It would also be pretty reasonable to fix the dialogs and the settings app as part of this bug which is already 2.2+ if you'd rather do that than file followup bugs.

r- not because there is anything wrong with the code but because there is more to do. If you're going to do the other stuff in followup bugs, then this will be a pretty automatic r+ when the tests are passing.

Comment 11

[David Flanagan [:djf]]

Attachment: screenshot of the paren bug in the music player view titlebar

Comment 12

[David Flanagan [:djf]]

Attachment: screenshot of the bug in the settings app even with this patch applied.

Comment 13

[David Flanagan [:djf]]

I've added screenshots of the music and settings app issues. Looks like I didn't capture screenshots of the bug in the delete confirmation dialog.

Comment 14

[Jim Porter (:squib)]

Comment on attachment 8595496 [details] [review]
[gaia] jimporter:ringtones-rtl > mozilla-b2g:master

Ok, so it looks like <bdi> *does* work[1], but only if you're not injecting the names via WebIDE (there's probably a bug here somewhere). Luckily, mozL10N lets me embed HTML in l10n strings, so we can fix the popup too. But this means late-l10n.

[1] Well, not really. There are still some edge cases, but I've done some more-thorough tests, and <bdi> should work provided there are no directionality changes in the middle. "english(arabic)english" will probably do something dumb, but hopefully that's fairly rare.

Comment 15

[David Flanagan [:djf]]

Comment on attachment 8595496 [details] [review]
[gaia] jimporter:ringtones-rtl > mozilla-b2g:master

I have not tested the code, but it looks good to me.

I don't think we'll be allowed to uplift the string changes to v2.2, so if you can make it work without that, it would be a stronger patch. See my github comment.

Comment 16

[David Flanagan [:djf]]

Thinking about this some more, I'm guessing that the L10N library won't allow you to pass in HTML tags as part of the parameters, so you either have to modify the shared dialog library somehow or have to define new strings. I suppose we could ask about uplifting new strings in this case, especially since the bug we're fixing here is a localization bug and the bdi tags are only really needed in rtl locales.  So if you can't do this without new strings, maybe set needinfo for :pike to ask about that.

Comment 17

[Jim Porter (:squib)]

Yeah, there's no other way that I know of that will make this work. Any hack I could do to work around this would be complex and bug-prone enough that it wouldn't be safe to uplift.

Comment 18

[Jim Porter (:squib)]

:pike, what do you think we should do about uplifting this to 2.2? It has some l10n changes that are required to make bidirectional text display properly. As I see it, we have 3 options:

1) Uplift the patch as-is and have localizers do the translations
2) Uplift the patch but without the l10n change
3) Uplift the patch without the l10n change, and notify RTL locales that they should update their translations in-place

Comment 19

[Hema Koka [:hema]]

(In reply to Jim Porter (:squib) from comment #18)
> :pike, what do you think we should do about uplifting this to 2.2? It has
> some l10n changes that are required to make bidirectional text display
> properly. As I see it, we have 3 options:
> 
> 1) Uplift the patch as-is and have localizers do the translations
> 2) Uplift the patch but without the l10n change
> 3) Uplift the patch without the l10n change, and notify RTL locales that
> they should update their translations in-place

Bhavana says we should NI Delphine on this. Adding her.

Comment 20

[GitHub Autolander Bot]

http://docs.taskcluster.net/tools/task-graph-inspector/#T5OonrG6QvWqjmw-D4V5Eg

The pull request failed to pass integration tests. It could not be landed, please try again.

Comment 21

[Delphine Lebédel [:delphine - Use needinfo]]

Sorry I don't know anything about L10N library, and I think Pike can better understand and comment on what these patches would do. 
That said, we're way past string freeze. Also localization testing on 2.2 finishes at the end of the week. There's no way localizers will get to it by then.
Having said that, it's ultimately up to RelMan to decide whether this is bad enough to break string freeze, or not.

Comment 22

[Kevin Grandon :kgrandon]

(In reply to Autolander from comment #20)
> http://docs.taskcluster.net/tools/task-graph-inspector/#T5OonrG6QvWqjmw-
> D4V5Eg
> 
> The pull request failed to pass integration tests. It could not be landed,
> please try again.

I want to try autolander here again and monitor the logs. Adding checkin-needed again.

Comment 23

[GitHub Autolander Bot]

http://docs.taskcluster.net/tools/task-graph-inspector/#agjE5a2RTbCeMj_X-kZ19A

The pull request failed to pass integration tests. It could not be landed, please try again.

Comment 24

[Kevin Grandon :kgrandon]

Ok, well there's some infra weirdness today, possibly with caching. We're going to look into it, but for manually landing this makes sense since the last try run was green.

In master: https://github.com/mozilla-b2g/gaia/commit/04738cac27a6676fad7147b4d769605fb0dcbb3e

Comment 25

[Axel Hecht [:Pike]]

(In reply to Jim Porter (:squib) from comment #18)
> :pike, what do you think we should do about uplifting this to 2.2? It has
> some l10n changes that are required to make bidirectional text display
> properly. As I see it, we have 3 options:
> 
> 1) Uplift the patch as-is and have localizers do the translations
> 2) Uplift the patch but without the l10n change
> 3) Uplift the patch without the l10n change, and notify RTL locales that
> they should update their translations in-place

I'm not really qualified to answer these, but as Delphine said, we can rule out 1). I don't understand 2), and maybe 3) works, but I don't know the devils around the details on this stuff myself. Gandalf might have more educated opinions.

Comment 26

[Jim Porter (:squib)]

To clarify, (2) means "I'll remove the part of the patch that affects l10n strings and uplift the rest", so there'd still be a bug in one place, but most of the other issues would be fixed.

Comment 27

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

So, I'm not sure if I fully understand the patch, but you guys don't need <bdi>. Instead, set dir="auto" on the parent element:

<p class="name" data-l10n-id="${l10nID}" dir="auto">${name}</p>

It should then not require any l10n changes.

Comment 28

[Jim Porter (:squib)]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #27)
> So, I'm not sure if I fully understand the patch, but you guys don't need
> <bdi>. Instead, set dir="auto" on the parent element:
> 
> <p class="name" data-l10n-id="${l10nID}" dir="auto">${name}</p>

Unfortunately, that's not the part of the code that requires an l10n change; see [1]. Those strings are used in a CustomDialog popup to confirm that we should delete a ringtone. Since the ringtone name is embedded inside of a sentence, we need to indicate that its directionality should be isolated from the rest of the sentence. Using dir="auto" as you suggest mucks up the order of all the text. I'll attach a pair of screenshots to show what happens.

[1] https://github.com/mozilla-b2g/gaia/pull/29643/files#diff-ec8328bfc5fe9492af84879838326bcbR13

Comment 29

[Jim Porter (:squib)]

Attachment: The behavior before this patch

As you can see here, the parens are misaligned (they should surround the number 5), but due to the fact that a paren has no inherent directionality in Unicode, they get rendered incorrectly.

(Note that the Arabic is just some text I copied from a Wikipedia example about bidi text, so if it says something rude, blame Wikipedia.)

Comment 30

[Jim Porter (:squib)]

Attachment: Using dir="auto"

This is what happens when dir="auto" is applied to the parent element. As you can see, the ringtone's name is displayed correctly, but now the directionality of the surrounding text is broken.

The same applies in reverse if you have an English ringtone name embedded in Arabic text, but it's easier (if you speak English) to see the error in this case.

Comment 31

[Jim Porter (:squib)]

In any case, if there's a style guide for RTL support that says we should prefer dir="auto" to <bdi>, I don't mind updating the code again (and letting everyone else on the media team know what to use), but I haven't seen any such guides. MDN is currently down too, so I can't double-check...

Comment 32

[Jim Porter (:squib)]

:zibi, any ideas? (See comment 28 especially.)

From comment 18, it seems like the best solution would be (2) or (3): I'll uplift my patch, but without the l10n change. Then, localizers can either choose to do nothing (in which case there will be a small localization bug), or update their string in-place (which might make sense for the Arabic locale).

Comment 33

[GitHub Autolander Bot]

Attachment: [gaia] jimporter:ringtone-rtl-2.2 > mozilla-b2g:v2.2

Comment 34

[Jim Porter (:squib)]

Attachment: Patch for v2.2

Since I haven't heard back from :zibi, here's a pull request that just removes the l10n change from the original patch.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): N/A
[User impact] if declined: LTR text with parens inside RTL locales (or vice versa) will look broken
[Testing completed]: Manually tested and has been on master for a while
[Risk to taking this patch] (and alternatives if risky): Low-risk
[String changes made]: None

Comment 35

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

Sorry for late response Jim! I'm not sure if I can make a decision here on behalf of the localizers. I'm CC'ing Flod who may have better sensitivity as to which solution is the best for 2.2.

Comment 36

[Sue]

Attachment: verify_v3.0_pass.png

This issue has been verified passed on latest build of Flame 3.0
Pre-requisite: Have at least 1 song containing parentheses in the title on the device.
STRs:
1. Set system language as Arabic.
2. Launch Settings -> Sound -> Manage Tones.
3. Tap "+" button to add a ringtone.
4. Select the song containing parentheses.
5. Tap "Done" button.
6. Tap "Save" button.
7. Observe the Manage Tones view.
8. Back to Sound view and observe the view.
**The parentheses are displayed correctly in all views.
See attachment:verify_v3.0_pass.png
Rate: 0/3

Device: Flame 3.0 (pass)
Build ID               20150429010205
Gaia Revision          6e35b0948c42a4398b8a5916015de167121683a1
Gaia Date              2015-04-28 16:06:07
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/1ad65cbeb2f4
Gecko Version          40.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150429.043837
Firmware Date          Wed Apr 29 04:38:48 EDT 2015
Bootloader             L1TC000118D0

Comment 37

[Sue]

This issue also has been verified passed on latest build of Nexus 5 3.0 with the same steps in comment 36.
Rate: 0/3

Device: Nexus 5 3.0 (pass)
Build ID               20150429010205
Gaia Revision          6e35b0948c42a4398b8a5916015de167121683a1
Gaia Date              2015-04-28 16:06:07
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/1ad65cbeb2f4
Gecko Version          40.0a1
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150429.044325
Firmware Date          Wed Apr 29 04:43:46 EDT 2015
Bootloader             HHZ12f

Comment 38

[Francesco Lodolo [:flod]]

I'm a bit lost in the comments. To summarize:
* The issue is properly and fully fixed in master.
* The patch for v2.2 fixes most of the errors, but not the 3 delete-message-*. 

Is it correct?

As Delphine said we're part string-freeze (even the extended one), so the current 2.2 patch seems the best we can take at this point in the cycle.

Comment 39

[Jim Porter (:squib)]

(In reply to Francesco Lodolo [:flod] (UTC+2) from comment #38)
> I'm a bit lost in the comments. To summarize:
> * The issue is properly and fully fixed in master.
> * The patch for v2.2 fixes most of the errors, but not the 3
> delete-message-*. 
> 
> Is it correct?

Correct.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/71a3bea9cedc87f5ddc5efea712c31d59520ba81

Comment 41

[Sue]

Attachment: verify_Flame v2.2&N5 2.2_fail.png

This issue has been verified fail on latest build of Flame 2.2 and Nexus 5 2.2, I have opened a new bug to tracing it. See Bug 1160918

Pre-requisite: Have at least 1 song containing parentheses in the title on the device.
STRs:
1. Set system language as Arabic.
2. Launch Settings -> Sound -> Manage Tones.
3. Tap "+" button to add a ringtone.
4. Select the song containing parentheses.
5. Tap "Done" button.
6. Tap "Save" button.
7. Observe the Manage Tones view.
8. Tap "..." button at left side of the song containing parentheses.
9. Select "Delete ringtone" and observe the prompt message. 
10. Back to Sound view and observe the view.
Result: The parentheses are displayed incorrectly in prompt message of delete ringtone view.
See attachment: verify_Flame v2.2&N5 2.2_fail.png
Rate: 3/3

Device: Flame 2.2 (fail)
Build ID               20150503162504
Gaia Revision          8d14361337e608c8cdf165ea5034db5eda23b618
Gaia Date              2015-05-01 18:23:46
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/cb7cb6597c91
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150503.200206
Firmware Date          Sun May  3 20:02:17 EDT 2015
Bootloader             L1TC000118D0

Device: Nexus 5 2.2 (fail)
Build ID               20150503002500
Gaia Revision          8d14361337e608c8cdf165ea5034db5eda23b618
Gaia Date              2015-05-01 18:23:46
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/cb7cb6597c91
Gecko Version          37.0
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150503.040229
Firmware Date          Sun May  3 04:02:46 EDT 2015
Bootloader             HHZ12f

Comment 42

[Jim Porter (:squib)]

As discussed above, the uplifted patch for 2.2 doesn't contain any L10N changes, so the delete confirmation will still exhibit this bug. Unfortunately, it's far too late to change any L10N. What you see on 2.2 is as good as it'll get, unless specific locales choose to update their localizations (but I wouldn't hold my breath).

Comment 43

[Sue]

(In reply to Jim Porter (:squib) from comment #42)
> As discussed above, the uplifted patch for 2.2 doesn't contain any L10N
> changes, so the delete confirmation will still exhibit this bug.
> Unfortunately, it's far too late to change any L10N. What you see on 2.2 is
> as good as it'll get, unless specific locales choose to update their
> localizations (but I wouldn't hold my breath).

Shall we close this bug and tracing the issue at delete confirmation view via Bug 1160918?

Comment 44

[Jim Porter (:squib)]

(In reply to Sue from comment #43)
> Shall we close this bug and tracing the issue at delete confirmation view
> via Bug 1160918?

Sounds good.

Comment 45

[Sue]

According to comment 41-44, change the status to VERIFIED, FIXED. And we could trace the wrong parentheses in confirmation message of delete ringtone view via Bug 1160918.