Prevent default actions for JS-generated mouse events other than click

Don't invoke default actions for MouseEvents generated by script (other than "click") 

Brings blink in-line with the DOM events spec and behavior of IE and Firefox.

BUG=423975
R=rbyers@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196987

[bokan, 2015-02-03 01:21:42]

bokan@chromium.org changed reviewers:
+ bokan@chromium.org

[bokan, 2015-02-03 01:27:01]

Looks like this also disables the exception carved out for the 'click' event: 

"Most untrusted events should not trigger default actions, with the exception of
the click event" [1]

You should make an exception for that. Additionally, you'll want to include a
test for this (most likely a layout test, see
http://www.chromium.org/developers/testing/webkit-layout-tests and
http://www.chromium.org/blink/coding-style/layout-test-style-guidelines). 

[1] http://www.w3.org/TR/DOM-Level-3-Events/#trusted-events

[Rick Byers, 2015-02-03 03:19:36]

This approach will change the value of the Event.defaultPrevented bit (visible
from JavaScript) for these events, right?  I assume IE has
defaultPrevented=false in these cases (unless JS itself has called
preventDefault), right?

I think you want to be making the change downstream instead to avoid changing
the behavior of defaultPrevented.  You could have a similar change in
EventDispatcher::dispatchEventPostProcess, but I'm a little worried about the
potential sweeping impact of such a change (ideally we'd enumerate each thing it
can effect).  I'm OK with trying something like this if you make an effort to
enumerate exactly what all the behavior changes are (i.e. what default event
handlers are impacted).

The safest / most incremental thing to do would be to find the specific default
event handler you want to suppress here and target your fix to that.  Perhaps we
can even make a list of all handlers and work through them one at a time (with a
test case for each)?

These sorts of changes tend to have subtle compat impact.  There's decent risk
that you'll have a long tail of compat bugs - each one requiring you to revert
your change before your change hits stable.  Taking a more incremental approach
would let you work through them individually, rather than requiring all issues
be solved before shipping any change to stable. 

Either approach could work though - we can always try the "all at once" approach
and see what breaks, then re-evaluate.

Rick

[kirkshoop_msft, 2015-02-03 17:23:36]

Thank you Rick and bokan for the feedback. 
We are working on tests and discussing the approach we want to take to manage
compat.

[kirkshoop_msft, 2015-03-04 00:23:04]

On 2015/02/03 17:23:36, kirkshoop_msft wrote:
> Thank you Rick and bokan for the feedback. 
> We are working on tests and discussing the approach we want to take to manage
> compat.

As usual adding tests helps. :)

This time we 
- Added Synthetic marking to MouseEvent to track the source of the mouse event
and uses that to skip default actions.
- Added new test that fails without the code changes and succeeds with them.
- Removed existing test that fails with the new changes.

[bokan, 2015-03-04 23:17:55]

Looks like this still doesn't address the exception (for 'click' events) I
mentioned in my first reply.

https://codereview.chromium.org/894913002/diff/20001/Source/core/event_idl_fi...
File Source/core/event_idl_files_list.tmp (right):

https://codereview.chromium.org/894913002/diff/20001/Source/core/event_idl_fi...
Source/core/event_idl_files_list.tmp:1: css/FontFaceSetLoadEvent.idl
Was this file added accidentally?

[kirkshoop_msft, 2015-03-04 23:57:47]

On 2015/03/04 23:17:55, bokan wrote:
> Looks like this still doesn't address the exception (for 'click' events) I
> mentioned in my first reply.
> 
>
https://codereview.chromium.org/894913002/diff/20001/Source/core/event_idl_fi...
> File Source/core/event_idl_files_list.tmp (right):
> 
>
https://codereview.chromium.org/894913002/diff/20001/Source/core/event_idl_fi...
> Source/core/event_idl_files_list.tmp:1: css/FontFaceSetLoadEvent.idl
> Was this file added accidentally?

Yes, the tmp file was accidental and has been removed.
Also, we missed the click check, sorry about that. It has been added and because
of that the removed test has been restored. 

Thanks!

[Rick Byers, 2015-03-05 16:34:18]

This approach seems reasonable to me modulo a couple small changes.

I'm still concerned that we don't have much understanding of the behavior
changes this will make in practice.  It's nice to have a test for the focus
case, but since this change might break some websites we really should try to
generate a list of the observable behavior differences in addition to just the
one you're testing.  Depending how extensive this list is, we may want to follow
the 'intent to ship' process for web-exposed API behavior changes
(http://www.chromium.org/blink#TOC-Web-Platform-Changes:-Process), but once I
see the list I expect I'll be comfortable claiming this change is just a bug fix
and therefore "trivial".

To get a reasonable approximation of this list, I'd suggest scanning through all
implementations of Node::defaultEventHandler and looking for any that apply to
mouse events other than click.  Ideally you'd whip up a little test page that
can demonstrate the behavior of each.

To get you started, here's an initial list from a quick scan:

1. mousedown on an input type=range element activates the slider.  Demo page:
http://jsbin.com/tupewe/1/edit?html,js,output

2. 'contextmenu' event: potentially show the contextmenu.  I wouldn't be
surprised to learn there's a UserGestureIndicator check somewhere that ensures
this is a no-op when we're not in the middle of processing some real user
gesture.  Test case which doesn't seem to ever trigger the context menu:
http://jsbin.com/dahobo/1 - might be worth digging into why this doesn't repro.

3. windows middle-button pan-scrolling

4. wheel event: scroll


+igsolla@ as FYI who added the synthetic mouse event type system in
http://crbug.com/401177

See related bug http://crbug.com/405990 whose fix appears to have been reverted
and never re-landed :-(.  

Also +hayato@ who years ago added the MouseEventDispatchMediator
'isSyntheticMouseEvent' system: https://bugs.webkit.org/show_bug.cgi?id=102681

Also see the WhatWG discussion around this class of issues here:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=12230#c58

https://codereview.chromium.org/894913002/diff/40001/LayoutTests/fast/events/...
File LayoutTests/fast/events/dispatch-synthetic-mouseevent-no-action.html
(right):

https://codereview.chromium.org/894913002/diff/40001/LayoutTests/fast/events/...
LayoutTests/fast/events/dispatch-synthetic-mouseevent-no-action.html:23:
node.addEventListener('focus', focusHandler, false);
as an extra sanity check that your test is functioning as intended, please add a
mousedown handler that outputs some text to the log (eg. with the 'debug'
method).

https://codereview.chromium.org/894913002/diff/40001/LayoutTests/fast/events/...
LayoutTests/fast/events/dispatch-synthetic-mouseevent-no-action.html:26: var
event = document.createEvent("MouseEvents");
nit: might as well use the constructor syntax (new MouseEvent) since that's now
preferred over createEvent/initMouseEvent.

https://codereview.chromium.org/894913002/diff/40001/Source/core/events/Event...
File Source/core/events/EventDispatcher.cpp (right):

https://codereview.chromium.org/894913002/diff/40001/Source/core/events/Event...
Source/core/events/EventDispatcher.cpp:210: // Prevent calling default handlers
for synthetic mouse events (issue #423975)
Rather than say what the code is doing, this comment should attempt to explain
why.  Eg. "The DOM Events spec says that events dispatched by JS (other than
"click") should not have their default handlers invoked.  To mitigate
compatibility risk, we are trying to apply this rule for only mouse events at
first."

https://codereview.chromium.org/894913002/diff/40001/Source/core/events/Mouse...
File Source/core/events/MouseEvent.h (right):

https://codereview.chromium.org/894913002/diff/40001/Source/core/events/Mouse...
Source/core/events/MouseEvent.h:81: bool isSynthetic() const { return
m_syntheticEventType == PlatformMouseEvent::Synthetic; }
Again this name is confusing. IIRC we recently replaced an 'isSynthetic'
accessor with the m_syntheticEventType system to reduce confusion over different
types of 'synthetic' events.  But now I see MouseEventDispatchMediator has it's
own notion of 'synthetic' that you're mirroring here.  To avoid confusion I
think we should unify the two mechanisms together.  What I'd suggest is this:

 - Remove MouseEventDispatchMediator::m_mouseEventType
 - Update the only place that sets this (Node::dispatchEvent) to instead call
MouseEvent::setFromScript() to reset the m_syntheticEventType to 'FromScript'.
 - For consistency, update the MouseEvent constructor / JS creation code to also
set the synthetic event type to 'FromScript'
 - The comment above the definition of 'FromScript' should make clear that the
event was either created or re-dispatched by JavaScript (we don't want script to
be able to catch a real MouseEvent and dispatch it later as if it was still
real).
 - Now you can just test 'isFromScript' in
MouseEventDispatchMediator::dispatchEvent and in your default handling logic.

https://codereview.chromium.org/894913002/diff/40001/Source/platform/Platform...
File Source/platform/PlatformMouseEvent.h (right):

https://codereview.chromium.org/894913002/diff/40001/Source/platform/Platform...
Source/platform/PlatformMouseEvent.h:45: Synthetic,
Nit: can we call this 'FromScript' to distinguish it from some other types of
'synthetic' mouse events (eg. FromTouch and some special cases currently lumped
into RealOrIndistinguishable like the synthetic mousemove that occurs during
scrolling)?

[kirkshoop_msft, 2015-03-09 17:32:28]

On 2015/03/05 16:34:18, Rick Byers wrote:
> I'm still concerned that we don't have much understanding of the behavior
> changes this will make in practice.  It's nice to have a test for the focus
> case, but since this change might break some websites we really should try to
> generate a list of the observable behavior differences in addition to just the
> one you're testing.  

We are generating this list.

>
https://codereview.chromium.org/894913002/diff/40001/LayoutTests/fast/events/...
> LayoutTests/fast/events/dispatch-synthetic-mouseevent-no-action.html:23:
> node.addEventListener('focus', focusHandler, false);
> as an extra sanity check that your test is functioning as intended, please add
a
> mousedown handler that outputs some text to the log (eg. with the 'debug'
> method).
> 
done.

>
https://codereview.chromium.org/894913002/diff/40001/LayoutTests/fast/events/...
> LayoutTests/fast/events/dispatch-synthetic-mouseevent-no-action.html:26: var
> event = document.createEvent("MouseEvents");
> nit: might as well use the constructor syntax (new MouseEvent) since that's
now
> preferred over createEvent/initMouseEvent.
>
done.
 
>
https://codereview.chromium.org/894913002/diff/40001/Source/core/events/Event...
> Source/core/events/EventDispatcher.cpp:210: // Prevent calling default
handlers
> for synthetic mouse events (issue #423975)
> Rather than say what the code is doing, this comment should attempt to explain
> why.  Eg. "The DOM Events spec says that events dispatched by JS (other than
> "click") should not have their default handlers invoked.  To mitigate
> compatibility risk, we are trying to apply this rule for only mouse events at
> first."
done.

>
https://codereview.chromium.org/894913002/diff/40001/Source/core/events/Mouse...
> Source/core/events/MouseEvent.h:81: bool isSynthetic() const { return
> m_syntheticEventType == PlatformMouseEvent::Synthetic; }
> Again this name is confusing. IIRC we recently replaced an 'isSynthetic'
> accessor with the m_syntheticEventType system to reduce confusion over
different
> types of 'synthetic' events.  But now I see MouseEventDispatchMediator has
it's
> own notion of 'synthetic' that you're mirroring here.  To avoid confusion I
> think we should unify the two mechanisms together.  What I'd suggest is this:
> 
>  - Remove MouseEventDispatchMediator::m_mouseEventType
done.

>  - Update the only place that sets this (Node::dispatchEvent) to instead call
> MouseEvent::setFromScript() to reset the m_syntheticEventType to 'FromScript'.
done.

>  - For consistency, update the MouseEvent constructor / JS creation code to
also
> set the synthetic event type to 'FromScript'
I was not sure how to find all the points that would be called from script. I
made no changes to construction in this patch.

>  - The comment above the definition of 'FromScript' should make clear that the
> event was either created or re-dispatched by JavaScript (we don't want script
to
> be able to catch a real MouseEvent and dispatch it later as if it was still
> real).
I missed the mention of 're-dispatched' in the comment, I can add that perhaps
with the changes for construction.

>  - Now you can just test 'isFromScript' in
> MouseEventDispatchMediator::dispatchEvent and in your default handling logic.
done.

>
https://codereview.chromium.org/894913002/diff/40001/Source/platform/Platform...
> Source/platform/PlatformMouseEvent.h:45: Synthetic,
> Nit: can we call this 'FromScript' to distinguish it from some other types of
> 'synthetic' mouse events (eg. FromTouch and some special cases currently
lumped
> into RealOrIndistinguishable like the synthetic mousemove that occurs during
> scrolling)?
done.


Thanks for the feedback.

[Rick Byers, 2015-03-12 16:59:15]

This is looking close, thanks!

Note that rietveld (the code review tool) lets you respond to individual
comments inline in the review.  You may find this makes it easier to track,
rather than trying to put all your responses into the one big text buffer you
get with 'reply'.

On 2015/03/09 17:32:28, kirkshoop_msft wrote:
> On 2015/03/05 16:34:18, Rick Byers wrote:
> > I'm still concerned that we don't have much understanding of the behavior
> > changes this will make in practice.  It's nice to have a test for the focus
> > case, but since this change might break some websites we really should try
to
> > generate a list of the observable behavior differences in addition to just
the
> > one you're testing.  
> 
> We are generating this list.

Great, thank you!  Non-trivial I'm sure, but I think this will do a lot to help
with your bigger picture goal of improving IE/Chrome interoperability.

> >
>
https://codereview.chromium.org/894913002/diff/40001/LayoutTests/fast/events/...
> > LayoutTests/fast/events/dispatch-synthetic-mouseevent-no-action.html:23:
> > node.addEventListener('focus', focusHandler, false);
> > as an extra sanity check that your test is functioning as intended, please
add
> a
> > mousedown handler that outputs some text to the log (eg. with the 'debug'
> > method).
> > 
> done.
> 
> >
>
https://codereview.chromium.org/894913002/diff/40001/LayoutTests/fast/events/...
> > LayoutTests/fast/events/dispatch-synthetic-mouseevent-no-action.html:26: var
> > event = document.createEvent("MouseEvents");
> > nit: might as well use the constructor syntax (new MouseEvent) since that's
> now
> > preferred over createEvent/initMouseEvent.
> >
> done.
>  
> >
>
https://codereview.chromium.org/894913002/diff/40001/Source/core/events/Event...
> > Source/core/events/EventDispatcher.cpp:210: // Prevent calling default
> handlers
> > for synthetic mouse events (issue #423975)
> > Rather than say what the code is doing, this comment should attempt to
explain
> > why.  Eg. "The DOM Events spec says that events dispatched by JS (other than
> > "click") should not have their default handlers invoked.  To mitigate
> > compatibility risk, we are trying to apply this rule for only mouse events
at
> > first."
> done.
> 
> >
>
https://codereview.chromium.org/894913002/diff/40001/Source/core/events/Mouse...
> > Source/core/events/MouseEvent.h:81: bool isSynthetic() const { return
> > m_syntheticEventType == PlatformMouseEvent::Synthetic; }
> > Again this name is confusing. IIRC we recently replaced an 'isSynthetic'
> > accessor with the m_syntheticEventType system to reduce confusion over
> different
> > types of 'synthetic' events.  But now I see MouseEventDispatchMediator has
> it's
> > own notion of 'synthetic' that you're mirroring here.  To avoid confusion I
> > think we should unify the two mechanisms together.  What I'd suggest is
this:
> > 
> >  - Remove MouseEventDispatchMediator::m_mouseEventType
> done.
> 
> >  - Update the only place that sets this (Node::dispatchEvent) to instead
call
> > MouseEvent::setFromScript() to reset the m_syntheticEventType to
'FromScript'.
> done.
> 
> >  - For consistency, update the MouseEvent constructor / JS creation code to
> also
> > set the synthetic event type to 'FromScript'
> I was not sure how to find all the points that would be called from script. I
> made no changes to construction in this patch.

The entry points are MouseEvent::create (for the ctor) and
MouseEvent::initMouseEvent (for the old-style), plus the same methods in any
subclasses (eg. WheelEvent).

See
http://www.chromium.org/blink/webidl/blink-idl-extended-attributes#TOC-Constr...
for some more details about how the constructor bindings work.

> 
> >  - The comment above the definition of 'FromScript' should make clear that
the
> > event was either created or re-dispatched by JavaScript (we don't want
script
> to
> > be able to catch a real MouseEvent and dispatch it later as if it was still
> > real).
> I missed the mention of 're-dispatched' in the comment, I can add that perhaps
> with the changes for construction.
> 
> >  - Now you can just test 'isFromScript' in
> > MouseEventDispatchMediator::dispatchEvent and in your default handling
logic.
> done.
> 
> >
>
https://codereview.chromium.org/894913002/diff/40001/Source/platform/Platform...
> > Source/platform/PlatformMouseEvent.h:45: Synthetic,
> > Nit: can we call this 'FromScript' to distinguish it from some other types
of
> > 'synthetic' mouse events (eg. FromTouch and some special cases currently
> lumped
> > into RealOrIndistinguishable like the synthetic mousemove that occurs during
> > scrolling)?
> done.
> 
> 
> Thanks for the feedback.

[kirkshoop_msft, 2015-03-12 20:49:05]

I have some html files for the cases that this change affects, is there a way to
attach them to this review?

On 2015/03/12 16:59:15, Rick Byers wrote:
> This is looking close, thanks!
> 
> Note that rietveld (the code review tool) lets you respond to individual
> comments inline in the review.  You may find this makes it easier to track,
> rather than trying to put all your responses into the one big text buffer you
> get with 'reply'.
> 

Ah yes, I see that now, Thanks!

> > I was not sure how to find all the points that would be called from script.
I
> > made no changes to construction in this patch.
> 
> The entry points are MouseEvent::create (for the ctor) and
> MouseEvent::initMouseEvent (for the old-style), plus the same methods in any
> subclasses (eg. WheelEvent).
> 
> See
>
http://www.chromium.org/blink/webidl/blink-idl-extended-attributes#TOC-Constr...
> for some more details about how the constructor bindings work.
> 

Done in Patch Set 5. The Wheel Event was the only subclass I found and it
delegates to the MouseEvent, so I only changed MouseEvent.

[kirkshoop_msft, 2015-03-12 22:34:14]

On 2015/03/12 20:49:05, kirkshoop_msft wrote:
> I have some html files for the cases that this change affects, is there a way
to
> attach them to this review?
> 

I built a jsbin for each:

unchanged
http://jsbin.com/rekike/6/edit?html,js,output
http://jsbin.com/zunema/18/edit?html,js,output
http://jsbin.com/saludu/40/edit?html,js,output
http://jsbin.com/xiguma/8/edit?html,js,output
http://jsbin.com/duzole/10/edit?html,js,output
http://jsbin.com/ninilu/10/edit?html,js,output
http://jsbin.com/lujizi/7/edit?html,js,output
http://jsbin.com/resimi/8/edit?html,js,output
http://jsbin.com/qudage/8/edit?html,js,output

changed - (default action now skipped)
http://jsbin.com/gixivo/10/edit?html,js,output
http://jsbin.com/jeloz/10/edit?html,js,output

[kirkshoop_msft, 2015-03-24 22:22:41]

Is any more data needed for these changes?

[Rick Byers, 2015-03-26 21:27:04]

On 2015/03/12 22:34:14, kirkshoop_msft wrote:
> On 2015/03/12 20:49:05, kirkshoop_msft wrote:
> > I have some html files for the cases that this change affects, is there a
way
> to
> > attach them to this review?
> > 
> 
> I built a jsbin for each:
> 
> unchanged
> http://jsbin.com/rekike/6/edit?html,js,output
> http://jsbin.com/zunema/18/edit?html,js,output
> http://jsbin.com/saludu/40/edit?html,js,output
> http://jsbin.com/xiguma/8/edit?html,js,output
> http://jsbin.com/duzole/10/edit?html,js,output
> http://jsbin.com/ninilu/10/edit?html,js,output
> http://jsbin.com/lujizi/7/edit?html,js,output
> http://jsbin.com/resimi/8/edit?html,js,output
> http://jsbin.com/qudage/8/edit?html,js,output
> 
> changed - (default action now skipped)
> http://jsbin.com/gixivo/10/edit?html,js,output
> http://jsbin.com/jeloz/10/edit?html,js,output

Shoot - sorry for the delay.  I've been swamped and missed these e-mails (in
part due to all my work for our Pointer Events announcement, so hopefully a net
win for MS <grin>).  I'm looking now...

[Rick Byers, 2015-03-26 21:36:45]

On 2015/03/12 22:34:14, kirkshoop_msft wrote:
> On 2015/03/12 20:49:05, kirkshoop_msft wrote:
> > I have some html files for the cases that this change affects, is there a
way
> to
> > attach them to this review?
> > 
> 
> I built a jsbin for each:
> 
> unchanged
> http://jsbin.com/rekike/6/edit?html,js,output
> http://jsbin.com/zunema/18/edit?html,js,output
> http://jsbin.com/saludu/40/edit?html,js,output
> http://jsbin.com/xiguma/8/edit?html,js,output
> http://jsbin.com/duzole/10/edit?html,js,output
> http://jsbin.com/ninilu/10/edit?html,js,output
> http://jsbin.com/lujizi/7/edit?html,js,output
> http://jsbin.com/resimi/8/edit?html,js,output
> http://jsbin.com/qudage/8/edit?html,js,output
> 
> changed - (default action now skipped)
> http://jsbin.com/gixivo/10/edit?html,js,output
> http://jsbin.com/jeloz/10/edit?html,js,output

This is great, thanks!  The unchanged ones are all around 'click' which is
treated specially by the specs (so not necessarily something we'll want to
change), right?  I'm surprised there doesn't seem to be any other changed
behavior for mousedown other than those two that we knew about.  Thanks for
doing the digging!  I still expect some surprises somewhere (I don't think I've
ever seen a non-trivial mouse-event change that didn't cause some sort of issue
on some random website <grin>), but nothing left to do now but try :-)

[Rick Byers, 2015-03-26 21:37:26]

LGTM

[Rick Byers, 2015-03-26 21:38:47]

rbyers@chromium.org changed reviewers:
+ pdr@chromium.org

[Rick Byers, 2015-03-26 21:39:10]

+pdr for OWNERS stamp in Source/platform

[kirkshoop_msft, 2015-03-26 21:42:59]

> Shoot - sorry for the delay.  I've been swamped and missed these e-mails (in
> part due to all my work for our Pointer Events announcement, so hopefully a
net
> win for MS <grin>).  I'm looking now...

Yes, my twitter feed lit up for that announcement. It is great to have everyone
working together on a consistent and performant web platform.

[pdr., 2015-03-26 21:43:20]

lgtm

[kirkshoop_msft, 2015-03-26 21:47:03]

On 2015/03/26 21:36:45, Rick Byers wrote:
> On 2015/03/12 22:34:14, kirkshoop_msft wrote:
> > On 2015/03/12 20:49:05, kirkshoop_msft wrote:
> > > I have some html files for the cases that this change affects, is there a
> way
> > to
> > > attach them to this review?
> > > 
> > 
> > I built a jsbin for each:
> > 
> > unchanged
> > http://jsbin.com/rekike/6/edit?html,js,output
> > http://jsbin.com/zunema/18/edit?html,js,output
> > http://jsbin.com/saludu/40/edit?html,js,output
> > http://jsbin.com/xiguma/8/edit?html,js,output
> > http://jsbin.com/duzole/10/edit?html,js,output
> > http://jsbin.com/ninilu/10/edit?html,js,output
> > http://jsbin.com/lujizi/7/edit?html,js,output
> > http://jsbin.com/resimi/8/edit?html,js,output
> > http://jsbin.com/qudage/8/edit?html,js,output
> > 
> > changed - (default action now skipped)
> > http://jsbin.com/gixivo/10/edit?html,js,output
> > http://jsbin.com/jeloz/10/edit?html,js,output
> 
> This is great, thanks!  The unchanged ones are all around 'click' which is
> treated specially by the specs (so not necessarily something we'll want to
> change), right?  I'm surprised there doesn't seem to be any other changed
> behavior for mousedown other than those two that we knew about.  Thanks for
> doing the digging!  I still expect some surprises somewhere (I don't think
I've
> ever seen a non-trivial mouse-event change that didn't cause some sort of
issue
> on some random website <grin>), but nothing left to do now but try :-)

correct, the click default actions are supposed to happen, so I am happy that
their behavior is unchanged.
Thank you for all your help and feedback! I look forward to seeing how it
performs in the wild. 
As a newbie, let me know if there is anything more I need to do, or any way that
I can help.

[Rick Byers, 2015-03-27 13:12:26]

Just click the "CQ" button to run tests and attempt to land your CL. I'll
loop you in on any potential issues that come up.
On Mar 26, 2015 5:47 PM, <kirk.shoop@microsoft.com> wrote:

> On 2015/03/26 21:36:45, Rick Byers wrote:
>
>> On 2015/03/12 22:34:14, kirkshoop_msft wrote:
>> > On 2015/03/12 20:49:05, kirkshoop_msft wrote:
>> > > I have some html files for the cases that this change affects, is
>> there a
>> way
>> > to
>> > > attach them to this review?
>> > >
>> >
>> > I built a jsbin for each:
>> >
>> > unchanged
>> > http://jsbin.com/rekike/6/edit?html,js,output
>> > http://jsbin.com/zunema/18/edit?html,js,output
>> > http://jsbin.com/saludu/40/edit?html,js,output
>> > http://jsbin.com/xiguma/8/edit?html,js,output
>> > http://jsbin.com/duzole/10/edit?html,js,output
>> > http://jsbin.com/ninilu/10/edit?html,js,output
>> > http://jsbin.com/lujizi/7/edit?html,js,output
>> > http://jsbin.com/resimi/8/edit?html,js,output
>> > http://jsbin.com/qudage/8/edit?html,js,output
>> >
>> > changed - (default action now skipped)
>> > http://jsbin.com/gixivo/10/edit?html,js,output
>> > http://jsbin.com/jeloz/10/edit?html,js,output
>>
>
>  This is great, thanks!  The unchanged ones are all around 'click' which is
>> treated specially by the specs (so not necessarily something we'll want to
>> change), right?  I'm surprised there doesn't seem to be any other changed
>> behavior for mousedown other than those two that we knew about.  Thanks
>> for
>> doing the digging!  I still expect some surprises somewhere (I don't think
>>
> I've
>
>> ever seen a non-trivial mouse-event change that didn't cause some sort of
>>
> issue
>
>> on some random website <grin>), but nothing left to do now but try :-)
>>
>
> correct, the click default actions are supposed to happen, so I am happy
> that
> their behavior is unchanged.
> Thank you for all your help and feedback! I look forward to seeing how it
> performs in the wild.
> As a newbie, let me know if there is anything more I need to do, or any
> way that
> I can help.
>
>
> https://codereview.chromium.org/894913002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[kirkshoop_msft, 2015-03-27 16:40:34]

The CQ bit was checked by kirk.shoop@microsoft.com

[commit-bot: I haz the power, 2015-03-27 16:41:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/894913002/80001

[Rick Byers, 2015-03-27 17:33:06]

On 2015/03/27 16:41:16, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/894913002/80001

Looks like you've got some test failures.  The one I looked at was due to a test
that expected to be able to use synthetic mouse events to manipulate a slider
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).
 You probably want to update such tests to use the eventSender API (see tests in
fast/events/) instead of relying on this behavior that you're fixing.

[kirkshoop_msft, 2015-03-27 17:35:25]

On 2015/03/27 17:33:06, Rick Byers wrote:
> On 2015/03/27 16:41:16, I haz the power (commit-bot) wrote:
> > CQ is trying da patch. Follow status at
> >  https://chromium-cq-status.appspot.com/patch-status/894913002/80001
> 
> Looks like you've got some test failures.  The one I looked at was due to a
test
> that expected to be able to use synthetic mouse events to manipulate a slider
>
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).
>  You probably want to update such tests to use the eventSender API (see tests
in
> fast/events/) instead of relying on this behavior that you're fixing.

Yes, I agree, we will work on those tests. Thanks for the pointer to
eventSender, that will save us time!

[commit-bot: I haz the power, 2015-03-27 19:31:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-27 19:31:55]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/5...)

[Rick Byers, 2015-03-31 18:28:36]

On 2015/03/27 19:31:55, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   linux_blink_rel on tryserver.blink (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/5...)

Note that you can now use the 'CQ dry run' option to re-run tests when you're
ready (since you already have my l-g-t-m).  I assume they're failing
consistently across platforms so hopefully you have no trouble running the tests
in question locally.

[kirkshoop_msft, 2015-04-17 15:54:12]

The CQ bit was checked by kirk.shoop@microsoft.com to run a CQ dry run

[kirkshoop_msft, 2015-04-17 15:54:12]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/894913002/#ps100001
(title: "changed failing tests to use eventsender")

[commit-bot: I haz the power, 2015-04-17 15:54:30]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/894913002/100001

[Rick Byers, 2015-04-17 16:05:49]

Kirk, it looks like I need you to follow the external contributors process (sign
the CLA and add yourself to Authors) before we can land this CL:
https://www.chromium.org/developers/contributing-code/external-contributor-ch....

I thought Scott Blomquist did this for MS OpenTech already, but I realized I
better check and it appears that he ultimately went through the individual
process instead of the corporate process.  You may want to discuss with him -
hopefully it's trivial for you to go through the individual process as well as
he did.

So this NOT LGTM until I can verify that (sorry it didn't occur to me to check
this earlier).

[kirkshoop_msft, 2015-04-17 16:12:50]

On 2015/04/17 16:05:49, Rick Byers wrote:
> Kirk, it looks like I need you to follow the external contributors process
(sign
> the CLA and add yourself to Authors) before we can land this CL:
>
https://www.chromium.org/developers/contributing-code/external-contributor-ch....
> 
> I thought Scott Blomquist did this for MS OpenTech already, but I realized I
> better check and it appears that he ultimately went through the individual
> process instead of the corporate process.  You may want to discuss with him -
> hopefully it's trivial for you to go through the individual process as well as
> he did.
> 
> So this NOT LGTM until I can verify that (sorry it didn't occur to me to check
> this earlier).

I did sign the Google iCLA for my Microsoft address (kirk.shoop@microsoft.com).
pdr@google.com confirmed this in February. Can you check again?

Thanks,

[Rick Byers, 2015-04-17 16:15:25]

On 2015/04/17 16:12:50, kirkshoop_msft wrote:
> On 2015/04/17 16:05:49, Rick Byers wrote:
> > Kirk, it looks like I need you to follow the external contributors process
> (sign
> > the CLA and add yourself to Authors) before we can land this CL:
> >
>
https://www.chromium.org/developers/contributing-code/external-contributor-ch....
> > 
> > I thought Scott Blomquist did this for MS OpenTech already, but I realized I
> > better check and it appears that he ultimately went through the individual
> > process instead of the corporate process.  You may want to discuss with him
-
> > hopefully it's trivial for you to go through the individual process as well
as
> > he did.
> > 
> > So this NOT LGTM until I can verify that (sorry it didn't occur to me to
check
> > this earlier).
> 
> I did sign the Google iCLA for my Microsoft address
(mailto:kirk.shoop@microsoft.com).
> mailto:pdr@google.com confirmed this in February. Can you check again?
> 
> Thanks,

Ah, sorry - yes I see it!  You still need to add yourself (or *@microsoft.com)
to src/OWNERS though.  sblom is the only one there at the moment.

[Rick Byers, 2015-04-17 16:16:00]

On 2015/04/17 16:15:25, Rick Byers wrote:
> On 2015/04/17 16:12:50, kirkshoop_msft wrote:
> > On 2015/04/17 16:05:49, Rick Byers wrote:
> > > Kirk, it looks like I need you to follow the external contributors process
> > (sign
> > > the CLA and add yourself to Authors) before we can land this CL:
> > >
> >
>
https://www.chromium.org/developers/contributing-code/external-contributor-ch....
> > > 
> > > I thought Scott Blomquist did this for MS OpenTech already, but I realized
I
> > > better check and it appears that he ultimately went through the individual
> > > process instead of the corporate process.  You may want to discuss with
him
> -
> > > hopefully it's trivial for you to go through the individual process as
well
> as
> > > he did.
> > > 
> > > So this NOT LGTM until I can verify that (sorry it didn't occur to me to
> check
> > > this earlier).
> > 
> > I did sign the Google iCLA for my Microsoft address
> (mailto:kirk.shoop@microsoft.com).
> > mailto:pdr@google.com confirmed this in February. Can you check again?
> > 
> > Thanks,
> 
> Ah, sorry - yes I see it!  You still need to add yourself (or
mailto:*@microsoft.com)
> to src/OWNERS though.  sblom is the only one there at the moment.

Hah, funny typo - that's src/AUTHORS (I knew something didn't sound right about
*@microsoft.com being an OWNER of everything under src/ <grin>).

[commit-bot: I haz the power, 2015-04-17 16:19:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-17 16:19:09]

Dry run: A disapproval has been posted by following reviewers:
rbyers@chromium.org.
Please make sure to get positive review before another CQ attempt.

[kirkshoop_msft, 2015-04-17 16:31:23]

On 2015/04/17 16:16:00, Rick Byers wrote:
> On 2015/04/17 16:15:25, Rick Byers wrote:
> > Ah, sorry - yes I see it!  You still need to add yourself (or
> mailto:*@microsoft.com)
> > to src/OWNERS though.  sblom is the only one there at the moment.
> 
> Hah, funny typo - that's src/AUTHORS (I knew something didn't sound right
about
> mailto:*@microsoft.com being an OWNER of everything under src/ <grin>).

okay. is there a way to add the AUTHORS change to this review or do I start
another one?

Thanks,

[Rick Byers, 2015-04-17 16:42:02]

On 2015/04/17 16:31:23, kirkshoop_msft wrote:
> On 2015/04/17 16:16:00, Rick Byers wrote:
> > On 2015/04/17 16:15:25, Rick Byers wrote:
> > > Ah, sorry - yes I see it!  You still need to add yourself (or
> > mailto:*@microsoft.com)
> > > to src/OWNERS though.  sblom is the only one there at the moment.
> > 
> > Hah, funny typo - that's src/AUTHORS (I knew something didn't sound right
> about
> > mailto:*@microsoft.com being an OWNER of everything under src/ <grin>).
> 
> okay. is there a way to add the AUTHORS change to this review or do I start
> another one?
> 
> Thanks,

No unfortunately that's still in the chromium tree.  We're close to merging the
chromium and blink repositories into one, but for now you'll need a separate CL.
 Should be trivial though - add me as a reviewer and I'll approve immediately.

[kirkshoop_msft, 2015-04-17 21:29:18]

On 2015/04/17 16:42:02, Rick Byers wrote:
> No unfortunately that's still in the chromium tree.  We're close to merging
the
> chromium and blink repositories into one, but for now you'll need a separate
CL.
>  Should be trivial though - add me as a reviewer and I'll approve immediately.

okay the AUTHORS change is here : https://codereview.chromium.org/1090293002

[kirkshoop_msft, 2015-04-20 18:19:05]

On 2015/04/17 21:29:18, kirkshoop_msft wrote:
> On 2015/04/17 16:42:02, Rick Byers wrote:
> > No unfortunately that's still in the chromium tree.  We're close to merging
> the
> > chromium and blink repositories into one, but for now you'll need a separate
> CL.
> >  Should be trivial though - add me as a reviewer and I'll approve
immediately.
> 
> okay the AUTHORS change is here : https://codereview.chromium.org/1090293002

The AUTHORS change has landed, can you l-g-t-m this again so that I can try CQ?

Thanks

[Rick Byers, 2015-04-20 18:39:35]

The CQ bit was checked by rbyers@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-04-20 18:40:11]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/894913002/100001

[Rick Byers, 2015-04-20 18:41:27]

Test changes look good, thanks.  This is also a nice validation that you're
changing only the things we expected (select boxes, range sliders, etc.)!

If you search for uses of eventSender in our tests, you'll see that most are
conditional somehow.  This is to ease interactive debugging/testing in a real
browser (instead of content_shell --dump-render-tree used for tests), in which
case eventSender won't exist.  Please follow a similar pattern (test / fail,
etc.) in the tests you're modifying.

I submitted a new CQ dry-run for you so you can get test results now.

https://codereview.chromium.org/894913002/diff/100001/LayoutTests/fast/forms/...
File LayoutTests/fast/forms/listbox-onchange.html (right):

https://codereview.chromium.org/894913002/diff/100001/LayoutTests/fast/forms/...
LayoutTests/fast/forms/listbox-onchange.html:25: 
Nit: this test was originally designed to work directly in a real browser,
that's now impossible due to the dependence on eventSender.  The normal way we
handle this is to check for the existence of eventSender and fail with a clear
error when it's not there.  Eg:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[commit-bot: I haz the power, 2015-04-20 23:37:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-20 23:37:17]

Dry run: Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/52629)

[Rick Byers, 2015-05-21 17:28:30]

On 2015/04/20 23:37:17, I haz the power (commit-bot) wrote:
> Dry run: Try jobs failed on following builders:
>   mac_blink_rel on tryserver.blink (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/52629)

Any update?  Looks like there are plugin tests failures on windows and Mac that
are related somehow (not getting some mouse-related events, eg. see
https://storage.googleapis.com/chromium-layout-test-archives/win_blink_rel/59...).
 These are using eventSender so shouldn't be considered 'synthetic' events but
maybe the plugin case causes them to be seen as synthetic somehow?  Or maybe the
fact that it's a plugin isn't too relevant but it's that drag and drop events
are involved (which are also MouseEvent instances I believe, and so effected by
your change).  Let me know if you 

BTW, the code changes are still LGTM (in case the CQ needs that stamp to
re-try).

I've also updated your CL description to match your current implementation. 
Feel free to adjust the wording - just didn't want to accidentally land it
describing the original (incorrect) implementation.

[kirkshoop_msft, 2015-05-28 20:28:40]

On 2015/05/21 17:28:30, Rick Byers wrote:
> On 2015/04/20 23:37:17, I haz the power (commit-bot) wrote:
> > Dry run: Try jobs failed on following builders:
> >   mac_blink_rel on tryserver.blink (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/52629)
> 
> Any update?  Looks like there are plugin tests failures on windows and Mac
that
> are related somehow (not getting some mouse-related events, eg. see
>
https://storage.googleapis.com/chromium-layout-test-archives/win_blink_rel/59...).

Hi, I just got back from a long vacation. I synced and rebased and I am working
on the failing tests.

>  These are using eventSender so shouldn't be considered 'synthetic' events but
> maybe the plugin case causes them to be seen as synthetic somehow?  Or maybe
the
> fact that it's a plugin isn't too relevant but it's that drag and drop events
> are involved (which are also MouseEvent instances I believe, and so effected
by
> your change).  Let me know if you 
> 

Thank you for your thoughts on this, I will keep it in mind.

> BTW, the code changes are still LGTM (in case the CQ needs that stamp to
> re-try).
> 

Thanks, once I have an update I'll re-try.

> I've also updated your CL description to match your current implementation. 
> Feel free to adjust the wording - just didn't want to accidentally land it
> describing the original (incorrect) implementation.

Yes, the description looks good. Thanks!

[kirkshoop_msft, 2015-05-29 20:32:04]

kirkshoop_msft wrote:
> On 2015/05/21 17:28:30, Rick Byers wrote:
> >  These are using eventSender so shouldn't be considered 'synthetic' events
but
> > maybe the plugin case causes them to be seen as synthetic somehow?  Or maybe
> the
> > fact that it's a plugin isn't too relevant but it's that drag and drop
events
> > are involved (which are also MouseEvent instances I believe, and so effected
> by
> > your change).  Let me know if you 
> > 
> 
> Thank you for your thoughts on this, I will keep it in mind.
> 

The issue seems to be that Drag events are dispatched differently than hardware
mouse events.

EventHandler::dispatchDragEvent calls dragTarget->dispatchEvent(me.get(),
IGNORE_EXCEPTION);
This ends up in Node::dispatchEvent which marks the event as 'FromScript' even
though it is not.

I have been experimenting with additional state in MouseEvent to track this, but
it would be much cleaner if the fromscript dispatch codepath was different from
the codepath for drag events. I would like to change
EventHandler::dispatchDragEvent to dispatch like a hardware event rather than
call the same dispatch method that a script would. 

@rbyers, do you have any thoughts or pointers?

Thanks,

[Rick Byers, 2015-06-05 20:13:48]

On 2015/05/29 20:32:04, kirkshoop_msft wrote:
> kirkshoop_msft wrote:
> > On 2015/05/21 17:28:30, Rick Byers wrote:
> > >  These are using eventSender so shouldn't be considered 'synthetic' events
> but
> > > maybe the plugin case causes them to be seen as synthetic somehow?  Or
maybe
> > the
> > > fact that it's a plugin isn't too relevant but it's that drag and drop
> events
> > > are involved (which are also MouseEvent instances I believe, and so
effected
> > by
> > > your change).  Let me know if you 
> > > 
> > 
> > Thank you for your thoughts on this, I will keep it in mind.
> > 
> 
> The issue seems to be that Drag events are dispatched differently than
hardware
> mouse events.
> 
> EventHandler::dispatchDragEvent calls dragTarget->dispatchEvent(me.get(),
> IGNORE_EXCEPTION);
> This ends up in Node::dispatchEvent which marks the event as 'FromScript' even
> though it is not.
> 
> I have been experimenting with additional state in MouseEvent to track this,
but
> it would be much cleaner if the fromscript dispatch codepath was different
from
> the codepath for drag events. I would like to change
> EventHandler::dispatchDragEvent to dispatch like a hardware event rather than
> call the same dispatch method that a script would. 
> 
> @rbyers, do you have any thoughts or pointers?
> 
> Thanks,

Ah, interesting - looks like a pre-existing bug (that probably doesn't have any
real observable manifestation yet).  Good catch!  /cc dcheng@ who is the
drag-and-drop expert.

I'd suggest the following:
 - Add a comment in Node.h on dispatchEvent making it clear it's intended to be
called for script dispatched events only.
 - Use an alternate code path for existing callers triggered by real input. 
Mostly they'd go through Node::dispatchMouseEvent today, but in this case you
don't have a PlatformMouseEvent so you don't want that.

So maybe just add another overload of Node::dispatchMouseEvent that takes a
MouseEvent (like the existing Node::dispatchTouchEvent).  Then it should be easy
for you to change the EventHandler::dispatchDragEvent path to go directly to
that instead of via EventTarget::dispatchEvent.

[kirkshoop_msft, 2015-06-09 21:22:38]

On 2015/06/05 20:13:48, Rick Byers wrote:
> I'd suggest the following:
>  - Add a comment in Node.h on dispatchEvent making it clear it's intended to
be
> called for script dispatched events only.

done.

>  - Use an alternate code path for existing callers triggered by real input. 
> Mostly they'd go through Node::dispatchMouseEvent today, but in this case you
> don't have a PlatformMouseEvent so you don't want that.
> 
> So maybe just add another overload of Node::dispatchMouseEvent that takes a
> MouseEvent (like the existing Node::dispatchTouchEvent).  Then it should be
easy
> for you to change the EventHandler::dispatchDragEvent path to go directly to
> that instead of via EventTarget::dispatchEvent.

Thanks for the pointer, this is done and looks good. 

I still have one test showing a regression locally.
  plugins/user-gesture.html

this test uses both eventSender and dispatchEvent to test that eventSender
results in gesture recognition and dispatchEvent does not.
With this change the dispatchEvent calls do not make it to the plugin and so the
test plugin output has changed.
Is it okay for the plugin not to receive events from dispatchEvent calls?

[Rick Byers, 2015-06-10 21:52:16]

On 2015/06/09 21:22:38, kirkshoop_msft wrote:
> On 2015/06/05 20:13:48, Rick Byers wrote:
> > I'd suggest the following:
> >  - Add a comment in Node.h on dispatchEvent making it clear it's intended to
> be
> > called for script dispatched events only.
> 
> done.
> 
> >  - Use an alternate code path for existing callers triggered by real input. 
> > Mostly they'd go through Node::dispatchMouseEvent today, but in this case
you
> > don't have a PlatformMouseEvent so you don't want that.
> > 
> > So maybe just add another overload of Node::dispatchMouseEvent that takes a
> > MouseEvent (like the existing Node::dispatchTouchEvent).  Then it should be
> easy
> > for you to change the EventHandler::dispatchDragEvent path to go directly to
> > that instead of via EventTarget::dispatchEvent.
> 
> Thanks for the pointer, this is done and looks good. 
> 
> I still have one test showing a regression locally.
>   plugins/user-gesture.html
> 
> this test uses both eventSender and dispatchEvent to test that eventSender
> results in gesture recognition and dispatchEvent does not.
> With this change the dispatchEvent calls do not make it to the plugin and so
the
> test plugin output has changed.
> Is it okay for the plugin not to receive events from dispatchEvent calls?

Ah, interesting - so this is another visible behavior change of your patch. 
Previously plugins would see mouse events generated by script, now they wont.  I
tend to think that's OK (perhaps even desirable) but it could be a little risky.
 I say just go for it, but update the test to make it clear that not only do we
not expect a user gesture in the synthetic case, we don't expect to see the
events at all!

[Rick Byers, 2015-06-10 21:56:57]

Was just reviewing again and then realized you haven't actually uploaded your
latest patch (to fix the drag issue)...

https://codereview.chromium.org/894913002/diff/100001/LayoutTests/fast/events...
File LayoutTests/fast/events/dispatch-synthetic-mouseevent-no-action.html
(right):

https://codereview.chromium.org/894913002/diff/100001/LayoutTests/fast/events...
LayoutTests/fast/events/dispatch-synthetic-mouseevent-no-action.html:3: <head>
nit: omit html, head, body tags:
https://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-...

https://codereview.chromium.org/894913002/diff/100001/LayoutTests/fast/events...
LayoutTests/fast/events/dispatch-synthetic-mouseevent-no-action.html:11:
description("Tests to ensure that default action does not occur.");
nit: add "for synthetic mouse events".

[kirkshoop_msft, 2015-06-10 23:07:34]

On 2015/06/10 21:56:57, Rick Byers wrote:
> Was just reviewing again and then realized you haven't actually uploaded your
> latest patch (to fix the drag issue)...

Sorry, I was waiting until I had the known issues fixed. Patch 7 has the drag
fix

>
https://codereview.chromium.org/894913002/diff/100001/LayoutTests/fast/events...
> File LayoutTests/fast/events/dispatch-synthetic-mouseevent-no-action.html
> (right):
> 
>
https://codereview.chromium.org/894913002/diff/100001/LayoutTests/fast/events...
> LayoutTests/fast/events/dispatch-synthetic-mouseevent-no-action.html:3: <head>
> nit: omit html, head, body tags:
>
https://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-...
> 
>
https://codereview.chromium.org/894913002/diff/100001/LayoutTests/fast/events...
> LayoutTests/fast/events/dispatch-synthetic-mouseevent-no-action.html:11:
> description("Tests to ensure that default action does not occur.");
> nit: add "for synthetic mouse events".

These comments have been addressed as well.

[kirkshoop_msft, 2015-06-10 23:13:10]

The CQ bit was checked by kirk.shoop@microsoft.com to run a CQ dry run

[kirkshoop_msft, 2015-06-10 23:13:11]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/894913002/#ps120001
(title: "fix for drag events and test updates")

[commit-bot: I haz the power, 2015-06-10 23:13:32]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/894913002/120001

[commit-bot: I haz the power, 2015-06-10 23:16:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-10 23:16:58]

Dry run: Try jobs failed on following builders:
  mac_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/58456)

[kirkshoop_msft, 2015-06-10 23:56:38]

The CQ bit was checked by kirk.shoop@microsoft.com to run a CQ dry run

[kirkshoop_msft, 2015-06-10 23:56:38]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/894913002/#ps140001
(title: "EventHandler.cpp moved from page to input dir")

[commit-bot: I haz the power, 2015-06-10 23:56:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/894913002/140001

[Rick Byers, 2015-06-11 00:06:48]

LGTM

https://codereview.chromium.org/894913002/diff/140001/LayoutTests/fast/events...
File LayoutTests/fast/events/dispatch-synthetic-mouseevent-no-action.html
(right):

https://codereview.chromium.org/894913002/diff/140001/LayoutTests/fast/events...
LayoutTests/fast/events/dispatch-synthetic-mouseevent-no-action.html:7:
description("Tests to ensure that default action does not occur.");
nit: looks like you missed updating this: ..."for synthetic mouse events".

[kirkshoop_msft, 2015-06-11 00:31:44]

ah shucks.. I updated the failure message instead of the test description.

I decided to use "for mouse events from script" instead since synthetic events
may not be from script. I understand that Touch and Keyboard will synthesize
mouse events and that Mouse events synthesize Drag events, but this test only
applies to scripted Mouse events.

I will add patch 9 with this text change when the dry run finishes, just in case
CQ finds an issue.

[commit-bot: I haz the power, 2015-06-11 01:02:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-11 01:02:24]

Dry run: Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6...)

[Rick Byers, 2015-06-11 01:16:55]

On 2015/06/11 00:31:44, kirkshoop_msft wrote:
> ah shucks.. I updated the failure message instead of the test description.
> 
> I decided to use "for mouse events from script" instead since synthetic events
> may not be from script. I understand that Touch and Keyboard will synthesize
> mouse events and that Mouse events synthesize Drag events, but this test only
> applies to scripted Mouse events.
> 
> I will add patch 9 with this text change when the dry run finishes, just in
case
> CQ finds an issue.

Sounds good, no worries :-)

[Rick Byers, 2015-06-11 01:21:28]

On 2015/06/11 01:02:24, commit-bot: I haz the power wrote:
> Dry run: Try jobs failed on following builders:
>   linux_blink_rel on tryserver.blink (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6...)

By checking
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/ it looks
like something is badly broken in tip of tree right now.  I assume sheriffs are
on it (but you could ping #chromium to verify), but it's easiest to just try
again in a little bit :-)

[kirkshoop_msft, 2015-06-11 16:51:05]

The CQ bit was checked by kirk.shoop@microsoft.com to run a CQ dry run

[kirkshoop_msft, 2015-06-11 16:51:06]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/894913002/#ps160001
(title: "changed description of new test")

[commit-bot: I haz the power, 2015-06-11 16:51:33]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/894913002/160001

[commit-bot: I haz the power, 2015-06-11 16:55:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-11 16:55:57]

Dry run: Try jobs failed on following builders:
  mac_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/58595)

[kirkshoop_msft, 2015-06-11 17:08:15]

The CQ bit was checked by kirk.shoop@microsoft.com to run a CQ dry run

[kirkshoop_msft, 2015-06-11 17:08:15]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/894913002/#ps180001
(title: "TIL, gclient sync may rebase changes back in time")

[commit-bot: I haz the power, 2015-06-11 17:08:38]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/894913002/180001

[commit-bot: I haz the power, 2015-06-11 18:34:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-11 18:34:31]

Dry run: This issue passed the CQ dry run.

[kirkshoop_msft, 2015-06-11 21:22:07]

On 2015/06/11 18:34:31, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

@rbyers, well this is exciting! what is the next step?

[dcheng, 2015-06-11 23:29:07]

On 2015/06/11 at 21:22:07, kirk.shoop wrote:
> On 2015/06/11 18:34:31, commit-bot: I haz the power wrote:
> > Dry run: This issue passed the CQ dry run.
> 
> @rbyers, well this is exciting! what is the next step?

Just check the "Commit" box (or click the "Commit" button if you're using the
non-deprecated UI).

[kirkshoop_msft, 2015-06-12 00:58:03]

The CQ bit was checked by kirk.shoop@microsoft.com

[commit-bot: I haz the power, 2015-06-12 00:58:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/894913002/180001

[commit-bot: I haz the power, 2015-06-12 01:02:19]

Committed patchset #10 (id:180001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196987

[kochi, 2015-06-12 08:41:08]

On 2015/06/12 01:02:19, commit-bot: I haz the power wrote:
> Committed patchset #10 (id:180001) as
> https://src.chromium.org/viewvc/blink?view=rev&revision=196987

It looks like this is blocking the blink-dep-roller to roll Blink changes to
Chromium.
This changed the behavior of popup and preventing existing popup test from
working?

See the first failed roll
https://codereview.chromium.org/1182663005/

content_browsertets's
DumpAccessibilityEventsTest.AccessibilityEventsMenuListPopup is
failing on win platforms.

[Rick Byers, 2015-06-12 17:02:56]

I'm away from my desk for a few hours but sounds like we should revert ASAP
while we investigate.  Blink CQ should have caught this, right?
On Jun 12, 2015 4:41 AM, <kochi@chromium.org> wrote:

> On 2015/06/12 01:02:19, commit-bot: I haz the power wrote:
>
>> Committed patchset #10 (id:180001) as
>> https://src.chromium.org/viewvc/blink?view=rev&revision=196987
>>
>
> It looks like this is blocking the blink-dep-roller to roll Blink changes
> to
> Chromium.
> This changed the behavior of popup and preventing existing popup test from
> working?
>
> See the first failed roll
> https://codereview.chromium.org/1182663005/
>
> content_browsertets's
> DumpAccessibilityEventsTest.AccessibilityEventsMenuListPopup is
> failing on win platforms.
>
> https://codereview.chromium.org/894913002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Rick Byers, 2015-06-12 17:04:46]

Actually, the right fix will almost certainly be to the test.  Maybe just
disable the test temporarily instead of its only one?
On Jun 12, 2015 1:02 PM, "Rick Byers" <rbyers@chromium.org> wrote:

> I'm away from my desk for a few hours but sounds like we should revert
> ASAP while we investigate.  Blink CQ should have caught this, right?
> On Jun 12, 2015 4:41 AM, <kochi@chromium.org> wrote:
>
>> On 2015/06/12 01:02:19, commit-bot: I haz the power wrote:
>>
>>> Committed patchset #10 (id:180001) as
>>> https://src.chromium.org/viewvc/blink?view=rev&revision=196987
>>>
>>
>> It looks like this is blocking the blink-dep-roller to roll Blink changes
>> to
>> Chromium.
>> This changed the behavior of popup and preventing existing popup test from
>> working?
>>
>> See the first failed roll
>> https://codereview.chromium.org/1182663005/
>>
>> content_browsertets's
>> DumpAccessibilityEventsTest.AccessibilityEventsMenuListPopup is
>> failing on win platforms.
>>
>> https://codereview.chromium.org/894913002/
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[kirkshoop_msft, 2015-06-12 17:06:24]

On 2015/06/12 17:02:56, Rick Byers wrote:
> I'm away from my desk for a few hours but sounds like we should revert ASAP
> while we investigate.  Blink CQ should have caught this, right?

I believe that it has already been manually reverted.

> > content_browsertets's
> > DumpAccessibilityEventsTest.AccessibilityEventsMenuListPopup is
> > failing on win platforms.
> >

this test is using initMouseEvent for mousedown. I have switched it to
eventSender locally.
I am building to test that it passes and then I was planning to upload that to a
new issue and link it to this one.

How does this sound?

[kirkshoop_msft, 2015-06-12 19:13:07]

On 2015/06/12 17:06:24, kirkshoop_msft wrote:
> this test is using initMouseEvent for mousedown. I have switched it to
> eventSender locally.
> I am building to test that it passes and then I was planning to upload that to
a
> new issue and link it to this one.

I uploaded https://codereview.chromium.org/1186623002 to fix the test.
I added @rbyers to review this since git cl owners did not recommend anyone.

[kirkshoop_msft, 2015-06-12 20:24:12]

@rbyers - once the other issue is finished, how do I re-commit this one?

[Rick Byers, 2015-06-12 20:53:02]

On 2015/06/12 17:02:56, Rick Byers wrote:
> I'm away from my desk for a few hours but sounds like we should revert ASAP
> while we investigate.  Blink CQ should have caught this, right?

By the way, the problem here was that this was a chromium test that failed, so
it wasn't (yet!) run by the blink CQ. 

> On Jun 12, 2015 4:41 AM, <mailto:kochi@chromium.org> wrote:
> 
> > On 2015/06/12 01:02:19, commit-bot: I haz the power wrote:
> >
> >> Committed patchset #10 (id:180001) as
> >> https://src.chromium.org/viewvc/blink?view=rev&revision=196987
> >>
> >
> > It looks like this is blocking the blink-dep-roller to roll Blink changes
> > to
> > Chromium.
> > This changed the behavior of popup and preventing existing popup test from
> > working?
> >
> > See the first failed roll
> > https://codereview.chromium.org/1182663005/
> >
> > content_browsertets's
> > DumpAccessibilityEventsTest.AccessibilityEventsMenuListPopup is
> > failing on win platforms.
> >
> > https://codereview.chromium.org/894913002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:blink-reviews+unsubscribe@chromium.org.

[Rick Byers, 2015-06-12 21:02:38]

On 2015/06/12 20:24:12, kirkshoop_msft wrote:
> @rbyers - once the other issue is finished, how do I re-commit this one?

The easiest way is probably to revert the revert (by using the 'revert patchset'
button at https://codereview.chromium.org/1184693003).  I'm guessing the system
won't let a non-committer click this - so I can do it for you once the chromium
side lands.  But first we can ask it to run the blink CL against the chromium
trybots to make sure there aren't any other issues (I didn't see any other
failures in the roll attempt, but we should make double sure that test is indeed
now passing).

[dtapuska, 2015-07-10 20:10:48]

On 2015/06/12 21:02:38, Rick Byers wrote:
> On 2015/06/12 20:24:12, kirkshoop_msft wrote:
> > @rbyers - once the other issue is finished, how do I re-commit this one?
> 
> The easiest way is probably to revert the revert (by using the 'revert
patchset'
> button at https://codereview.chromium.org/1184693003).  I'm guessing the
system
> won't let a non-committer click this - so I can do it for you once the
chromium
> side lands.  But first we can ask it to run the blink CL against the chromium
> trybots to make sure there aren't any other issues (I didn't see any other
> failures in the roll attempt, but we should make double sure that test is
indeed
> now passing).

The dispatch-synthetic-mouseevent-no-action.html is incorrect.

The focus event isn't generated via the defaultEventHandler code path. It is
generated via the EventHandler code path
which if dispatchEvent returns false then it adjust the focus. Calling
node.dispatchEvent doesn't follow that code path.
So although the test case looks correct I think it will pass even without this
change.

I am working on isTrusted event support and was going to pick up that test
because that support will pretty much negate
this change but just wanted to bring it up on this review.