Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2982533002:
Remove the BubbleFrameView close button from tab traversal on all platforms. (Closed)
|
Created:
3 years, 5 months ago by tapted Modified:
3 years, 5 months ago Reviewers:
msw CC:
chromium-reviews, tfarina, groby+bubble_chromium.org, rouslan+bubble_chromium.org, hcarmona+bubble_chromium.org, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove the BubbleFrameView close button from tab traversal on all platforms.
The button defaults to FocusBehavior::ACCESSIBLE_ONLY which (apart from
on Mac) only affects buttons in AccessiblePaneViews (which the close
button is not). Make it FocusBehavior::NEVER so the behaviour is
consistent across platforms.
Note this does not affect screen readers' ability to focus the element.
Keyboard access to this element when not using a screen reader is done
via the ESC key handler in DialogClientView.
BUG=741251
Review-Url: https://codereview.chromium.org/2982533002
Cr-Commit-Position: refs/heads/master@{#486228}
Committed: https://chromium.googlesource.com/chromium/src/+/5602cce8dce523ac65415a36db784361d80a6074
Patch Set 1 #
Total comments: 6
Patch Set 2 : update comment #Messages
Total messages: 18 (13 generated)
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== FocusBehavior::NEVER BUG= ========== to ========== Remove the BubbleFrameView close button from tab traversal on all platforms. The button defaults to FocusBehavior::ACCESSIBLE_ONLY which (apart from on Mac) only affects buttons in AccessiblePaneViews (which the close button is not). Make it FocusBehavior::NEVER so the behaviour is consistent across platforms. Note this does not affect screen readers' ability to focus the element. Keyboard access to this element when not using a screen reader is done via the ESC key handler in DialogClientView. BUG=741251 ==========
tapted@chromium.org changed reviewers: + msw@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi mike, wdyt? (note i could equally go for FocusBehavior::ALWAYS, I just want it to be consistent between platforms).
lgtm with comment nits, Esc should suffice. https://codereview.chromium.org/2982533002/diff/1/ui/views/bubble/bubble_fram... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/2982533002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view.cc:137: // Remove the close button from tab traversal on all platforms. It defaults to nit: remove the "It defaults to" sentence https://codereview.chromium.org/2982533002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view.cc:140: // does not affect screen readers' ability to focus the element. Keyboard nit: s/element/close button/ https://codereview.chromium.org/2982533002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view.cc:141: // access to this element when not using a screen reader is done via the ESC nit: s/this element/the close button/
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks Mike! https://codereview.chromium.org/2982533002/diff/1/ui/views/bubble/bubble_fram... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/2982533002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view.cc:137: // Remove the close button from tab traversal on all platforms. It defaults to On 2017/07/12 17:20:25, msw wrote: > nit: remove the "It defaults to" sentence Done. https://codereview.chromium.org/2982533002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view.cc:140: // does not affect screen readers' ability to focus the element. Keyboard On 2017/07/12 17:20:25, msw wrote: > nit: s/element/close button/ Done. https://codereview.chromium.org/2982533002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view.cc:141: // access to this element when not using a screen reader is done via the ESC On 2017/07/12 17:20:25, msw wrote: > nit: s/this element/the close button/ Done.
The CQ bit was unchecked by tapted@chromium.org
The CQ bit was checked by tapted@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org Link to the patchset: https://codereview.chromium.org/2982533002/#ps20001 (title: "update comment")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1499907322103890,
"parent_rev": "910f24f730ba20e343a870f4415790fce012ee16", "commit_rev":
"5602cce8dce523ac65415a36db784361d80a6074"}
Message was sent while issue was closed.
Description was changed from ========== Remove the BubbleFrameView close button from tab traversal on all platforms. The button defaults to FocusBehavior::ACCESSIBLE_ONLY which (apart from on Mac) only affects buttons in AccessiblePaneViews (which the close button is not). Make it FocusBehavior::NEVER so the behaviour is consistent across platforms. Note this does not affect screen readers' ability to focus the element. Keyboard access to this element when not using a screen reader is done via the ESC key handler in DialogClientView. BUG=741251 ========== to ========== Remove the BubbleFrameView close button from tab traversal on all platforms. The button defaults to FocusBehavior::ACCESSIBLE_ONLY which (apart from on Mac) only affects buttons in AccessiblePaneViews (which the close button is not). Make it FocusBehavior::NEVER so the behaviour is consistent across platforms. Note this does not affect screen readers' ability to focus the element. Keyboard access to this element when not using a screen reader is done via the ESC key handler in DialogClientView. BUG=741251 Review-Url: https://codereview.chromium.org/2982533002 Cr-Commit-Position: refs/heads/master@{#486228} Committed: https://chromium.googlesource.com/chromium/src/+/5602cce8dce523ac65415a36db78... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/5602cce8dce523ac65415a36db78... |
