Event fluffing fails to generate a click under certain conditions

RESOLVED FIXED in Firefox 28

Status

()

defect
P3
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: vingtetun, Assigned: vingtetun)

Tracking

(Blocks 1 bug)

Trunk
mozilla29
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:1.3+, firefox27 wontfix, firefox28 fixed, firefox29 fixed, b2g-v1.3 fixed, b2g-v1.4 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

I noticed in Gaia that sometimes the back button of the the setting application header and the sms application header got some :hover/:active style but does not trigger a click.

Steps to reproduce:
 - Open the Settings application
 - Click on the Wifi panel
 - Once it is loaded, click on the rigth side of the back button to trigger the fluffing code (if you click on the button it works correctly).

Actual results:
 - The button received some CSS style (:hover/:active) but is not clicked.

Expected results:
 - The button is clicked and the app navigates back to the previous page.

Touch Events are not affected directly, but the mousemove,mousedown,mouseup sequence which is generated after a touchend is.

dumping the events received into those applications, I see the following when it works:
I/GeckoDump( 4944): mousemove: app://settings.gaiamobile.org/index.html#wifi (227-225)
I/GeckoDump( 4944): mousedown: app://settings.gaiamobile.org/index.html#wifi (227-225)
I/GeckoDump( 4944): mouseup: app://settings.gaiamobile.org/index.html#wifi (227-225)

But when it fails I see the following:
I/GeckoDump( 4944): mousemove: app://settings.gaiamobile.org/index.html#wifi (227-225)
I/GeckoDump( 4944): mousedown: app://settings.gaiamobile.org/index.html#wifi (227-225)
I/GeckoDump( 4944): mouseup: [object HTMLHeadingElement] (227-225)

I spend some time investigating and it seems like adding a rule |section[role=region] > header:first-child > a { -moz-user-select: none;}| into the $GAIA/apps/settings/style/list.css workaround the issue, so it seems related to selection.

I digged a little bit more and it seems like the main difference between the 2 use cases is http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#6088

In the case where it does not work, capturingContent is defined to a scrollFrame (I assume it is set by http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#2602)

Not sure how to fix that. I would also land a fix in Firefox OS 1.2 since missing click is a bad bug imo. so koi?

This is hard for me to say how often this stuff could happen on web content directly, but since some developers use Gaia building blocks to create their apps and those same building blocks are affected by this bug it sounds that it will happen relatively often.

I'm also not sure how much fluffing in general is affected by this.

Milan, do you have anyone handy that can help here?
Flags: needinfo?(milan)
Kats is away for a week, and this seems like it should be handled quickly; Neil, Olli, any pointers?
Flags: needinfo?(milan)
Flags: needinfo?(enndeakin)
Flags: needinfo?(bugs)
click event requires mousedown/up to happen on the same element. Does the app do something during
mousedown?
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] from comment #2)
> click event requires mousedown/up to happen on the same element. Does the
> app do something during
> mousedown?

No the app does nothing specifically and it also happens across all apps. Disabling fluffling solve the issue 100%.
Flags: needinfo?(enndeakin)
We'd like fluffing to be disbaled.
blocking-b2g: koi? → koi+
(In reply to Preeti Raghunath(:Preeti) from comment #4)
> We'd like fluffing to be disabled.

Just to be clear - we want to use this bug to disable the even fluffing at the preference level?  Seems like this bug is talking about fixing the behaviour, rather than disabling it, and we don't want to fix it as koi+.  Perhaps we should open another koi+ bug to disable it, and use this one to actually fix it?

Either way, somebody please confirm.
Flags: needinfo?(21)
(In reply to Preeti Raghunath(:Preeti) from comment #4)
> We'd like fluffing to be disabled.

Disabling fluffling will make clicking on a small element much harder across the device and web content. 

That says I don't mind at all about disabling fluffing for 1.2 (it sounds the safest solution) but I want to make sure the side effects are well understood. (NeedInfo :Preeti)

So if this is something we really want let's disable it for 1.2 in a separate bug and let's re-target this bug for 1.3.

I opened bug 944347 (with a patch) to disable it and request koi? on it. Preeti, let me know if this is the final decision.
Flags: needinfo?(21) → needinfo?(praghunath)
The plan above sounds fine.

Moving to 1.3+ then - bug 944347 is turning off event fluffing for 1.2. 1.3 will include this fix though.
blocking-b2g: koi+ → 1.3+
Priority: -- → P3
Lets do it right for 1.3
Flags: needinfo?(praghunath)
Assignee: nobody → bugmail.mozilla
Dale, do you have any ideas on why this might be happening? You probably know the fluffing code best.
Flags: needinfo?(dale)
From the top of my head / quick code review not at all, but feel free to assign it to me to investigate, ill do so myself once I get my current bugs done
Flags: needinfo?(dale)
I spent some time tracing this issue. It's pretty easy to reproduce and I was seeing the same thing Vivien was that the mouseup event has a different target than the mousedown. The mousedown and mouseup events are dispatched from [1], but when those two events are passed in to the event fluffing code at [2], the "frame" variable is different. The difference in "frame" results in different targets getting selected for the mousedown vs mouseup. I looked into where the difference was coming from. It looks like during the processing of the mousedown event, gCaptureInfo.mContent is set to some non-null value (backtrace attached). Then, when the mouseup event is processed, the capturingContent variable at [3] is non-null which results in the line at [4] changing the "frame" variable.

smaug, any idea what this code does, and/or what gCaptureInfo is?

[1] http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp?rev=343ac499c993#1815
[2] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp?rev=343ac499c993#6471
[3] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp?rev=343ac499c993#6215
[4] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp?rev=343ac499c993#6357
Flags: needinfo?(bugs)
capturingContent is used for example for dragging and such, or JS
may call http://mxr.mozilla.org/mozilla-central/source/dom/webidl/Element.webidl#93

In this casehttp://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp?rev=5a9badd6db00&mark=2639-2643,2651-2651#2639
Event fluffing code should probably handle the case when there is capturing content.
Flags: needinfo?(bugs)
The stack shows that you are pressing the mouse button on the page and dragging. This will cause the content to be selected. The capture is needed so that the page will scroll when you move the mouse to the edge of the page, and the events are retargeted to the scrollable area instead of what the mouse is actually over.
(In reply to Neil Deakin from comment #13)
> The stack shows that you are pressing the mouse button on the page and
> dragging.

I assume you mean the code path in the stack is used to handle the dragging case. I just want to clear up confusion because the mouse events being dispatched in this case are definitely not a drag action - there is just one down and one up, with no move in between.

It seems odd to me that we want to set a capturing content before we get any move events (i.e. before we know that it is a drag). But presumably that's baked deep in the code and so is not easy to change.

(In reply to Olli Pettay [:smaug] from comment #12)
> Event fluffing code should probably handle the case when there is capturing
> content.

I'm thinking of two main approaches to deal with this. One is to add a flag to the WidgetMouseEvent (set when we dispatch it in TabChild.cpp) that indicates this is not a drag and to not set a capturing content. The other is to keep another "frame" variable in PresShell::HandleEvent that is unaffected by the capturing content, pass that in to FindFrameTargetedByInputEvent, and use that instead of the other thing as the "root frame". I'm not sure which of these approaches (or something else) is better, and if going with the latter approach I don't understand PresShell::HandleEvent enough to implement it without investing a lot of time.

It really seems like this is outside my area of expertise, anybody else want to take this bug?
Somebody else should be looking at this; somebody who understands the code in PresShell::HandleEvent.
Flags: needinfo?(milan)
:roc, what do you think?  You, somebody else you can recommend?
Flags: needinfo?(milan) → needinfo?(roc)
Assignee: bugmail.mozilla → nobody
Is there a way to reproduce this on desktop?
I've had luck debugging event fluffing issues on desktop in the past by setting "ui.mouse.radius.enabled" to true and "ui.mouse.radius.inputSource.touchOnly" to false.
I spent some time to create a testcase to debug that on desktop. 

Here is the set of prefs I used to debug on desktop:
  Services.prefs.setBoolPref("ui.mouse.radius.enabled", true);
  Services.prefs.setBoolPref("ui.mouse.radius.inputSource.touchOnly", false);
  Services.prefs.setIntPref("ui.mouse.radius.leftmm", 3);
  Services.prefs.setIntPref("ui.mouse.radius.topmm", 5);
  Services.prefs.setIntPref("ui.mouse.radius.rightmm", 3);
  Services.prefs.setIntPref("ui.mouse.radius.bottommm", 2);


Then I clicked around the image you can see in the testcase. A click should be generated on the attached link, but the mousedown target and the mouseup target are different.
There you go, Olli :-)
Assignee: nobody → bugs
Flags: needinfo?(roc)
(can't promise a fix anytime soon.)
I'm prososing this patch because I think there is 2 separate issues here.

One if about making PositionnedEventTargetting.cpp supports capturing content. This would be useful but my assumption is that this is mostly useful for desktop.

The other is about not capturing when the mouse are generated from a touch event as they result of a tap gesture, and not any drag/selection gesture.

The proposed patch fix the latter, while it completely ignore the former that could be a separate bug imho. As a result the proposed patch does not solve the testcase on desktop since on desktop you probably want to keep the possible dragging behavior and PositionnedEventTargetting.cpp should support it.
Attachment #8359876 - Flags: review?(bugs)
Looks like we have browser.ignoreNativeFrameTextSelection pref.
That is used later in ::HandlePress.


What if we had a bool variable to check whether nsIPresShell::SetCapturingContent was called.
If it was, then in
if (!offsets.content->IsEditable() &&
    Preferences::GetBool("browser.ignoreNativeFrameTextSelection", false)))
release the capture before calling HandleClick.

(And would be good to test editable content.)
Attachment #8359876 - Flags: review?(bugs)
Comment on attachment 8360431 [details] [diff] [review]
bug942246.patch v2

Yeah, let's try this.
Attachment #8360431 - Flags: review?(bugs) → review+
Assignee: bugs → 21
Target Milestone: --- → mozilla29
https://hg.mozilla.org/mozilla-central/rev/e8040fedf116
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.