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

Status: VERIFIED FIXED

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

Description

[Eric Chang [:ericcc] [:echang]]

Attachment: CalllogWithheldNumber.png

### 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

Comment 1

[Maria Angeles Oteo (:oteo)]

Paco, can you have a look at it?

Comment 2

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

triage: not a release blocker

Comment 3

[Maria Angeles Oteo (:oteo)]

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

Comment 4

[Paco Rampas [:paco]]

Attachment: patch in github

Comment 5

[Paco Rampas [:paco]]

Comment on attachment 8451554 [details] [review]
patch in github

>https://github.com/mozilla-b2g/gaia/pull/21445

Comment 6

[Paco Rampas [:paco]]

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?

Comment 7

[Anthony Ricaud (:rik)]

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.

Comment 8

[Doug Sherk (:drs) (inactive)]

Attachment: Workaround

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 9

[Doug Sherk (:drs) (inactive)]

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.

Comment 10

[Doug Sherk (:drs) (inactive)]

(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.

Comment 11

[Paco Rampas [:paco]]

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?

Comment 12

[Doug Sherk (:drs) (inactive)]

(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.

Comment 13

[Paco Rampas [:paco]]

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.

Comment 14

[Anthony Ricaud (:rik)]

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 ?

Comment 15

[Doug Sherk (:drs) (inactive)]

(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.

Comment 16

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

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 17

[Doug Sherk (:drs) (inactive)]

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.

Comment 18

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

Comment on attachment 8451554 [details] [review]
patch in github

The shared part LGTM ;)
Thanks!

Comment 19

[Germán Toro del Valle (:gtorodelvalle)]

Merged in master: https://github.com/mozilla-b2g/gaia/commit/479199c96fecf43216c89f287ae0c5e624ad9e08

Comment 20

[Germán Toro del Valle (:gtorodelvalle)]

Paco, please contact Mari Ángeles in case asking for approval to land the patch in v2.0 and v1.4 is needed ;) Thanks!

Comment 21

[Maria Angeles Oteo (: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.

Comment 22

[Paco Rampas [:paco]]

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

Comment 23

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/50badd22dfc110a2fcae225d7f205676e7b4252a

Comment 24

[Eric Chang [:ericcc] [:echang]]

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

Comment 25

[Sue]

Attachment: 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

Comment 26

[Sue]

Attachment: logcat of flame 2.0

Hi Hubert,
Could you help with it, thanks.