Bug 55104 – Timer-based events should inherit the user gesture state of their originating event in certain cases.

• Component: DOM
• Status: RESOLVED FIXED

[Andy Estes]

Created attachment 83578 [details]
testcase

See the attached test case. Firefox 4 allows a programatic click to open a file chooser dialog even when the the event originating from the user clicking the anchor element and the event that emitted a programatic click are not the same (the latter event is triggered by a 1sec setTimeout()). We should also allow a file chooser dialog to open in these cases.

[Andy Estes]

From my testing of Firefox 4, they implement the following restrictions when forwarding gesture state to timer events:

1) Events fired from setTimeout() only inherit the gesture state if they fire within 1000 ms of the user gesture.
2) Only the first firing of a setInterval() event gets the gesture state (and only if it was within 1000 ms of the user gesture).

[Andy Estes]

Created attachment 85646 [details]
Patch

[Adam Barth]

Comment on attachment 85646 [details]
Patch

I can keep chaining these together?  setTimeout from setTimeout to keep the user gesture?

[Andy Estes]

(In reply to comment #3)
> (From update of attachment 85646 [details])
> I can keep chaining these together?  setTimeout from setTimeout to keep the user gesture?

Good catch. My patch allows for that but shouldn't. I should be able to fix that by checking the timer nesting level.

[Andy Estes]

Created attachment 85647 [details]
Patch

[Darin Adler]

Comment on attachment 85647 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=85647&action=review

> Source/WebCore/page/DOMTimer.cpp:42
> +static const int maxTimeoutForUserGestureForwarding = 1000; // One second matches Gecko.

The word “timeout” is used multiple ways in this file that are confusing. For example, the number of milliseconds is called a timeout in the name of the argument to the constructor and the data member m_originalTimeout. But the timers themselves are also referred to as timeouts, as in the data member m_timeoutId.

Because of that ambiguity, and because we also have two other constants here that are floating point numbers with time in milliseconds, I think this constant is slightly confusing.

> Source/WebCore/page/DOMTimer.cpp:64
> +    m_indicateUserGestureWhenFired = UserGestureIndicator::processingUserGesture()
> +        && timeout <= maxTimeoutForUserGestureForwarding
> +        && m_nestingLevel == 1;

I’d like to see all the initialization of data members be done with data member initialization instead of assignment. That could easily be done by making a separate function to return the ID and putting this logic in a function too. They can be inline functions if desired. I wouldn’t insist on that change for this patch, but it would be nice.

I think this expression needs a why comment for the nesting level check. The other two parts of this expression explain themselves, given the names of what’s involved, but the reason the nesting level should have an effect is more subtle.

> Source/WebCore/page/DOMTimer.cpp:111
> +    UserGestureIndicator gestureIndicator(m_indicateUserGestureWhenFired ? DefinitelyProcessingUserGesture : PossiblyProcessingUserGesture);

Is it helpful to set the gesture indication to “possibly” when the boolean is false? Does the test somehow cover that?

> Source/WebCore/page/DOMTimer.h:73
> +        bool m_indicateUserGestureWhenFired;

For a boolean data member, we normally do not want to use something that can be read as a verb phrase. Perhaps add the word “should”.

Another possible name is m_representsForwardedUserGesture, which is consistent with the name of the maxTimeoutForUserGestureForwarding constant.

> LayoutTests/fast/events/popup-blocking-timers.html:5
> +        var win;

I’m as big a fan of winning as the next guy, but I don’t think this is a great variable name.

> LayoutTests/fast/events/popup-blocking-timers.html:12
> +            layoutTestController.waitUntilDone();

Can we make this test run fast by using the leapForward feature or something else like that? It’s unfortunate that the test takes a second to run.

> LayoutTests/fast/events/popup-blocking-timers.html:54
> +                }
> +                else {

Brace style.

[Andy Estes]

(In reply to comment #6)
> (From update of attachment 85647 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=85647&action=review
> 
> > Source/WebCore/page/DOMTimer.cpp:42
> > +static const int maxTimeoutForUserGestureForwarding = 1000; // One second matches Gecko.
> 
> The word “timeout” is used multiple ways in this file that are confusing. For example, the number of milliseconds is called a timeout in the name of the argument to the constructor and the data member m_originalTimeout. But the timers themselves are also referred to as timeouts, as in the data member m_timeoutId.
> 
> Because of that ambiguity, and because we also have two other constants here that are floating point numbers with time in milliseconds, I think this constant is slightly confusing.

Perhaps interval is a better term both for the ctor argument and for my static constant, since that is the term used by TimerBase. I'm not sure how to rectify the float vs. int issue; perhaps including units in the variable names would help (e.g. intervalInMs and maxIntervalForUserGestureForwardingInMs)?

> 
> > Source/WebCore/page/DOMTimer.cpp:64
> > +    m_indicateUserGestureWhenFired = UserGestureIndicator::processingUserGesture()
> > +        && timeout <= maxTimeoutForUserGestureForwarding
> > +        && m_nestingLevel == 1;
> 
> I’d like to see all the initialization of data members be done with data member initialization instead of assignment. That could easily be done by making a separate function to return the ID and putting this logic in a function too. They can be inline functions if desired. I wouldn’t insist on that change for this patch, but it would be nice.

Will do.

> 
> I think this expression needs a why comment for the nesting level check. The other two parts of this expression explain themselves, given the names of what’s involved, but the reason the nesting level should have an effect is more subtle.

Ok.

> 
> > Source/WebCore/page/DOMTimer.cpp:111
> > +    UserGestureIndicator gestureIndicator(m_indicateUserGestureWhenFired ? DefinitelyProcessingUserGesture : PossiblyProcessingUserGesture);
> 
> Is it helpful to set the gesture indication to “possibly” when the boolean is false? Does the test somehow cover that?

I was confused by the creation of the 'possibly' state, as the difference between PossiblyProcessingUserGesture and DefinitelyNotProcessingUserGesture is unclear to me. It seems like we should always know one way or the other.

PossiblyProcessingUserGesture is the default state for UserGestureIndicator so it's the state you'd get during a timer event prior to this patch when no gesture was present. That's why I chose it, and I don't see any behavior difference between it and DefinitelyNotProcessingUserGesture.

> 
> > Source/WebCore/page/DOMTimer.h:73
> > +        bool m_indicateUserGestureWhenFired;
> 
> For a boolean data member, we normally do not want to use something that can be read as a verb phrase. Perhaps add the word “should”.
> 
> Another possible name is m_representsForwardedUserGesture, which is consistent with the name of the maxTimeoutForUserGestureForwarding constant.

Ok, I'll stick with the "forwarded" terminology.

> 
> > LayoutTests/fast/events/popup-blocking-timers.html:5
> > +        var win;
> 
> I’m as big a fan of winning as the next guy, but I don’t think this is a great variable name.

Are you saying I should rename all instances of "PASS" to "WINNING"?

> 
> > LayoutTests/fast/events/popup-blocking-timers.html:12
> > +            layoutTestController.waitUntilDone();
> 
> Can we make this test run fast by using the leapForward feature or something else like that? It’s unfortunate that the test takes a second to run.

I was unaware of leapForward(). I should be able to use that.

> 
> > LayoutTests/fast/events/popup-blocking-timers.html:54
> > +                }
> > +                else {
> 
> Brace style.

[Andy Estes]

Committed r81057: <http://trac.webkit.org/changeset/81057>