988772 - [tarako][buri] sometimes contact index shows nothing when tap on a index

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Contacts, defect, P1)

Description

[Peipei Cheng (needinfo if you need my action)]

Attachment: STR video

* Steps to reproduce
1. Launch Contact
2. Quickly tap on contact index on the right side of Contacts page
   --> You will see sometimes the grey sqaure appears does not show anything on top of it. 

Please see attached video for details.


* Build info:
Gaia d8ff994bd96c37ba9a93c343932a5441a78a0eec
Gecko 6db37b3f76b4fe2aa6f8fb5ae9e036ed99344772
BuildID 20140327060053
Version 28.1
ro.build.version.incremental=69
ro.build.date=Thu Mar 27 06:07:07 CST 2014

Comment 1

[Joe Cheng [:jcheng] (please needinfo)]

triage: 1.3T+ partner blocker

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Steve, can you try to reproduce and see if there are any JS error in the logcat?

Can you loop Contacts owners too?

Thanks!

Comment 3

[Evelyn Hung]

Hey, peipei, may I know the reproduce rate? I tried to reproduce the issue in your video, but I can't successfully make once.

Comment 4

[Steve Chung [:steveck]]

This bug seems also reproducible in current master. It need to touch the area with 0.5cm distance from scroll bar and no contact list covered.

Comment 5

[Steve Chung [:steveck]]

Hi Francisco, it seems the touch event tartget didn't match the expected element queried by touch.pageX/pageY using document.elementFromPoint. For example touchstart event is triggered by touching the scroll bar, but elementFromPoint will return container div if we give touch.pageX/pageY as parameters, and that's why we sometimes shows empty shortcut. Using the touch.target may return the correct result than elementFromPoint, but sadly the tartget will not be changed by touch moves :(.

If we still rely on the elementFromPoint to fetch the element, how about we check the element when touchstart? If the elementFromPoint is not valid, we skip the scrolling setting and do nothing. Wdyt?

Comment 6

[Peipei Cheng (needinfo if you need my action)]

(In reply to Evelyn Hung [:evelyn] from comment #3)
> Hey, peipei, may I know the reproduce rate? I tried to reproduce the issue
> in your video, but I can't successfully make once.

Hi evelyn, if you tap on the "#" letter, you will see it everytime.

Comment 7

[Peipei Cheng (needinfo if you need my action)]

(In reply to pcheng from comment #6)
> (In reply to Evelyn Hung [:evelyn] from comment #3)
> > Hey, peipei, may I know the reproduce rate? I tried to reproduce the issue
> > in your video, but I can't successfully make once.
> 
> Hi evelyn, if you tap on the "#" letter, you will see it everytime.

And also you could see this happen if you tap on the left area the index (about 1cm on the left).

Comment 8

[Peipei Cheng (needinfo if you need my action)]

More comment:
Taping on the area between the "grey sqaure" and the index can consistently reproduce this problem.

Comment 9

[Steve Chung [:steveck]]

Attachment: 0001-Bug-988772-tarako-buri-sometimes-contact-index-shows.patch

Hey Jose/Francisco,
I have a quick patch for this issue. It solved 2 main problem in the shortcut scroll bar:
- The touched object is not the shortcut letter or icon.
- Reset lastY when touch end to prevent the next touch which is near the last touchend position shows nothing because the mechanism for reducing the element render. I also adjust the mechanism because the showing letter might be different from actual touched area.

There is no test case for contact shortcut now. If you think it's necessary to have a test case for this part, I can create a contact shortcut test for touch start/move/end part.

Comment 10

[Evelyn Hung]

@Thomas, can you help SPRD test Steve's WIP patch on comment 9?

Comment 12

[Steve Chung [:steveck]]

Comment on attachment 8402480 [details] [diff] [review]
0001-Bug-988772-tarako-buri-sometimes-contact-index-shows.patch

Hi Ben, are you available for the patch review? Thanks.

Comment 13

[Jose Manuel Cantera]

(In reply to Steve Chung [:steveck] from comment #9)
> Created attachment 8402480 [details] [diff] [review]
> 0001-Bug-988772-tarako-buri-sometimes-contact-index-shows.patch
> 
> Hey Jose/Francisco,
> I have a quick patch for this issue. It solved 2 main problem in the
> shortcut scroll bar:
> - The touched object is not the shortcut letter or icon.
> - Reset lastY when touch end to prevent the next touch which is near the
> last touchend position shows nothing because the mechanism for reducing the
> element render. I also adjust the mechanism because the showing letter might
> be different from actual touched area.
> 
> There is no test case for contact shortcut now. If you think it's necessary
> to have a test case for this part, I can create a contact shortcut test for
> touch start/move/end part.

Hi Steve, Yes a test would be very helpful . Could you also create a GH PR? It makes easier for us to review and provide feedback

thanks!

Comment 14

[Wayne Chen [:xwaynec]]

Hi James,

could you help to try attachment 8402480 [details] [diff] [review].
Thanks

Sincerely,
Wayne

Comment 15

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

Comment on attachment 8402480 [details] [diff] [review]
0001-Bug-988772-tarako-buri-sometimes-contact-index-shows.patch

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

Hi Steve!

Sorry for the delay on the answer, I've been on PTO for some days.
The solution looks good to me, also tried on the phone and works great thanks!

As Jose commented can you add a unit test, just for this case, even if there is no test for the other part, we will be adding more later.

Also, feel free to flag me for review.

Comment 16

[James Zhang (Spreadtrum)]

(In reply to Wayne Chen [:xwaynec] from comment #14)
> Hi James,
> 
> could you help to try attachment 8402480 [details] [diff] [review].
> Thanks
> 
> Sincerely,
> Wayne

Loop Yang.Zhao

Comment 17

[yang.zhao]

(In reply to James Zhang from comment #16)
> (In reply to Wayne Chen [:xwaynec] from comment #14)
> > Hi James,
> > 
> > could you help to try attachment 8402480 [details] [diff] [review].
> > Thanks
> > 
> > Sincerely,
> > Wayne
> 
> Loop Yang.Zhao

I've tried attachment 8402480 [details] [diff] [review].It seems works.

Comment 18

[Steve Chung [:steveck]]

Attachment: Link to github

Hey Francisco, I create a pr for the patch with integration test for scrolling behavior. It seems a little bit weird to verify it with unit test, but please ping me if you got another thought for the testing, thanks!

Comment 19

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

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

Hi Steve!

Solution works great! And I do agree that for testing this is better an integration test, but if you check travis you'll realise that the test is failing.

Could you take a look?

(Just clean the review flag, add me again when ready, will directly r+)

Thanks for the great work!

Comment 20

[Steve Chung [:steveck]]

I fix some integration test problem... test case works fine on device but failed on b2g desktop. Some timing issue is fixed in the original pr but need to wait for Travis to verify again.

Comment 21

[Steve Chung [:steveck]]

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

Hi Francisco, the Travis is green finally. It took time to figure out what's going on for the integration test failed...

Comment 22

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

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

\o/

Great work Steve!

Thanks a lot.

Comment 23

[Steve Chung [:steveck]]

in master: 0a0f087a809f6fac5caa6fe4d7bbf3536d8c246a

Comment 24

[Jose Manuel Cantera]

Comment on attachment 8402480 [details] [diff] [review]
0001-Bug-988772-tarako-buri-sometimes-contact-index-shows.patch

Hi Steve,

f+ but I left some comments on GH. 

Many thanks and good work!

Comment 25

[Steve Chung [:steveck]]

Attachment: Link to github

Hi Jose, this follow-up patch stressed your suggestion, thanks for the feedback!

Comment 26

[Jose Manuel Cantera]

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

looks good

thanks Steve!

Comment 27

[ying.xu]

v1.3t
https://github.com/mozilla-b2g/gaia/commit/aca7556f3b11c6bcb5eff2993b3ae09a1875fe85

Comment 28

[Steve Chung [:steveck]]

Follow up patch : a60098ff726f23cec356a234ad7ffaad5f1894e8

Comment 29

[Ivan Tsay (:ITsay)]

Hi Preeti,

Need to get release management approval for uplifting this fix to v1.4.

Comment 30

[Ivan Tsay (:ITsay)]

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

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined:
[Testing completed]:
[Risk to taking this patch] (and alternatives if risky):
[String changes made]:

Comment 31

[Ivan Tsay (:ITsay)]

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Found from partner PTR test
[User impact] if declined: Partner blocker for Dolphin PTR test.
[Testing completed]: Yes
[Risk to taking this patch] (and alternatives if risky): Don't see obvious risk on this 
[String changes made]:No change

Comment 32

[Preeti Raghunath(:Preeti)]

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

Partner ask hence taking in 1.4

Comment 33

[Ryan VanderMeulen [:RyanVM]]

v1.4: https://github.com/mozilla-b2g/gaia/commit/0655b83619e34f3a619e93fc5ca66f501d8e6cf0