Don't turn middleclicks into click events.

Don't turn middleclicks into click events.

The specs are fuzzy on when click events should be generated.
It's bad if they are generated together with some other action
since many scripts just listen to onclick without checking button.
For that reason right clicks are already treated differently and
this patch changes the code to also not turn middle clicks into
click events.

This makes us more compatible with Gecko, Presto and MSIE/Trident though
there is undocumented interaction with various pan-to-scroll features that
makes this not the whole truth.

BUG=255

[abarth-chromium, 2013-08-22 19:22:24]

@pkasting: I seem to remember that you looked at this issue before.  Any
thoughts about this approach?

@bratell: We'll need a test for this CL.  You should be able to test it using
the EventDispatcher in a LayoutTest.

[Peter Kasting, 2013-08-22 19:31:29]

On 2013/08/22 19:22:24, abarth wrote:
> @pkasting: I seem to remember that you looked at this issue before.  Any
> thoughts about this approach?

The WebKit bug for this is https://bugs.webkit.org/show_bug.cgi?id=22382 .  I
strongly encourage people to read that bug, at the very least from comment 80
onward.  Comment 80 summarizes some thoughts about how the implementation should
work.  Then there are four testcases attached to the bug, and descriptions in
comments 81-83 and 89 of what ought to happen in each.  Ensure that any fix here
does the right thing w.r.t. those testcases.

[Daniel Bratell, 2013-08-23 06:28:19]

On 2013/08/22 19:31:29, Peter Kasting wrote:
> On 2013/08/22 19:22:24, abarth wrote:
> > @pkasting: I seem to remember that you looked at this issue before.  Any
> > thoughts about this approach?
> 
> The WebKit bug for this is https://bugs.webkit.org/show_bug.cgi?id=22382 .  I
> strongly encourage people to read that bug, at the very least from comment 80
> onward.  Comment 80 summarizes some thoughts about how the implementation
should
> work.  Then there are four testcases attached to the bug, and descriptions in
> comments 81-83 and 89 of what ought to happen in each.  Ensure that any fix
here
> does the right thing w.r.t. those testcases.

I've read the things you quoted and I see there are more work to be done in
Blink, but I also don't see that it invalidates this change. It doesn't make
this change wrong. It just says that it only fixes some of the
bugs/incompatibilities that are present in Blink/WebKit, and I think it's
reasonable not trying to fix all bugs at once. :-)

(The other/remaining bugs being when links are followed in response to mouse
events and auto-panning)

And of course there should be a test. I'll work on that.

[Peter Kasting, 2013-08-23 18:49:37]

I'm not qualified to review this change.  Linking to the other bug and hoping
the right reviewers study it when looking at this is all I can contribute :)

[Daniel Bratell, 2013-08-26 14:35:59]

Found an existing LayoutTest (after writing my own test that I then discarded)
that I could just modify expected for.

I also found another test that had a workaround for the click event (I think -
since it worked fine without any click event).

I can't trigger the trybots though so I can't be sure there is no other test.

[Daniel Bratell, 2013-08-26 16:09:10]

On 2013/08/26 14:35:59, Daniel Bratell wrote:
> Found an existing LayoutTest (after writing my own test that I then discarded)
> that I could just modify expected for.
> 
> I also found another test that had a workaround for the click event (I think -
> since it worked fine without any click event).
> 
> I can't trigger the trybots though so I can't be sure there is no other test.

You don't have to review or look at this just yet. I'm still investigating if
this is the best approach (i.e. causing the least problems while fixing the most
problems given the complexity if the patch).

[Daniel Bratell, 2013-08-26 16:29:20]

On 2013/08/26 16:09:10, Daniel Bratell wrote:
> On 2013/08/26 14:35:59, Daniel Bratell wrote:
> > Found an existing LayoutTest (after writing my own test that I then
discarded)
> > that I could just modify expected for.
> > 
> > I also found another test that had a workaround for the click event (I think
-
> > since it worked fine without any click event).
> > 
> > I can't trigger the trybots though so I can't be sure there is no other
test.
> 
> You don't have to review or look at this just yet. I'm still investigating if
> this is the best approach (i.e. causing the least problems while fixing the
most
> problems given the complexity if the patch).

So I have the result of my investigation in a Google Docs spreadsheet I'm not
allowed by Google Apps to share (I've asked how to unlock it but no response
yet).

The result is:
* Gecko always sends a click event with target [real target] but dispatched only
to document and window. I'll classify them as weird and ignore that because
nobody else is like that.
* auto-scroll (the panning thing you can activate) always disables click and
mouse events. That is already true in all browsers.
* auto-scroll only activate for scrollable documents except for Presto (ignoring
Presto).
* Mid-click on links sends a click event to the element only in Blink/WebKit.
Gecko sends the click event to document and MSIE and Presto sends no click
event. They all open the link in the background.
* Mid-clicking a div always sends a click event to the div (except in Gecko who
still is weird) unless auto-scroll is activated.

The summary for what needs fixing seems to be:

* Make "open link in background" prevent the click event the same way as a
context menu prevents the click event. I have not checked how easy that is but
pointers are always welcome.

[Daniel Bratell, 2013-08-26 16:30:13]

Spreadsheet that probably will be blocked:
https://docs.google.com/a/opera.com/spreadsheet/pub?key=0AscKoSknuFdjdHVTRjhG...

[Daniel Bratell, 2013-08-27 18:22:15]

On 2013/08/26 16:30:13, Daniel Bratell wrote:
> Spreadsheet that probably will be blocked:
>
https://docs.google.com/a/opera.com/spreadsheet/pub?key=0AscKoSknuFdjdHVTRjhG...

So, now it might be ready for some serious reviewing.

There are two things to consider. The most important being, is this when we want
to send the click event. The least important, can this be done in a more
beautiful way.

Regarding "Is this when we want to send click events?" I choose to keep the
click events for all elements except elements inside links. That makes the
reported bugs pass. It makes Blink and MSIE10 compatible and for links all
browsers will agree. The main alternative is to never send click for middle
click at all (like Presto and almost like Gecko).

Regarding "can this be more beautiful?" I had the feeling there had to be helper
functions for what I tried to do so that the 15 lines could have been 5, but I
don't know the whole toolbox.

The new testcase here depend on https://codereview.chromium.org/23517003 -
content_shell debug crash fix for middle click on links.

About the spreadsheet. I still can't publish it outside @opera.com. Maybe if
someone at Google makes a spreadsheet I'm allowed to edit I can paste into it.

Phew! abart, Peter Kasting - your turn. :-)

[Daniel Bratell, 2013-09-05 11:17:52]

On 2013/08/27 18:22:15, Daniel Bratell wrote:
> Phew! abarth, Peter Kasting - your turn. :-)

Just a friendly *ping*.

[abarth-chromium, 2013-09-06 05:01:25]

Maybe this CL is right, but I'm not sure.  It would require a significant amount
of time for me to study this CL to determine if it's correct.  Other folks
should feel free to review the CL, but I'm unlikely to devote several hours it
would take to review this CL properly.

https://codereview.chromium.org/23060022/diff/14001/Source/core/page/EventHan...
File Source/core/page/EventHandler.cpp (right):

https://codereview.chromium.org/23060022/diff/14001/Source/core/page/EventHan...
Source/core/page/EventHandler.cpp:1662: WebCore::Node* node = mev.targetNode();
"WebCore::" <-- no need for this namespace.  We're already in the WebCore
namespace.

https://codereview.chromium.org/23060022/diff/14001/Source/core/page/EventHan...
Source/core/page/EventHandler.cpp:1664: node = node->parentNode();
Hum...  This crawling around the DOM doesn't seem like it can be right.

[Daniel Bratell, 2013-09-06 07:03:24]

On 2013/09/06 05:01:25, abarth wrote:
> Maybe this CL is right, but I'm not sure.  It would require a significant
amount
> of time for me to study this CL to determine if it's correct.  Other folks
> should feel free to review the CL, but I'm unlikely to devote several hours it
> would take to review this CL properly.

What can I do to make these 15 lines of code take less than that long to review?

The first ~13 lines (including comments) try to detect if the mouseup event is
on a <a href>. The last ~2 surpresses the onclick event in that case.

I have done investigations in all other browser engines and I would be happy to
publish it in google drive if there was a document I could put my data in.
Unfortunately Google Drive on my account is locked down to opera.com internal
storage so I can't publish it.

>
https://codereview.chromium.org/23060022/diff/14001/Source/core/page/EventHan...
> Source/core/page/EventHandler.cpp:1664: node = node->parentNode();
> Hum...  This crawling around the DOM doesn't seem like it can be right.

I also didn't like it. If I remove it the code will become MSIE compatible but
worse off for users. 
Is it the way of doing it or the actual looping itself that you feel is wrong?

[abarth-chromium, 2013-09-06 16:04:24]

On 2013/09/06 07:03:24, Daniel Bratell wrote:
> What can I do to make these 15 lines of code take less than that long to
review?

It's not the code itself that's difficult to review, it's whether this new
behavior is correct.  For example, maybe the loop you've got is correct, but
that's very suspicious.  Maybe we should find the Firefox code that does this
work to see how it works?  It's going to be almost as much work to review this
CL as it was to write it in the first place.

> Is it the way of doing it or the actual looping itself that you feel is wrong?

The actual looping.  It's very unusual for the default behavior of an event to
depend on something other than the specific element to which it was targeted.

[abarth-chromium, 2013-09-06 16:06:46]

Your best path forward here is probably to find another reviewer who's willing
to dedicate the time it take to review this CL properly.

[ojan, 2013-09-13 00:59:48]

I think I might be misunderstanding the patch, but I think we should make
middle-clicks never fire a click event, whether they're on a link or not. Does
this patch try to do that?

[Daniel Bratell, 2013-09-13 06:44:28]

On 2013/09/13 00:59:48, ojan wrote:
> I think I might be misunderstanding the patch, but I think we should make
> middle-clicks never fire a click event, whether they're on a link or not. Does
> this patch try to do that?

An early version of the patch does that but while it's what Opera Presto has
been doing all along, and what Mozilla seems to want to implement, it was also
the biggest deviation from the current behaviour. I will publish the analysis I
have of what browsers do if I so have to make a screenshot of it and put in
Picasa. :p

I suspect that any patch that removes the onclick from midbutton on links will
be fine because I suspect that there is not a single page in the world that
depends on midbutton generating click events. But impossible to be sure.

[Daniel Bratell, 2013-09-13 06:54:48]

On 2013/09/13 06:44:28, Daniel Bratell wrote:
> On 2013/09/13 00:59:48, ojan wrote:
> > I think I might be misunderstanding the patch, but I think we should make
> > middle-clicks never fire a click event, whether they're on a link or not.
Does
> > this patch try to do that?
> 
> I will publish the analysis I
> have of what browsers do if I so have to make a screenshot of it and put in
> Picasa. :p

Picasa seemed to have merged with Google Plus so that didn't work but I find
this cat site which would allow me to publish things. :-)

http://i.imgur.com/039VHDk.png

The thing to take away from it is that browsers differ a lot (since it's
something pages don't care about). Simon Pieters (simonp/zcorpan) tried to get
some interest from the w3/whatwg people to define a correct behaviour but nobody
was interested.

I'm mostly interested in killing the extra event that causes Blink bases
browsers to do strange things when midclicking on some sites.

[ojan, 2013-09-13 16:43:26]

On 2013/09/13 06:54:48, Daniel Bratell wrote:
> On 2013/09/13 06:44:28, Daniel Bratell wrote:
> > On 2013/09/13 00:59:48, ojan wrote:
> > > I think I might be misunderstanding the patch, but I think we should make
> > > middle-clicks never fire a click event, whether they're on a link or not.
> Does
> > > this patch try to do that?
> > 
> > I will publish the analysis I
> > have of what browsers do if I so have to make a screenshot of it and put in
> > Picasa. :p
> 
> Picasa seemed to have merged with Google Plus so that didn't work but I find
> this cat site which would allow me to publish things. :-)
> 
> http://i.imgur.com/039VHDk.png
> 
> The thing to take away from it is that browsers differ a lot (since it's
> something pages don't care about). Simon Pieters (simonp/zcorpan) tried to get
> some interest from the w3/whatwg people to define a correct behaviour but
nobody
> was interested.
> 
> I'm mostly interested in killing the extra event that causes Blink bases
> browsers to do strange things when midclicking on some sites.

Thanks for doing all the requisite legwork here. My preference would be to try
the original patch (i.e. never fire click for middle button) first and see if
it's backwards compatible. The only case I can think of it mattering is if a
site has links that don't really work, but uses script to make the click do
something (i.e. they preventDefault on the click), but as Simon said earlier,
the can catch mousedown and preventDefault that if they need to.

[Daniel Bratell, 2013-10-11 12:56:16]

On 2013/09/13 16:43:26, ojan wrote:
> Thanks for doing all the requisite legwork here. My preference would be to try
> the original patch (i.e. never fire click for middle button) first and see if
> it's backwards compatible. The only case I can think of it mattering is if a
> site has links that don't really work, but uses script to make the click do
> something (i.e. they preventDefault on the click), but as Simon said earlier,
> the can catch mousedown and preventDefault that if they need to.

ojan, I also like the simple solution and I know it didn't cause any bug reports
on Opera/Presto when we did it like that so it's at least low risk. I'm ready to
take that risk. .-)

[ojan, 2013-10-11 20:39:21]

https://codereview.chromium.org/23060022/diff/29001/LayoutTests/editing/paste...
File LayoutTests/editing/pasteboard/paste-when-over-link.html (right):

https://codereview.chromium.org/23060022/diff/29001/LayoutTests/editing/paste...
LayoutTests/editing/pasteboard/paste-when-over-link.html:6: focuses a textarea
does not paste into the textarea.
This test isn't really testing anything anymore. May as well just delete it.

[Daniel Bratell, 2013-10-14 15:00:44]

On 2013/10/11 20:39:21, ojan wrote:
>
https://codereview.chromium.org/23060022/diff/29001/LayoutTests/editing/paste...
> File LayoutTests/editing/pasteboard/paste-when-over-link.html (right):
> 
>
https://codereview.chromium.org/23060022/diff/29001/LayoutTests/editing/paste...
> LayoutTests/editing/pasteboard/paste-when-over-link.html:6: focuses a textarea
> does not paste into the textarea.
> This test isn't really testing anything anymore. May as well just delete it.

Thanks, I didn't notice. Uploaded a new version with the files deleted.

[ojan, 2013-10-14 22:23:17]

lgtm

[commit-bot: I haz the power, 2013-10-14 22:23:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bratell@opera.com/23060022/35001

[commit-bot: I haz the power, 2013-10-14 23:33:13]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[esprehn, 2014-04-29 22:06:52]

What happened here? Can we land this?

[Daniel Bratell, 2014-04-30 07:46:34]

On 2014/04/29 22:06:52, esprehn wrote:
> What happened here? Can we land this?

I discovered that the UI command "open in new window" depends on this code so
the scope of the fix grew beyond what I was prepared to invest into this issue
at the time. I don't know if it's better to let it linger as a reminder for me
(I see it on my list all the time) or to close it to keep lists clear.

The plan forward is to split the code so that "open in new window" is an
explicit instruction, not caring how it was initiated.

so right now this is not lgtm.