New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 665801 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Mac
Pri: 1
Type: Bug-Regression



Sign in to add a comment

Regression: [DevTools] Unable to close the shadow window using 'Esc' key.

Reported by dmascare...@etouch.net, Nov 16 2016

Issue description

Chrome Version:56.0.2920.0 (Official Build) 378fc3fd49ebb678fc98cd99831b858fd304691f-refs/heads/master@{#432057} 32/64-bit.
OS: Windows (7,8,10), Mac (10.11.6, 10.12.1), Linux (14.04 LTS)

What steps will reproduce the problem?
1. Launch chrome, go to NTP and open 'DevTools' window.
2. Go to 'Style' section and hover the mouse pointer on three dot icon to open menu icons. 
3. Click on 'Add box-shadow' or 'Add text-shadow' icon to open the shadow window.
4. Now, press 'Esc' key from keyboard to close the shadow window and observe.

On pressing 'Esc' key, Unable to close the shadow window and unnecessary console drawer gets opened.
Shadow window should get closed on pressing 'Esc' key from keyboard.

This is a regression issue, broken in M-56 series, below is manual regression range.

Good build: 56.0.2884.0
Bad build: 56.0.2886.0

Kindly review the attached screen-cast for reference.
 
Actual behavior.mp4
1018 KB View Download
Expected behavior.mp4
1.2 MB View Download
Labels: hasbisect-per-revision
Owner: dgozman@chromium.org
Status: Assigned (was: Unconfirmed)
Using the per-revision bisect providing the bisect results,
Good build: 56.0.2884.0 (Revision: 424021).
Bad build: 56.0.2886.0 (Revision: 424099).

You are probably looking for a change made after 424034 (known good), but no later than 424035 (first known bad).
CHANGELOG URL:
  https://chromium.googlesource.com/chromium/src/+log/5dfbc0a96c848ec1e232d543921330cc7aaa15f4..c320a900cd8320048e1e1f553cdc01f2633c68b3

Adding RB Label as this is a recent Regression. Please remove if not required.

@dgozman -- Could you please look into the issue, pardon me if it has nothing to do with your changes and if possible please assign it to concern owner.
Thank You.
Labels: ReleaseBlock-Stable
Cc: dgozman@chromium.org
Owner: l...@chromium.org
Eric, could you please take a look?
Since color picker works fine, but shadow editor does not, I believe the problem is in shadow editor code somewhere.
Project Member

Comment 4 by bugdroid1@chromium.org, Nov 19 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0384ce04d25ba97446b20e7ca2d423cec10c2ffd

commit 0384ce04d25ba97446b20e7ca2d423cec10c2ffd
Author: luoe <luoe@chromium.org>
Date: Sat Nov 19 02:25:10 2016

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 

Review-Url: https://codereview.chromium.org/2510883002
Cr-Commit-Position: refs/heads/master@{#433382}

[modify] https://crrev.com/0384ce04d25ba97446b20e7ca2d423cec10c2ffd/third_party/WebKit/Source/devtools/front_end/components/Spectrum.js
[modify] https://crrev.com/0384ce04d25ba97446b20e7ca2d423cec10c2ffd/third_party/WebKit/Source/devtools/front_end/ui/BezierEditor.js
[modify] https://crrev.com/0384ce04d25ba97446b20e7ca2d423cec10c2ffd/third_party/WebKit/Source/devtools/front_end/ui/CSSShadowEditor.js
[modify] https://crrev.com/0384ce04d25ba97446b20e7ca2d423cec10c2ffd/third_party/WebKit/Source/devtools/front_end/ui/Popover.js
[modify] https://crrev.com/0384ce04d25ba97446b20e7ca2d423cec10c2ffd/third_party/WebKit/Source/devtools/front_end/ui/SwatchPopoverHelper.js

Comment 5 by l...@chromium.org, Nov 21 2016

Labels: Merge-Request-56

Comment 6 by dimu@chromium.org, Nov 21 2016

Labels: -Merge-Request-56 Merge-Approved-56 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M56 (branch: 2924)

Comment 7 by l...@chromium.org, Nov 21 2016

After discussion, we've decided not to merge this immediately.  Focus lacks coverage so we will give it some more time on Canary.
Project Member

Comment 8 by bugdroid1@chromium.org, Nov 23 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7f89a2a5efbd41b83b66c8d18bea8eb9804e175b

commit 7f89a2a5efbd41b83b66c8d18bea8eb9804e175b
Author: luoe <luoe@chromium.org>
Date: Wed Nov 23 01:12:06 2016

Revert of DevTools: all swatches should have a default focused element (patchset #4 id:60001 of https://codereview.chromium.org/2510883002/ )

Reason for revert:
This introduced a color picker bug  crbug.com/667739 .  To land, we need to fix the regression.

Original issue's description:
> 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}

TBR=dgozman@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 665801 

Review-Url: https://codereview.chromium.org/2527503002
Cr-Commit-Position: refs/heads/master@{#434060}

[modify] https://crrev.com/7f89a2a5efbd41b83b66c8d18bea8eb9804e175b/third_party/WebKit/Source/devtools/front_end/components/Spectrum.js
[modify] https://crrev.com/7f89a2a5efbd41b83b66c8d18bea8eb9804e175b/third_party/WebKit/Source/devtools/front_end/ui/BezierEditor.js
[modify] https://crrev.com/7f89a2a5efbd41b83b66c8d18bea8eb9804e175b/third_party/WebKit/Source/devtools/front_end/ui/CSSShadowEditor.js
[modify] https://crrev.com/7f89a2a5efbd41b83b66c8d18bea8eb9804e175b/third_party/WebKit/Source/devtools/front_end/ui/Popover.js
[modify] https://crrev.com/7f89a2a5efbd41b83b66c8d18bea8eb9804e175b/third_party/WebKit/Source/devtools/front_end/ui/SwatchPopoverHelper.js

Project Member

Comment 9 by bugdroid1@chromium.org, Nov 26 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/69cc45e5e9b178c78fede5e9c4271aad78847854

commit 69cc45e5e9b178c78fede5e9c4271aad78847854
Author: luoe <luoe@chromium.org>
Date: Sat Nov 26 07:21:11 2016

DevTools: all swatches get default focused element (reland with fix)

This is a reland of another CL, but with an additional fix:
https://codereview.chromium.org/2510883002/

Instead of modifying the color picker's focus logic, this CL simply
adds a focus() call for all swatches and sets the default focused
element for the other swatch types.

BUG= 665801 

Review-Url: https://codereview.chromium.org/2514353006
Cr-Commit-Position: refs/heads/master@{#434572}

[modify] https://crrev.com/69cc45e5e9b178c78fede5e9c4271aad78847854/third_party/WebKit/Source/devtools/front_end/ui/BezierEditor.js
[modify] https://crrev.com/69cc45e5e9b178c78fede5e9c4271aad78847854/third_party/WebKit/Source/devtools/front_end/ui/CSSShadowEditor.js
[modify] https://crrev.com/69cc45e5e9b178c78fede5e9c4271aad78847854/third_party/WebKit/Source/devtools/front_end/ui/SwatchPopoverHelper.js

dmascarenhas@ please verify the fix in latest canary.

luoe@ If all looks good, please merge your change to M56 (branch: 2924) ASAP so that we could take it for next Dev Release.
Labels: TE-Verified-57.0.2936.0 TE-Verified-57.0.2936.4 TE-Verified-M57
Tested the issue on Latest Canary# 57.0.2936.0 on Windows & Mac and #57.0.2936.4 on Ubuntu 14.04 and is working as intended.
Clicking on "Esc" closes the shadow window.
Adding TE-Verified Labels accordingly.
Attaching screen cast for reference.
Forgot to add the screen cast.
Thank You.
665801.mov
3.2 MB Download

Comment 13 by l...@chromium.org, Nov 30 2016

Labels: -ReleaseBlock-Stable
Status: Fixed (was: Assigned)
Thank you for the verification.  This bug was introduced in #433382 (https://codereview.chromium.org/2510883002/) which had not made it into Beta.  The corresponding fix in #434572.  Both were committed after the M56 branch point.  Since this is not a major issue, removing RBS label.
Project Member

Comment 14 by sheriffbot@chromium.org, Nov 30 2016

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 15 by l...@chromium.org, Dec 1 2016

Labels: -M-56 -Hotlist-Merge-Approved -Merge-Approved-56 M-57
Following comment #7, this would be risky to land now.  Canceling merge request and allowing to move to M57.

Sign in to add a comment