Closed Bug 988772 Opened 6 years ago Closed 6 years ago

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

Categories

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

Other
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:1.3T+, b2g-v1.3 wontfix, b2g-v1.3T fixed, b2g-v1.4 fixed, b2g-v2.0 fixed)

RESOLVED FIXED
1.4 S5 (11apr)
blocking-b2g 1.3T+
Tracking Status
b2g-v1.3 --- wontfix
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: angelc04, Assigned: steveck)

References

Details

(Whiteboard: [sprd294195])

Attachments

(4 files)

Attached video 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
blocking-b2g: --- → 1.3T?
triage: 1.3T+ partner blocker
blocking-b2g: 1.3T? → 1.3T+
Priority: -- → P1
Whiteboard: [priority]
Steve, can you try to reproduce and see if there are any JS error in the logcat?

Can you loop Contacts owners too?

Thanks!
Assignee: nobody → schung
Flags: needinfo?(schung)
Severity: normal → blocker
Hey, peipei, may I know the reproduce rate? I tried to reproduce the issue in your video, but I can't successfully make once.
Flags: needinfo?(pcheng)
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.
Flags: needinfo?(schung)
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?
Flags: needinfo?(francisco.jordano)
(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.
Flags: needinfo?(pcheng)
(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).
More comment:
Taping on the area between the "grey sqaure" and the index can consistently reproduce this problem.
Summary: [tarako] sometimes contact index shows nothing when tap on a index → [tarako][buri] sometimes contact index shows nothing when tap on a index
Whiteboard: [priority] → [priority][MP_Blocker]
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.
Attachment #8402480 - Flags: feedback?(jmcf)
Attachment #8402480 - Flags: feedback?(francisco.jordano)
@Thomas, can you help SPRD test Steve's WIP patch on comment 9?
Flags: needinfo?(ttsai)
Whiteboard: [priority][MP_Blocker] → [priority]
Whiteboard: [priority] → [priority] eta:4/11
Duplicate of this bug: 945538
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.
Flags: needinfo?(bkelly)
(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!
Hi James,

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

Sincerely,
Wayne
Flags: needinfo?(james.zhang)
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.
Attachment #8402480 - Flags: feedback?(francisco.jordano) → feedback+
Flags: needinfo?(francisco.jordano)
Flags: needinfo?(bkelly)
(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
Flags: needinfo?(james.zhang) → needinfo?(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.
Flags: needinfo?(yang.zhao)
Attached file 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!
Attachment #8403903 - Flags: review?(francisco.jordano)
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!
Attachment #8403903 - Flags: review?(francisco.jordano)
Whiteboard: [priority] eta:4/11 → eta:4/11
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 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...
Attachment #8403903 - Flags: review?(francisco.jordano)
Comment on attachment 8403903 [details] [review]
Link to github

\o/

Great work Steve!

Thanks a lot.
Attachment #8403903 - Flags: review?(francisco.jordano) → review+
Flags: in-testsuite+
in master: 0a0f087a809f6fac5caa6fe4d7bbf3536d8c246a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
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!
Attachment #8402480 - Flags: feedback?(jmcf) → feedback+
Flags: needinfo?(ttsai)
Attached file Link to github
Hi Jose, this follow-up patch stressed your suggestion, thanks for the feedback!
Attachment #8405255 - Flags: review?(jmcf)
Comment on attachment 8405255 [details] [review]
Link to github

looks good

thanks Steve!
Attachment #8405255 - Flags: review?(jmcf) → review+
Follow up patch : a60098ff726f23cec356a234ad7ffaad5f1894e8
Hi Preeti,

Need to get release management approval for uplifting this fix to v1.4.
Flags: needinfo?(praghunath)
Whiteboard: eta:4/11 → [eta:4/11][1.4-approval-needed]
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]:
Attachment #8403903 - Flags: approval-gaia-v1.4?(praghunath)
[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 on attachment 8403903 [details] [review]
Link to github

Partner ask hence taking in 1.4
Attachment #8403903 - Flags: approval-gaia-v1.4?(praghunath) → approval-gaia-v1.4+
Flags: needinfo?(praghunath)
v1.4: https://github.com/mozilla-b2g/gaia/commit/0655b83619e34f3a619e93fc5ca66f501d8e6cf0
Whiteboard: [eta:4/11][1.4-approval-needed]
Target Milestone: --- → 1.4 S5 (11apr)
Whiteboard: [sprd294195]
You need to log in before you can comment on or make changes to this bug.