1091511 - [Flame][v2.1][Message]The number/email or URL can't be tapped if there is full-width text in this message.

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect)

Description

[Alissa（leave from Mozilla）]

[1.Description]:
The number/email or URL can't be tapped if there is other text in this message.
Occurence time:
see attachments:logcat3.txt 1.png.
[2.Testing Steps]: 
1.Send one message (contain number/email or URL and other text,see figure sample.png)to test device.
2.open messsage and open this message.
3.Try to tap the number,email or URL.

[3.Expected Result]: 
3.It should be able to be tapped.

[4.Actual Result]: 
3.It can't be tapped.

[5.Reproduction build]: 
Gaia-Rev        eb0aab0f13c78c7ac378ad860e865c4b6eaf669f
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/318019f80a8e
Build-ID        20141029001202
Version         34.0

[6.Reproduction Frequency]: 
Always Recurrence,5/5

Comment 1

[Alissa（leave from Mozilla）]

Attachment: logcat3.txt

Comment 2

[Julien Wajsberg [:julienw]]

qawanted for branch checks

Comment 3

[Wilfred Mathanaraj [:WDM]]

triage: basic functionality and should work. if there are issues with this it must be fixed for release.

Comment 4

[Cody Roesch [:croesch]]

Hi Alissa,
   I've been working on this bug and I've tried 4 different builds including the build you found this on. Every message received by the test device (Flame) has all it's links working, even when it's in the middle of a bunch of other text.

What device are you on?

Also you say to view a screenshot but I'm not seeing a screenshot attached. This would be super helpful!

Thank you!

Comment 5

[Alissa（leave from Mozilla）]

Attachment: 1.png

Please see the added attachment,we test on flame.

Comment 6

[Ken Chang[:kenkai]]

Steve, please check this bug to see if you can take.

Comment 7

[Steve Chung [:steveck]]

[Blocking Requested - why for this release]:

(In reply to Wilfred Mathanaraj [:WDM] from comment #3)
> triage: basic functionality and should work. if there are issues with this
> it must be fixed for release.

It *did* work from the beginning and we seldom change this part. The root cause is because of the full-width char in front of the url/phone number. If there is half-width space/comma/... before the url, everything works fine. This issue should not be a blocker since it's not be block IMO, but it shouldn't be difficult to fix in this first char checking[1].

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/link_helper.js#L61

Comment 8

[Steve Chung [:steveck]]

(In reply to Steve Chung [:steveck] from comment #7)
> This issue should not be a blocker since it's not be block IMO,

Sorry for the typo: This issue should not be a blocker since it's not regression

Comment 9

[Julien Wajsberg [:julienw]]

Is this usual in chinese that we can have URL between chinese characters without spaces to separate it?

Comment 10

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

The more usual case is when a Chinese-type comma put right before the URL without a space in between. (Like the snapshot attached by Alissa).
Per comment 8, it's not a regression and is more like a feature. We shouldn't block on 2.1. 
Needinfo Jenny, if UX feels this is important please proceed to nominate to 2.2.

Comment 11

[Jenny Lee]

This should be fixed, thanks!

Comment 12

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

This issue is reproducible on Flame 2.1, Flame 2.2, Flame 2.0, and v188-1 base image itself.

Observed behavior: Without a space before a number/email/URL, a tappable item is not tappable (it doesn't become a link) in a Message. Tested with Chinese and Japanese full-width characters.

Device: Flame 2.1
BuildID: 20141104070645
Gaia: 9d80d556123553e5fd8acd982192e33807e9e1fe
Gecko: 0953b46ee9a8
Version: 34.0 (2.1)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Device: Flame 2.2 Master
BuildID: 20141104041844
Gaia: 3c50520982560ccba301474d1ac43706138fc851
Gecko: cadcd47db610
Version: 36.0a1 (2.2 Master)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

Device: Flame 2.0
BuildID: 20141104053644
Gaia: fe2167fa5314c7e71c143a590914cbf3771905a8
Gecko: 093de6b632c5
Version: 32.0 (2.0)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

-------------

(In reply to Julien Wajsberg [:julienw] from comment #9)
> Is this usual in chinese that we can have URL between chinese characters
> without spaces to separate it?

As a Traditional Chinese user myself, I don't expect a number/email/URL to become a link in a message without a half-width/ascii space before and after it (if there's text after it). I think it's common sense for those users to add a space between items they expect to become tappable. Even my Android 4.4.4 phone can't detect those items in all scenarios without a space. The behavior is consistent in this case (if there is a space before the link, the link does always become a link) so I personally don't feel it's a big issue.

Comment 13

[Julien Wajsberg [:julienw]]

Thanks Pi-Wei, another question about this: what about a full-width chinese comma located before the link?

Comment 14

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

(In reply to Julien Wajsberg [:julienw] from comment #13)
> Thanks Pi-Wei, another question about this: what about a full-width chinese
> comma located before the link?

The issue reproduces (link does not become a link) with a full-width comma right before the link.

Comment 15

[Julien Wajsberg [:julienw]]

Pi Wei, I mean, is it expected, as a chinese user, that a link is detected when there is a full-width comma before?

Comment 16

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

(In reply to Julien Wajsberg [:julienw] from comment #15)
> Pi Wei, I mean, is it expected, as a chinese user, that a link is detected
> when there is a full-width comma before?

Oh I see. My opinion is no. A full width comma isn't more common than other full-width punctuation marks such as full width space, full width colon, full width parentheses... etc.

Comment 18

[Julien Wajsberg [:julienw]]

See dupe bug 1115583, this can be a real issue in the real world => requesting blocking status.

We have had this issue forever but with the Japan launch it's likely the first time we can have this issue with shipped devices, that's why I think it's worth fixing it in v2.1.

Comment 19

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

ni to Bhavana and Rachelle.
Bug 1115583 is a situation that might annoy Japanese users.
Can we live w/o this fix in 2.1?

Comment 20

[Rachelle Yang [:ryang][[e-mail]]]

Hi Wesley, so far we are fine.

Per comment 18 from Julien, I agreed with his point to solve this bug in 2.1.

Thank you very much!

Comment 21

[Brian Birtles (:birtles)]

Is this just a matter of updating the safeStart regexp to match full-width characters?
https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/link_helper.js#L61

Comment 22

[Julien Wajsberg [:julienw]]

Yes, maybe it's enough; I'm always wary when changing this algorithm, it took some time to get it right :)

Comment 23

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

triage: per triage meeting, it's a really annoying problem to user as bug 115583.
I'll leave the final call to RM as 2.1 is code complete already.

Comment 24

[bhavana bajaj [:bajaj]]

I agree with blocking on this per comment #18, but can we please get the lowest risk patch here to help with the issue rather than making more changes to the algorithm ?

Julienw, how risky do you estimate the patch to be, do we need to re-test a lot of messaging pieces with this change ? Lets land on on master/2.2 first and see how it plays out in terms of no fallouts etc before uplifting.

Comment 25

[Julien Wajsberg [:julienw]]

(In reply to bhavana bajaj [:bajaj] from comment #24)
> I agree with blocking on this per comment #18, but can we please get the
> lowest risk patch here to help with the issue rather than making more
> changes to the algorithm ?

Yes sure.


> Julienw, how risky do you estimate the patch to be, do we need to re-test a
> lot of messaging pieces with this change ? Lets land on on master/2.2 first
> and see how it plays out in terms of no fallouts etc before uplifting.

No, I think we only need to retest the algorithm itself, and it's already very well unit-tested. So very few risk of functional regressions. We need to carefully test the performance change though.

Comment 26

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Per comment 25 and comment 24.

Comment 27

[Steve Chung [:steveck]]

Attachment: 0001-Bug-1091511-Message-The-number-email-or-URL-can-t-be.patch

Hi Oleg, it's a small patch that treat all the non-ascii char as safe start, since it's not eazy to filter out all the full-width char here.

Comment 28

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8550222 [details] [diff] [review]
0001-Bug-1091511-Message-The-number-email-or-URL-can-t-be.patch

Review of attachment 8550222 [details] [diff] [review]:
-----------------------------------------------------------------

Works like a charm, thanks!

Comment 29

[Steve Chung [:steveck]]

Attachment: Link to github

Hi Oleg, I added 2 tests for the patch, but not sure if it's sufficient for the coverage(I didn't add for email/phone number case because the logic is the same). Since it's not possible to test every non-ascii char, maybe you will have better idea about this unit test.

Comment 30

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8551822 [details] [review]
Link to github

(In reply to Steve Chung [:steveck] from comment #29)
> Created attachment 8551822 [details] [review]
> Link to github
> 
> Hi Oleg, I added 2 tests for the patch, but not sure if it's sufficient for
> the coverage(I didn't add for email/phone number case because the logic is
> the same). Since it's not possible to test every non-ascii char, maybe you
> will have better idea about this unit test.

Nope, don't have better idea either, I think it's fine. I've left one small question related to full-width punctuation symbols at GitHub.

Thanks!

Comment 31

[Steve Chung [:steveck]]

Thanks!

In master: https://github.com/mozilla-b2g/gaia/commit/15279214de6ca99324ebd5ed7d2614b4d49be1da

Comment 32

[Steve Chung [:steveck]]

Comment on attachment 8551822 [details] [review]
Link to github

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Original behavior for URL link detection.
[User impact] if declined:URL could not detect as a valid link when full-width char is added in front of URL string.
[Testing completed]: Yes
[Risk to taking this patch] (and alternatives if risky): Low
[String changes made]: N/A

Comment 33

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/53700152899c573a0bec2fc2d5d96546c7a1317a
v2.1: https://github.com/mozilla-b2g/gaia/commit/9df43e6e696da43b6a1433283dc5b155987747bc

Comment 34

[Sue]

Attachment: 2015-03-19-15-00-15.png

This issue has been verified successfully on Flame 2.1/2.2/3.0 and 2.1S(512M), the number, email and URL in message content can be tapped.
See attachment:2015-03-19-15-00-15.png
Rate: 0/5

Flame 2.1 build: pass
Build ID               20150318001207
Gaia Revision          13c85d57f49b4bfd657ff674f2b530c141c94803
Gaia Date              2015-03-17 13:31:54
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/2fbd284621e2
Gecko Version          34.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150318.040035
Firmware Date          Wed Mar 18 04:00:46 EDT 2015
Bootloader             L1TC000118D0

2.1s_512mb: pass
Build ID               20150318161205
Gaia Revision          13c85d57f49b4bfd657ff674f2b530c141c94803
Gaia Date              2015-03-17 13:31:54
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/e64b1776894a
Gecko Version          34.0
Device Name            scx15_sp7715ea
Firmware(Release)      4.4.2
Firmware(Incremental)  122
Firmware Date          Thu Feb  5 12:42:58 CST 2015

Flame 2.2 build: pass
Build ID               20150318002504
Gaia Revision          306772a58335ac4cad285d27c3805090a8cc6886
Gaia Date              2015-03-17 17:12:36
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/a6f5f4035ea5
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150318.040534
Firmware Date          Wed Mar 18 04:05:45 EDT 2015
Bootloader             L1TC000118D0

Flame 3.0 build: pass
Build ID               20150318055750
Gaia Revision          b8051d370ddf4e5bd8e7d8a19fb9eeb5fd6ffb39
Gaia Date              2015-03-18 07:48:50
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/41a61514461e
Gecko Version          39.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150318.093634
Firmware Date          Wed Mar 18 09:36:44 EDT 2015
Bootloader             L1TC000118D0