942246 - Event fluffing fails to generate a click under certain conditions

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

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?

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

Kats is away for a week, and this seems like it should be handled quickly; Neil, Olli, any pointers?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

click event requires mousedown/up to happen on the same element. Does the app do something during
mousedown?

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(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%.

Comment 4

[Preeti Raghunath(:Preeti)]

We'd like fluffing to be disbaled.

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

(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.

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(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.

Comment 7

[Jason Smith [:jsmith]]

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.

Comment 8

[Preeti Raghunath(:Preeti)]

Lets do it right for 1.3

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Dale, do you have any ideas on why this might be happening? You probably know the fluffing code best.

Comment 10

[Dale Harvey (:daleharvey)]

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

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Attachment: Backtrace of the call to set the capturing content

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

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 13

[Neil Deakin]

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.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

(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?

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Somebody else should be looking at this; somebody who understands the code in PresShell::HandleEvent.

Comment 16

[Milan Sreckovic [:milan] (needinfo for best results)]

:roc, what do you think?  You, somebody else you can recommend?

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Is there a way to reproduce this on desktop?

Comment 18

[Timothy Nikkel (:tnikkel)]

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.

Comment 19

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: testcase-fluffing-2.html

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.

Comment 20

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

There you go, Olli :-)

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

(can't promise a fix anytime soon.)

Comment 22

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bug942246.fluffing.fails.to.generate.a.click

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.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

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.)

Comment 24

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bug942246.patch v2

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8360431 [details] [diff] [review]
bug942246.patch v2

Yeah, let's try this.

Comment 26

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e8040fedf116

I opened bug 960553 as a followup.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e8040fedf116

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/af30f8d0d27f