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

Issue 712479 link

Starred by 1 user
Status: Fixed
Owner:
█ x...@chromium.org
Last visit > 30 days ago
Closed: Apr 2017
Cc:
m...@chromium.org
bettes@chromium.org
sko...@chromium.org
mlamouri@chromium.org
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 1
Type: Bug



Sign in to add a comment

Media Remoting UI Edits

Project Member Reported by bettes@chromium.org, Apr 18 2017 
Based off the following attachment: 

1. Update 1x/2x icon assets. I uploaded the wrong dimensions for you.
https://drive.google.com/open?id=0BxMIIGI80eU-ZVM0S1pqZ25yVEE
2. "Casting to your TV" should be 13pt. Looks to be ~17pt
3. spacing between "casting to your tv" text and button looks too small
4. Button height should be 28pt. Looks to be ~54pt
5. Button border should be 2pt. Looks to be 3pt
6. Button text should be vertically centered within button border
7. Button text and body text should be #FFF 0.54a (or #757575), not #FFF 1.0a

Redlines 
https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Media/Media%20Remoting/specs#%3Fpos=top-center&z=fit&fr=0
media_remoting_noposter.png
10.9 KB View Download

Comment 1 by sko...@chromium.org, Apr 18 2017

Labels: M-59

Comment 2 Deleted

Comment 3 by bettes@chromium.org, Apr 18 2017

Description: Show this description

Comment 4 by bettes@chromium.org, Apr 18 2017 

8. Frame color should be #0E0E0E, not #424242
9. Missing transition animation (https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Media/Media%20Remoting/motion#%3Fpos=top-center&z=fit&fr=0)
implementation.png
404 KB View Download

Comment 5 by bettes@chromium.org, Apr 18 2017 

10. Change mouse cursor to "pointer" when over the button

Comment 6 by x...@chromium.org, Apr 18 2017 

Thanks bettes@. Working on the comments...

1. Update 1x/2x icon assets. 
Will do.

2. "Casting to your TV" should be 13pt. Looks to be ~17pt
It is set to 13pt: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/mediaControls.css?rcl=064a811986ce8b3bdcb9bd134d4927b9b36fee2a&l=223

Do you mean 13px instead?

3. spacing between "casting to your tv" text and button looks too small
Will increase the spacing. 

4. Button height should be 28pt. Looks to be ~54pt
It is set to 28pt. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/mediaControls.css?rcl=064a811986ce8b3bdcb9bd134d4927b9b36fee2a&l=215

Now add box-sizing: border-box. Seems better. :)

5. Button border should be 2pt. Looks to be 3pt
It is set to 2pt. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/mediaControls.css?rcl=064a811986ce8b3bdcb9bd134d4927b9b36fee2a&l=217

Do you mean 2px instead?

6. Button text should be vertically centered within button border
Will do.

7. Button text and body text should be #FFF 0.54a (or #757575), not #FFF 1.0a
Will do.

8. Frame color should be #0E0E0E, not #424242
This has to be done in fullscreen-in-tab UI, which is in our OKR. Can we leave this enhancement to after launch?

9. Missing transition animation (https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Media/Media%20Remoting/motion#%3Fpos=top-center&z=fit&fr=0)
Did set this in the css file. Not sure why it doesn't work though. Will try figure out.
 https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/mediaControls.css?rcl=064a811986ce8b3bdcb9bd134d4927b9b36fee2a&l=161
 https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/mediaControls.css?rcl=064a811986ce8b3bdcb9bd134d4927b9b36fee2a&l=226

10. Change mouse cursor to "pointer" when over the button
This is not a quick fix, since similar to the hover effect, it also requires the button on the top. Can we leave this feature to after launch too?

Comment 7 by x...@chromium.org, Apr 18 2017

Cc: sko...@chromium.org m...@chromium.org bettes@chromium.org mlamouri@chromium.org
By design, the transition animation (https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Media/Media%20Remoting/motion#%3Fpos=top-center&z=fit&fr=0) should work when opacity changes from 0 to 1. But the remoting UI is created and shown only when remoting starts, and is hidden (display set to none) when remoting stops. There is no change in opacity. So the transition doesn't work. 

To make the transition work, we may need the following changes:
1. Create the remoting UI before remoting starts (maybe at the same time when the media controls is created, or when mirroring starts, which is currently unknown to the video element). Set the opacity to 0 by default.
2. When remoting starts, set the opacity to 1.
3. When remoting stops, set the opacity back to 0 (instead of setting the display to none).

+mlamouri@: Do these changes sgty?
+skonig@, +bettes@: Is this transition feature a must for the initial launch?

Comment 8 by mlamouri@chromium.org, Apr 18 2017 

Can't we start the transition when remoting starts? Basically, shows the remoting UI with a opacity 0 to 1 transition instead of directly to 1? Same when we hide, we wouldn't hide directly but go from 1 to 0 opacity. Note that you will have to make sure you set display: none when opacity is set to 0.

I would prefer this as it's the less invasive solution for the rest of the code base. Requiring the remoting UI to always be there is a no-go as it would add the memory footprint to all our users. Doing this when a remoting session starts has less memory impact but will add code complexity that I would prefer if we could avoid.

Comment 9 by x...@chromium.org, Apr 18 2017 

Seems that by setting the z-index of the button to 1 can make the hover effect and the mouse cursor changing to "pointer" work. Will fix it.

Comment 10 by x...@chromium.org, Apr 18 2017 

mlamouri@: I understand your concern. I don't like that way either. But I find that the transition doesn't work when set the opacity immediately after RemoveInlineStyleProperty(CSSPropertyDisplay); I even tried set the opacity to 0 first, then changes it to 1, and the transition still doesn't work.
Similarly, when hides, the transition doesn't work when set the opacity to 0 and then set the display to none by calling this: SetInlineStyleProperty(CSSPropertyDisplay, CSSValueNone);

The transition does work when only changes the opacity to show/hide the UI without setting/resetting the display.

Comment 11 by x...@chromium.org, Apr 19 2017 

mlamouri@: Uploaded a CL for reviewing regarding to this transition behavior. Introduced a timer to let the change of display property stable before/after the opacity change, which seems work well. Please see if this change lgty. https://codereview.chromium.org/2825493005/

Comment 12 by x...@chromium.org, Apr 19 2017 

Based on the discussion with bettes@ today, all other issues are addressed in this CL: https://codereview.chromium.org/2830443004/, which should be landed later today.
screenshot_fixed.png
10.9 KB View Download

Comment 15 by x...@chromium.org, Apr 25 2017

Status: Fixed (was: Assigned)

Comment 16 by x...@chromium.org, Apr 25 2017

Labels: Merge-Request-59 OS-Linux
Requesting to merge to M59. Low risk.
Project Member

Comment 17 by sheriffbot@chromium.org, Apr 25 2017

Labels: -Merge-Request-59 Hotlist-Merge-Approved Merge-Approved-59
Your change meets the bar and is auto-approved for M59. Please go ahead and merge the CL to branch 3071 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), gkihumba@(ChromeOS), Abdul Syed@(Desktop)

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

Comment 18 by bugdroid1@chromium.org, Apr 25 2017

Labels: -merge-approved-59 merge-merged-3071
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/306605a63eadc5aeaf4c104c6f1f9eb46d1b07b4

commit 306605a63eadc5aeaf4c104c6f1f9eb46d1b07b4
Author: xjz <xjz@chromium.org>
Date: Tue Apr 25 01:03:21 2017

Merge M59: Media Remoting: Style fix for UI.

Follow-up CL for media remoting UI. Fixed styles according to the UX
designer's comments. Original CL is:
https://codereview.chromium.org/2767823002/

BUG= 712479 
NOTRY=true
NOPRESUBMIT=true
TBR=kinuko@chromium.org

Review-Url: https://codereview.chromium.org/2830443004
Cr-Commit-Position: refs/heads/master@{#465488}
(cherry picked from commit 387a67d23d6fb927769b793d322838515e5b3795)

Review-Url: https://codereview.chromium.org/2841643004
Cr-Commit-Position: refs/branch-heads/3071@{#188}
Cr-Branched-From: a106f0abbf69dad349d4aaf4bcc4f5d376dd2377-refs/heads/master@{#464641}

[modify] https://crrev.com/306605a63eadc5aeaf4c104c6f1f9eb46d1b07b4/third_party/WebKit/Source/core/css/mediaControls.css
[modify] https://crrev.com/306605a63eadc5aeaf4c104c6f1f9eb46d1b07b4/third_party/WebKit/public/default_100_percent/blink/mediaremoting_cast.png
[modify] https://crrev.com/306605a63eadc5aeaf4c104c6f1f9eb46d1b07b4/third_party/WebKit/public/default_200_percent/blink/mediaremoting_cast.png
►

Sign in to add a comment