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

Issue 840516 link

Starred by 3 users

Issue metadata

Status: Verified
Owner:
Closed: Jun 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

PiP button should toggle PiP state

Project Member Reported by fbeaufort@chromium.org, May 7 2018

Issue description

Clicking the native PiP button (after entering PiP) should exit PiP.
It may require a different icon or text but all similar states have toggles (mute, cast, captions, fullscreen).
 
Cc: apaci...@chromium.org mlamouri@chromium.org
Owner: amyroberts@chromium.org
Amy, is this part of the mocks we currently have? It seems fair to have PIP behave as a toggle like mute, fullscreen, cast or captions but I'm not sure how we should represent this.
Labels: -Pri-3 M-69 Pri-1
Owner: fbeaufort@chromium.org
Status: Assigned (was: Untriaged)
Can you do this for the context menu too as the implementation that's landing will only allow to enter Picture-in-Picture.

Also, mocks are available here: https://docs.google.com/presentation/d/1crVckjRA7LZ2S-SPuO8zXMQPw_uZwC0-eBhZ5EEumCw/edit#slide=id.g390a4866e3_0_7 with SVG icon on slide 43. (google.com restricted)
Status: Started (was: Assigned)
WIP at https://chromium-review.googlesource.com/c/chromium/src/+/1084833
Project Member

Comment 7 by bugdroid1@chromium.org, Jun 6 2018

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

commit f832578e38ffcb735cb1a414ac580ea781b809b9
Author: François Beaufort <beaufort.francois@gmail.com>
Date: Wed Jun 06 06:49:43 2018

[Media Controls] Add Exit Picture-in-Picture button.

This makes sure clicking the native Picture-in-Picture button (after
entering Picture-in-Picture) exits Picture-in-Picture and that video
controls are reflected when entering and exiting Picture-in-Picture.

Screenshot: https://i.imgur.com/081Xbmb.png

Bug:  840516 ,  806249 
Change-Id: I410a4a06cc4eea62dae8e1d368e4d05394f13c86
Reviewed-on: https://chromium-review.googlesource.com/1084833
Commit-Queue: François Beaufort <beaufort.francois@gmail.com>
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: apacible <apacible@chromium.org>
Cr-Commit-Position: refs/heads/master@{#564807}
[modify] https://crrev.com/f832578e38ffcb735cb1a414ac580ea781b809b9/content/app/strings/content_strings.grd
[modify] https://crrev.com/f832578e38ffcb735cb1a414ac580ea781b809b9/content/child/blink_platform_impl.cc
[modify] https://crrev.com/f832578e38ffcb735cb1a414ac580ea781b809b9/third_party/WebKit/LayoutTests/media/picture-in-picture/controls/picture-in-picture-button.html
[modify] https://crrev.com/f832578e38ffcb735cb1a414ac580ea781b809b9/third_party/WebKit/LayoutTests/media/picture-in-picture/picture-in-picture-interstitial.html
[modify] https://crrev.com/f832578e38ffcb735cb1a414ac580ea781b809b9/third_party/blink/public/platform/web_localized_string.h
[modify] https://crrev.com/f832578e38ffcb735cb1a414ac580ea781b809b9/third_party/blink/renderer/core/frame/picture_in_picture_controller.h
[modify] https://crrev.com/f832578e38ffcb735cb1a414ac580ea781b809b9/third_party/blink/renderer/modules/accessibility/ax_media_controls.cc
[modify] https://crrev.com/f832578e38ffcb735cb1a414ac580ea781b809b9/third_party/blink/renderer/modules/media_controls/elements/media_control_element_type.h
[modify] https://crrev.com/f832578e38ffcb735cb1a414ac580ea781b809b9/third_party/blink/renderer/modules/media_controls/elements/media_control_picture_in_picture_button_element.cc
[modify] https://crrev.com/f832578e38ffcb735cb1a414ac580ea781b809b9/third_party/blink/renderer/modules/media_controls/elements/media_control_picture_in_picture_button_element.h
[modify] https://crrev.com/f832578e38ffcb735cb1a414ac580ea781b809b9/third_party/blink/renderer/modules/media_controls/media_controls_impl.cc
[modify] https://crrev.com/f832578e38ffcb735cb1a414ac580ea781b809b9/third_party/blink/renderer/modules/media_controls/media_controls_impl.h
[modify] https://crrev.com/f832578e38ffcb735cb1a414ac580ea781b809b9/third_party/blink/renderer/modules/media_controls/media_controls_media_event_listener.cc
[add] https://crrev.com/f832578e38ffcb735cb1a414ac580ea781b809b9/third_party/blink/renderer/modules/media_controls/resources/ic_picture_in_picture_exit.svg
[modify] https://crrev.com/f832578e38ffcb735cb1a414ac580ea781b809b9/third_party/blink/renderer/modules/media_controls/resources/modernMediaControls.css
[modify] https://crrev.com/f832578e38ffcb735cb1a414ac580ea781b809b9/third_party/blink/renderer/modules/picture_in_picture/picture_in_picture_controller_impl.h

Project Member

Comment 8 by bugdroid1@chromium.org, Jun 6 2018

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

commit 4596eea736d27ba33efa2d8d81f806807c4d8228
Author: Tommy Steimel <steimel@chromium.org>
Date: Wed Jun 06 18:20:12 2018

Revert "[Media Controls] Add Exit Picture-in-Picture button."

This reverts commit f832578e38ffcb735cb1a414ac580ea781b809b9.

Reason for revert: GetOverflowStringName is firing a DCHECK on audio elements. Note that audio elements have the picture-in-picture button (it's just hidden), so the code will need to handle that case

Original change's description:
> [Media Controls] Add Exit Picture-in-Picture button.
> 
> This makes sure clicking the native Picture-in-Picture button (after
> entering Picture-in-Picture) exits Picture-in-Picture and that video
> controls are reflected when entering and exiting Picture-in-Picture.
> 
> Screenshot: https://i.imgur.com/081Xbmb.png
> 
> Bug:  840516 ,  806249 
> Change-Id: I410a4a06cc4eea62dae8e1d368e4d05394f13c86
> Reviewed-on: https://chromium-review.googlesource.com/1084833
> Commit-Queue: François Beaufort <beaufort.francois@gmail.com>
> Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
> Reviewed-by: Jochen Eisinger <jochen@chromium.org>
> Reviewed-by: apacible <apacible@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#564807}

TBR=beaufort.francois@gmail.com,mlamouri@chromium.org,apacible@chromium.org,jochen@chromium.org

Change-Id: Ie6373e9c2e77bb41ac6a5e9a32f59a2b6ce3bf03
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  840516 ,  806249 
Reviewed-on: https://chromium-review.googlesource.com/1089209
Reviewed-by: Tommy Steimel <steimel@chromium.org>
Commit-Queue: Tommy Steimel <steimel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#564971}
[modify] https://crrev.com/4596eea736d27ba33efa2d8d81f806807c4d8228/content/app/strings/content_strings.grd
[modify] https://crrev.com/4596eea736d27ba33efa2d8d81f806807c4d8228/content/child/blink_platform_impl.cc
[modify] https://crrev.com/4596eea736d27ba33efa2d8d81f806807c4d8228/third_party/WebKit/LayoutTests/media/picture-in-picture/controls/picture-in-picture-button.html
[modify] https://crrev.com/4596eea736d27ba33efa2d8d81f806807c4d8228/third_party/WebKit/LayoutTests/media/picture-in-picture/picture-in-picture-interstitial.html
[modify] https://crrev.com/4596eea736d27ba33efa2d8d81f806807c4d8228/third_party/blink/public/platform/web_localized_string.h
[modify] https://crrev.com/4596eea736d27ba33efa2d8d81f806807c4d8228/third_party/blink/renderer/core/frame/picture_in_picture_controller.h
[modify] https://crrev.com/4596eea736d27ba33efa2d8d81f806807c4d8228/third_party/blink/renderer/modules/accessibility/ax_media_controls.cc
[modify] https://crrev.com/4596eea736d27ba33efa2d8d81f806807c4d8228/third_party/blink/renderer/modules/media_controls/elements/media_control_element_type.h
[modify] https://crrev.com/4596eea736d27ba33efa2d8d81f806807c4d8228/third_party/blink/renderer/modules/media_controls/elements/media_control_picture_in_picture_button_element.cc
[modify] https://crrev.com/4596eea736d27ba33efa2d8d81f806807c4d8228/third_party/blink/renderer/modules/media_controls/elements/media_control_picture_in_picture_button_element.h
[modify] https://crrev.com/4596eea736d27ba33efa2d8d81f806807c4d8228/third_party/blink/renderer/modules/media_controls/media_controls_impl.cc
[modify] https://crrev.com/4596eea736d27ba33efa2d8d81f806807c4d8228/third_party/blink/renderer/modules/media_controls/media_controls_impl.h
[modify] https://crrev.com/4596eea736d27ba33efa2d8d81f806807c4d8228/third_party/blink/renderer/modules/media_controls/media_controls_media_event_listener.cc
[delete] https://crrev.com/558635267b37db79b830022c78e209ab31a68c6c/third_party/blink/renderer/modules/media_controls/resources/ic_picture_in_picture_exit.svg
[modify] https://crrev.com/4596eea736d27ba33efa2d8d81f806807c4d8228/third_party/blink/renderer/modules/media_controls/resources/modernMediaControls.css
[modify] https://crrev.com/4596eea736d27ba33efa2d8d81f806807c4d8228/third_party/blink/renderer/modules/picture_in_picture/picture_in_picture_controller_impl.h

Project Member

Comment 10 by bugdroid1@chromium.org, Jun 12 2018

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

commit 9cf2378b995e368366a1fc64df25e95f82cd54c9
Author: François Beaufort <beaufort.francois@gmail.com>
Date: Tue Jun 12 01:43:25 2018

[Media Controls] Add Exit Picture-in-Picture button.

This makes sure clicking the native Picture-in-Picture button (after
entering Picture-in-Picture) exits Picture-in-Picture and that video
controls are reflected when entering and exiting Picture-in-Picture.

Screenshot: https://i.imgur.com/081Xbmb.png

Bug:  840516 ,  806249 
Change-Id: I9b07630c90323454a774ee147b67b5bb98ed7094
Reviewed-on: https://chromium-review.googlesource.com/1089332
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Commit-Queue: François Beaufort <beaufort.francois@gmail.com>
Cr-Commit-Position: refs/heads/master@{#566238}
[modify] https://crrev.com/9cf2378b995e368366a1fc64df25e95f82cd54c9/content/app/strings/content_strings.grd
[modify] https://crrev.com/9cf2378b995e368366a1fc64df25e95f82cd54c9/content/child/blink_platform_impl.cc
[modify] https://crrev.com/9cf2378b995e368366a1fc64df25e95f82cd54c9/third_party/WebKit/LayoutTests/media/picture-in-picture/controls/picture-in-picture-button.html
[modify] https://crrev.com/9cf2378b995e368366a1fc64df25e95f82cd54c9/third_party/WebKit/LayoutTests/media/picture-in-picture/picture-in-picture-interstitial.html
[modify] https://crrev.com/9cf2378b995e368366a1fc64df25e95f82cd54c9/third_party/blink/public/platform/web_localized_string.h
[modify] https://crrev.com/9cf2378b995e368366a1fc64df25e95f82cd54c9/third_party/blink/renderer/core/frame/picture_in_picture_controller.h
[modify] https://crrev.com/9cf2378b995e368366a1fc64df25e95f82cd54c9/third_party/blink/renderer/modules/accessibility/ax_media_controls.cc
[modify] https://crrev.com/9cf2378b995e368366a1fc64df25e95f82cd54c9/third_party/blink/renderer/modules/media_controls/elements/media_control_element_type.h
[modify] https://crrev.com/9cf2378b995e368366a1fc64df25e95f82cd54c9/third_party/blink/renderer/modules/media_controls/elements/media_control_picture_in_picture_button_element.cc
[modify] https://crrev.com/9cf2378b995e368366a1fc64df25e95f82cd54c9/third_party/blink/renderer/modules/media_controls/elements/media_control_picture_in_picture_button_element.h
[modify] https://crrev.com/9cf2378b995e368366a1fc64df25e95f82cd54c9/third_party/blink/renderer/modules/media_controls/media_controls_impl.cc
[modify] https://crrev.com/9cf2378b995e368366a1fc64df25e95f82cd54c9/third_party/blink/renderer/modules/media_controls/media_controls_impl.h
[modify] https://crrev.com/9cf2378b995e368366a1fc64df25e95f82cd54c9/third_party/blink/renderer/modules/media_controls/media_controls_media_event_listener.cc
[add] https://crrev.com/9cf2378b995e368366a1fc64df25e95f82cd54c9/third_party/blink/renderer/modules/media_controls/resources/ic_picture_in_picture_exit.svg
[modify] https://crrev.com/9cf2378b995e368366a1fc64df25e95f82cd54c9/third_party/blink/renderer/modules/media_controls/resources/modernMediaControls.css
[modify] https://crrev.com/9cf2378b995e368366a1fc64df25e95f82cd54c9/third_party/blink/renderer/modules/picture_in_picture/picture_in_picture_controller_impl.h

Status: Verified (was: Started)
Verified in Chromium 69.0.3456.0.
Status: Started (was: Verified)
https://chromium-review.googlesource.com/c/chromium/src/+/1086796 need to land as well so I'll remove "Verified" status.
Project Member

Comment 13 by bugdroid1@chromium.org, Jun 13 2018

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

commit 26a709f833ae93273514a2057cb22423480bf25a
Author: François Beaufort <beaufort.francois@gmail.com>
Date: Wed Jun 13 18:52:17 2018

Add exit Picture-in-Picture browser context menu.

This makes sure clicking the checked Picture-in-Picture browser context
menu (after entering Picture-in-Picture) exits Picture-in-Picture.

Screenshot: https://i.imgur.com/asJ1qgT.png

Bug:  840516 ,  806249 
Change-Id: Ic283ca8cb431bfcb56613067e0fec43db2bbf53b
Reviewed-on: https://chromium-review.googlesource.com/1086796
Commit-Queue: François Beaufort <beaufort.francois@gmail.com>
Reviewed-by: apacible <apacible@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Brian White <bcwhite@chromium.org>
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566935}
[modify] https://crrev.com/26a709f833ae93273514a2057cb22423480bf25a/chrome/app/chrome_command_ids.h
[modify] https://crrev.com/26a709f833ae93273514a2057cb22423480bf25a/chrome/app/generated_resources.grd
[modify] https://crrev.com/26a709f833ae93273514a2057cb22423480bf25a/chrome/browser/renderer_context_menu/render_view_context_menu.cc
[modify] https://crrev.com/26a709f833ae93273514a2057cb22423480bf25a/chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc
[modify] https://crrev.com/26a709f833ae93273514a2057cb22423480bf25a/chrome/browser/renderer_context_menu/render_view_context_menu_test_util.cc
[modify] https://crrev.com/26a709f833ae93273514a2057cb22423480bf25a/chrome/browser/renderer_context_menu/render_view_context_menu_test_util.h
[modify] https://crrev.com/26a709f833ae93273514a2057cb22423480bf25a/third_party/blink/public/web/web_context_menu_data.h
[modify] https://crrev.com/26a709f833ae93273514a2057cb22423480bf25a/third_party/blink/renderer/core/exported/web_view_impl.cc
[modify] https://crrev.com/26a709f833ae93273514a2057cb22423480bf25a/third_party/blink/renderer/core/page/context_menu_controller.cc
[modify] https://crrev.com/26a709f833ae93273514a2057cb22423480bf25a/third_party/blink/renderer/core/page/context_menu_controller_test.cc
[modify] https://crrev.com/26a709f833ae93273514a2057cb22423480bf25a/tools/metrics/actions/actions.xml

Status: Verified (was: Started)
Verified in Chromium 69.0.3458.0

Sign in to add a comment