Closed Bug 1019359 Opened 6 years ago Closed 6 years ago

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

Categories

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

defect

Tracking

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

RESOLVED FIXED
2.0 S4 (20june)
blocking-b2g 1.4+
Tracking Status
b2g-v1.3 --- affected
b2g-v1.3T --- affected
b2g-v1.4 --- fixed
b2g-v2.0 --- unaffected
b2g-v2.1 --- unaffected

People

(Reporter: sync-1, Assigned: paco)

Details

Attachments

(5 files)

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:
Attached image pic
The build I used to test is Firefox OS v1.3.
Mozilla build ID:20140422024003
Can you please do a video ?
Flags: needinfo?(sync-1)
(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.
Attached image reproduce-step1
Attached image reproduce-step2
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.
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();
--
If there is an issue in the event fluffing code, this needs to be fixed at the event fluffing level.
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).
 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?
    );
No, I mean, something like:

document.querySelector('#messages-edit-form > menu').addEventListener('click', function() {});
From 767ae8b262b57a3a6f54c870eb4e76968383f220 Mon Sep 17 00:00:00 2001
From: repo sync <tianm@tcl.com>
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
(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?
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.
(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.
mmm ok, then I'll have a look at this again.
Flags: needinfo?(felash)
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?
Flags: needinfo?(felash) → needinfo?(tianm)
(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.
Flags: needinfo?(tianm)
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.
Flags: needinfo?(tianm)
Flags: needinfo?(tianm)
Tian, another question: which firefox os version is it? Is it in v1.3? Do you see this in all versions?
Flags: needinfo?(tianm)
The newest version I have is 2014042202400.
Flags: needinfo?(tianm)
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)
Flags: needinfo?(tianm)
(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.
Flags: needinfo?(tianm)
Thanks, I'll try again later today.
Flags: needinfo?(felash)
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.
Flags: needinfo?(felash)
(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.
  .edit-button {
    pointer-events: auto !important;
  }

works, but 

  .edit-button {
    pointer-events: auto ;
  }  
not work.
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.
blocking-b2g: --- → 1.4?
(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.
Flags: needinfo?(sync-1)
Blocking on it for 1.4 as it is seen happening on 1.3 and 1.4. The UX is quite confusing
blocking-b2g: 1.4? → 1.4+
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?
Flags: needinfo?(arnau)
Paco can take it, then either you or me could review it ;)
Assignee: nobody → pacorampas
Flags: needinfo?(arnau)
Attachment #8442745 - Flags: review?(felash)
Attachment #8442745 - Flags: feedback?(arnau)
Comment on attachment 8442745 [details] [review]
patch in github for v1.4

Looks good :)
Attachment #8442745 - Flags: feedback?(arnau) → feedback+
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 on attachment 8442745 [details] [review]
patch in github for v1.4

ok, let's do this, r=me

I'll merge
Attachment #8442745 - Flags: review?(felash) → review+
v1.4: 479db54b660e39f7b0dd55219f79c3657d06ec0a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S4 (20june)
You need to log in before you can comment on or make changes to this bug.