1134453 - [RTL] SMS notification for dual sim puts wrong parenthesis on wrong side of the sentence

Status: VERIFIED FIXED

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

Description

[Pi Wei Cheng [:piwei] (inactive)]

Attachment: screenshot of bug

Description:
SMS Notification for dual-sim environment puts which SIM a message is received from in parenthesis. And in RTL environment, the beginning of the parenthesis is being put to the end and it's using the wrong round bracket.

Prerequisite:
DUT is using dual sims

STR:
(optional) Create contact A in Contacts app
1) Have contact A send an SMS to DUT
2) Observe the SMS notification on DUT

Expected: It should say:
(SIM 1) Name
Message content

Actual: It says:
SIM 1) Name)
Message content

See screenshot for bug behavior.

Comment 1

[Pi Wei Cheng [:piwei] (inactive)]

This issue occurs on Flame 3.0 Master and on Flame 2.2.

Device: Flame 3.0
BuildID: 20150218010226
Gaia: 82f286f10a41aab84a0796c89fbefe67b179994b
Gecko: 9696d1c4b3ba
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 38.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0

Device: Flame 2.2
BuildID: 20150218002515
Gaia: da509caa7395d3d090ce973e8de082b4680a590d
Gecko: 96da179a7d3a
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 37.0a2 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Comment 2

[Pi Wei Cheng [:piwei] (inactive)]

Attachment: logcat on Flame 3.0

Comment 3

[Stephany Wilkes]

Flagging Francisco to see if we can get anyone assigned to this before Monday. P2.

Comment 4

[Francisco Jordano [:arcturus] [:francisco]]

Hi Stephany, this is not SMS specific, is happening in the window management component.

You'll need more info from Gregor.

Comment 5

[Michael Henretty [:mikehenrty][:mhenretty]]

I'll take a look.

Comment 6

[Michael Henretty [:mikehenrty][:mhenretty]]

Actually, let's have Guillaume take a look.

Comment 7

[Pi Wei Cheng [:piwei] (inactive)]

Attachment: similar issue on Browser value selector

I found that this similar issue also occurs when selecting a language in Browser. I'm not sure if these two are of the same cause, but here is the STR from Browser:

1) Navigate to http://news.google.com
2) Scroll to the bottom of the page and tap on dropdown list of 'Select edition'
3) Scroll to the bottom of the value selection page

See attached screenshot for bug behavior.

Please let me know if a separate bug needs to be filed.

Comment 8

[GitHub Autolander Bot]

Attachment: [gaia] gmarty:Bug-1134453-RTL-value-selector-options > mozilla-b2g:master

Comment 9

[Guillaume Marty [:gmarty]]

Comment on attachment 8568574 [details] [review]
[gaia] gmarty:Bug-1134453-RTL-value-selector-options > mozilla-b2g:master

Julien, can you review this RTL related patch?

Comment 10

[Julien Wajsberg [:julienw]]

Comment on attachment 8568574 [details] [review]
[gaia] gmarty:Bug-1134453-RTL-value-selector-options > mozilla-b2g:master

As discussed on IRC, we miss the fix for the notification part.

Also I think the patch is not correct, we should add dir="auto" on each <li> otherwise I fear of incorrect behavior in some cases.

Comment 11

[Pi Wei Cheng [:piwei] (inactive)]

Changing title to make this a global issue instead of just one single area.

Comment 12

[Julien Wajsberg [:julienw]]

Pi Wei, I don't think this is a good idea.

The global issue is related in how Unicode works, and we need to adjust the markup or the CSS in all places where this can happen. We won't fix it all in one patch.
(The parenthesis issue should eventually be fixed in bug 922963, but not the ".S.U" one).

I think it's better to file separate bug for different areas.

We _could_ fix the 2 cases you reported here, but please don't dupe other bugs.

Comment 13

[Pi Wei Cheng [:piwei] (inactive)]

We'll hold off on filing separate bugs since we're unsure what the scope of bug 922963 will be fixing and we want to avoid dupes. Changing title back.

Comment 14

[Julien Wajsberg [:julienw]]

TBH I don't see bug 922963 moving soon. So don't refrain yourself ;)

Comment 15

[GitHub Autolander Bot]

Attachment: [gaia] gmarty:Bug-1134453-RTL-value-selector-options-Notification > mozilla-b2g:master

Comment 16

[Guillaume Marty [:gmarty]]

Comment on attachment 8569936 [details] [review]
[gaia] gmarty:Bug-1134453-RTL-value-selector-options-Notification > mozilla-b2g:master

I'm still unsure whether I should open a new bug or not, so I created a different patch for the notification issue.
Etienne, can you review it?

Comment 17

[Guillaume Marty [:gmarty]]

Comment on attachment 8568574 [details] [review]
[gaia] gmarty:Bug-1134453-RTL-value-selector-options > mozilla-b2g:master

Julien, I applied the changes we discussed offline. How does it look now?

Comment 18

[Julien Wajsberg [:julienw]]

Comment on attachment 8568574 [details] [review]
[gaia] gmarty:Bug-1134453-RTL-value-selector-options > mozilla-b2g:master

look at http://everlong.org/mozilla/test-select/
It works fine in RTL :) (I think it was not fine with your first patch)

But in LTR the "arabic" option is aligned right, I think it should be aligned left. We need a "text-align: left" somewhere :)

Comment 19

[Guillaume Marty [:gmarty]]

Comment on attachment 8568574 [details] [review]
[gaia] gmarty:Bug-1134453-RTL-value-selector-options > mozilla-b2g:master

Patch updated!

Comment 20

[Etienne Segonzac (:etienne)]

Comment on attachment 8569936 [details] [review]
[gaia] gmarty:Bug-1134453-RTL-value-selector-options-Notification > mozilla-b2g:master

r=me with a small unit test in notifications_test.js

Comment 21

[Julien Wajsberg [:julienw]]

Comment on attachment 8568574 [details] [review]
[gaia] gmarty:Bug-1134453-RTL-value-selector-options > mozilla-b2g:master

lgtm, let's see what the peers think :)

Comment 22

[Etienne Segonzac (:etienne)]

Comment on attachment 8568574 [details] [review]
[gaia] gmarty:Bug-1134453-RTL-value-selector-options > mozilla-b2g:master

stamp (looks like it already landed :))

Comment 23

[Guillaume Marty [:gmarty]]

Landed in https://github.com/mozilla-b2g/gaia/commit/b3fe0b0741252e18fdceded00595bd559e6c2bf1 and https://github.com/mozilla-b2g/gaia/commit/6766a5fdc959923fd8c6cb63d35bf35ba1bda777

Comment 24

[Guillaume Marty [:gmarty]]

Comment on attachment 8568574 [details] [review]
[gaia] gmarty:Bug-1134453-RTL-value-selector-options > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): RTL
[User impact] if declined: The parentheses are mismatched in the select tags in web pages
[Testing completed]: Manual testing
[Risk to taking this patch] (and alternatives if risky): Low as it's only markup and CSS change
[String changes made]: None

Comment 25

[Guillaume Marty [:gmarty]]

Comment on attachment 8569936 [details] [review]
[gaia] gmarty:Bug-1134453-RTL-value-selector-options-Notification > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): RTL
[User impact] if declined: The parentheses are mismatched in notifications toaster and in the utility tray
[Testing completed]: Manual testing
[Risk to taking this patch] (and alternatives if risky): Low as it's only markup and CSS change
[String changes made]: None

Comment 26

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/28eb26672103475b722b0477501aa6ce7cb37941
v2.2: https://github.com/mozilla-b2g/gaia/commit/062d395273e70b8688a91b5a2b34c0cc7afc679f

Comment 27

[Pi Wei Cheng [:piwei] (inactive)]

The issues on comment 0 and comment 7 have been verified fixed on Flame 3.0 master,

Parentheses now display as expected in Arabic in those cases.

Device: Flame 3.0 Master (full flash 319MB mem)
BuildID: 20150303010233
Gaia: c8ed1085a67490a1ecd7f275e5de9487e1b93b1d
Gecko: 0b3c520002ad
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 39.0a1 (3.0 Master)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0

Device: Flame 2.2 (full flash 319MB mem)
BuildID: 20150303002527
Gaia: 3d188c414e30acc392253d5389a42352fcfbc183
Gecko: c89aad487aa5
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 37.0 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0

Comment 28

[Pi Wei Cheng [:piwei] (inactive)]

Issue is also verified fixed on Flame 2.2 (missed this sentence on comment 27)