1090682 - [Contacts]The delete function in contact details view does not work.

Status: VERIFIED FIXED

(Core :: DOM: Selection, defect, P2)

Description

[Sue]

Attachment: screenshot

[1.Description]:
When you delete a contact from contact details view, the delete button colour will turn faint, but it can not be deleted successfully.
See attachment(logcat_0955 and 0955.png)
Found time:09:55

[2.Testing Steps]: 
1. Launch Contacts.
2. Tap a contact to check details.
3. Tap edit icon at right top.
4. Slide down and tap delete button. 

[3.Expected Result]: 
4. The contact should be delete.

[4.Actual Result]: 
4. There is no response when you tap delete button.

[5.Reproduction build]: 
Gaia-Rev        86d83f4b4111ca45ebc92ca779348cc966f43cff
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/f8432250efb7
Build-ID        20141023160201
Version         32.0

[6.Reproduction Frequency]: 
occasionally Recurrence,3/10

Comment 1

[Sue]

Attachment: logcat

Comment 2

[Sue]

Attachment: logcat_v2.2_1809.txt

When long press at "Delete contact" button, sometimes, there is no response.
See attachment: logcat_v2.2_1809.txt and video.MP4

Comment 3

[Sue]

Attachment: video.MP4

Comment 4

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

Nominating for 2.2 as it's a mayor feature broken

Comment 5

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

Just tried on master and cannot reproduce, Johan can you take a look?

Comment 6

[Johan Lorenzo [:jlorenzo]]

I triggered that bug on the first try by long pressing the delete button like Sue did on the video. I haven't succeeded to reproduce the problem on 2.1. We'll need a regression window here.

Comment 7

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

Yes the key to repro is to "long press" on the Delete contact button. 2.2 is affected and 2.1 is unaffected. Working on getting the window now.

Comment 8

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

mozilla-inbound regression window:

Last Working Environmental Variables:
Device: Flame
BuildID: 20141102230636
Gaia: c2c55870de22d596de4f41612c0b44090f90ebad
Gecko: 14f544cf024a
Version: 36.0a1 (2.2 Master)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

First Broken Environmental Variables:
Device: Flame
BuildID: 20141103000037
Gaia: c2c55870de22d596de4f41612c0b44090f90ebad
Gecko: 667a5bff1599
Version: 36.0a1 (2.2 Master)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

Gaia is the same so it's a Gecko issue.

Gecko pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=14f544cf024a&tochange=667a5bff1599

Comment 9

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

Not really sure what could be the cause of this issue, but we did verify the Last working build with addition checks.  

Johan do you have any opinions on the pushlog on comment 8?

Comment 10

[Johan Lorenzo [:jlorenzo]]

Triage: Visible regression in 2.2 which might lead a user to think that the functionality is broken.

Comment 11

[Johan Lorenzo [:jlorenzo]]

This might be related to bug 1029943, what do you think Ting-Yu?

Comment 12

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Yes. It relates to bug 1029943. SelectionCarets::SelectWord() consumes long press event even if it doesn't  select a word. SelectionCarets shouldn't consume it in this case.

Comment 13

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Do not consume NS_MOUSE_MOZLONGTAP if SelectWord failed. f=mtseng, r=roc (v1)

Comment 14

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8540597 [details] [diff] [review]
Do not consume NS_MOUSE_MOZLONGTAP if SelectWord failed. f=mtseng, r=roc (v1)

Nice!

Comment 15

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Do not consume NS_MOUSE_MOZLONGTAP if SelectWord failed. f=mtseng, r=roc (v2)

Rebase and update comments.

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4fe0037f05c9

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8541106 [details] [diff] [review]
Do not consume NS_MOUSE_MOZLONGTAP if SelectWord failed. f=mtseng, r=roc (v2)

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

::: layout/base/SelectionCarets.cpp
@@ +554,2 @@
>    if (!mPresShell) {
> +    return succeeded;

For success results, you should keep using nsresult, but you should return a failure code here since it failed.

Comment 17

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Do not consume NS_MOUSE_MOZLONGTAP if SelectWord failed. f=mtseng, r=roc (v3)

Thanks, roc. Addressed your comments by returning nsresult.

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b4ee66696e3

Comment 18

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8541377 [details] [diff] [review]
Do not consume NS_MOUSE_MOZLONGTAP if SelectWord failed. f=mtseng, r=roc (v3)

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

::: layout/base/SelectionCarets.cpp
@@ +264,5 @@
>      if (!mVisible) {
>        SELECTIONCARETS_LOG("SelectWord from APZ");
> +      nsresult wordSelected = SelectWord();
> +
> +      if (NS_FAILED(wordSelected)) {

Should we call SelectWord() directly here? Looks like 'wordSelected' never be used.

@@ +1210,5 @@
>  
>    SELECTIONCARETS_LOG_STATIC("SelectWord from non-APZ");
> +  nsresult wordSelected = self->SelectWord();
> +
> +  if (NS_FAILED(wordSelected)) {

Ditto.

Comment 19

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8541377 [details] [diff] [review]
Do not consume NS_MOUSE_MOZLONGTAP if SelectWord failed. f=mtseng, r=roc (v3)

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

(In reply to Morris Tseng [:mtseng] from comment #18)
> Comment on attachment 8541377 [details] [diff] [review]
> Do not consume NS_MOUSE_MOZLONGTAP if SelectWord failed. f=mtseng, r=roc (v3)
> 
> Review of attachment 8541377 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/SelectionCarets.cpp
> @@ +264,5 @@
> >      if (!mVisible) {
> >        SELECTIONCARETS_LOG("SelectWord from APZ");
> > +      nsresult wordSelected = SelectWord();
> > +
> > +      if (NS_FAILED(wordSelected)) {
> 
> Should we call SelectWord() directly here? Looks like 'wordSelected' never
> be used.

I would like to make the return value meaningful and explicit. Thanks.

Comment 20

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Try run looks good.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cfab0e8ec399

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/597b1585ab72

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/597b1585ab72

Comment 23

[Sue]

Attachment: verify_v2.2.MP4

This issue is verified not happen in latest build of Flame 2.2

See attachment:verify_v2.2.MP4
Flame 2.2 build:
Gaia-Rev        69ac77cfa938fae2763ac426a80ca6e5feb6ad25
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/33781a3a5201
Build-ID        20150107010216
Version         37.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150107.044401
FW-Date         Wed Jan  7 04:44:12 EST 2015
Bootloader      L1TC000118D0

Comment 24

[Sue]

This issue still exist on Woodduck 2.0M and Flame 2.0

Flame 2.0 build:
Gaia-Rev        f76014fd2c7528493b90d759c68ec3070233d094
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/53ff92e647a0
Build-ID        20150107000201
Version         32.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150107.033038
FW-Date         Wed Jan  7 03:30:49 EST 2015
Bootloader      L1TC000118D0

Woodduck build:
Gaia-Rev        
06b544f8a9ee03754dd3f5020f279cffa7a75804
Gecko-Rev       6b1e24a1cba1d5542bebb503ce05a1ec61ceb7d3
Build-ID        20150108050313
Version         32.0
Device-Name     jrdhz72_w_ff
FW-Release      4.4.2
FW-Incremental  1420664769
FW-Date         Thu Jan  8 05:06:39 CST 2015

Comment 25

[Jose Manuel Cantera]

I think we should ask for an approval b2g32 and b2g34. Ting?

Comment 26

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Uplifting my patch to v2.0 and v2.1 branch will not help.  Since the copy & paste feature (TouchCaret and SelectionCarets) does not being enabled on those branches, the code does not affect the behavior. 

Without my patch, nearly all the buttons do not work if the user long press on them such as the following steps in description. 
2. Tap a contact to check details.
3. Tap edit icon at right top.

If they are working on v2.0 or v2.1, it might be other issue.

Comment 27

[Johan Lorenzo [:jlorenzo]]

I checked on the same build as you on 2.0, I was able to repro.

The difference between 2.2 is that the button keep working if you press again after long pressing on it. It wasn't the case when the text got selected in 2.2.

Sue, as the issue has different causes in 2.0 and 2.2, would you mind opening a new bug for 2.0 with a logcat on this branch?

Clearing the woodduck-confidential group as the bug occurred on the Flame.

Comment 28

[Sue]

(In reply to Johan Lorenzo [:jlorenzo] (QA) from comment #27)
> I checked on the same build as you on 2.0, I was able to repro.
> 
> The difference between 2.2 is that the button keep working if you press
> again after long pressing on it. It wasn't the case when the text got
> selected in 2.2.
> 
> Sue, as the issue has different causes in 2.0 and 2.2, would you mind
> opening a new bug for 2.0 with a logcat on this branch?
> 
> Clearing the woodduck-confidential group as the bug occurred on the Flame.

Hi Johan,
I have opened a new bug for Flame 2.0, see Bug 1119668