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

Issue 821131 link

Starred by 2 users
Status: Fixed
Owner:
mlamouri@chromium.org
Closed: Apr 2018
Cc:
█ hbengali@chromium.org
beccahughes@chromium.org
mlamouri@chromium.org
steimel@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 1
Type: Bug


Participants' hotlists:
Modern-Media-Controls


Sign in to add a comment

Media Controls: keyboard interaction for overflow menu

Project Member Reported by steimel@chromium.org, Mar 12 2018 
When a user focuses on the overflow menu button and presses spacebar, the overflow menu. If they press space again, it should close. However, the MediaControlsWindowEventListener sees the click event first and calls MediaControlsImpl::HideAllMenus, hiding the overflow menu button. So when the overflow menu button gets the click, it ends up re-showing the overflow menu

Comment 1 by steimel@chromium.org, Mar 12 2018 

Also of note:

1) This happens for both legacy and modern controls

2) This also happens if you manage to actually click the overflow menu button (not just by hitting space again)

3) This fires a DCHECK in MediaControlOverflowMenuListElement.cpp because we hide and re-show the overflow menu before MaybeRecordTimeTaken is run

Comment 2 by mlamouri@chromium.org, Mar 31 2018

Cc: steimel@chromium.org hbengali@chromium.org
 Issue 827563  has been merged into this issue.

Comment 3 by mlamouri@chromium.org, Apr 3 2018

Owner: ----
Status: Available (was: Assigned)
(auto-clear ownership of P3 media controls bugs)

Comment 4 by mlamouri@chromium.org, Apr 5 2018

Labels: -Pri-3 Pri-1
Owner: mlamouri@chromium.org
Status: Started (was: Available)
Summary: Media Controls: keyboard interaction for overflow menu (was: Media Controls: Cannot close overflow menu with keyboard)
Bumping to P1 because of accessibility.
Project Member

Comment 5 by bugdroid1@chromium.org, Apr 11 2018 

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

commit 9a14e94e6f176a61e67a662a75cbf83196f32b49
Author: Mounir Lamouri <mlamouri@chromium.org>
Date: Wed Apr 11 01:19:44 2018

Media Controls: create popup menu element, base for text track/overflow lists.

This is introducing MediaControlPopupMenuElement as a base class. At
the moment, the class handles positioning of the popup and some basic
visibility. It's also cleaning up a bit of code around popup menus.

This new class will allow the introduction of an event handler to deal
with keyboard events on both popup menus.

Bug:  821131 
Change-Id: I19ad4193b5ccbd6acd3c59c35701e32c91e634c6
Reviewed-on: https://chromium-review.googlesource.com/1005334
Reviewed-by: Becca Hughes <beccahughes@chromium.org>
Commit-Queue: Mounir Lamouri <mlamouri@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549712}
[modify] https://crrev.com/9a14e94e6f176a61e67a662a75cbf83196f32b49/third_party/blink/renderer/modules/media_controls/BUILD.gn
[modify] https://crrev.com/9a14e94e6f176a61e67a662a75cbf83196f32b49/third_party/blink/renderer/modules/media_controls/elements/media_control_overflow_menu_list_element.cc
[modify] https://crrev.com/9a14e94e6f176a61e67a662a75cbf83196f32b49/third_party/blink/renderer/modules/media_controls/elements/media_control_overflow_menu_list_element.h
[add] https://crrev.com/9a14e94e6f176a61e67a662a75cbf83196f32b49/third_party/blink/renderer/modules/media_controls/elements/media_control_popup_menu_element.cc
[add] https://crrev.com/9a14e94e6f176a61e67a662a75cbf83196f32b49/third_party/blink/renderer/modules/media_controls/elements/media_control_popup_menu_element.h
[modify] https://crrev.com/9a14e94e6f176a61e67a662a75cbf83196f32b49/third_party/blink/renderer/modules/media_controls/elements/media_control_text_track_list_element.cc
[modify] https://crrev.com/9a14e94e6f176a61e67a662a75cbf83196f32b49/third_party/blink/renderer/modules/media_controls/elements/media_control_text_track_list_element.h
[modify] https://crrev.com/9a14e94e6f176a61e67a662a75cbf83196f32b49/third_party/blink/renderer/modules/media_controls/media_controls_impl.cc
[modify] https://crrev.com/9a14e94e6f176a61e67a662a75cbf83196f32b49/third_party/blink/renderer/modules/media_controls/media_controls_impl.h
Project Member

Comment 6 by bugdroid1@chromium.org, Apr 17 2018 

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

commit 523243f1f0734973717130cbe48fcd86cbf26f44
Author: Mounir Lamouri <mlamouri@chromium.org>
Date: Tue Apr 17 16:18:53 2018

Media Controls: keyboard interaction for the overflow menu.

This is allowing the user to use Tab and up/down keys to navigation
throughout the menu. It can also be closed using the Escape key.

This change also trap the focus inside the menu while it's visible and
make it so that hovenig an item will move the focus to it, like most
menus will do.

This is mostly for accessibility so users can interact with the
controls using only a keyboard. The change applies to both the overflow
menu and the text track list.

Bug:  821131 
Change-Id: I22db341d0d55a8e3297b3c7a18c92331e296cd9e
Reviewed-on: https://chromium-review.googlesource.com/1013478
Reviewed-by: Becca Hughes <beccahughes@chromium.org>
Commit-Queue: Mounir Lamouri <mlamouri@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551353}
[add] https://crrev.com/523243f1f0734973717130cbe48fcd86cbf26f44/third_party/WebKit/LayoutTests/media/controls/overflow-menu-keyboard-navigation.html
[add] https://crrev.com/523243f1f0734973717130cbe48fcd86cbf26f44/third_party/WebKit/LayoutTests/media/controls/overflow-menu-pointer-selection.html
[add] https://crrev.com/523243f1f0734973717130cbe48fcd86cbf26f44/third_party/WebKit/LayoutTests/media/controls/text-track-menu-keyboard-navigation.html
[add] https://crrev.com/523243f1f0734973717130cbe48fcd86cbf26f44/third_party/WebKit/LayoutTests/media/controls/text-track-menu-pointer-selection.html
[modify] https://crrev.com/523243f1f0734973717130cbe48fcd86cbf26f44/third_party/WebKit/LayoutTests/media/media-controls.js
[modify] https://crrev.com/523243f1f0734973717130cbe48fcd86cbf26f44/third_party/blink/renderer/modules/media_controls/elements/media_control_input_element.cc
[modify] https://crrev.com/523243f1f0734973717130cbe48fcd86cbf26f44/third_party/blink/renderer/modules/media_controls/elements/media_control_overflow_menu_list_element.cc
[modify] https://crrev.com/523243f1f0734973717130cbe48fcd86cbf26f44/third_party/blink/renderer/modules/media_controls/elements/media_control_popup_menu_element.cc
[modify] https://crrev.com/523243f1f0734973717130cbe48fcd86cbf26f44/third_party/blink/renderer/modules/media_controls/elements/media_control_popup_menu_element.h
[modify] https://crrev.com/523243f1f0734973717130cbe48fcd86cbf26f44/third_party/blink/renderer/modules/media_controls/elements/media_control_text_track_list_element.cc
[modify] https://crrev.com/523243f1f0734973717130cbe48fcd86cbf26f44/third_party/blink/renderer/modules/media_controls/resources/modernMediaControls.css

Comment 7 by mlamouri@chromium.org, Apr 17 2018

Labels: Merge-Request-67 M-67 OS-Android
Project Member

Comment 8 by sheriffbot@chromium.org, Apr 18 2018

Labels: -Merge-Request-67 Merge-Approved-67 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M67. Please go ahead and merge the CL to branch 3396 manually. Please contact milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

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

Comment 9 by bugdroid1@chromium.org, Apr 18 2018

Labels: -merge-approved-67 merge-merged-3396
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8255fea6db9c5ca1256ee40fd9f570e7e26c7f8a

commit 8255fea6db9c5ca1256ee40fd9f570e7e26c7f8a
Author: Mounir Lamouri <mlamouri@chromium.org>
Date: Wed Apr 18 17:10:54 2018

Media Controls: keyboard interaction for the overflow menu.

This is allowing the user to use Tab and up/down keys to navigation
throughout the menu. It can also be closed using the Escape key.

This change also trap the focus inside the menu while it's visible and
make it so that hovenig an item will move the focus to it, like most
menus will do.

This is mostly for accessibility so users can interact with the
controls using only a keyboard. The change applies to both the overflow
menu and the text track list.

Bug:  821131 
Change-Id: I22db341d0d55a8e3297b3c7a18c92331e296cd9e
Reviewed-on: https://chromium-review.googlesource.com/1013478
Reviewed-by: Becca Hughes <beccahughes@chromium.org>
Commit-Queue: Mounir Lamouri <mlamouri@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#551353}(cherry picked from commit 523243f1f0734973717130cbe48fcd86cbf26f44)
Reviewed-on: https://chromium-review.googlesource.com/1017260
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Cr-Commit-Position: refs/branch-heads/3396@{#82}
Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428}
[add] https://crrev.com/8255fea6db9c5ca1256ee40fd9f570e7e26c7f8a/third_party/WebKit/LayoutTests/media/controls/overflow-menu-keyboard-navigation.html
[add] https://crrev.com/8255fea6db9c5ca1256ee40fd9f570e7e26c7f8a/third_party/WebKit/LayoutTests/media/controls/overflow-menu-pointer-selection.html
[add] https://crrev.com/8255fea6db9c5ca1256ee40fd9f570e7e26c7f8a/third_party/WebKit/LayoutTests/media/controls/text-track-menu-keyboard-navigation.html
[add] https://crrev.com/8255fea6db9c5ca1256ee40fd9f570e7e26c7f8a/third_party/WebKit/LayoutTests/media/controls/text-track-menu-pointer-selection.html
[modify] https://crrev.com/8255fea6db9c5ca1256ee40fd9f570e7e26c7f8a/third_party/WebKit/LayoutTests/media/media-controls.js
[modify] https://crrev.com/8255fea6db9c5ca1256ee40fd9f570e7e26c7f8a/third_party/blink/renderer/modules/media_controls/elements/media_control_input_element.cc
[modify] https://crrev.com/8255fea6db9c5ca1256ee40fd9f570e7e26c7f8a/third_party/blink/renderer/modules/media_controls/elements/media_control_overflow_menu_list_element.cc
[modify] https://crrev.com/8255fea6db9c5ca1256ee40fd9f570e7e26c7f8a/third_party/blink/renderer/modules/media_controls/elements/media_control_popup_menu_element.cc
[modify] https://crrev.com/8255fea6db9c5ca1256ee40fd9f570e7e26c7f8a/third_party/blink/renderer/modules/media_controls/elements/media_control_popup_menu_element.h
[modify] https://crrev.com/8255fea6db9c5ca1256ee40fd9f570e7e26c7f8a/third_party/blink/renderer/modules/media_controls/elements/media_control_text_track_list_element.cc
[modify] https://crrev.com/8255fea6db9c5ca1256ee40fd9f570e7e26c7f8a/third_party/blink/renderer/modules/media_controls/resources/modernMediaControls.css

Comment 10 by mlamouri@chromium.org, Apr 18 2018

Status: Fixed (was: Started)
►

Sign in to add a comment