Regression: [DevTools] Unable to close the shadow window using 'Esc' key.
Reported by
dmascare...@etouch.net,
Nov 16 2016
|
||||||||
Issue descriptionChrome 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.
,
Nov 16 2016
,
Nov 16 2016
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.
,
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
,
Nov 21 2016
,
Nov 21 2016
Your change meets the bar and is auto-approved for M56 (branch: 2924)
,
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.
,
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
,
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
,
Nov 30 2016
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.
,
Nov 30 2016
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.
,
Nov 30 2016
Forgot to add the screen cast. Thank You.
,
Nov 30 2016
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.
,
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
,
Dec 1 2016
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 |
||||||||
Comment 1 by msrchandra@chromium.org
, Nov 16 2016Owner: dgozman@chromium.org
Status: Assigned (was: Unconfirmed)