[MacViewsBrowser] Pressing Space on the Remove Bookmark Button Relaunches the Bookmark Flow |
|||||||||||||||||||||||||
Issue descriptionChrome Version: 60.0.3072.0 (MacViews) OS: macOS 10.12 What steps will reproduce the problem? (1) Use the Tab key to focus on the bookmarks star - press Space to click it (2) In the Bookmarks dialog that appears, tab until the Remove button is selected (3) Press Space What is the expected result? The current URL is removed from bookmarks and the Bookmarks dialog orders out. What happens instead? The Bookmarks dialog orders out but then orders back in, and the bookmark star is blue, indicating that the current URL is still bookmarked.
,
Apr 18 2017
I suspect the `Remove` button in secondary UI correctly hooks keydown, but we haven't gotten around to updating the location decoration to _not_ hook keyup on Mac. (this suggests the browser view bookmarks star location bar decoration is doing its own keystroke handling rather than just relying on the framework, which is probably what needs to be fixed).
,
Feb 8 2018
[Bulk Edit] Applying M-68 milestone per email discussion with ellyjones@. Pls change it if milestone is incorrectly applied.
,
Mar 23 2018
MacViews triage: let's target a fix for this in M-68. Over to you, robliao@ :)
,
Mar 27 2018
,
Mar 29 2018
** Bulk Edit ** FYI: Starting 04/13 M68 will be in canary, M68 Dev promotion will be on 04/26.
,
Apr 25 2018
Pls mark the bug as fixed if CL is landed in trunk and nothing else is pending. Thank you.
,
Jun 1 2018
Investigating...
,
Jun 20 2018
,
Jun 22 2018
Issue 850458 has been merged into this issue.
,
Jun 22 2018
,
Jun 22 2018
,
Jun 28 2018
This requires enabling MacOS full keyboard access.
,
Jul 2
,
Jul 12
,
Jul 12
,
Jul 26
,
Aug 13
,
Sep 13
,
Sep 13
Yoink!
,
Sep 18
The root cause is this:
bool PageActionIconView::OnKeyPressed(const ui::KeyEvent& event) {
if (event.key_code() != ui::VKEY_RETURN && event.key_code() != ui::VKEY_SPACE)
return false;
AnimateInkDrop(views::InkDropState::ACTIVATED, nullptr /* &event */);
// As with Button, return activates on key down and space activates on
// key up.
if (event.key_code() == ui::VKEY_RETURN)
ExecuteCommand(EXECUTE_SOURCE_KEYBOARD);
return true;
}
bool PageActionIconView::OnKeyReleased(const ui::KeyEvent& event) {
if (event.key_code() != ui::VKEY_SPACE)
return false;
ExecuteCommand(EXECUTE_SOURCE_KEYBOARD);
return true;
}
I don't know why this behavior exists but it means that the key down event is handled by the bubble (dismissing it) and then the key up is handled by the star, activating it.
,
Sep 18
,
Sep 19
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5540a9cb7ec95a28dcfb97f9ce92520799a71647 commit 5540a9cb7ec95a28dcfb97f9ce92520799a71647 Author: Elly Fong-Jones <ellyjones@chromium.org> Date: Wed Sep 19 13:22:41 2018 views: use platform spacebar behavior in PageActionIconView On Mac, the spacebar activates buttons on keydown; on other platforms it activates them on keyup. Not implementing this behavior here caused the following bug: 1) User focuses the bookmark bar star 2) User hits space to activate it, which opens the bubble 3) User tabs to the "Remove" button 4) User hits space to activate that button 5) The "Remove" button activates on keydown and the bubble dismisses 6) The star gets keyboard focus again 7) The star immediately handles the keyup and reopens the bubble! The fix is for PageActionIconView to honor the platform spacebar affinity. There is an old TODO in this code to merge it with views::Button but I would not want to merge such a change back to beta so this small fix seems more expedient. Bug: 712341 Change-Id: If932928bb069493e61f78f2d34e95a0aa45fb504 Reviewed-on: https://chromium-review.googlesource.com/1230437 Reviewed-by: Leonard Grey <lgrey@chromium.org> Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org> Cr-Commit-Position: refs/heads/master@{#592379} [modify] https://crrev.com/5540a9cb7ec95a28dcfb97f9ce92520799a71647/chrome/browser/ui/views/page_action/page_action_icon_view.cc [modify] https://crrev.com/5540a9cb7ec95a28dcfb97f9ce92520799a71647/chrome/browser/ui/views/page_action/page_action_icon_view.h
,
Sep 19
Requesting merge to M70, although I don't intend to actually merge until this has had some time on canary.
,
Sep 20
Able to reproduce the issue on chrome version# 68.0.3440.17(Build without fix) using comment# 11 Verified the fix on Mac 10.12.6 on Chrome version# 71.0.3557.0 Attaching screen cast for reference. Observed "Current URL is removed from bookmarks and the Bookmarks dialog orders out" Hence, the fix is working as expected. Adding the verified labels. Thanks!
,
Sep 20
This bug requires manual review: M70 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: benmason@(Android), kariahda@(iOS), geohsu@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Sep 20
,
Sep 21
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/185042e178f10dee0659eb98ea9f1e497325949a commit 185042e178f10dee0659eb98ea9f1e497325949a Author: Elly Fong-Jones <ellyjones@chromium.org> Date: Fri Sep 21 15:03:35 2018 views: use platform spacebar behavior in PageActionIconView On Mac, the spacebar activates buttons on keydown; on other platforms it activates them on keyup. Not implementing this behavior here caused the following bug: 1) User focuses the bookmark bar star 2) User hits space to activate it, which opens the bubble 3) User tabs to the "Remove" button 4) User hits space to activate that button 5) The "Remove" button activates on keydown and the bubble dismisses 6) The star gets keyboard focus again 7) The star immediately handles the keyup and reopens the bubble! The fix is for PageActionIconView to honor the platform spacebar affinity. There is an old TODO in this code to merge it with views::Button but I would not want to merge such a change back to beta so this small fix seems more expedient. Bug: 712341 Change-Id: If932928bb069493e61f78f2d34e95a0aa45fb504 Reviewed-on: https://chromium-review.googlesource.com/1230437 Reviewed-by: Leonard Grey <lgrey@chromium.org> Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#592379}(cherry picked from commit 5540a9cb7ec95a28dcfb97f9ce92520799a71647) Reviewed-on: https://chromium-review.googlesource.com/1238773 Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org> Cr-Commit-Position: refs/branch-heads/3538@{#558} Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811} [modify] https://crrev.com/185042e178f10dee0659eb98ea9f1e497325949a/chrome/browser/ui/views/page_action/page_action_icon_view.cc [modify] https://crrev.com/185042e178f10dee0659eb98ea9f1e497325949a/chrome/browser/ui/views/page_action/page_action_icon_view.h
,
Sep 21
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/185042e178f10dee0659eb98ea9f1e497325949a Commit: 185042e178f10dee0659eb98ea9f1e497325949a Author: ellyjones@chromium.org Commiter: ellyjones@chromium.org Date: 2018-09-21 15:03:35 +0000 UTC views: use platform spacebar behavior in PageActionIconView On Mac, the spacebar activates buttons on keydown; on other platforms it activates them on keyup. Not implementing this behavior here caused the following bug: 1) User focuses the bookmark bar star 2) User hits space to activate it, which opens the bubble 3) User tabs to the "Remove" button 4) User hits space to activate that button 5) The "Remove" button activates on keydown and the bubble dismisses 6) The star gets keyboard focus again 7) The star immediately handles the keyup and reopens the bubble! The fix is for PageActionIconView to honor the platform spacebar affinity. There is an old TODO in this code to merge it with views::Button but I would not want to merge such a change back to beta so this small fix seems more expedient. Bug: 712341 Change-Id: If932928bb069493e61f78f2d34e95a0aa45fb504 Reviewed-on: https://chromium-review.googlesource.com/1230437 Reviewed-by: Leonard Grey <lgrey@chromium.org> Commit-Queue: Elly Fong-Jones <ellyjones@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#592379}(cherry picked from commit 5540a9cb7ec95a28dcfb97f9ce92520799a71647) Reviewed-on: https://chromium-review.googlesource.com/1238773 Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org> Cr-Commit-Position: refs/branch-heads/3538@{#558} Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
,
Sep 21
Merge complete to M70.
,
Sep 26
Able to reproduce the issue on chrome version# 68.0.3440.17(Build without fix) using comment# 11 Verified the fix on Mac 10.12.6 on Chrome version# 70.0.3538.35 Attaching screen cast for reference. Observed "Current URL is removed from bookmarks and the Bookmarks dialog orders out" Hence, the fix is working as expected. Adding the verified labels. Thanks! |
|||||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||||
Comment 1 by shrike@chromium.org
, Apr 17 2017