Start sending auxclick instead of click for non-primary buttons

Start sending auxclick instead of click for non-primary buttons

Stop sending dblclick and click evnt for non-primary buttons
and send auxclick event for the click action of non-primary
buttons.

BUG=625847, 255
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

TBR=dglazkov@chromium.org, holte@chromium.org

Committed: https://crrev.com/6efe6e4b42ab875fee4ed7029f693fefc04c2658
Cr-Commit-Position: refs/heads/master@{#412005}

[Navid Zolghadr, 2016-07-20 18:49:56]

Description was changed from

==========
Start sending auxclick instead of click for non-primary buttons

Stop sending dblclick and click evnt for non-primary buttons
and send auxclick event for the click action of non-primary
buttons.

BUG=625847
==========

to

==========
Start sending auxclick instead of click for non-primary buttons

Stop sending dblclick and click evnt for non-primary buttons
and send auxclick event for the click action of non-primary
buttons.

BUG=625847
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Navid Zolghadr, 2016-07-20 18:50:05]

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

[commit-bot: I haz the power, 2016-07-20 18:50:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Navid Zolghadr, 2016-07-20 18:51:18]

The CQ bit was unchecked by nzolghadr@chromium.org

[Navid Zolghadr, 2016-07-20 18:56:58]

Description was changed from

==========
Start sending auxclick instead of click for non-primary buttons

Stop sending dblclick and click evnt for non-primary buttons
and send auxclick event for the click action of non-primary
buttons.

BUG=625847
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Start sending auxclick instead of click for non-primary buttons

Stop sending dblclick and click evnt for non-primary buttons
and send auxclick event for the click action of non-primary
buttons.

BUG=625847, 255
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Navid Zolghadr, 2016-07-27 16:14:29]

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

[commit-bot: I haz the power, 2016-07-27 16:14:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Navid Zolghadr, 2016-07-27 16:20:33]

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

[commit-bot: I haz the power, 2016-07-27 16:20:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Navid Zolghadr, 2016-07-27 18:01:08]

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

[commit-bot: I haz the power, 2016-07-27 18:02:17]

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

[commit-bot: I haz the power, 2016-07-27 18:02:17]

Dry run: This issue passed the CQ dry run.

[dgozman, 2016-07-27 20:30:42]

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

[dgozman, 2016-07-27 20:30:43]

https://codereview.chromium.org/2163893003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js (right):

https://codereview.chromium.org/2163893003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js:1078:
tabElement.addEventListener("auxclick", this._tabClicked.bind(this), false);
Note that if you stop sending regular click events for non-primary buttons, this
code deployed in the wild will break.

[Navid Zolghadr, 2016-07-27 20:39:09]

https://codereview.chromium.org/2163893003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js (right):

https://codereview.chromium.org/2163893003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js:1078:
tabElement.addEventListener("auxclick", this._tabClicked.bind(this), false);
On 2016/07/27 20:30:43, dgozman wrote:
> Note that if you stop sending regular click events for non-primary buttons,
this
> code deployed in the wild will break.

I was under the impression that this code in the wild gets updated with this
version of the Chrome as they are internal Chrome pages. So the change of not
sending the click for non-primary buttons will deploy along with the change to
this file. Am I right? or is there something else that I'm missing?

[dgozman, 2016-07-27 20:49:28]

https://codereview.chromium.org/2163893003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js (right):

https://codereview.chromium.org/2163893003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js:1078:
tabElement.addEventListener("auxclick", this._tabClicked.bind(this), false);
On 2016/07/27 20:39:09, Navid Zolghadr wrote:
> On 2016/07/27 20:30:43, dgozman wrote:
> > Note that if you stop sending regular click events for non-primary buttons,
> this
> > code deployed in the wild will break.
> 
> I was under the impression that this code in the wild gets updated with this
> version of the Chrome as they are internal Chrome pages. So the change of not
> sending the click for non-primary buttons will deploy along with the change to
> this file. Am I right? or is there something else that I'm missing?

We have old version in the cloud, which are used regularly, so this will break
them.

Anyway, this seems like breaking web platform compat. Did you send an intent to
remove to blink-dev?

[Navid Zolghadr, 2016-07-27 20:56:51]

On 2016/07/27 20:49:28, dgozman wrote:
>
https://codereview.chromium.org/2163893003/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js (right):
> 
>
https://codereview.chromium.org/2163893003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js:1078:
> tabElement.addEventListener("auxclick", this._tabClicked.bind(this), false);
> On 2016/07/27 20:39:09, Navid Zolghadr wrote:
> > On 2016/07/27 20:30:43, dgozman wrote:
> > > Note that if you stop sending regular click events for non-primary
buttons,
> > this
> > > code deployed in the wild will break.
> > 
> > I was under the impression that this code in the wild gets updated with this
> > version of the Chrome as they are internal Chrome pages. So the change of
not
> > sending the click for non-primary buttons will deploy along with the change
to
> > this file. Am I right? or is there something else that I'm missing?
> 
> We have old version in the cloud, which are used regularly, so this will break
> them.
> 
> Anyway, this seems like breaking web platform compat. Did you send an intent
to
> remove to blink-dev?

Although right now I'm adding it to the web experimental flag however the intent
to remove for click event is this one
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/pYAh8bBl5Yc

Is there anything I can do to fix that cloud version? I'm not sure when we serve
this page from cloud and what the overall flow is there.

[dgozman, 2016-07-28 17:43:05]

> Although right now I'm adding it to the web experimental flag however the
intent
> to remove for click event is this one
> https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/pYAh8bBl5Yc
What's the time frame?

> 
> Is there anything I can do to fix that cloud version? I'm not sure when we
serve
> this page from cloud and what the overall flow is there.
We have a compat layer in
third_party/WebKit/Source/devtools/front_end/devtools.js.

[Navid Zolghadr, 2016-07-28 18:00:49]

On 2016/07/28 17:43:05, dgozman wrote:
> > Although right now I'm adding it to the web experimental flag however the
> intent
> > to remove for click event is this one
> > https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/pYAh8bBl5Yc
> What's the time frame?
> 

We'd like to get it in to 54.

> > 
> > Is there anything I can do to fix that cloud version? I'm not sure when we
> serve
> > this page from cloud and what the overall flow is there.
> We have a compat layer in
> third_party/WebKit/Source/devtools/front_end/devtools.js.

I need a little more background. I couldn't find any listener or click event in
that file. But I can see some codes for different versions of Chrome.
What I don't understand is the flow here. So do we have parts of devtools being
served from our clouds or something? What happens if we are offline then? Also
where do those files come from in our cloud?

[dgozman, 2016-07-28 19:54:09]

On 2016/07/28 18:00:49, Navid Zolghadr wrote:
> On 2016/07/28 17:43:05, dgozman wrote:
> > > Although right now I'm adding it to the web experimental flag however the
> > intent
> > > to remove for click event is this one
> > >
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/pYAh8bBl5Yc
> > What's the time frame?
> > 
> 
> We'd like to get it in to 54.
> 
> > > 
> > > Is there anything I can do to fix that cloud version? I'm not sure when we
> > serve
> > > this page from cloud and what the overall flow is there.
> > We have a compat layer in
> > third_party/WebKit/Source/devtools/front_end/devtools.js.
> 
> I need a little more background. I couldn't find any listener or click event
in
> that file. But I can see some codes for different versions of Chrome.
> What I don't understand is the flow here. So do we have parts of devtools
being
> served from our clouds or something? What happens if we are offline then? Also
> where do those files come from in our cloud?

This file is executed in DevTools frontend before anything else, and it comes
with Chrome (which means it knows for sure which features are present).
We load entire frontend from cloud sometimes (e.g. for android). I'm not sure
how to polyfill your change: ideally we would refire click/dblclick for
auxclick.
If it turns out to be unreliable, I'd propose to postpone the breaking change
for a while, and commit this patch as earlier as possible.

[dtapuska, 2016-07-28 23:41:11]

https://codereview.chromium.org/2163893003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js (right):

https://codereview.chromium.org/2163893003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js:1078:
tabElement.addEventListener("auxclick", this._tabClicked.bind(this), false);
On 2016/07/27 20:49:27, dgozman wrote:
> On 2016/07/27 20:39:09, Navid Zolghadr wrote:
> > On 2016/07/27 20:30:43, dgozman wrote:
> > > Note that if you stop sending regular click events for non-primary
buttons,
> > this
> > > code deployed in the wild will break.
> > 
> > I was under the impression that this code in the wild gets updated with this
> > version of the Chrome as they are internal Chrome pages. So the change of
not
> > sending the click for non-primary buttons will deploy along with the change
to
> > this file. Am I right? or is there something else that I'm missing?
> 
> We have old version in the cloud, which are used regularly, so this will break
> them.
> 
> Anyway, this seems like breaking web platform compat. Did you send an intent
to
> remove to blink-dev?

We can probably polyfill for auxclick. Although you do need to detect somehow
that the code you downloaded from the cloud now supports auxclick so don't
re-dispatch the auxclick as a click event. Not entirely sure how the devtools
follows a pattern for that. Likewise the auxclick is runtime enabled; so that
polyfill would have to be handled somehow like that.

[Navid Zolghadr, 2016-08-03 18:09:05]

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

[commit-bot: I haz the power, 2016-08-03 18:09:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Navid Zolghadr, 2016-08-03 18:13:04]

ptal.
dgozman@ I added the polyfill change to devtool.js. Can you have a look and see
whether I could have done it better? As far as I tested with the usecases I was
aware of now this works for the remote inspectors as well. I was not sure if the
support check I used for InspectorFrontendAPI is right or not. Basically I
wanted to send the event if the old inspector does not support this new event.

[commit-bot: I haz the power, 2016-08-03 18:13:38]

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

[commit-bot: I haz the power, 2016-08-03 18:13:39]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dgozman, 2016-08-03 20:54:28]

https://codereview.chromium.org/2163893003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/devtools.js (right):

https://codereview.chromium.org/2163893003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/devtools.js:1092: if
(window.hasOwnProperty("onauxclick") &&
!window["InspectorFrontendAPI"].hasOwnProperty("onauxclick")) {
This detection won't work: the hosting Chrome browser either has auxclick or
not. All the code is executed in the same browser, so this should be always
false.

https://codereview.chromium.org/2163893003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js (right):

https://codereview.chromium.org/2163893003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js:1084:
tabElement.addEventListener("auxclick", this._tabClicked.bind(this), false);
I took a closer look, and now I think that we can probably regress closing tabs
with middle click behavior for legacy frontends - it's not a very big deal. This
means we could avoid polyfill entirely.

On a side note though, how did you find all the places which have to be fixed?
I'm worried that it's easy to miss them.

https://codereview.chromium.org/2163893003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js:1101: var
middleButton = event.button === 1;
Does the event.button work for auxclick? Will it continue to work for click?

https://codereview.chromium.org/2163893003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js (right):

https://codereview.chromium.org/2163893003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:367:
this._listItemNode.addEventListener("auxclick",
this._treeElementToggled.bind(this), false);
Why change this? Are you going to stop sending dblclick events for main button
as well?

[Navid Zolghadr, 2016-08-03 21:35:20]

https://codereview.chromium.org/2163893003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/devtools.js (right):

https://codereview.chromium.org/2163893003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/devtools.js:1092: if
(window.hasOwnProperty("onauxclick") &&
!window["InspectorFrontendAPI"].hasOwnProperty("onauxclick")) {
On 2016/08/03 20:54:28, dgozman wrote:
> This detection won't work: the hosting Chrome browser either has auxclick or
> not. All the code is executed in the same browser, so this should be always
> false.

I tested this and this did cause my remote devtools (tab closing with middle
click) work there.  But anyways, as per your other comment I'm going to remove
it in the next patch.

https://codereview.chromium.org/2163893003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js (right):

https://codereview.chromium.org/2163893003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js:1084:
tabElement.addEventListener("auxclick", this._tabClicked.bind(this), false);
On 2016/08/03 20:54:28, dgozman wrote:
> I took a closer look, and now I think that we can probably regress closing
tabs
> with middle click behavior for legacy frontends - it's not a very big deal.
This
> means we could avoid polyfill entirely.
> 
> On a side note though, how did you find all the places which have to be fixed?
> I'm worried that it's easy to miss them.

Awesome. Then I will get rid of that polyfill.
Regarding whether I covered all the usecases or not previously I broke this
behavior and listed all the regressions it caused as blocking bugs for
http://crbug.com/255. Also I did some code search over button checks and
basically fixed them all here in this patch. I will continue monitoring if
anything comes up and will fix that. Note that this feature will remain behind
experimental flag for some time just to make sure everything is fine.

https://codereview.chromium.org/2163893003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js:1101: var
middleButton = event.button === 1;
On 2016/08/03 20:54:28, dgozman wrote:
> Does the event.button work for auxclick? Will it continue to work for click?

Yes. It works for both. Basically they have the same interface (MouseEvent) with
different event name.

https://codereview.chromium.org/2163893003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js (right):

https://codereview.chromium.org/2163893003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:367:
this._listItemNode.addEventListener("auxclick",
this._treeElementToggled.bind(this), false);
On 2016/08/03 20:54:28, dgozman wrote:
> Why change this? Are you going to stop sending dblclick events for main button
> as well?

We stopped sending dblclick for middle button but not for primary (left) button.
There is this behavior for middle double click in devtools when you double
middle click an html attribute it selects the attribute name and the value
together as oppose to double clicking with left button which only selects the
value. Anyhow this way of handling dblclick would handle them both in one shot
and I thought it is cleaner to handle them both with the detail check. Is that
okay?

[dgozman, 2016-08-03 22:31:50]

https://codereview.chromium.org/2163893003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js (right):

https://codereview.chromium.org/2163893003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js:1084:
tabElement.addEventListener("auxclick", this._tabClicked.bind(this), false);
On 2016/08/03 21:35:20, Navid Zolghadr wrote:
> On 2016/08/03 20:54:28, dgozman wrote:
> > I took a closer look, and now I think that we can probably regress closing
> tabs
> > with middle click behavior for legacy frontends - it's not a very big deal.
> This
> > means we could avoid polyfill entirely.
> > 
> > On a side note though, how did you find all the places which have to be
fixed?
> > I'm worried that it's easy to miss them.
> 
> Awesome. Then I will get rid of that polyfill.
> Regarding whether I covered all the usecases or not previously I broke this
> behavior and listed all the regressions it caused as blocking bugs for
> http://crbug.com/255. Also I did some code search over button checks and
> basically fixed them all here in this patch. I will continue monitoring if
> anything comes up and will fix that. Note that this feature will remain behind
> experimental flag for some time just to make sure everything is fine.

Thanks for explanation.

https://codereview.chromium.org/2163893003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js (right):

https://codereview.chromium.org/2163893003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:367:
this._listItemNode.addEventListener("auxclick",
this._treeElementToggled.bind(this), false);
On 2016/08/03 21:35:20, Navid Zolghadr wrote:
> On 2016/08/03 20:54:28, dgozman wrote:
> > Why change this? Are you going to stop sending dblclick events for main
button
> > as well?
> 
> We stopped sending dblclick for middle button but not for primary (left)
button.
> There is this behavior for middle double click in devtools when you double
> middle click an html attribute it selects the attribute name and the value
> together as oppose to double clicking with left button which only selects the
> value. Anyhow this way of handling dblclick would handle them both in one shot
> and I thought it is cleaner to handle them both with the detail check. Is that
> okay?

I cannot repro the behavior you mention on linux. Perhaps, that's the default
"select on double click" behavior which differs between OSes? Judging by this
code, we have no special handling. I'd prefer to continue using dblclick, so
that double middle click doesn't do this.

[Navid Zolghadr, 2016-08-04 14:42:05]

ptal.

https://codereview.chromium.org/2163893003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js (right):

https://codereview.chromium.org/2163893003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:367:
this._listItemNode.addEventListener("auxclick",
this._treeElementToggled.bind(this), false);
On 2016/08/03 22:31:49, dgozman wrote:
> On 2016/08/03 21:35:20, Navid Zolghadr wrote:
> > On 2016/08/03 20:54:28, dgozman wrote:
> > > Why change this? Are you going to stop sending dblclick events for main
> button
> > > as well?
> > 
> > We stopped sending dblclick for middle button but not for primary (left)
> button.
> > There is this behavior for middle double click in devtools when you double
> > middle click an html attribute it selects the attribute name and the value
> > together as oppose to double clicking with left button which only selects
the
> > value. Anyhow this way of handling dblclick would handle them both in one
shot
> > and I thought it is cleaner to handle them both with the detail check. Is
that
> > okay?
> 
> I cannot repro the behavior you mention on linux. Perhaps, that's the default
> "select on double click" behavior which differs between OSes? Judging by this
> code, we have no special handling. I'd prefer to continue using dblclick, so
> that double middle click doesn't do this.

Sure. done.

[dgozman, 2016-08-08 17:00:12]

On 2016/08/04 14:42:05, Navid Zolghadr wrote:
> ptal.
> 
>
https://codereview.chromium.org/2163893003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js (right):
> 
>
https://codereview.chromium.org/2163893003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/devtools/front_end/ui/treeoutline.js:367:
> this._listItemNode.addEventListener("auxclick",
> this._treeElementToggled.bind(this), false);
> On 2016/08/03 22:31:49, dgozman wrote:
> > On 2016/08/03 21:35:20, Navid Zolghadr wrote:
> > > On 2016/08/03 20:54:28, dgozman wrote:
> > > > Why change this? Are you going to stop sending dblclick events for main
> > button
> > > > as well?
> > > 
> > > We stopped sending dblclick for middle button but not for primary (left)
> > button.
> > > There is this behavior for middle double click in devtools when you double
> > > middle click an html attribute it selects the attribute name and the value
> > > together as oppose to double clicking with left button which only selects
> the
> > > value. Anyhow this way of handling dblclick would handle them both in one
> shot
> > > and I thought it is cleaner to handle them both with the detail check. Is
> that
> > > okay?
> > 
> > I cannot repro the behavior you mention on linux. Perhaps, that's the
default
> > "select on double click" behavior which differs between OSes? Judging by
this
> > code, we have no special handling. I'd prefer to continue using dblclick, so
> > that double middle click doesn't do this.
> 
> Sure. done.

Thanks! DevTools front_end looks good now.

[Navid Zolghadr, 2016-08-09 00:42:10]

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

[Dan Beam, 2016-08-09 02:04:34]

this looks WAY better to me.  has anybody else voice support for this?

anyways, this code looks pretty good to me, just wanted to ensure i'm
interpreting the blink code / behavior correctly.

https://codereview.chromium.org/2163893003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/ntp4/nav_dot.js (right):

https://codereview.chromium.org/2163893003/diff/60001/chrome/browser/resource...
chrome/browser/resources/ntp4/nav_dot.js:58: this.addEventListener('auxclick',
this.onClick_);
did people file bugs on your about not switching to the correct apps page when
/middle/-clicking a "nav dot"?  that seems kind of invalid to me...

https://codereview.chromium.org/2163893003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/2163893003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1190: :
EventTypeNames::auxclick,
so this guarantees there'll always be either 'click' with button == 1 OR an
'auxclick'?

[Navid Zolghadr, 2016-08-09 13:34:47]

https://codereview.chromium.org/2163893003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/ntp4/nav_dot.js (right):

https://codereview.chromium.org/2163893003/diff/60001/chrome/browser/resource...
chrome/browser/resources/ntp4/nav_dot.js:58: this.addEventListener('auxclick',
this.onClick_);
On 2016/08/09 02:04:34, Dan Beam wrote:
> did people file bugs on your about not switching to the correct apps page when
> /middle/-clicking a "nav dot"?  that seems kind of invalid to me...

There was no bug filed for this. This was one of the occurrences that I found
with search and fixed in the sense that I kept the previous behavior. Do you
think that behavior is not-wanted behavior of listening to the click that was
sent for all buttons?

https://codereview.chromium.org/2163893003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/2163893003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1190: :
EventTypeNames::auxclick,
On 2016/08/09 02:04:34, Dan Beam wrote:
> so this guarantees there'll always be either 'click' with button == 1 OR an
> 'auxclick'?

That is correct. After this change for the primary button (which is Left mouse
button for example with enum value 0) we send click event and for all other
buttons we send auxclick. These events will have the exact same properties and
everything. So the previous js codes should just work listening to auxclick
instead of click or listening to both if they want.

[Dan Beam, 2016-08-09 16:22:54]

lgtm

https://codereview.chromium.org/2163893003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/ntp4/nav_dot.js (right):

https://codereview.chromium.org/2163893003/diff/60001/chrome/browser/resource...
chrome/browser/resources/ntp4/nav_dot.js:58: this.addEventListener('auxclick',
this.onClick_);
On 2016/08/09 13:34:46, Navid Zolghadr wrote:
> On 2016/08/09 02:04:34, Dan Beam wrote:
> > did people file bugs on your about not switching to the correct apps page
when
> > /middle/-clicking a "nav dot"?  that seems kind of invalid to me...
> 
> There was no bug filed for this. This was one of the occurrences that I found
> with search and fixed in the sense that I kept the previous behavior. Do you
> think that behavior is not-wanted behavior of listening to the click that was
> sent for all buttons?

I think it's fine just to ignore auxiliary clicks for this particular thing.  I
wouldn't expect something useful to happen when middle clicking anywhere on the
page ;)

[Navid Zolghadr, 2016-08-10 14:32:09]

https://codereview.chromium.org/2163893003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/ntp4/nav_dot.js (right):

https://codereview.chromium.org/2163893003/diff/60001/chrome/browser/resource...
chrome/browser/resources/ntp4/nav_dot.js:58: this.addEventListener('auxclick',
this.onClick_);
On 2016/08/09 16:22:54, Dan Beam wrote:
> On 2016/08/09 13:34:46, Navid Zolghadr wrote:
> > On 2016/08/09 02:04:34, Dan Beam wrote:
> > > did people file bugs on your about not switching to the correct apps page
> when
> > > /middle/-clicking a "nav dot"?  that seems kind of invalid to me...
> > 
> > There was no bug filed for this. This was one of the occurrences that I
found
> > with search and fixed in the sense that I kept the previous behavior. Do you
> > think that behavior is not-wanted behavior of listening to the click that
was
> > sent for all buttons?
> 
> I think it's fine just to ignore auxiliary clicks for this particular thing. 
I
> wouldn't expect something useful to happen when middle clicking anywhere on
the
> page ;)

If I recall correctly there is this little functionality that will be changed.
There is this web store link in the bottom right of the apps page which if you
middle click normally would open the web store in a new tab. I believe removing
this would remove that functionality.

[Dan Beam, 2016-08-10 22:35:27]

https://codereview.chromium.org/2163893003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/ntp4/nav_dot.js (right):

https://codereview.chromium.org/2163893003/diff/60001/chrome/browser/resource...
chrome/browser/resources/ntp4/nav_dot.js:58: this.addEventListener('auxclick',
this.onClick_);
On 2016/08/10 14:32:09, Navid Zolghadr wrote:
> On 2016/08/09 16:22:54, Dan Beam wrote:
> > On 2016/08/09 13:34:46, Navid Zolghadr wrote:
> > > On 2016/08/09 02:04:34, Dan Beam wrote:
> > > > did people file bugs on your about not switching to the correct apps
page
> > when
> > > > /middle/-clicking a "nav dot"?  that seems kind of invalid to me...
> > > 
> > > There was no bug filed for this. This was one of the occurrences that I
> found
> > > with search and fixed in the sense that I kept the previous behavior. Do
you
> > > think that behavior is not-wanted behavior of listening to the click that
> was
> > > sent for all buttons?
> > 
> > I think it's fine just to ignore auxiliary clicks for this particular thing.

> I
> > wouldn't expect something useful to happen when middle clicking anywhere on
> the
> > page ;)
> 
> If I recall correctly there is this little functionality that will be changed.
> There is this web store link in the bottom right of the apps page which if you
> middle click normally would open the web store in a new tab. I believe
removing
> this would remove that functionality.

see other comment

https://codereview.chromium.org/2163893003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/ntp4/new_tab.js (right):

https://codereview.chromium.org/2163893003/diff/60001/chrome/browser/resource...
chrome/browser/resources/ntp4/new_tab.js:108: onChromeWebStoreButtonClick);
you're thinking of this code

[dtapuska, 2016-08-11 14:12:17]

On 2016/08/10 22:35:27, Dan Beam wrote:
>
https://codereview.chromium.org/2163893003/diff/60001/chrome/browser/resource...
> File chrome/browser/resources/ntp4/nav_dot.js (right):
> 
>
https://codereview.chromium.org/2163893003/diff/60001/chrome/browser/resource...
> chrome/browser/resources/ntp4/nav_dot.js:58: this.addEventListener('auxclick',
> this.onClick_);
> On 2016/08/10 14:32:09, Navid Zolghadr wrote:
> > On 2016/08/09 16:22:54, Dan Beam wrote:
> > > On 2016/08/09 13:34:46, Navid Zolghadr wrote:
> > > > On 2016/08/09 02:04:34, Dan Beam wrote:
> > > > > did people file bugs on your about not switching to the correct apps
> page
> > > when
> > > > > /middle/-clicking a "nav dot"?  that seems kind of invalid to me...
> > > > 
> > > > There was no bug filed for this. This was one of the occurrences that I
> > found
> > > > with search and fixed in the sense that I kept the previous behavior. Do
> you
> > > > think that behavior is not-wanted behavior of listening to the click
that
> > was
> > > > sent for all buttons?
> > > 
> > > I think it's fine just to ignore auxiliary clicks for this particular
thing.
> 
> > I
> > > wouldn't expect something useful to happen when middle clicking anywhere
on
> > the
> > > page ;)
> > 
> > If I recall correctly there is this little functionality that will be
changed.
> > There is this web store link in the bottom right of the apps page which if
you
> > middle click normally would open the web store in a new tab. I believe
> removing
> > this would remove that functionality.
> 
> see other comment
> 
>
https://codereview.chromium.org/2163893003/diff/60001/chrome/browser/resource...
> File chrome/browser/resources/ntp4/new_tab.js (right):
> 
>
https://codereview.chromium.org/2163893003/diff/60001/chrome/browser/resource...
> chrome/browser/resources/ntp4/new_tab.js:108: onChromeWebStoreButtonClick);
> you're thinking of this code

lgtm; do we want to add a stable layout test in the meantime to ensure the
shipped behavior doesn't regress? Or do we expect the intent to ship be approved
in short order?

[Navid Zolghadr, 2016-08-11 16:11:03]

I copied the click test to the stable test suite so that we are sure we don't
fire auxclick in that case without experimental flag.
Note that the existence of the property and the handler test already exist in
webexposed directory for the stable virtual test suite.

https://codereview.chromium.org/2163893003/diff/60001/chrome/browser/resource...
File chrome/browser/resources/ntp4/nav_dot.js (right):

https://codereview.chromium.org/2163893003/diff/60001/chrome/browser/resource...
chrome/browser/resources/ntp4/nav_dot.js:58: this.addEventListener('auxclick',
this.onClick_);
On 2016/08/10 22:35:26, Dan Beam wrote:
> On 2016/08/10 14:32:09, Navid Zolghadr wrote:
> > On 2016/08/09 16:22:54, Dan Beam wrote:
> > > On 2016/08/09 13:34:46, Navid Zolghadr wrote:
> > > > On 2016/08/09 02:04:34, Dan Beam wrote:
> > > > > did people file bugs on your about not switching to the correct apps
> page
> > > when
> > > > > /middle/-clicking a "nav dot"?  that seems kind of invalid to me...
> > > > 
> > > > There was no bug filed for this. This was one of the occurrences that I
> > found
> > > > with search and fixed in the sense that I kept the previous behavior. Do
> you
> > > > think that behavior is not-wanted behavior of listening to the click
that
> > was
> > > > sent for all buttons?
> > > 
> > > I think it's fine just to ignore auxiliary clicks for this particular
thing.
> 
> > I
> > > wouldn't expect something useful to happen when middle clicking anywhere
on
> > the
> > > page ;)
> > 
> > If I recall correctly there is this little functionality that will be
changed.
> > There is this web store link in the bottom right of the apps page which if
you
> > middle click normally would open the web store in a new tab. I believe
> removing
> > this would remove that functionality.
> 
> see other comment

Removed it. I guess if any bug comes up later we can always add listeners.

[Navid Zolghadr, 2016-08-11 16:11:27]

nzolghadr@chromium.org changed reviewers:
+ dglazkov@chromium.org, thestig@chromium.org

[Navid Zolghadr, 2016-08-11 16:11:28]

thestig@chromium.org: Please review changes in 
chrome/browser/ui/browser_browsertest.cc

dglazkov@chromium.org: Please review changes in 
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in

[Navid Zolghadr, 2016-08-12 14:34:52]

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

[bokan, 2016-08-12 15:29:17]

https://codereview.chromium.org/2163893003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/2163893003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1155: bool
macContextMenuEvent = false;
Is this a change in behaviour? It looks like we'd previously prevent sending the
click event on right clicks and now we send it.

[Navid Zolghadr, 2016-08-12 15:39:16]

https://codereview.chromium.org/2163893003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/2163893003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1155: bool
macContextMenuEvent = false;
On 2016/08/12 15:29:17, bokan wrote:
> Is this a change in behaviour? It looks like we'd previously prevent sending
the
> click event on right clicks and now we send it.

That is correct. But not quite a change in the behavior. We were preventing
click event on right click (and left click when happens with control on Mac).
Now we send auxclick for right clicks. So we don't prevent that and we send
auxclick if right click happens.

[bokan, 2016-08-12 15:42:20]

https://codereview.chromium.org/2163893003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/2163893003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1155: bool
macContextMenuEvent = false;
On 2016/08/12 15:39:16, Navid Zolghadr wrote:
> On 2016/08/12 15:29:17, bokan wrote:
> > Is this a change in behaviour? It looks like we'd previously prevent sending
> the
> > click event on right clicks and now we send it.
> 
> That is correct. But not quite a change in the behavior. We were preventing
> click event on right click (and left click when happens with control on Mac).
> Now we send auxclick for right clicks. So we don't prevent that and we send
> auxclick if right click happens.

Got it. But if RuntimeEnabledFeatures::auxclickEnabled is off, won't we now send
a click event?

[Navid Zolghadr, 2016-08-12 16:01:18]

https://codereview.chromium.org/2163893003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/input/EventHandler.cpp (right):

https://codereview.chromium.org/2163893003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/input/EventHandler.cpp:1155: bool
macContextMenuEvent = false;
On 2016/08/12 15:42:20, bokan wrote:
> On 2016/08/12 15:39:16, Navid Zolghadr wrote:
> > On 2016/08/12 15:29:17, bokan wrote:
> > > Is this a change in behaviour? It looks like we'd previously prevent
sending
> > the
> > > click event on right clicks and now we send it.
> > 
> > That is correct. But not quite a change in the behavior. We were preventing
> > click event on right click (and left click when happens with control on
Mac).
> > Now we send auxclick for right clicks. So we don't prevent that and we send
> > auxclick if right click happens.
> 
> Got it. But if RuntimeEnabledFeatures::auxclickEnabled is off, won't we now
send
> a click event?

Right. Thanks for the catch. I'm surprised why the output of the test in stable
didn't break in that case. Probably context menu opens and consumes the events
or something like that. But I fixed this contextMenuEvent to consider whether
auxclick is enabled or not and initialized it based on that.

[bokan, 2016-08-12 16:02:27]

Source/core lgtm

[Navid Zolghadr, 2016-08-12 16:49:53]

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

[Navid Zolghadr, 2016-08-12 16:49:54]

holte@chromium.org: Please review changes in 

tools/metrics/histograms/histograms.xml

[Lei Zhang, 2016-08-13 02:15:30]

On 2016/08/11 16:11:28, Navid Zolghadr wrote:
> mailto:thestig@chromium.org: Please review changes in 
> chrome/browser/ui/browser_browsertest.cc

Still lgtm since it's the same as the last attempt.

[Navid Zolghadr, 2016-08-15 14:20:29]

Description was changed from

==========
Start sending auxclick instead of click for non-primary buttons

Stop sending dblclick and click evnt for non-primary buttons
and send auxclick event for the click action of non-primary
buttons.

BUG=625847, 255
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Start sending auxclick instead of click for non-primary buttons

Stop sending dblclick and click evnt for non-primary buttons
and send auxclick event for the click action of non-primary
buttons.


BUG=625847, 255
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

TBR=dglazkov@chromium.org, holte@chromium.org
==========

[Navid Zolghadr, 2016-08-15 14:29:10]

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

[commit-bot: I haz the power, 2016-08-15 14:29:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-15 14:34:10]

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

[commit-bot: I haz the power, 2016-08-15 14:34:11]

Dry run: Try jobs failed on following builders:
  closure_compilation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)

[Navid Zolghadr, 2016-08-15 14:46:34]

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

[commit-bot: I haz the power, 2016-08-15 14:46:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Navid Zolghadr, 2016-08-15 16:44:13]

The CQ bit was unchecked by nzolghadr@chromium.org

[Navid Zolghadr, 2016-08-15 16:44:23]

The CQ bit was checked by nzolghadr@chromium.org

[Navid Zolghadr, 2016-08-15 16:44:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtapuska@chromium.org, dbeam@chromium.org, thestig@chromium.org,
bokan@chromium.org
Link to the patchset: https://codereview.chromium.org/2163893003/#ps140001
(title: "Another rebase")

[commit-bot: I haz the power, 2016-08-15 16:44:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-15 18:57:00]

Description was changed from

==========
Start sending auxclick instead of click for non-primary buttons

Stop sending dblclick and click evnt for non-primary buttons
and send auxclick event for the click action of non-primary
buttons.


BUG=625847, 255
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

TBR=dglazkov@chromium.org, holte@chromium.org
==========

to

==========
Start sending auxclick instead of click for non-primary buttons

Stop sending dblclick and click evnt for non-primary buttons
and send auxclick event for the click action of non-primary
buttons.


BUG=625847, 255
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

TBR=dglazkov@chromium.org, holte@chromium.org
==========

[commit-bot: I haz the power, 2016-08-15 18:57:03]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-08-15 19:00:00]

Description was changed from

==========
Start sending auxclick instead of click for non-primary buttons

Stop sending dblclick and click evnt for non-primary buttons
and send auxclick event for the click action of non-primary
buttons.


BUG=625847, 255
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

TBR=dglazkov@chromium.org, holte@chromium.org
==========

to

==========
Start sending auxclick instead of click for non-primary buttons

Stop sending dblclick and click evnt for non-primary buttons
and send auxclick event for the click action of non-primary
buttons.

BUG=625847, 255
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

TBR=dglazkov@chromium.org, holte@chromium.org

Committed: https://crrev.com/6efe6e4b42ab875fee4ed7029f693fefc04c2658
Cr-Commit-Position: refs/heads/master@{#412005}
==========

[commit-bot: I haz the power, 2016-08-15 19:00:02]

Patchset 8 (id:??) landed as
https://crrev.com/6efe6e4b42ab875fee4ed7029f693fefc04c2658
Cr-Commit-Position: refs/heads/master@{#412005}

[Steven Holte, 2016-08-15 22:00:18]

histograms lgtm