Fix an issue in checkbox handler to enable extension.

Fix an issue in checkbox handler to enable extension.

The code is supposed to preventDefault() always and update the
checkbox's state when enabling/disabling result comes back from
management API call. However, this uses <input>'s onchange event
handler to preventDefault() and onchange event is not cancelable
(https://developer.mozilla.org/en-US/docs/Web/Events/change),
so calling preventDefault() has no effect.

This regressed in
https://codereview.chromium.org/893453002/diff/300001/chrome/browser/resources/extensions/extension_list.js
as I understand.

The issue is a bit serious when user tries to enable a permission
increased extension. Enabling permission increased extension results
in showing a "Re-enable" prompt. Because the preventDefault() code
was busted, it will keep showing that the extension is "Enabled" even
if you cancel the "Re-enable" prompt.

This CL fixes the issue.

BUG=607353
Test=a) Update an extension so it has more permissions now and it shows
"extension was disabled due to permission increase" warning.
b) Try to enable the extension from chrome://extensions, it will
show you a "Cancel"/"Re-enable" prompt.
c) Hit "Cancel". Without this change, you'd see that extension's Enable
checkbox gets checked incorrectly. With this CL, you'd see that
the "Enable" checkbox stays unchecked.

I've also tested enabling extension with:
a. click/mousedown with a mouse on desktop
b. tap on chromebook pixel
c. screenreader: with OSX voiceover. (cmd+f5 to turn on voice over, tab to select the checkbox, ctrl+option+space to select/deselect the checkbox).
d. keyboard: focus the checkbox and press space.
In *all* of the above cases, a "click" event gets dispatched, so this should not break anything.

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

[lazyboy, 2016-04-26 01:38:22]

lazyboy@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[lazyboy, 2016-04-26 01:38:23]

I couldn't find any related crbug, do you know of any?

[Devlin, 2016-04-26 14:24:48]

rdevlin.cronin@chromium.org changed reviewers:
+ dbeam@chromium.org

[Devlin, 2016-04-26 14:24:48]

+Dan from that original review - it looks like he requested the change here:
https://codereview.chromium.org/893453002/diff/200001/chrome/browser/resource....
 Dan, what was the motivation for using 'change' instead of 'click'?  (See CL
description for context).

[Dan Beam, 2016-04-27 02:31:34]

On 2016/04/26 14:24:48, Devlin wrote:
> +Dan from that original review - it looks like he requested the change here:
>
https://codereview.chromium.org/893453002/diff/200001/chrome/browser/resource....
>  Dan, what was the motivation for using 'change' instead of 'click'?  (See CL
> description for context).

screenreaders can do whacky things, maybe?

basically, as long as you've tested with keyboard, mouse, tap, and screenreader
that 'click' is generated all times change is, I'm fine with switching.

[Devlin, 2016-04-27 13:52:01]

lgtm as long as the tests Dan mentioned pass.

https://codereview.chromium.org/1920153002/diff/1/chrome/browser/resources/ex...
File chrome/browser/resources/extensions/extension_list.js (left):

https://codereview.chromium.org/1920153002/diff/1/chrome/browser/resources/ex...
chrome/browser/resources/extensions/extension_list.js:553: // TODO(devlin): What
should we do if this fails?
nit: I think this isn't entirely addressed - the state is accurate, but we don't
surface any kind of error to the user, etc.  It just looks totally broken then. 
So let's go ahead and leave in the TODO.

[lazyboy, 2016-04-27 22:12:55]

I've checked [1] that 'click' event is dispatched with mouse, tap (chromeos
pixel) and keyboard.
No clue about screenreaders though, any hint?

[1] fiddle https://jsfiddle.net/lazyboybd/43ymbgj8/

https://codereview.chromium.org/1920153002/diff/1/chrome/browser/resources/ex...
File chrome/browser/resources/extensions/extension_list.js (left):

https://codereview.chromium.org/1920153002/diff/1/chrome/browser/resources/ex...
chrome/browser/resources/extensions/extension_list.js:553: // TODO(devlin): What
should we do if this fails?
On 2016/04/27 13:52:01, Devlin wrote:
> nit: I think this isn't entirely addressed - the state is accurate, but we
don't
> surface any kind of error to the user, etc.  It just looks totally broken
then. 
> So let's go ahead and leave in the TODO.

Ah, got it.
I've also updated the document to say we want to show some kind of error to the
user.
Done.

[Dan Beam, 2016-04-27 22:39:41]

On 2016/04/27 22:12:55, lazyboy wrote:
> I've checked [1] that 'click' event is dispatched with mouse, tap (chromeos
> pixel) and keyboard.
> No clue about screenreaders though, any hint?

Cmd+F5 to start voiceover on mac, browser to control via tab or
ctrl+option+(arrows), press ctrl+option+space to activate

[lazyboy, 2016-04-27 23:12:11]

Description was changed from

==========
Fix an issue in checkbox handler to enable extension.

The code is supposed to preventDefault() always and update the
checkbox's state when enabling/disabling result comes back from
management API call. However, this uses <input>'s onchange event
handler to preventDefault() and onchange event is not cancelable
(https://developer.mozilla.org/en-US/docs/Web/Events/change),
so calling preventDefault() has no effect.

This regressed in
https://codereview.chromium.org/893453002/diff/300001/chrome/browser/resource...
as I understand.

The issue is a bit serious when user tries to enable a permission
increased extension. Enabling permission increased extension results
in showing a "Re-enable" prompt. Because the preventDefault() code
was busted, it will keep showing that the extension is "Enabled" even
if you cancel the "Re-enable" prompt.

This CL fixes the issue.

BUG=None, can't find one, will file one.
Test=a) Update an extension so it has more permissions now and it shows
"extension was disabled due to permission increase" warning.
b) Try to enable the extension from chrome://extensions, it will
show you a "Cancel"/"Re-enable" prompt.
c) Hit "Cancel". Without this change, you'd see that extension's Enable
checkbox gets checked incorrectly. With this CL, you'd see that
the "Enable" checkbox stays unchecked.
==========

to

==========
Fix an issue in checkbox handler to enable extension.

The code is supposed to preventDefault() always and update the
checkbox's state when enabling/disabling result comes back from
management API call. However, this uses <input>'s onchange event
handler to preventDefault() and onchange event is not cancelable
(https://developer.mozilla.org/en-US/docs/Web/Events/change),
so calling preventDefault() has no effect.

This regressed in
https://codereview.chromium.org/893453002/diff/300001/chrome/browser/resource...
as I understand.

The issue is a bit serious when user tries to enable a permission
increased extension. Enabling permission increased extension results
in showing a "Re-enable" prompt. Because the preventDefault() code
was busted, it will keep showing that the extension is "Enabled" even
if you cancel the "Re-enable" prompt.

This CL fixes the issue.

BUG=607353
Test=a) Update an extension so it has more permissions now and it shows
"extension was disabled due to permission increase" warning.
b) Try to enable the extension from chrome://extensions, it will
show you a "Cancel"/"Re-enable" prompt.
c) Hit "Cancel". Without this change, you'd see that extension's Enable
checkbox gets checked incorrectly. With this CL, you'd see that
the "Enable" checkbox stays unchecked.

I've also tested enabling extension with:
a. click/mousedown with a mouse on desktop
b. tap on chromebook pixel
c. screenreader: with OSX voiceover. (cmd+f5 to turn on voice over, tab to
select the checkbox, ctrl+option+space to select/deselect the checkbox).
d. keyboard: focus the checkbox and press space.
In *all* of the above cases, a "click" event gets dispatched, so this should not
break anything.
==========

[lazyboy, 2016-04-27 23:14:10]

On 2016/04/27 22:39:41, Dan Beam wrote:
> On 2016/04/27 22:12:55, lazyboy wrote:
> > I've checked [1] that 'click' event is dispatched with mouse, tap (chromeos
> > pixel) and keyboard.
> > No clue about screenreaders though, any hint?
> 
> Cmd+F5 to start voiceover on mac, browser to control via tab or
> ctrl+option+(arrows), press ctrl+option+space to activate

Cool that worked, I've updated the CL's TEST section to mention all the
possible ways I've checked/unchecked a checkbox.

[Dan Beam, 2016-04-28 00:00:20]

lgtm

[lazyboy, 2016-04-28 00:16:32]

The CQ bit was checked by lazyboy@chromium.org

[lazyboy, 2016-04-28 00:16:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/1920153002/#ps20001
(title: "add TODO back")

[commit-bot: I haz the power, 2016-04-28 00:17:02]

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

[commit-bot: I haz the power, 2016-04-28 00:27:12]

Description was changed from

==========
Fix an issue in checkbox handler to enable extension.

The code is supposed to preventDefault() always and update the
checkbox's state when enabling/disabling result comes back from
management API call. However, this uses <input>'s onchange event
handler to preventDefault() and onchange event is not cancelable
(https://developer.mozilla.org/en-US/docs/Web/Events/change),
so calling preventDefault() has no effect.

This regressed in
https://codereview.chromium.org/893453002/diff/300001/chrome/browser/resource...
as I understand.

The issue is a bit serious when user tries to enable a permission
increased extension. Enabling permission increased extension results
in showing a "Re-enable" prompt. Because the preventDefault() code
was busted, it will keep showing that the extension is "Enabled" even
if you cancel the "Re-enable" prompt.

This CL fixes the issue.

BUG=607353
Test=a) Update an extension so it has more permissions now and it shows
"extension was disabled due to permission increase" warning.
b) Try to enable the extension from chrome://extensions, it will
show you a "Cancel"/"Re-enable" prompt.
c) Hit "Cancel". Without this change, you'd see that extension's Enable
checkbox gets checked incorrectly. With this CL, you'd see that
the "Enable" checkbox stays unchecked.

I've also tested enabling extension with:
a. click/mousedown with a mouse on desktop
b. tap on chromebook pixel
c. screenreader: with OSX voiceover. (cmd+f5 to turn on voice over, tab to
select the checkbox, ctrl+option+space to select/deselect the checkbox).
d. keyboard: focus the checkbox and press space.
In *all* of the above cases, a "click" event gets dispatched, so this should not
break anything.
==========

to

==========
Fix an issue in checkbox handler to enable extension.

The code is supposed to preventDefault() always and update the
checkbox's state when enabling/disabling result comes back from
management API call. However, this uses <input>'s onchange event
handler to preventDefault() and onchange event is not cancelable
(https://developer.mozilla.org/en-US/docs/Web/Events/change),
so calling preventDefault() has no effect.

This regressed in
https://codereview.chromium.org/893453002/diff/300001/chrome/browser/resource...
as I understand.

The issue is a bit serious when user tries to enable a permission
increased extension. Enabling permission increased extension results
in showing a "Re-enable" prompt. Because the preventDefault() code
was busted, it will keep showing that the extension is "Enabled" even
if you cancel the "Re-enable" prompt.

This CL fixes the issue.

BUG=607353
Test=a) Update an extension so it has more permissions now and it shows
"extension was disabled due to permission increase" warning.
b) Try to enable the extension from chrome://extensions, it will
show you a "Cancel"/"Re-enable" prompt.
c) Hit "Cancel". Without this change, you'd see that extension's Enable
checkbox gets checked incorrectly. With this CL, you'd see that
the "Enable" checkbox stays unchecked.

I've also tested enabling extension with:
a. click/mousedown with a mouse on desktop
b. tap on chromebook pixel
c. screenreader: with OSX voiceover. (cmd+f5 to turn on voice over, tab to
select the checkbox, ctrl+option+space to select/deselect the checkbox).
d. keyboard: focus the checkbox and press space.
In *all* of the above cases, a "click" event gets dispatched, so this should not
break anything.
==========

[commit-bot: I haz the power, 2016-04-28 00:27:13]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-04-30 17:14:56]

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

[commit-bot: I haz the power, 2016-04-30 17:53:16]

Description was changed from

==========
Fix an issue in checkbox handler to enable extension.

The code is supposed to preventDefault() always and update the
checkbox's state when enabling/disabling result comes back from
management API call. However, this uses <input>'s onchange event
handler to preventDefault() and onchange event is not cancelable
(https://developer.mozilla.org/en-US/docs/Web/Events/change),
so calling preventDefault() has no effect.

This regressed in
https://codereview.chromium.org/893453002/diff/300001/chrome/browser/resource...
as I understand.

The issue is a bit serious when user tries to enable a permission
increased extension. Enabling permission increased extension results
in showing a "Re-enable" prompt. Because the preventDefault() code
was busted, it will keep showing that the extension is "Enabled" even
if you cancel the "Re-enable" prompt.

This CL fixes the issue.

BUG=607353
Test=a) Update an extension so it has more permissions now and it shows
"extension was disabled due to permission increase" warning.
b) Try to enable the extension from chrome://extensions, it will
show you a "Cancel"/"Re-enable" prompt.
c) Hit "Cancel". Without this change, you'd see that extension's Enable
checkbox gets checked incorrectly. With this CL, you'd see that
the "Enable" checkbox stays unchecked.

I've also tested enabling extension with:
a. click/mousedown with a mouse on desktop
b. tap on chromebook pixel
c. screenreader: with OSX voiceover. (cmd+f5 to turn on voice over, tab to
select the checkbox, ctrl+option+space to select/deselect the checkbox).
d. keyboard: focus the checkbox and press space.
In *all* of the above cases, a "click" event gets dispatched, so this should not
break anything.
==========

to

==========
Fix an issue in checkbox handler to enable extension.

The code is supposed to preventDefault() always and update the
checkbox's state when enabling/disabling result comes back from
management API call. However, this uses <input>'s onchange event
handler to preventDefault() and onchange event is not cancelable
(https://developer.mozilla.org/en-US/docs/Web/Events/change),
so calling preventDefault() has no effect.

This regressed in
https://codereview.chromium.org/893453002/diff/300001/chrome/browser/resource...
as I understand.

The issue is a bit serious when user tries to enable a permission
increased extension. Enabling permission increased extension results
in showing a "Re-enable" prompt. Because the preventDefault() code
was busted, it will keep showing that the extension is "Enabled" even
if you cancel the "Re-enable" prompt.

This CL fixes the issue.

BUG=607353
Test=a) Update an extension so it has more permissions now and it shows
"extension was disabled due to permission increase" warning.
b) Try to enable the extension from chrome://extensions, it will
show you a "Cancel"/"Re-enable" prompt.
c) Hit "Cancel". Without this change, you'd see that extension's Enable
checkbox gets checked incorrectly. With this CL, you'd see that
the "Enable" checkbox stays unchecked.

I've also tested enabling extension with:
a. click/mousedown with a mouse on desktop
b. tap on chromebook pixel
c. screenreader: with OSX voiceover. (cmd+f5 to turn on voice over, tab to
select the checkbox, ctrl+option+space to select/deselect the checkbox).
d. keyboard: focus the checkbox and press space.
In *all* of the above cases, a "click" event gets dispatched, so this should not
break anything.

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