Closed Bug 1033937 Opened 9 years ago Closed 9 years ago

[Calllog] Tap on Withheld number in call log, item highlighted, but stay highlighted when finger released

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:-, b2g-v1.3 unaffected, b2g-v1.4 affected, b2g-v2.0 fixed, b2g-v2.1 verified)

VERIFIED FIXED
2.1 S1 (1aug)
blocking-b2g -
Tracking Status
b2g-v1.3 --- unaffected
b2g-v1.4 --- affected
b2g-v2.0 --- fixed
b2g-v2.1 --- verified

People

(Reporter: ericcc, Assigned: paco, NeedInfo)

References

Details

(Whiteboard: [planned-sprint])

Attachments

(5 files)

### STR
1. Make a call(hide number) to Flame.
2. Tap on that Withheld number in call log.

### Actual 
Item highlighted, but stay highlighted when finger released.

### Expected
Not highlighted.

### Version
Flame v2.0 aurora
Gaia      90605754e9bdbe20f3999522f9e1aec600c422a4
Gecko     https://hg.mozilla.org/releases/mozilla-aurora/rev/0bffc7e5d8a2
BuildID   20140702160207
Version   32.0a2
ro.build.version.incremental=109
ro.build.date=Mon Jun 16 16:51:29 CST 2014
t2m.sw.version=B1TC00011220
blocking-b2g: --- → 2.0?
Paco, can you have a look at it?
Flags: needinfo?(pacorampas)
Assignee: nobody → pacorampas
Flags: needinfo?(pacorampas)
triage: not a release blocker
blocking-b2g: 2.0? → -
Just adding another scenario that Wesley has pointed out during the triage and seems to be a similar issue:

1. Make a call to a contact included in the Agenda
2. Long press on the entry in the Call log so we go to the contact details
3. Come back to the call log pressing back button "<"

### Actual 
Entry highlighted

### Expected
Not highlighted
Attached file patch in github
Comment on attachment 8451554 [details] [review]
patch in github

>https://github.com/mozilla-b2g/gaia/pull/21445
Attachment #8451554 - Attachment description: pathc → patch in github
Target Milestone: --- → 2.0 S6 (18july)
Whiteboard: [planned-sprint]
Comment on attachment 8451554 [details] [review]
patch in github

Hello,

I have a issue on this bug. With this patch the withheld numbers don't highlight. Also, when you were doing scroll the items are highlighted and when you stop the scroll, the last item was highlighted, this patch prevent this.

The issue happens with the postulation on comment 3. If you do long press on contact saved the contact profile is opened, when you came back the highlight doesn't disappear. Is like that the active status is blocking there. I think this could happen because the contact is opened in iframe. Some solution for deleting the active status?
Attachment #8451554 - Flags: review?(anthony)
Comment on attachment 8451554 [details] [review]
patch in github

Passing on to Doug since he knows more about those CSS states matching.

Paco: If you know a patch is not ready to land but you'd like feedback on it, please use the feedback? flag. Thanks.
Attachment #8451554 - Flags: review?(anthony) → feedback?(drs+bugzilla)
Attached patch WorkaroundSplinter Review
There's an additional problem here. If you long press a "withheld number" item and then move your finger away from it, the highlighting sticks. This is an APZ problem though, so I've filed bug 1038002.

(In reply to Paco Rampas [:paco] from comment #6)
> I have a issue on this bug. With this patch the withheld numbers don't
> highlight. Also, when you were doing scroll the items are highlighted and
> when you stop the scroll, the last item was highlighted, this patch prevent
> this.

What's the point of the new selectors that you added? The removal of the hover selectors seemed to fix it for me.

I'm not really happy about this removal, though. This will break Mulet's mouse hovering. I can't think of a good alternative, though.

> The issue happens with the postulation on comment 3. If you do long press on
> contact saved the contact profile is opened, when you came back the
> highlight doesn't disappear. Is like that the active status is blocking
> there. I think this could happen because the contact is opened in iframe.
> Some solution for deleting the active status?

I've run into this before, and it's a nightmare to fix. I've attached a patch that works around this. As far as I know, it's the only thing you can do that will work.

This actually isn't because of the iframe. It's because we're preventDefaulting the contextmenu event. The W3 touch event spec states the following:

(From http://www.w3.org/TR/2011/WD-touch-events-20110505/#mouse-events)
> The user agent may dispatch both touch events and mouse events
> [DOM-LEVEL-2-EVENTS] in response to the same user input. If the
> user agent dispatches both touch events and mouse events in
> response to a single user action, then the touchstart event type
> must be dispatched before any mouse event types for that action.
> If the preventDefault method of touchstart or touchmove is called,
> the user agent should not dispatch any mouse event that would be
> a consequential result of the the prevented touch event.

States in the DOM such as :hover and :active are triggered entirely with mouse events, even when using a touch device. The Gecko touch event code spoofs equivalent mouse events, which are used for things such as clicking on links and highlighting. I think what's happening is we're not getting a mouseup event after calling preventDefault, which the lack of would prevent us from removing the :active and :hover states.
Comment on attachment 8451554 [details] [review]
patch in github

f+ since this still needs a lot of work, though it's going in the right direction.
Attachment #8451554 - Flags: feedback?(drs+bugzilla) → feedback+
(In reply to Doug Sherk (:drs) from comment #8)
> States in the DOM such as :hover and :active are triggered entirely with
> mouse events, even when using a touch device. The Gecko touch event code
> spoofs equivalent mouse events, which are used for things such as clicking
> on links and highlighting. I think what's happening is we're not getting a
> mouseup event after calling preventDefault, which the lack of would prevent
> us from removing the :active and :hover states.

Oh, also, note that the spec is referring to events dispatched by the UA. What we're doing internally can be different. But a lot of the time the platform code sees looks very similar to what the UA ends up seeing. In this case, without doing further investigation, this is what I suspect is happening.
Doug, your workaround patch together with removing :hover states in lists Building blocks works great.
Arnau has already reviewed the shared part and works for him.

Should I just send your proposed patch + the BB thing for review? or do you want to further iterate your patch?
Flags: needinfo?(drs+bugzilla)
(In reply to Paco Rampas [:paco] from comment #11)
> Doug, your workaround patch together with removing :hover states in lists
> Building blocks works great.
> Arnau has already reviewed the shared part and works for him.
> 
> Should I just send your proposed patch + the BB thing for review? or do you
> want to further iterate your patch?

I think the workaround is so bad that I'd rather we just landed the :hover selector removal and live with the bug that the workaround fixes. But if you want to put it into review anyways, I would suggest adding a comment above it explaining why we're doing it, and referring to this bug.
Flags: needinfo?(drs+bugzilla)
Comment on attachment 8451554 [details] [review]
patch in github

I have left the patch like I had done it before as Doug said in comment 12. Therefore, the issue is there but we think is the best option.
Attachment #8451554 - Flags: review?(anthony)
Status: NEW → ASSIGNED
(In reply to Anthony Ricaud (:rik) from comment #14)
> Isn't the proper fix here to preventDefault in
> https://github.com/mozilla-b2g/gaia/blob/
> 3aff04248c90bddf65d5227da9682c6129be0f22/apps/communications/dialer/js/
> call_log.js#L660-L663 ?

No.
See Also: → 1035183
guys, make sense if we split this patch into two?
I could review the BB part (removing the :hover) as is very annoying seeing the blue background going under the sticky header.
I could review that part in an new bug, and you could keep this one for the js part.
Comment on attachment 8451554 [details] [review]
patch in github

(In reply to Arnau March  [:arnau] from comment #16)
> guys, make sense if we split this patch into two?
> I could review the BB part (removing the :hover) as is very annoying seeing
> the blue background going under the sticky header.
> I could review that part in an new bug, and you could keep this one for the
> js part.

Unless someone else wants to land the JS part (I don't), I think we could keep everything here. I'm adding you on review for the CSS part since I don't think Anthony is a peer there.
Attachment #8451554 - Flags: review?(rnowmrch)
Comment on attachment 8451554 [details] [review]
patch in github

The shared part LGTM ;)
Thanks!
Attachment #8451554 - Flags: review?(rnowmrch) → review+
Attachment #8451554 - Flags: review?(anthony) → review+
Merged in master: https://github.com/mozilla-b2g/gaia/commit/479199c96fecf43216c89f287ae0c5e624ad9e08
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Paco, please contact Mari Ángeles in case asking for approval to land the patch in v2.0 and v1.4 is needed ;) Thanks!
Flags: needinfo?(pacorampas)
Flags: needinfo?(oteo)
Too late for v1.4, and not sure if we are in time for 2.0, let me confirm with Jason if we can still ask for approval to 2.0 for this type of bugs.
Flags: needinfo?(oteo)
Comment on attachment 8451554 [details] [review]
patch in 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 #): Building Blocks lists
[User impact] if declined: incorrect UI
[Testing completed]: yes
[Risk to taking this patch] (and alternatives if risky): no risk
[String changes made]: none
Attachment #8451554 - Flags: approval-gaia-v2.0?
Flags: needinfo?(pacorampas)
Attachment #8451554 - Flags: approval-gaia-v2.0? → approval-gaia-v2.0+
Verified okay
Flame v2.1 Master
Gaia      15c84c943e41ad834640a45e1e1c2ac804168af7
Gecko     https://hg.mozilla.org/mozilla-central/rev/30907d52c4c2
BuildID   20140723160203
Version   34.0a1
ro.build.version.incremental=109
ro.build.date=Mon Jun 16 16:51:29 CST 2014
B1TC00011220

Flame v2.0 Aurora
Gaia      91986777d0942b63e37fbfeec19d69d6176d6d74
Gecko     https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/392054b2e899
BuildID   20140723160206
Version   32.0
ro.build.version.incremental=109
ro.build.date=Mon Jun 16 16:51:29 CST 2014
B1TC00011220
Status: RESOLVED → VERIFIED
Attached video verify_video.MP4
This issue has been verified successfully on Flame v2.1
See attachment: verify_video.MP4
Flame 2.1 reproducing rate: 0/5
Flame 2.1 versions:
Gaia-Rev        5655269098c7e82254e56933f1af05b4abe2a2f3
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/86608c9389b5
Build-ID        20141204001201
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141204.034958
FW-Date         Thu Dec  4 03:50:09 EST 2014
Bootloader      L1TC00011880

This issue has been verified unsuccessfully on Flame v2.0
STR:
1. Launch Phone app.
2. Made a call to a saved contacts.
3. Long press at the Withheld number in call log.
4. Press back key back to call list.
**There is a highlight with the Withheld number in call log.
See attachment: verify_video.MP4 and logcat_v2.0.txt
Flame 2.0 reproducing rate: 3/5
Flame 2.0 versions:
Gaia-Rev        8d1e868864c8a8f1e037685f0656d1da70d08c06
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/ff1100ba2ab8
Build-ID        20141204000228
Version         32.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141204.040425
FW-Date         Thu Dec  4 04:04:36 EST 2014
Bootloader      L1TC00011880
Attached file logcat of flame 2.0
Hi Hubert,
Could you help with it, thanks.
Flags: needinfo?(hlu)
You need to log in before you can comment on or make changes to this bug.