DevTools: all swatches should have a default focused element

DevTools: all swatches should have a default focused element

Color swatches correctly respond to 'Enter' and 'Esc' because Spectrum calls
focus() whenever it loads. Bezier and shadow editors did not. This CL makes
bezier and shadow editors set a default focused element. SwatchPopoverHelper now
calls focus() after showing a view, instead of requiring each swatch-popover
implementation call focus.

One peculiarity with color swatches is they may hide and show themselves after
loading a palette. To keep focus on the popover, focus restoring logic was added
to the SwatchPopoverHelper whenever hiding an old popover.

BUG=665801
Committed: https://crrev.com/0384ce04d25ba97446b20e7ca2d423cec10c2ffd
Cr-Commit-Position: refs/heads/master@{#433382}

[luoe, 2016-11-16 20:47:03]

Description was changed from

==========
DevTools: all swatches should have a default focused element

BUG=665801
==========

to

==========
DevTools: all swatches should have a default focused element

Color swatches depended on the Spectrum class, which internally called focus().
Since all swatch popovers need an 'Escape' keydown, all popovers should be
focused when shown. This CL removes Spectrum's focus code and lets the
SwatchPopoverHelper call focus and each popover defaults to focusing its
contentElement.

BUG=665801
==========

[luoe, 2016-11-17 00:08:47]

Description was changed from

==========
DevTools: all swatches should have a default focused element

Color swatches depended on the Spectrum class, which internally called focus().
Since all swatch popovers need an 'Escape' keydown, all popovers should be
focused when shown. This CL removes Spectrum's focus code and lets the
SwatchPopoverHelper call focus and each popover defaults to focusing its
contentElement.

BUG=665801
==========

to

==========
DevTools: all swatches should have a default focused element

Color swatches correctly respond to 'Enter' and 'Esc' because Spectrum calls
focus() whenever it loads. Bezier and shadow editors did not. This CL makes
bezier and shadow editors set a default focused element. SwatchPopoverHelper now
calls focus() after showing a view, instead of requiring each swatch-popover
implementation call focus.

One peculiarity with color swatches is they may hide and show themselves after
loading a palette. To keep focus on the popover, focus restoring logic was added
to the SwatchPopoverHelper whenever hiding an old popover.

BUG=665801
==========

[luoe, 2016-11-17 00:09:16]

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

[luoe, 2016-11-17 00:09:16]

PTAL

[luoe, 2016-11-17 00:09:45]

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

[commit-bot: I haz the power, 2016-11-17 00:11:22]

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

[dgozman, 2016-11-17 01:29:50]

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

https://codereview.chromium.org/2510883002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/Popover.js:105: view.focus();
How do you focus view before showing it in line 108?

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

https://codereview.chromium.org/2510883002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/SwatchPopoverHelper.js:56:
view.focus();
Shouldn't call to popover.showView handle this?

[commit-bot: I haz the power, 2016-11-17 04:14:01]

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

[commit-bot: I haz the power, 2016-11-17 04:14:02]

Dry run: This issue passed the CQ dry run.

[luoe, 2016-11-17 21:38:08]

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

https://codereview.chromium.org/2510883002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/Popover.js:105: view.focus();
On 2016/11/17 01:29:50, dgozman wrote:
> How do you focus view before showing it in line 108?

Ahh, good point.  We should restore focus after the view/contentElement is
showing.

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

https://codereview.chromium.org/2510883002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/ui/SwatchPopoverHelper.js:56:
view.focus();
On 2016/11/17 01:29:50, dgozman wrote:
> Shouldn't call to popover.showView handle this?

I don't think so.  Not all popovers need to take focus for themselves.  For
swatches, I think it's always reasonable for them to steal focus because
swatches open for editing, which requires user interaction.

Placing .focus() in the general popover.showView will affect popovers shown on
hover, e.g. hovering over rows/blocks in Network, Timeline, Animations panel
which are meant to be ephemeral and hide on mouseout.  If we put .focus() there,
we'd also need to put a focus restorer there, which isn't necessary.

[luoe, 2016-11-18 20:14:30]

PTAL

[dgozman, 2016-11-18 23:31:42]

lgtm

[luoe, 2016-11-18 23:32:48]

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

[commit-bot: I haz the power, 2016-11-18 23:33:57]

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-11-19 02:20:16]

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

[commit-bot: I haz the power, 2016-11-19 02:20:17]

Dry run: This issue passed the CQ dry run.

[luoe, 2016-11-19 02:20:43]

The CQ bit was checked by luoe@chromium.org

[commit-bot: I haz the power, 2016-11-19 02:21:03]

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-11-19 02:25:38]

Description was changed from

==========
DevTools: all swatches should have a default focused element

Color swatches correctly respond to 'Enter' and 'Esc' because Spectrum calls
focus() whenever it loads. Bezier and shadow editors did not. This CL makes
bezier and shadow editors set a default focused element. SwatchPopoverHelper now
calls focus() after showing a view, instead of requiring each swatch-popover
implementation call focus.

One peculiarity with color swatches is they may hide and show themselves after
loading a palette. To keep focus on the popover, focus restoring logic was added
to the SwatchPopoverHelper whenever hiding an old popover.

BUG=665801
==========

to

==========
DevTools: all swatches should have a default focused element

Color swatches correctly respond to 'Enter' and 'Esc' because Spectrum calls
focus() whenever it loads. Bezier and shadow editors did not. This CL makes
bezier and shadow editors set a default focused element. SwatchPopoverHelper now
calls focus() after showing a view, instead of requiring each swatch-popover
implementation call focus.

One peculiarity with color swatches is they may hide and show themselves after
loading a palette. To keep focus on the popover, focus restoring logic was added
to the SwatchPopoverHelper whenever hiding an old popover.

BUG=665801
==========

[commit-bot: I haz the power, 2016-11-19 02:25:39]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-19 02:28:01]

Description was changed from

==========
DevTools: all swatches should have a default focused element

Color swatches correctly respond to 'Enter' and 'Esc' because Spectrum calls
focus() whenever it loads. Bezier and shadow editors did not. This CL makes
bezier and shadow editors set a default focused element. SwatchPopoverHelper now
calls focus() after showing a view, instead of requiring each swatch-popover
implementation call focus.

One peculiarity with color swatches is they may hide and show themselves after
loading a palette. To keep focus on the popover, focus restoring logic was added
to the SwatchPopoverHelper whenever hiding an old popover.

BUG=665801
==========

to

==========
DevTools: all swatches should have a default focused element

Color swatches correctly respond to 'Enter' and 'Esc' because Spectrum calls
focus() whenever it loads. Bezier and shadow editors did not. This CL makes
bezier and shadow editors set a default focused element. SwatchPopoverHelper now
calls focus() after showing a view, instead of requiring each swatch-popover
implementation call focus.

One peculiarity with color swatches is they may hide and show themselves after
loading a palette. To keep focus on the popover, focus restoring logic was added
to the SwatchPopoverHelper whenever hiding an old popover.

BUG=665801

Committed: https://crrev.com/0384ce04d25ba97446b20e7ca2d423cec10c2ffd
Cr-Commit-Position: refs/heads/master@{#433382}
==========

[commit-bot: I haz the power, 2016-11-19 02:28:03]

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

[luoe, 2016-11-22 19:59:57]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2527503002/ by luoe@chromium.org.

The reason for reverting is: This introduced a color picker bug
crbug.com/667739.  To land, we need to fix the regression..