Closed Bug 889717 Opened 6 years ago Closed 6 years ago

[Settings] Do not work when you are scrolling and select a menu

Categories

(Core :: DOM: Device Interfaces, defect, P1)

ARM
Gonk (Firefox OS)
defect

Tracking

()

RESOLVED FIXED
1.1 QE4 (15jul)
blocking-b2g leo+
Tracking Status
firefox24 --- wontfix
firefox25 --- wontfix
firefox26 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix
b2g-v1.1hd --- fixed

People

(Reporter: leo.bugzilla.gaia, Assigned: rik)

References

Details

(Whiteboard: [TD-56305][LeoVB+])

Attachments

(3 files, 11 obsolete files)

2.64 MB, video/3gpp
Details
24.73 KB, image/png
Details
2.13 KB, patch
rik
: review+
Details | Diff | Splinter Review
Attached video video contents
1. Title : Do not work when you are scrolling and select a menu
2. Precondition : 
3. Tester's Action : Settings - scroll and select any menu
4. Detailed Symptom : selected menu is in Highlight but do not go into sub menu
5. Expected : Shoule be display sub menu
6. Reproducibility: Y
   - Frequency Rate : 100%
7.Gaia Master/v1-train : Reproduced
8. Version Info
Build ID : 20130616070209
GAIA : f2d6ed54a236e6e3b94f0abad9f0dacb8a1cc7b3
GECKO : be276cf55ce160bca09f36d9ca679a2ae20ea7cc
blocking-b2g: --- → leo+
Priority: -- → P1
Whiteboard: [TD-56305]
Target Milestone: --- → 1.1 QE4 (15jul)
Assignee: nobody → mshiao
It seems related with Bug 857105 patch.
Attached file candidate_patch (obsolete) —
Please check the patch
Attachment #771155 - Flags: review?(mshiao)
Hi Leo, 

Not sure if we need the patch.  I tested with lastest master and could not reproduce the problem.

Build Id: 20130624034413
Gaia: 1e8dfab984252468cb61e85963cf45f951004a6b

Can you guys confirm?

Thanks,
Mark
Flags: needinfo?(hyuna.cho82)
I had tested with your test master version and it's reproduced.
Gaia : 1e8dfab984252468cb61e85963cf45f951004a6b

And it's reproduced in the latest master also
Gaia: 1b6752d91ee604fe62bb42c2c03c80724b920b98

It's easy to reproduce when touch a menu during the scrolling
Flags: needinfo?(hyuna.cho82)
Comment on attachment 771155 [details]
candidate_patch

Hi Pavel,

Spoke with Arthur Chen and he suggested you were the person to seek for css review. The patch addresses highlight behavior while scrolling the settings list.  Currently, while the list is still scrolling, if you select an item it'll highlight but doesn't enter the submenu which is kinda misleading.  The fix prevents the highlight unless the item is in 'active' state.  

STR
1) Scrolling list
2) While list is scrolling, select a menu item
3) Item highlights but doesn't enter submenu

Thanks,
Mark
Attachment #771155 - Flags: review?(mshiao) → review?(pivanov)
Comment on attachment 771155 [details]
candidate_patch

Looks OK for me
Attachment #771155 - Flags: review?(pivanov) → review+
landed on master 
commit: 6990d381055cafaa28995c221bb69cf4023c64b9
https://github.com/mozshiao9/gaia/commit/6990d381055cafaa28995c221bb69cf4023c64b9
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
I was not able to uplift this bug to v1-train.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1-train, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1-train
  git cherry-pick -x  6990d381055cafaa28995c221bb69cf4023c64b9
  <RESOLVE MERGE CONFLICTS>
  git commit
Flags: needinfo?(mshiao)
Hi John,

Uplifted to v1-train
commit: 7ee822b8ad62cef335c60bda489887353fcd0806

https://github.com/mozilla-b2g/gaia/commit/7ee822b8ad62cef335c60bda489887353fcd0806

Please let me know if there are any issues.

Thanks,
Mark
Flags: needinfo?(mshiao)
Whiteboard: [TD-56305] → [TD-56305][LeoVB+]
Blocks: 857105
Depends on: 869338
v1.1.0hd: 7ee822b8ad62cef335c60bda489887353fcd0806
v1.1.0hd: 9c432ab3b473736ad0d02a1d7ba2c09df66aadbc
After discussing with several people about bug 869338, I feel like the fix here is not the good one. So I'm reverting it and reopening and we'll discuss alternative approaches.

Reverted:
master: e5c761283f23097848e711b7865e0ca0a9f95205
v1-train: 9c868050bcc6f522f54f4afc0f0e4aa9a6149771
v1.1.0hd: 4d5918f511bc6b8e1f120ddf7e47e2b8d6e38a04
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
The problem here is that when you scroll, let the scroll momentum go for a moment and then tap to stop the scrolling, the element under the tap will receive the :focus pseudo-class.

I think we shouldn't receive the :focus pseudo-class in that case. If you scroll a really long list, you'll tap to stop the scrolling, not to focus the underlying element. I think we should change the Gecko behaviour in that case.

needinfo-ing Neil and Eitan to get some opinions on this.
Flags: needinfo?(enndeakin)
Flags: needinfo?(eitan)
How would you propose fixing this in gecko?
Flags: needinfo?(eitan)
Don't set the :focus pseudo-class (and don't fire the focus event either) when it's a tap to stop scrolling. From a quick conversation with Vivien, he told me that would be done in dom/browser-element/BrowserElementPanning.js.
(In reply to Anthony Ricaud (:rik) from comment #14)
> Don't set the :focus pseudo-class (and don't fire the focus event either)
> when it's a tap to stop scrolling. From a quick conversation with Vivien, he
> told me that would be done in dom/browser-element/BrowserElementPanning.js.

That looks right. I have no opinion on the matter :)
I've been hacking around inside the code suggested by Vivien, and I'm getting something that seems to have the desired behavior, but it's a bit hackish, and I fear about side effects.

But one point that is important is that the 'focus' event is not cancelable, confere https://developer.mozilla.org/en-US/docs/Web/Reference/Events/focus. So maybe handling this in BrowserElementPanning.js is already too late?
Please find attached a patch for this issue. The idea is to benefit from "pointer-events: none;" to avoid receiving a 'focus' event and pseudo-class while we are scrolling, mimicking the current behavior for 'click' event.

We can't just wait for 'focus' and cancel it, since it's not cancelable. As far as I've been able to test on Inari, this patch fixes the present bug and does not make bug 869338 coming back to bite us.
Assignee: mshiao → lissyx+mozillians
Attachment #771155 - Attachment is obsolete: true
Attachment #782174 - Flags: review?(21)
Updating the previous patch by adding comments to explain and fixing coding style.
Attachment #782174 - Attachment is obsolete: true
Attachment #782174 - Flags: review?(21)
Attachment #782175 - Flags: review?(21)
Addressing comment from Anthony on IRC: it's better to save the pointer-events value when setting it to "none" and to restore it at the end that just blindly overwrite it.
Attachment #782175 - Attachment is obsolete: true
Attachment #782175 - Flags: review?(21)
Attachment #782237 - Flags: review?(21)
Comment on attachment 782237 [details] [diff] [review]
Inhibits pointer-events while scrolling v3

>+++ b/dom/browser-element/BrowserElementPanning.js
>@@ -94,6 +94,8 @@ const ContentPanning = {
>         let view = target.ownerDocument ? target.ownerDocument.defaultView
>                                         : target;
>         view.removeEventListener('click', this, true, true);
>+        this.targetPointerEvents.style.pointerEvents =
>+          this.targetPointerEventsOldStyle;
>         break;

This is never a good idea to alter the document directly from the platform.


>@@ -166,14 +168,14 @@ const ContentPanning = {
> 
>     // If there is a pan animation running (from a previous pan gesture) and
>     // the user touch back the screen, stop this animation immediatly and
>-    // prevent the possible click action if the touch happens on the same
>-    // target.
>-    this.preventNextClick = false;
>+    // prevent the possible click or focus action if the touch happens on the
>+    // same target.
>+    this.preventNextClickOrFocus = false;

|focus| happens on a mousedown for most elements. I assume what you're trying to avoid is the trigger of :active, not focus ?

r- because you should never change the dom of page directly.
Attachment #782237 - Flags: review?(21) → review-
(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #20)
> Comment on attachment 782237 [details] [diff] [review]
> Inhibits pointer-events while scrolling v3
> 
> >+++ b/dom/browser-element/BrowserElementPanning.js
> >@@ -94,6 +94,8 @@ const ContentPanning = {
> >         let view = target.ownerDocument ? target.ownerDocument.defaultView
> >                                         : target;
> >         view.removeEventListener('click', this, true, true);
> >+        this.targetPointerEvents.style.pointerEvents =
> >+          this.targetPointerEventsOldStyle;
> >         break;
> 
> This is never a good idea to alter the document directly from the platform.

I'd be very happy for a nicer solution, but we want to stop the 'focus' event (cf comment #12) and this one is not cancelable.

> 
> 
> >@@ -166,14 +168,14 @@ const ContentPanning = {
> > 
> >     // If there is a pan animation running (from a previous pan gesture) and
> >     // the user touch back the screen, stop this animation immediatly and
> >-    // prevent the possible click action if the touch happens on the same
> >-    // target.
> >-    this.preventNextClick = false;
> >+    // prevent the possible click or focus action if the touch happens on the
> >+    // same target.
> >+    this.preventNextClickOrFocus = false;
> 
> |focus| happens on a mousedown for most elements. I assume what you're
> trying to avoid is the trigger of :active, not focus ?

As far as I can tell, it's the focus that we don't want when the users taps to stop scrolling, as explained in comment #12.

> 
> r- because you should never change the dom of page directly.
It sounds like you just want to cancel the mousedown event in the case where you don't want it to react to the press.
Flags: needinfo?(enndeakin)
(In reply to Alexandre LISSY :gerard-majax from comment #21)
> (In reply to Vivien Nicolas (:vingtetun) (:21) from comment #20) 
> > >@@ -166,14 +168,14 @@ const ContentPanning = {
> > > 
> > >     // If there is a pan animation running (from a previous pan gesture) and
> > >     // the user touch back the screen, stop this animation immediatly and
> > >-    // prevent the possible click action if the touch happens on the same
> > >-    // target.
> > >-    this.preventNextClick = false;
> > >+    // prevent the possible click or focus action if the touch happens on the
> > >+    // same target.
> > >+    this.preventNextClickOrFocus = false;
> > 
> > |focus| happens on a mousedown for most elements. I assume what you're
> > trying to avoid is the trigger of :active, not focus ?
> 
> As far as I can tell, it's the focus that we don't want when the users taps
> to stop scrolling, as explained in comment #12.
>

In this case preventing the |mousedown| event should be enough.
(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #23)
> (In reply to Alexandre LISSY :gerard-majax from comment #21)
> > (In reply to Vivien Nicolas (:vingtetun) (:21) from comment #20) 
> > > >@@ -166,14 +168,14 @@ const ContentPanning = {
> > > > 
> > > >     // If there is a pan animation running (from a previous pan gesture) and
> > > >     // the user touch back the screen, stop this animation immediatly and
> > > >-    // prevent the possible click action if the touch happens on the same
> > > >-    // target.
> > > >-    this.preventNextClick = false;
> > > >+    // prevent the possible click or focus action if the touch happens on the
> > > >+    // same target.
> > > >+    this.preventNextClickOrFocus = false;
> > > 
> > > |focus| happens on a mousedown for most elements. I assume what you're
> > > trying to avoid is the trigger of :active, not focus ?
> > 
> > As far as I can tell, it's the focus that we don't want when the users taps
> > to stop scrolling, as explained in comment #12.
> >
> 
> In this case preventing the |mousedown| event should be enough.

Thanks, that matches Neil's previous comment :). I'm testing this right now !
Thanks to your suggestion, please find attached the following much cleaner patch. From my current testing, it fixes the current bug, and it does not manipulate the DOM as the previous patch.
Attachment #782237 - Attachment is obsolete: true
Attachment #782589 - Flags: review?(21)
Comment on attachment 782589 [details] [diff] [review]
Do not send focus when the user taps to stop scrolling, v4

I will prefer this code to lives inside onTouchStart :)
Attachment #782589 - Flags: review?(21)
Like this ? :)
Attachment #782589 - Attachment is obsolete: true
Attachment #782689 - Flags: review?(21)
Attached image screenshot.png
I has tested the patch after reverting the Bug 889717 patch. 
But keep the highligt and can't enter their subment like the attached screenshot.

STR:
1) Scrolling list
2) While list is scrolling, touch a menu
3) Touch again after finishing scrolling

Please check again. Thansk.
(In reply to Leo from comment #28)
> Created attachment 782994 [details]
> screenshot.png
> 
> I has tested the patch after reverting the Bug 889717 patch. 
> But keep the highligt and can't enter their subment like the attached
> screenshot.
> 
> STR:
> 1) Scrolling list
> 2) While list is scrolling, touch a menu
> 3) Touch again after finishing scrolling
> 
> Please check again. Thansk.

It looks like you're right, that happens if when you touch again you perform a slight scrolling from what I've been able to test.
Please find attached a new version of the patch that calls "blur()" when discarding the "click" event, which seems to be fixing the latest issue exposed. There might be a better way to do this, unfortunately I don't see it :(

Anthony, since I'm off this week and you followed a bit this bug with me, could you take the lead ?

Leo, could you test with this patch ?
Attachment #782689 - Attachment is obsolete: true
Attachment #782689 - Flags: review?(21)
Attachment #783036 - Flags: review?(21)
Attachment #783036 - Flags: feedback?(anthony)
Flags: needinfo?(leo.bugzilla.gaia)
Comment on attachment 783036 [details] [diff] [review]
Do not send focus when the user taps to stop scrolling, v6

> dom/browser-element/BrowserElementPanning.js | 13 +++++++++++++
>+
>+        // Bug 889717, comment #28: If there is a slight movement when taping,
>+        // we will stop the 'click' event successfully but we might have
>+        // sent focus, so we remove it.
>+        if ('blur' in target) {
>+          target.blur();
>+        }
>+

It can be focus/blur handlers on the target thay you don't want to trigger. So that does not sounds right :(
Attachment #783036 - Flags: review?(21) → review-
Sadly, someone else will have to look at this one, I really don't have the time this week.
Assignee: lissyx+mozillians → nobody
I was requested to take a look on this issue in Leo side, and I took the liberty of making some changes on Alexandre's patch.

In this version, I tried to evt.preventDefault on touchStart and touchEnd events. As result, the focus event is not sent to Gaia. Also, the click event is not sent to Gaia. So there is no need to listen the click event and preventDefault/stopPropagation it.

That worked fine to me.
Attachment #784887 - Flags: review?(21)
Flags: needinfo?(leo.bugzilla.gaia)
Be careful when reviewing this, the splinter view doesn't show all the patch. Use the diff view instead. I opened bug 900930 to track this.
Andre: Thanks, this is working as expected on my device. I was trying something similar yesterday with no success.
Attachment #783036 - Attachment is obsolete: true
Attachment #783036 - Flags: feedback?(anthony)
Hi Anthony,

Can this bug be closed? Since  bug 900930 has been opened for tracking it.
Flags: needinfo?(anthony)
Preeti: No, bug 900930 is about a Bugzilla issue. We still want to land a fix for this. I think attachment 784887 [details] [diff] [review] is the correct fix but I'm not a reviewer for that part of the code.
Flags: needinfo?(anthony)
Comment on attachment 784887 [details] [diff] [review]
Do not send focus when the user taps to stop scrolling, v7

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

I worked with Vivien to test this. Sadly, this breaks the value selector. It always selects the item where you start dragging. You can find one in the Settings App > Date & Time > Disable Set Automatically > City.

From what I understood, this is because the system app receives both mouse and touch events. It's called hybridEvents in BrowserElementPanning.js.
Attachment #784887 - Flags: review?(21)
Another try at this. If we receive both mouse events and touch events, we still need to cancel click events so we have to attach an event listener to do so.
Attachment #784887 - Attachment is obsolete: true
Attachment #788145 - Flags: review?(21)
Attachment #788145 - Attachment description: Do not send focus when the user taps to stop scrolling, v7 → Do not send focus when the user taps to stop scrolling, v8
Hi Anthony,

Any progress here? When can the fix land on 1.1?
Flags: needinfo?(anthony)
We're waiting on a review. Vivien is on PTO till September 3rd. I don't know who else could review this patch before that.
Flags: needinfo?(anthony)
Assignee: nobody → timdream
(In reply to Anthony Ricaud (:rik) from comment #41)
> We're waiting on a review. Vivien is on PTO till September 3rd. I don't know
> who else could review this patch before that.

browser-element? You should ask :jlebar :)
Assignee: timdream → anthony
Component: Gaia::Settings → DOM
Product: Boot2Gecko → Core
Attachment #788145 - Flags: review?(21) → review?(justin.lebar+bug)
Yeesh.  I don't think I'm qualified to review this; given how many regressions you all have seen with previous attempts at this fix, you probably want a domain expert to look at this, and most of this code is a mystery to me.

Let's leave this up to release drivers.  If getting this patch in is urgent enough that they're willing to accept an increased risk of regressions, I can rubber-stamp this change.  Otherwise, we should wait for someone who understands this code.

Also, Vivien may know if we need to make a similar change to APZC.
Flags: needinfo?(praghunath)
Also, Anthony, it's customary to attach -U8 patches.  You can't change git diff's behavior, but you can alias |git dif| to do the right thing; in your ~/.gitconfig, add

[alias]
  dif = diff -U8 --histogram -M -C

or you can use |git bz| to attach your patches to bugs: https://github.com/jlebar/moz-git-tools

The latter is preferable because the attachment will include the commit's metadata, formatted properly as an hg patch.
Attachment #788145 - Flags: review?(justin.lebar+bug)
Duh, I knew about the -U8 rule for patches, sorry. Thanks for the link to those tools.
Attachment #788145 - Attachment is obsolete: true
Comment on attachment 796738 [details] [diff] [review]
Do not send focus when the user taps to stop scrolling, v9

Let's put it back in Vivien's review queue then. He should be back next Monday so I think we can wait for him.
Attachment #796738 - Flags: review?(21)
Indentation is off in the first hunk.
Component: DOM → DOM: Device Interfaces
Also, it would be very nice to add comments in the code to let people understand what's going on.
Attachment #796738 - Attachment is obsolete: true
Attachment #796738 - Flags: review?(21)
Attachment #797933 - Flags: review?(21)
Comment on attachment 797933 [details] [diff] [review]
Do not send focus when the user taps to stop scrolling, v10

>diff --git a/dom/browser-element/BrowserElementPanning.js b/dom/browser-element/BrowserElementPanning.js
>@@ -578,27 +589,27 @@ const ContentPanning = {
>   _zoomOut: function() {
>     let rect = new Rect(0, 0, 0, 0);
>     var os = Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService);
>     os.notifyObservers(docShell, 'browser-zoom-to-rect', JSON.stringify(rect));
>   },
> 
>   _isRectZoomedIn: function(aRect, aViewport) {
>     // This function checks to see if the area of the rect visible in the
>-    // viewport (i.e. the "overlapArea" variable below) is approximately 
>+    // viewport (i.e. the "overlapArea" variable below) is approximately
>     // the max area of the rect we can show.
>     let vRect = new Rect(aViewport.x, aViewport.y, aViewport.width, aViewport.height);
>     let overlap = vRect.intersect(aRect);
>     let overlapArea = overlap.width * overlap.height;
>     let availHeight = Math.min(aRect.width * vRect.height / vRect.width, aRect.height);
>     let showing = overlapArea / (aRect.width * availHeight);
>     let ratioW = (aRect.width / vRect.width);
>     let ratioH = (aRect.height / vRect.height);
> 
>-    return (showing > 0.9 && (ratioW > 0.9 || ratioH > 0.9)); 
>+    return (showing > 0.9 && (ratioW > 0.9 || ratioH > 0.9));
>   },
> 

Please remove the whitespaces changes, they are not needed for this patch.

AZPC should already take care of this so I think it should be fine.
Attachment #797933 - Flags: review?(21) → review+
Attachment #797933 - Attachment is obsolete: true
Attachment #799441 - Flags: review?
Comment on attachment 799441 [details] [diff] [review]
Do not send focus when the user taps to stop scrolling, v10

Carrying review+ by vingtetun.
Attachment #799441 - Flags: review? → review+
Thanks a lot André and Alexandre for the earlier versions of that patch.
Flags: needinfo?(praghunath)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e92a97923864
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.