1019359 - [Sora][Message] The "slecet all/deselect all"still can use when it display as grey.

Status: RESOLVED FIXED

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

Description

[sync-1]

Created an attachment (id=756502)
 pic
 
 DEFECT DESCRIPTION:
 ->If a message under the  "slecet all/deselect all" item,tap the grey selection,MS will select a message
 
  REPRODUCING PROCEDURES:
 ->There are some messages in MS.
 ->Enter"Message",Select a thread to enter;
 ->In conversation list,tap the "option"icon select"Delete messages";
 ->If a message under the  "slecet all/deselect all" item,tap the grey selection ,MS will select the message.(ko)
  EXPECTED BEHAVIOUR:
 ->Shouldn't select the message.
 
  ASSOCIATE SPECIFICATION:
 
  TEST PLAN REFERENCE:
 
  TOOLS AND PLATFORMS USED:
 
  USER IMPACT:
 
  REPRODUCING RATE:
 
  For FT PR, Please list reference mobile's behavior:

Comment 1

[sync-1]

Attachment: pic

Comment 2

[田旻]

The build I used to test is Firefox OS v1.3.
Mozilla build ID:20140422024003

Comment 3

[Julien Wajsberg [:julienw]]

Can you please do a video ?

Comment 4

[田旻]

(In reply to Julien Wajsberg [:julienw] from comment #3)
> Can you please do a video ?

Video is too big for Mozilla bugzilla's attachment limitation. But I will add some pictures to attachment and tell you how to reproduce.

Comment 5

[田旻]

Attachment: reproduce－step1

Comment 6

[田旻]

Attachment: reproduce－step2

Comment 7

[Julien Wajsberg [:julienw]]

I think that what happens is that the event fluffing kicks in here: it tries to find the closest most likely target, and in that case, it's the text message.

So, now, there probably is some way to ignore this (for example, add a click handler on the button and call preventDefault), but do we really want it?

In my own testing I couldn't find it to be a real problem (on Buri / master). It seems to happen a little more on v1.4 (tried on a Peak with a slightly old build).

But here is what happens, there are basically 2 STR (for each buttons):

STR1:
1. nothing is selected
2. user clicks near disabled "deselect all"
3. as a result, an item is selected
=> only one item is selected, "deselect all" is now actionable
=> the user can simply tap the close "deselect all" to unselect it.

STR2:
1. everything is selected
2. user clicks near disabled "select all"
3. as a result, an item is deselected
=> only one item is deselect, "select all" is now actionable
=> the user can tap the close "select all" to select it again

So I'm tempted to close WONTFIX.

Comment 8

[田旻]

This patch can fix this problem.
---
 apps/sms/js/thread_list_ui.js |   11 +++++++++++++
 apps/sms/js/thread_ui.js      |   12 +++++++++++++-
 2 files changed, 22 insertions(+), 1 deletions(-)

diff --git a/apps/sms/js/thread_list_ui.js b/apps/sms/js/thread_list_ui.js
index f85e19e..efe6810 100644
--- a/apps/sms/js/thread_list_ui.js
+++ b/apps/sms/js/thread_list_ui.js
@@ -141,6 +141,19 @@ var ThreadListUI = {
         // Duck type determination; if the click event occurred on
         // a target with a |type| property, then assume it could've
         // been a checkbox and proceed w/ validation condition
+
+        var height = document.querySelector('#messages-edit-form > menu');
+        for (var t = height.offsetTop; height = height.offsetParent;) {
+          t += height.offsetTop;
+        }
+        if (evt.clientY > t) {
+          evt.stopPropagation();
+          evt.preventDefault();
+          break;
+        }
+
         if (evt.target.type && evt.target.type === 'checkbox') {
           ThreadListUI.checkInputs();
         }
diff --git a/apps/sms/js/thread_ui.js b/apps/sms/js/thread_ui.js
index b8d6d17..cef6258 100644
--- a/apps/sms/js/thread_ui.js
+++ b/apps/sms/js/thread_ui.js
@@ -1896,8 +1896,20 @@ var ThreadUI = global.ThreadUI = {
           }
           return;
         }
-
         var input = evt.target.parentNode.querySelector('input');
+
+        var height = document.querySelector('#messages-edit-form > menu');
+        for (var t = height.offsetTop; height = height.offsetParent;) {
+          t += height.offsetTop;
+        }
+        if (evt.clientY > t) {
+          evt.stopPropagation();
+          evt.preventDefault();
+          break;
+        }
+
         if (input) {
           this.chooseMessage(input);
           this.checkInputs();
--

Comment 9

[Julien Wajsberg [:julienw]]

If there is an issue in the event fluffing code, this needs to be fixed at the event fluffing level.

Comment 10

[Julien Wajsberg [:julienw]]

Note that this could also probably be fixed by simply adding a click event listener on "menu" that would just do nothing (then the event fluffing code would chose this one instead of the label).

Comment 11

[田旻]

 this.container.addEventListener(
      'click', this.handleEvent.bind(this), true // Do you mean  useCapture should be true?
    );

this.container.addEventListener(
      'click', this, true // Do you mean  useCapture should be true?
    );

Comment 12

[Julien Wajsberg [:julienw]]

No, I mean, something like:

document.querySelector('#messages-edit-form > menu').addEventListener('click', function() {});

Comment 13

[田旻]

From 767ae8b262b57a3a6f54c870eb4e76968383f220 Mon Sep 17 00:00:00 2001
From: repo sync <[e-mail]>
Date: Tue, 10 Jun 2014 15:45:33 +0800
Subject: [PATCH] BUG#679650_modified for the button and checkbox mistake

Change-Id: Ic8db381fc31e12ec53895322c24f9381a9f3c539
---
 apps/sms/js/thread_list_ui.js |   15 ++++++++++++++-
 apps/sms/js/thread_ui.js      |   18 +++++++++++++++---
 2 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/apps/sms/js/thread_list_ui.js b/apps/sms/js/thread_list_ui.js
index f85e19e..308f7f8 100644
--- a/apps/sms/js/thread_list_ui.js
+++ b/apps/sms/js/thread_list_ui.js
@@ -56,7 +56,7 @@ var ThreadListUI = {
     );
 
     this.container.addEventListener(
-      'click', this
+      'click', this, true  //tcl_tianmin modified for bug#679650
     );
 
     this.editForm.addEventListener(
@@ -141,6 +141,19 @@ var ThreadListUI = {
         // Duck type determination; if the click event occurred on
         // a target with a |type| property, then assume it could've
         // been a checkbox and proceed w/ validation condition
+
+        //tcl_tianmin modified for bug#679650 begin
+        var height = document.querySelector('#messages-edit-form > menu');
+        for (var t = height.offsetTop; height = height.offsetParent;) {
+          t += height.offsetTop;
+        }
+        if (evt.clientY > t) {
+          evt.stopPropagation();
+          evt.preventDefault();
+          break;
+        }
+        //tcl_tianmin modified for bug#679650 end
+
         if (evt.target.type && evt.target.type === 'checkbox') {
           ThreadListUI.checkInputs();
         }
diff --git a/apps/sms/js/thread_ui.js b/apps/sms/js/thread_ui.js
index b8d6d17..18c4710 100644
--- a/apps/sms/js/thread_ui.js
+++ b/apps/sms/js/thread_ui.js
@@ -249,8 +249,8 @@ var ThreadUI = global.ThreadUI = {
       'click', assimilate
     );
 
-    this.container.addEventListener(
-      'click', this.handleEvent.bind(this)
+    this.container.addEventListener(//tcl_tianmin modified for bug#679650
+      'click', this.handleEvent.bind(this), true
     );
     this.container.addEventListener(
       'contextmenu', this.handleEvent.bind(this)
@@ -1896,8 +1896,20 @@ var ThreadUI = global.ThreadUI = {
           }
           return;
         }
-
         var input = evt.target.parentNode.querySelector('input');
+
+        //tcl_tianmin modified for bug#679650 begin
+        var height = document.querySelector('#messages-edit-form > menu');
+        for (var t = height.offsetTop; height = height.offsetParent;) {
+          t += height.offsetTop;
+        }
+        if (evt.clientY > t) {
+          evt.stopPropagation();
+          evt.preventDefault();
+          break;
+        }
+        //tcl_tianmin modified for bug#679650 end
+
         if (input) {
           this.chooseMessage(input);
           this.checkInputs();
-- 
1.7.0.4

Comment 14

[田旻]

(In reply to Julien Wajsberg [:julienw] from comment #12)
> No, I mean, something like:
> 
> document.querySelector('#messages-edit-form >
> menu').addEventListener('click', function() {});

I understand what you said. You want the document.querySelector('#messages-edit-form > menu') should receive the event before the check box, right?

Comment 15

[Julien Wajsberg [:julienw]]

The "event fluffing" mechanism works by trying to find the closest area that can receive a click. The issue here is that because the button is disabled, it doesn't have a click handler. So the fluffing mechanism tries to find another area, and finds the checkbox label.

So if there is an event handler on the full menu, this should not happen anymore.

Comment 16

[田旻]

(In reply to Julien Wajsberg [:julienw] from comment #15)
> The "event fluffing" mechanism works by trying to find the closest area that
> can receive a click. The issue here is that because the button is disabled,
> it doesn't have a click handler. So the fluffing mechanism tries to find
> another area, and finds the checkbox label.
> 

 It isn't because the miss of the 
disable of a "Select all/Deselect all" button, because the check button and "Select all/Deselect all" always work at the same time. Sometimes even the "Select all/Deselect all" button is active, the check box near them also can be active at the same time.

Comment 17

[Julien Wajsberg [:julienw]]

mmm ok, then I'll have a look at this again.

Comment 18

[Julien Wajsberg [:julienw]]

Sorry, I can't reproduce this :(

It would help a lot if you could post a video on youtube (or somewhere else), or try to compress it more so that you can attach it here?

Comment 19

[田旻]

(In reply to Julien Wajsberg [:julienw] from comment #18)
> Sorry, I can't reproduce this :(
> 
> It would help a lot if you could post a video on youtube (or somewhere
> else), or try to compress it more so that you can attach it here?

I just use the method as you told me, I add something as follow in thread_ui.js and thread_list_ui.js:

document.querySelector('#messages-edit-form > menu').addEventListener('click', function() {
  evt.stopPropagation();
  evt.preventDefault();
});

But the checkbox still can receive the click event because they (buttons and checkbox) are not a parent-child relation. Thus I think they both can be receive the event when click.

Comment 20

[Julien Wajsberg [:julienw]]

Still a video could help because maybe I don't understand the issue correctly.

My thought was that the event fluffing was doing this, but you seems to say it's not the issue. But then I really can't reproduce on my side, or I am not testing the right thing.

Comment 21

[田旻]

Attachment: Please check VID_0001.3gp .

Comment 22

[Julien Wajsberg [:julienw]]

Tian, another question: which firefox os version is it? Is it in v1.3? Do you see this in all versions?

Comment 23

[田旻]

The newest version I have is 2014042202400.

Comment 24

[Julien Wajsberg [:julienw]]

Tian: is it a v1.3 version?
(sorry, I need all information to test on my side; I tried only v1.4 and master for now)

Comment 25

[田旻]

(In reply to Julien Wajsberg [:julienw] from comment #24)
> Tian: is it a v1.3 version?
> (sorry, I need all information to test on my side; I tried only v1.4 and
> master for now)

Yes, it a FFOS v1.3 version.

Comment 26

[Julien Wajsberg [:julienw]]

Thanks, I'll try again later today.

Comment 27

[Julien Wajsberg [:julienw]]

Ok, I definitely reproduce on v1.3 and v1.4, and not on v2.0.

I think I found the issue. Can you try to add this CSS rule in sms.css ?

  .edit-button {
    pointer-events: auto !important;
  }


Maybe we can do it better (without !important) if it works for you.

Comment 28

[田旻]

(In reply to Julien Wajsberg [:julienw] from comment #27)
> Ok, I definitely reproduce on v1.3 and v1.4, and not on v2.0.
> 
> I think I found the issue. Can you try to add this CSS rule in sms.css ?
> 
>   .edit-button {
>     pointer-events: auto !important;
>   }
> 
> 
> Maybe we can do it better (without !important) if it works for you.

OK, I will have a try.

Comment 29

[田旻]

  .edit-button {
    pointer-events: auto !important;
  }

works, but 

  .edit-button {
    pointer-events: auto ;
  }  
not work.

Comment 30

[Julien Wajsberg [:julienw]]

Yes that's expected, thanks.

I'll mark 1.4 as affected. I also think email and call log and everything that uses the "edit mode" building block is affected, so I'm requesting a blocker status for this.

Triagers: please see attachment 8438932 [details].

Tian: if this is made a blocker, I think we'll do a patch for building blocks because other apps are likely affected. If it's not a blocker, I think you can just use the simple patch for SMS only.

Comment 31

[田旻]

(In reply to Julien Wajsberg [:julienw] from comment #30)
> Yes that's expected, thanks.
> 
> I'll mark 1.4 as affected. I also think email and call log and everything
> that uses the "edit mode" building block is affected, so I'm requesting a
> blocker status for this.
> 
> Triagers: please see attachment 8438932 [details].
> 
> Tian: if this is made a blocker, I think we'll do a patch for building
> blocks because other apps are likely affected. If it's not a blocker, I
> think you can just use the simple patch for SMS only.

OK.

Comment 32

[Preeti Raghunath(:Preeti)]

Blocking on it for 1.4 as it is seen happening on 1.3 and 1.4. The UX is quite confusing

Comment 33

[Julien Wajsberg [:julienw]]

Hey Arnau,

do you think you can provide a patch for this in the BB for v1.4? Or would you prefer that I do it and ask you review?

Comment 34

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

Paco can take it, then either you or me could review it ;)

Comment 35

[Paco Rampas [:paco]]

Attachment: patch in github for v1.4

Comment 36

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

Comment on attachment 8442745 [details] [review]
patch in github for v1.4

Looks good :)

Comment 37

[Julien Wajsberg [:julienw]]

Comment on attachment 8442745 [details] [review]
patch in github for v1.4

I left a question on github.

The patch fixes the issue but I think we can actually remove the "pointer-events: none" we have in buttons.css, unless we're afraid of other regressions.

Comment 38

[Julien Wajsberg [:julienw]]

Comment on attachment 8442745 [details] [review]
patch in github for v1.4

ok, let's do this, r=me

I'll merge

Comment 39

[Julien Wajsberg [:julienw]]

v1.4: 479db54b660e39f7b0dd55219f79c3657d06ec0a