Bug 46733 – REGRESSION (r67261): Middle-mouse-release opens links regardless of "press" location

• Component: DOM
• Status: RESOLVED FIXED

[Peter Kasting]

Click and hold the middle mouse button on a blank portion of a page.  Move over a link.  Release.  The link opens, when it shouldn't.

This is a regression from r67261, the patch to not fire onclick for middle-clicks.

I could use help fixing this.

[Darin Adler]

(In reply to comment #0)
> I could use help fixing this.

What kind of help would you like?

[Darin Adler]

I think the first step is to make sure it’s correct to deliver a mouseup event to an element you just happen to be over when letting up on the mouse. I’d expect that event to instead be delivered to the element that got the mousedown event.

If it is not correct to deliver these events, then the fix is probably in EventHandler and not specific to link clicks.

If it is correct to deliver these events, then the fix is more likely in the isLinkClick function, which may need additional arguments.

[Peter Kasting]

Created attachment 69064 [details]
onmouseup target testcase

I grabbed this testcase online.  According to it, onmouseup fires on the element the mouse is over during release regardless of the mousedown element, in at least Fx and IE.

[Darin Adler]

It’s not clear exactly what to do.

In theory, this should all work when the mousedown and mouseup events are created by script, and so it seems the elements themselves somehow need to keep track of whether the mousedown was on the same element. But a script can create a mousedown without ever sending a mouseup later, so there may be something fundamentally flawed in that thinking.

It’s not clear what level of the software can answer the question: “Is this a mouseup that corresponds to a mousedown on the same element?”

More experiments with other browsers and research might establish whether the abstract model is state in the element, state in the browser, or hidden state in the event itself.

[Peter Kasting]

I'll speculate that if script fires mousedown, mouseup on a link, it's not going to open at all in other browsers, meaning that the link clicking and state tracking stuff is all tied to user input.

Now to try and test that hypothesis...

[Darin Adler]

(In reply to comment #5)
> I'll speculate that if script fires mousedown, mouseup on a link, it's not going to open at all in other browsers, meaning that the link clicking and state tracking stuff is all tied to user input.

Probably obvious to you, but you’ll also want to see if preventDefault or the other ways of preventing event handling on the mousedown or mouseup have any effect and whether the link is followed before or after the mouseup event handler is run.

[Darin Adler]

It may turn out that the link following is driven by a completely internal process unrelated to DOM event handling. If so, we can add code to do that.

[Peter Kasting]

(In reply to comment #6)
> Probably obvious to you, but you’ll also want to see if preventDefault or the other ways of preventing event handling on the mousedown or mouseup have any effect and whether the link is followed before or after the mouseup event handler is run.

Not obvious to me.  I was never a web developer, so I haven't written HTML and JS in general.  I do a lot of web searching and copy-and-pasting :)

(In reply to comment #7)
> It may turn out that the link following is driven by a completely internal process unrelated to DOM event handling. If so, we can add code to do that.

If you think about other events, generally an event fired by some action won't in turn cause the action if you fake it up and fire it.  For example, firing onfocus on an element doesn't actually cause the browser to give it focus.  Or at least, that's what some reference on DOM events I'm reading is telling me...

[Peter Kasting]

Created attachment 69077 [details]
opening links on events testcase

I wrote this testcase to see what would happen on artificially-created events.

Unmodified testcase:

* In WebKit trunk (tested in Chrome Dev channel), mousing over link 2 will open two background tabs displaying bing.com (link 3).
* In Opera, mousing over link 2 will navigate the current tab to bing.com.
* In Firefox, neither mousing over link 2, nor mousing over link 1 and link 2, will open anything.

Changing the mouse button from "1" to "0":

* In WebKit trunk and Opera, mousing over link 2 will navigate the current tab to bing.com.
* In Firefox, neither mousing over link 2, nor mousing over link 1 and link 2, will open anything.

Uncommenting the preventDefault() call in displayEventName():

* In WebKit trunk, neither left- nor middle-clicking link 3 will open anything.
* In Firefox and Opera, left-clicking link 3 will not open anything, while middle-clicking will open one background tab displaying bing.com.

Uncommenting the preventDefault() call in the link 3 click listener:

* In all three browsers, left-clicking link 3 will not open anything, while middle-clicking will open one background tab displaying bing.com.

Personally, I think Firefox' handling is the most sane.

[Darin Adler]

Thanks for doing the testing. I wasn’t able to understand the pattern given your list of test results. I can help you figure out how to implement a change once you describe the behavior you think we want.

Generally speaking, if preventDefault needs to prevent something, but that something should not be done in response to that event if it is DOM-created, then we need to either:

    1) Put some hidden state on the “real” event so that the defaultEventHandler function can tell what to do.

    2) Put the code to implement the behavior at the call site that sends the event rather than in the defaultEventHandler function. But to prevent EventHandler from becoming a giant god class we still may want to involve some virtual functions on the element involved.

There may possibly be a way to construct tests that could tell the difference between (1) and (2).

[Peter Kasting]

(In reply to comment #9)
> Personally, I think Firefox' handling is the most sane.

I talked some to Erik Arvidsson (JS hacker extraordinaire) about this and after some reflection he agreed that Firefox' behavior is the best route to go.

* For maximum web compat, we probably want to match Gecko.  It's far more likely people rely on its behavior than on Opera's, and because IE has historically had such different event handling, authors are likely to have separate codepaths for IE and everyone else for cases where behaviors differ.

* Gecko's behavior is the most consistent with other types of events.  Textareas don't add text when given artificial keydown/keyup/keypress events; focus doesn't change by firing an artificial focus event.  In general, as I noted in comment 8, having a behavior that causes an event doesn't mean you can create that event to cause that behavior.

* Opera's behavior is buggy anyway.  Even if we wanted to match them, they're not checking the button number on artificially-created click events.

So we'd need to change our behavior as such:
* Links are not opened in response to DOM click events.  Instead, they're opened directly in response to UI actions such as clicks and keypresses.
* For left-clicks, we need to process the DOM click event first, so that if it is default-prevented, we do not open the link.

(In reply to comment #10)
> Generally speaking, if preventDefault needs to prevent something, but that something should not be done in response to that event if it is DOM-created, then we need to either:
> 
>     1) Put some hidden state on the “real” event so that the defaultEventHandler function can tell what to do.
> 
>     2) Put the code to implement the behavior at the call site that sends the event rather than in the defaultEventHandler function. But to prevent EventHandler from becoming a giant god class we still may want to involve some virtual functions on the element involved.

I initially thought of #2, but you're right, #1 would work too.  We can't use the existing fromUserGesture() function because that just tells us whether we're in the chain of functionality triggered by a user event, meaning a click on one element could call a handler function that generates a click on a different element, and we'd go ahead and navigate.  I tested Firefox to make sure that in this case it still doesn't navigate links.

> There may possibly be a way to construct tests that could tell the difference between (1) and (2).

If we do it right, I can't imagine any way.  But as I said, JS is far from my strong suit.

[Brenton]

This is a regression.  Whatever you guys had before worked awesome (except, apparently, for the @onclick bug that r67261 resolved).

Allowing DOM events to trigger native handlers would make the Web an even more powerful platform, as it would allow JavaScript authors to have finer control over the page.  With both Chrome and Safari now supporting extensions, as well as the innovations going on in input devices, I'm sure there are use cases where someone could build something really cool by dispatching phantom MouseEvents.  That said, I really don't see the point in contesting this.  If you guys want to mimic Firefox's separation of DOM and native handling, I won't complain.

I would, however, like middle-mouse events to have the same level of control as left-mouse events.  Specifically, just as preventDefault() can interrupt the native handler for an onLeftClick, it should be able to block the native handling of an onMiddleClick.

It's easier make accurate predictions and expectations of the browser when there are fewer special cases (for instance, when the middle-click API behaves just like the left-click API).  Also, extension authors often provide custom functionality in response to middle-clicks, and mouseEvent.preventDefault() is an important tool to that end.

If I misunderstood your remarks about preventDefault, I apologize.  


This sounds like it's more than just a quick fix.  Any chance that the r67261 patch can be rolled-back and reapplied when it includes a fix for this issue?  I use an extension that is bound to middle-click.  The constant spawning of links I didn't click is getting annoying.

Thanks as always for your constant work to improve the Web.

[Peter Kasting]

(In reply to comment #12)
> I would, however, like middle-mouse events to have the same level of control as left-mouse events.  Specifically, just as preventDefault() can interrupt the native handler for an onLeftClick, it should be able to block the native handling of an onMiddleClick.

There is no click event to preventDefault() on.  Re-adding one breaks numerous sites across the web (as we know from years of having it).

> It's easier make accurate predictions and expectations of the browser when there are fewer special cases (for instance, when the middle-click API behaves just like the left-click API).

That ship has sailed (sadly).  Regardless of what would logically make sense, "click" is a de facto euphemism for "left click".  If we want more programmatic control of middle-clicks, we need to invent a new event for them.

> Also, extension authors often provide custom functionality in response to middle-clicks, and mouseEvent.preventDefault() is an important tool to that end.

I am much more concerned about compatibility across the web than convenience for extension authors when the two conflict with each other.  But as I noted, it might be possible to invent a new, dedicated event for middle clicks, which would re-add this ability without the web compat headaches.  Perhaps you would like to raise the issue with the DOM folks?

> This sounds like it's more than just a quick fix.  Any chance that the r67261 patch can be rolled-back and reapplied when it includes a fix for this issue?

I have to find someone to fix this soon since it's a blocker for Chrome 7, which has passed feature freeze a while back.

[Brenton]

So, you just recently pulled the click event with a button value of 1 after years of having it?  I've never heard of middle click events causing an issue, but the Web is a big place.


I've been using mousedown event.preventDefault() to block smooth-scrolling and contexual menus.  Will I still be able to do that?


Making changes to the way a browser behaves seems like a dangerous move.  Any apps designed around the old behavior (which as you've said is also the logical one) will now be broken.

I'm honestly considering switching away from Chrome as my main browser as it seems to be breaking things quite often.  Perhaps I should move off the dev channel, but if I do that, it's even more likely that the broken bits will make it downstream.  

I don't have time to debug my projects against every single Chrome release, but it seems I need to start doing so or else broken functionality will creep into a major browser.


Is there some sort of resource where you guys publish major changes?  I'm afraid that you're going to change the way two major browsers behave in a given situation and nobody is going to know until they realize their apps are now misbehaving.  

In the last couple months, my extension has gone vacillated between working perfectly, partially working, and not working at all depending on the Chrome build installed.

[Peter Kasting]

(In reply to comment #14)
> So, you just recently pulled the click event with a button value of 1 after years of having it?  I've never heard of middle click events causing an issue, but the Web is a big place.

Politely: you're getting off-track for this bug.  This is not a bug about whether middle clicks should send click events.

> Making changes to the way a browser behaves seems like a dangerous move.

Welcome to the web.

> Is there some sort of resource where you guys publish major changes?

This is not a bug (or even a bug tracker) about Chrome and its release policies.  Please go look at the chromium.org website for information about Chrome.

[Darin Adler]

I think we should start here by adding tests to LayoutTests that cover everything we care about here. Next step would be to land the tests with the expected results showing the current problems and failures. Then we can consider implementation strategy without confusion about the desired behavior -- the test cases will make that clear.

To test the difference between DOM-created events and user-created ones we will need to use eventSender.

[Peter Kasting]

(In reply to comment #16)
> Next step would be to land the tests with the expected results showing the current problems and failures.

Do you mean, make the expected results be what we want to eventually happen, as opposed to what happens now?  And then put the tests on the skipped lists?

[Darin Adler]

(In reply to comment #17)
> (In reply to comment #16)
> > Next step would be to land the tests with the expected results showing the current problems and failures.
> 
> Do you mean, make the expected results be what we want to eventually happen, as opposed to what happens now?  And then put the tests on the skipped lists?

What I mean is that the test should be written so that “PASS” is what we want to eventually happen. Then we should land expected results that show the current failures.

Then the patch to fix the bug starts with a patch to the expected results to show expected PASS, and we add all the code changes needed to make that happen.

No use of skipped lists, except perhaps for platforms with insufficient eventSender support.

A benefit of this approach is that the preparation stage does not require any expertise at making the code changes -- the early step is entirely about embodying our research results and plans in test form.

[Brenton]

I didn't mean to deviate from the issue.  I saw this regression was caused by a change in the way middle-mouse events are handled and, as a potentially-affected party, wanted to discuss that change.  


Also, I realize this is not the Chrome bug tracker, but Chrome releases the latest WebKit builds rapidly, and most of the compatibility issues Chrome releases introduce are sourced in WebKit changes.  I don't know where to learn about WebKit changes that could potentially affect my existing apps, and I'm not sure that others do either. 

As you pointed out, that issue affects more than just this bug and there are probably better venues for it to be addressed.


If you feel I've wasted your time, I apologize.

[Peter Kasting]

(In reply to comment #19)
> I saw this regression was caused by a change in the way middle-mouse events are handled and, as a potentially-affected party, wanted to discuss that change.

A better context for that would probably be either a separate bug or a concrete proposal mailed to the webkit-dev mailing list.

If you want a more informal discussion you might try the IRC #webkit channel.

> Also, I realize this is not the Chrome bug tracker, but Chrome releases the latest WebKit builds rapidly, and most of the compatibility issues Chrome releases introduce are sourced in WebKit changes.  I don't know where to learn about WebKit changes that could potentially affect my existing apps, and I'm not sure that others do either.

trac.webkit.org shows all changes as they go in.  Chrome has googlechromereleases.blogspot.com ; there is also a tool at http://omahaproxy.appspot.com/cl?os=win&channel=canary which will show you the changelist for the latest release (substitute your choice of OS and channel).

[Peter Kasting]

Dimitri, do you know anyone who could help work on this?

[Peter Kasting]

Created attachment 69381 [details]
middle-mouse autoscroll testcase

Here's another test.  This one calls preventDefault() on mouse down/up/click events on the body.  If you resize your window small enough to get scrollbars, then middle-click, in Firefox the autoscroll (or "pan scroll") cursor appears, but in WebKit, it doesn't.  This is because, similarly to link-middle-clicks, we trigger autoscrolling off a default event handler: in this case, Node::defaultEventHandler().

I think middle-mouse autoscroll, even more than middle-click to open links, should be triggered in parallel to event handling (i.e. like Firefox) as opposed to because of it.

[Brenton]

(In reply to comment #22)
> Here's another test.  This one calls preventDefault() on mouse down/up/click events on the body.  If you resize your window small enough to get scrollbars, then middle-click, in Firefox the autoscroll (or "pan scroll") cursor appears, but in WebKit, it doesn't.  This is because, similarly to link-middle-clicks, we trigger autoscrolling off a default event handler: in this case, Node::defaultEventHandler().

We explicitly fixed this (e.g. made autoScroll preventable) in December:

https://bugs.webkit.org/show_bug.cgi?id=32303

[Peter Kasting]

(In reply to comment #23)
> We explicitly fixed this (e.g. made autoScroll preventable) in December:
> 
> https://bugs.webkit.org/show_bug.cgi?id=32303

The primary problem there -- that middle-clicks weren't firing events when autoscroll triggered -- was clearly a bug that needed to be fixed.

Making autoscroll preventable at the same time was a mistake.  It doesn't match Firefox, it doesn't match our desired behavior here for middle-clicks, and it doesn't make a lot of sense in the abstract (we actively don't want web pages to be able to prevent autoscroll).

[Brenton]

I disagree.

Isn't the whole point of preventDefault to give authors the ability to provide custom handlers for events that would otherwise trigger native ones?

Google and Apple both frequently grandstand about the web as a platform, where JavaScript apps can supplant desktop ones.  HTML 5 is supposed to enable us to do so much that we'll stop using platforms like Flash or Cocoa and start targeting the browser.

If you bind a native handler to the middle-mouse button that app creators cannot interrupt, that keeps them from using the middle-mouse button to trigger custom functionality.  Worse yet, auto-scroll only happens occasionally - if you are not on Windows or if the page is shorter than the window height, auto-scroll will not spawn.  If a creator is using another OS or a giant monitor, he can write an app that listens for middle-mouse events and not realize that Windows users are seeing auto-scroll.  If your proposed changes are implemented, he won't be able to do anything about it other than scrapping middle-mouse handling and finding a different way to implement the feature.

What you are proposing will break mouse gestures extensions.

[Peter Kasting]

(In reply to comment #25)
> Isn't the whole point of preventDefault to give authors the ability to provide custom handlers for events that would otherwise trigger native ones?

Not all native events can be prevented this way.  It's important for browsers to match each other's behavior so authors can write code that behaves the same on all browsers.  And it's secondarily important for there to be at least some kind of consistency in how mouse events are handled on one platform.

> If your proposed changes are implemented, he won't be able to do anything about it

Not quite true.  Autoscroll never triggers when nothing under the mouse is scrollable.  Many apps do their scrolling internally or are not scrollable at all, and in these situations there's no conflict.  I realize that's not all situations, but things are not as apocalyptic as you imply.

> What you are proposing will break mouse gestures extensions.

Other browsers (e.g. Firefox) implement this behavior and still have mouse gestures.  Chrome had mouse gesture extensions in the gallery before the patch you linked was checked into WebKit.  So I don't buy this argument.

And again, making the web platform behave equivalently across browsers is more important than what extension authors can or cannot do with the current set of APIs.  If we need to enable extension authors to do something, we'll find a way.

[Brenton]

I agree with you in principle about web browsers behaving similarly to one another, but I'm not sure regressing WebKit's functionality to match Gecko's is the right way to go about it.  (Honestly, it makes me wonder what the point of WebKit is if you are trying to mimic Gecko.)


Firefox's extensions used their own custom platform (I believe XUL).  Chrome started the trend of DOM-based extensions.  

The Dec 09 patch came about because Chrome mouse gestures extensions that were triggered with the middle-mouse button weren't behaving on Windows.  The auto-scroll was getting in the way.

[Peter Kasting]

(In reply to comment #26)
> (In reply to comment #25)
> > Isn't the whole point of preventDefault to give authors the ability to provide custom handlers for events that would otherwise trigger native ones?
> 
> Not all native events can be prevented this way.  It's important for browsers to match each other's behavior so authors can write code that behaves the same on all browsers.

After consulting with a bunch of different web developers, I'm inclined to reverse course and say this is still true, but maybe Gecko should change instead of WebKit.  There are other arguments for changing this I haven't mentioned but I'm not convinced they're strong enough.

[Brenton]

(In reply to comment #28)

> After consulting with a bunch of different web developers, I'm inclined to reverse course and say this is still true, but maybe Gecko should change instead of WebKit.  There are other arguments for changing this I haven't mentioned but I'm not convinced they're strong enough.


I appreciate that.  I try not to be overly persistent about these things, but as a web developer, I have strong feelings about the way browsers handle my code.  =)

Where does that leave us regarding this bug and the way WebKit handles middle-mouse events?

[Peter Kasting]

(In reply to comment #29)
> Where does that leave us regarding this bug and the way WebKit handles middle-mouse events?

As things stood before comment 22.

[Peter Kasting]

I still have not had time to try and fix this.

If anyone else has time to at least sketch out a fix by firing navigations after running middle click handlers, as opposed to during the default handler, that would be helpful.

[Alexey Proskuryakov]

Why don't we just revert r67261 for now? The bug it fixed was minor compared to this regression.

[Alexey Proskuryakov]

<rdar://problem/8895856>

[Adele Peterson]

I agree with Alexey.  I think we should roll out the original change for now.

[Darin Adler]

(In reply to comment #34)
> I agree with Alexey.  I think we should roll out the original change for now.

I concur.

[Peter Kasting]

Me too.

[Peter Kasting]

Created attachment 79775 [details]
Roll back r67261 (needs real review)

Here's an attempted rollback.  I'd like Darin or someone else who understands HTMLInputElement.cpp to review as that file has been significantly refactored in the meantime and I'm not sure whether my attempted change is right.

[WebKit Review Bot]

Attachment 79775 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7627245

[Early Warning System Bot]

Attachment 79775 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7496269

[Peter Kasting]

Created attachment 79778 [details]
Roll back r67261 (needs real review), v2

Try to fix compile errors

[WebKit Review Bot]

Attachment 79775 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7634117

[Darin Adler]

Comment on attachment 79778 [details]
Roll back r67261 (needs real review), v2

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

> Source/WebCore/html/HTMLInputElement.cpp:945
> -    if (event->type() != eventNames().clickEvent)
> +    if (!event->isMouseEvent() || event->type() != eventNames().clickEvent || static_cast<MouseEvent*>(event)->button() != LeftButton)
>          return 0;

We could put the re-added code in a separate return statement. That way it can more easily be removed when we redo this change later.

I think it’s unnecessarily confusing to have the isMouseEvent check separated from the code that casts the event to a MouseEvent. Putting the new check into a separate if statement would fix that as well.

If we thought the code was going to be like this longer term, we’d want to either pass the event in to the input type and let it judge if it wants to handle the click event or rename willDispatchClick to willDispatchLeftClick. But I supposed for now it’s OK to leave the button check here.

> Source/WebCore/html/HTMLInputElement.cpp:954
> -    if (event->type() != eventNames().clickEvent)
> +    if (!event->isMouseEvent() || event->type() != eventNames().clickEvent || static_cast<MouseEvent*>(event)->button() != LeftButton)
>          return;

This change isn’t needed. In fact, the if statement that checks the event type is unnecessary as well. We’re guaranteed that state will be 0 unless the preDispatchEventHandler returned something, so we don’t need to redo the checks it did.

> Source/WebCore/html/HTMLInputElement.cpp:963
> -    if (evt->isMouseEvent() && evt->type() == eventNames().clickEvent) {
> +    if (evt->isMouseEvent() && evt->type() == eventNames().clickEvent && static_cast<MouseEvent*>(evt)->button() == LeftButton) {
>          m_inputType->handleClickEvent(static_cast<MouseEvent*>(evt));

If we thought the code was going to be like this longer term, we’d want to either let RadioButtonType::handleClickEvent do the checking for the left button or rename the InputType function to handleLeftClickEvent. But I suppose for now it’s OK to have this match HTMLInputElement::preDispatchEventHandler; both have the same issue.

> Source/WebCore/page/EventHandler.cpp:1428
> -    bool swallowClickEvent = mouseEvent.button() == LeftButton && mev.targetNode() == m_clickNode && dispatchMouseEvent(eventNames().clickEvent, mev.targetNode(), true, m_clickCount, mouseEvent, true);
> +    bool swallowClickEvent = false;
> +    // Don't ever dispatch click events for right clicks
> +    if (mouseEvent.button() != RightButton && mev.targetNode() == m_clickNode)
> +        swallowClickEvent = dispatchMouseEvent(eventNames().clickEvent, mev.targetNode(), true, m_clickCount, mouseEvent, true);

I don’t think we should add back the comment. The code says != right button; a comment that says the same thing just creates confusion about what the rest of what the code does and why the rest of the code is there.

I don’t think we need to roll back the coding style change we made in the original patch. We can just change the condition back from == LeftButton to != RightButton. That was the substantive change. The rest was coding style only and there is no good reason to keep changing it back and forth.

> Source/WebCore/page/EventHandler.cpp:1435
> -
> -    bool swallowMouseReleaseEvent = !swallowMouseUpEvent && handleMouseReleaseEvent(mev);
> +            
> +    bool swallowMouseReleaseEvent = false;
> +    if (!swallowMouseUpEvent)
> +        swallowMouseReleaseEvent = handleMouseReleaseEvent(mev);

There’s no need to roll this out. It was a coding style change that has no effect on behavior.

> Source/WebCore/page/EventHandler.cpp:1624
> -    bool swallowClickEvent = m_clickCount > 0 && mouseEvent.button() == LeftButton && mev.targetNode() == m_clickNode && dispatchMouseEvent(eventNames().clickEvent, mev.targetNode(), true, m_clickCount, mouseEvent, true);
> +    // Don't ever dispatch click events for right clicks
> +    bool swallowClickEvent = false;
> +    if (m_clickCount > 0 && mouseEvent.button() != RightButton && mev.targetNode() == m_clickNode)
> +        swallowClickEvent = dispatchMouseEvent(eventNames().clickEvent, mev.targetNode(), true, m_clickCount, mouseEvent, true);

As above, I suggest rolling out the substantive change, and not the style change. And, as above, I suggest leaving the comment out.

[Peter Kasting]

Comment on attachment 79778 [details]
Roll back r67261 (needs real review), v2

Will upload new patch for commit momentarily.

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

>> Source/WebCore/html/HTMLInputElement.cpp:945
>
> We could put the re-added code in a separate return statement. That way it can more easily be removed when we redo this change later.
> 
> I think it’s unnecessarily confusing to have the isMouseEvent check separated from the code that casts the event to a MouseEvent. Putting the new check into a separate if statement would fix that as well.

Changed to place the new conditional in a separate statement.

>> Source/WebCore/html/HTMLInputElement.cpp:954
>>          return;
> 
> This change isn’t needed. In fact, the if statement that checks the event type is unnecessary as well. We’re guaranteed that state will be 0 unless the preDispatchEventHandler returned something, so we don’t need to redo the checks it did.

Removed the entire statement.

>> Source/WebCore/page/EventHandler.cpp:1428
>> +        swallowClickEvent = dispatchMouseEvent(eventNames().clickEvent, mev.targetNode(), true, m_clickCount, mouseEvent, true);
> 
> I don’t think we should add back the comment. The code says != right button; a comment that says the same thing just creates confusion about what the rest of what the code does and why the rest of the code is there.
> 
> I don’t think we need to roll back the coding style change we made in the original patch. We can just change the condition back from == LeftButton to != RightButton. That was the substantive change. The rest was coding style only and there is no good reason to keep changing it back and forth.

Changed this change to only convert "== LeftButton" to "!= RightButton".

>> Source/WebCore/page/EventHandler.cpp:1435
>> +        swallowMouseReleaseEvent = handleMouseReleaseEvent(mev);
> 
> There’s no need to roll this out. It was a coding style change that has no effect on behavior.

Removed.

>> Source/WebCore/page/EventHandler.cpp:1624
>> +        swallowClickEvent = dispatchMouseEvent(eventNames().clickEvent, mev.targetNode(), true, m_clickCount, mouseEvent, true);
> 
> As above, I suggest rolling out the substantive change, and not the style change. And, as above, I suggest leaving the comment out.

Changed this change to only convert "== LeftButton" to "!= RightButton".

[Peter Kasting]

Created attachment 79992 [details]
Roll back r67261, for commit

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 79992 [details]:

http/tests/xmlhttprequest/cross-origin-no-authorization.html bug 33357 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 79992 [details]
Roll back r67261, for commit

Clearing flags on attachment: 79992

Committed r76557: <http://trac.webkit.org/changeset/76557>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.