Fix history page middle click action

Fix history page middle click action

Send synthetic middle click in history page and also
prevent default it as the links in history page will
trigger twice if we don't prevent default it.

BUG=609020
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/a154aed1a3813cf28c6f477579ed7974a2528570
Cr-Commit-Position: refs/heads/master@{#397512}

[Navid Zolghadr, 2016-05-27 19:45:34]

Description was changed from

==========
Fix history page middle click action

Send synthetic middle click in history page and also
prevent default it as the links in history page will
trigger twice if we don't prevent default it.

BUG=609020
==========

to

==========
Fix history page middle click action

Send synthetic middle click in history page and also
prevent default it as the links in history page will
trigger twice if we don't prevent default it.

BUG=609020
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[Navid Zolghadr, 2016-05-27 20:26:28]

nzolghadr@chromium.org changed reviewers:
+ dtapuska@chromium.org, mustaq@chromium.org

[Navid Zolghadr, 2016-05-30 16:59:46]

nzolghadr@chromium.org changed reviewers:
+ dbeam@chromium.org

[Navid Zolghadr, 2016-05-31 19:08:14]

On 2016/05/30 16:59:46, Navid Zolghadr wrote:

Hey Dan, can you have a look at this one and see if you are okay with the
approach?

[Dan Beam, 2016-05-31 21:34:32]

this is not getting better...

https://codereview.chromium.org/2018943003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/synthetic_middleclick.js (right):

https://codereview.chromium.org/2018943003/diff/20001/chrome/browser/resource...
chrome/browser/resources/synthetic_middleclick.js:26: // event.
i don't understand what this comment means.  can you try again?

https://codereview.chromium.org/2018943003/diff/20001/chrome/browser/resource...
chrome/browser/resources/synthetic_middleclick.js:28: if (e.button == 1 &&
!e.isTrusted)
don't we control the code that's doing this?  why not just do:

if (e.path.length > 0 && e.path[0] == middleButtonMouseDownTarget) {
  var click = new MouseEvent('click', e);
  middleButtonMouseDownTarget.dispatchEvent(click);
  click.preventDefault();
}

[Navid Zolghadr, 2016-06-01 18:46:16]

ptal.

https://codereview.chromium.org/2018943003/diff/20001/chrome/browser/resource...
File chrome/browser/resources/synthetic_middleclick.js (right):

https://codereview.chromium.org/2018943003/diff/20001/chrome/browser/resource...
chrome/browser/resources/synthetic_middleclick.js:26: // event.
On 2016/05/31 21:34:32, Dan Beam wrote:
> i don't understand what this comment means.  can you try again?

How about this?

https://codereview.chromium.org/2018943003/diff/20001/chrome/browser/resource...
chrome/browser/resources/synthetic_middleclick.js:28: if (e.button == 1 &&
!e.isTrusted)
On 2016/05/31 21:34:32, Dan Beam wrote:
> don't we control the code that's doing this?  why not just do:
> 
> if (e.path.length > 0 && e.path[0] == middleButtonMouseDownTarget) {
>   var click = new MouseEvent('click', e);
>   middleButtonMouseDownTarget.dispatchEvent(click);
>   click.preventDefault();
> }

Maybe the new comment explains this a little better. Basically when js
dispatches the click event the default handlers will run right after all the
handlers. So basically your proposal doesn't prevent the default behavior of the
click event as the new tab is already opened right before the
click.preventDefault() line. So I need to preventDefault it in a handler (that
runs the last preferably). The other solution is to prevent defaulting it before
dispatching it but that causes some unexpected behavior in the current code as
the handlers we have here ignores the events if they were preventDefaulted
already.

[Navid Zolghadr, 2016-06-02 17:02:19]

On 2016/06/01 18:46:16, Navid Zolghadr wrote:
> ptal.
> 
>
https://codereview.chromium.org/2018943003/diff/20001/chrome/browser/resource...
> File chrome/browser/resources/synthetic_middleclick.js (right):
> 
>
https://codereview.chromium.org/2018943003/diff/20001/chrome/browser/resource...
> chrome/browser/resources/synthetic_middleclick.js:26: // event.
> On 2016/05/31 21:34:32, Dan Beam wrote:
> > i don't understand what this comment means.  can you try again?
> 
> How about this?
> 
>
https://codereview.chromium.org/2018943003/diff/20001/chrome/browser/resource...
> chrome/browser/resources/synthetic_middleclick.js:28: if (e.button == 1 &&
> !e.isTrusted)
> On 2016/05/31 21:34:32, Dan Beam wrote:
> > don't we control the code that's doing this?  why not just do:
> > 
> > if (e.path.length > 0 && e.path[0] == middleButtonMouseDownTarget) {
> >   var click = new MouseEvent('click', e);
> >   middleButtonMouseDownTarget.dispatchEvent(click);
> >   click.preventDefault();
> > }
> 
> Maybe the new comment explains this a little better. Basically when js
> dispatches the click event the default handlers will run right after all the
> handlers. So basically your proposal doesn't prevent the default behavior of
the
> click event as the new tab is already opened right before the
> click.preventDefault() line. So I need to preventDefault it in a handler (that
> runs the last preferably). The other solution is to prevent defaulting it
before
> dispatching it but that causes some unexpected behavior in the current code as
> the handlers we have here ignores the events if they were preventDefaulted
> already.

Hey Dan, can you have another look at this one?

[Dan Beam, 2016-06-02 19:18:33]

lgtm because it fixes a bug (but I don't love the code, before or after)

so, when are you putting synthetic_middleclick.js into dev tools?  because
that's the bug that annoys the shit out of me and others.

https://codereview.chromium.org/2018943003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/synthetic_middleclick.js (right):

https://codereview.chromium.org/2018943003/diff/40001/chrome/browser/resource...
chrome/browser/resources/synthetic_middleclick.js:32: // as well if the event is
not prevented.
yeah, sorry, this still doesn't make much sense to me.

probably just remove it

[Navid Zolghadr, 2016-06-02 20:03:19]

On 2016/06/02 19:18:33, Dan Beam wrote:
> lgtm because it fixes a bug (but I don't love the code, before or after)
> 
> so, when are you putting synthetic_middleclick.js into dev tools?  because
> that's the bug that annoys the shit out of me and others.
> 
>
https://codereview.chromium.org/2018943003/diff/40001/chrome/browser/resource...
> File chrome/browser/resources/synthetic_middleclick.js (right):
> 
>
https://codereview.chromium.org/2018943003/diff/40001/chrome/browser/resource...
> chrome/browser/resources/synthetic_middleclick.js:32: // as well if the event
is
> not prevented.
> yeah, sorry, this still doesn't make much sense to me.
> 
> probably just remove it

I'm going to work on dev tools right after this CL lands.
I removed the comment altogether.

[Navid Zolghadr, 2016-06-02 20:08:10]

The CQ bit was checked by nzolghadr@chromium.org

[Navid Zolghadr, 2016-06-02 20:08:11]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2018943003/#ps60001
(title: "Removing the comment")

[commit-bot: I haz the power, 2016-06-02 20:09:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2018943003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2018943003/60001

[commit-bot: I haz the power, 2016-06-02 21:37:25]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-02 21:40:19]

Description was changed from

==========
Fix history page middle click action

Send synthetic middle click in history page and also
prevent default it as the links in history page will
trigger twice if we don't prevent default it.

BUG=609020
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Fix history page middle click action

Send synthetic middle click in history page and also
prevent default it as the links in history page will
trigger twice if we don't prevent default it.

BUG=609020
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/a154aed1a3813cf28c6f477579ed7974a2528570
Cr-Commit-Position: refs/heads/master@{#397512}
==========

[commit-bot: I haz the power, 2016-06-02 21:40:20]

Patchset 4 (id:??) landed as
https://crrev.com/a154aed1a3813cf28c6f477579ed7974a2528570
Cr-Commit-Position: refs/heads/master@{#397512}