Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
17382003:
Sync Android <video controls> with the default controls (Closed)
|
Created:
7 years, 6 months ago by philipj_slow Modified:
7 years, 2 months ago CC:
blink-reviews, eae+blinkwatch, feature-media-reviews_chromium.org, dglazkov+blink, apavlov+blink_chromium.org, darktears, vcarbune.chromium Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionSync Android <video controls> with the default controls
Add back the rounded border and the bottom 5px padding.
BUGS=251244
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=158344
Patch Set 1 #Patch Set 2 : sync with the default controls #
Total comments: 7
Messages
Total messages: 21 (0 generated)
A simple change, should hopefully not need much discussion :)
This will impact the default look and feel of current media control as we tried to be consistent with the desktop look and feel. I think you can add MediaControlsOperaAndroid.css, and inherit from MediaControlsChromiumAndroid. In that case, if opera later on decides to have more css changes, it won't impact the default one.
This will impact the default look and feel of current media control as we tried to be consistent with the desktop look and feel. I think you can add MediaControlsOperaAndroid.css, and inherit from MediaControlsChromiumAndroid. In that case, if opera later on decides to have more css changes, it won't impact the default one.
On 2013/06/18 14:40:23, qinmin wrote: > This will impact the default look and feel of current media control as we tried > to be consistent with the desktop look and feel. > I think you can add MediaControlsOperaAndroid.css, and inherit from > MediaControlsChromiumAndroid. In that case, if opera later on decides to have > more css changes, it won't impact the default one. philipj: before we go down the path of introducing numerous CSS files, does Opera plan on going down a completely different path with respect media controls styling? qinmin: it's possible we may want this change too -- do you want to try applying it to see what it feels like? maybe philipj can upload a before + after images :)
I added some screenshots to the crbug in description. On 2013/06/18 23:19:24, scherkus wrote: > On 2013/06/18 14:40:23, qinmin wrote: > > This will impact the default look and feel of current media control as we > tried > > to be consistent with the desktop look and feel. > > I think you can add MediaControlsOperaAndroid.css, and inherit from > > MediaControlsChromiumAndroid. In that case, if opera later on decides to have > > more css changes, it won't impact the default one. > > philipj: before we go down the path of introducing numerous CSS files, does > Opera plan on going down a completely different path with respect media controls > styling? > > qinmin: it's possible we may want this change too -- do you want to try applying > it to see what it feels like? maybe philipj can upload a before + after images > :)
On 2013/06/18 14:40:22, qinmin wrote: > This will impact the default look and feel of current media control as we tried > to be consistent with the desktop look and feel. > I think you can add MediaControlsOperaAndroid.css, and inherit from > MediaControlsChromiumAndroid. In that case, if opera later on decides to have > more css changes, it won't impact the default one. I think that the inclusion of that padding is just an accident from initially copying the standard Chromium CSS file, don't you agree? I've attached a screenshot and commented on the bug.
On 2013/06/18 23:19:24, scherkus wrote: > philipj: before we go down the path of introducing numerous CSS files, does > Opera plan on going down a completely different path with respect media controls > styling? That's possible, but for this patch I just thought that there was an obvious bug and that you'd like the fix as well. If the current padding is deliberate, then it would probably be a good idea to say "padding: 0px 5px 0px 5px;" instead for clarity. In that case I will look at a way to introduce our own CSS file so that you can keep the padding while we remove it.
Ping @qinmin as per https://groups.google.com/a/chromium.org/d/msg/blink-dev/7caxMJ8b7iA/vpnxuY8o...
On 2013/09/10 07:48:46, philipj wrote: > Ping @qinmin as per > https://groups.google.com/a/chromium.org/d/msg/blink-dev/7caxMJ8b7iA/vpnxuY8o... Is this CL still valid? the lines have been moved to mediacontrols.css. If you change that, you change the desktop UI. we do want to keep the UI in sync between desktop/android. Android does not necessarily mean small screen devices, it can have a same screen size as the desktop.
On 2013/09/19 16:51:33, qinmin wrote: > On 2013/09/10 07:48:46, philipj wrote: > > Ping @qinmin as per > > > https://groups.google.com/a/chromium.org/d/msg/blink-dev/7caxMJ8b7iA/vpnxuY8o... > > Is this CL still valid? the lines have been moved to mediacontrols.css. > If you change that, you change the desktop UI. > we do want to keep the UI in sync between desktop/android. Android does not > necessarily mean small screen devices, it can have a same screen size as the > desktop. The CL is out of date, it's blocking on deciding what to actually do. Do you want me to just align Android with what Desktop does?
I've uploaded a new CL which makes the Android controls more in line with the default controls. Note that the 40px height of video::-webkit-media-controls-enclosure is 5px more than the 35px in the base CSS file. A bit fragile, some time in the future it might be nice to do some of this using CSS variables and calc.
https://codereview.chromium.org/17382003/diff/12001/Source/core/css/mediaCont... File Source/core/css/mediaControlsAndroid.css (left): https://codereview.chromium.org/17382003/diff/12001/Source/core/css/mediaCont... Source/core/css/mediaControlsAndroid.css:57: border-radius: 0; why this? no rounded corners? https://codereview.chromium.org/17382003/diff/12001/Source/core/css/mediaCont... File Source/core/css/mediaControlsAndroid.css (right): https://codereview.chromium.org/17382003/diff/12001/Source/core/css/mediaCont... Source/core/css/mediaControlsAndroid.css:37: video::-webkit-media-controls-enclosure { why the difference between video and audio ?
https://codereview.chromium.org/17382003/diff/12001/Source/core/css/mediaCont... File Source/core/css/mediaControlsAndroid.css (left): https://codereview.chromium.org/17382003/diff/12001/Source/core/css/mediaCont... Source/core/css/mediaControlsAndroid.css:57: border-radius: 0; On 2013/09/20 14:52:09, qinmin wrote: > why this? no rounded corners? I don't know why this was here, as you've probably noticed there are no rounded corners in current Chrome or Opera for Android. This is fixing that to align with Desktop. https://codereview.chromium.org/17382003/diff/12001/Source/core/css/mediaCont... File Source/core/css/mediaControlsAndroid.css (right): https://codereview.chromium.org/17382003/diff/12001/Source/core/css/mediaCont... Source/core/css/mediaControlsAndroid.css:37: video::-webkit-media-controls-enclosure { On 2013/09/20 14:52:09, qinmin wrote: > why the difference between video and audio ? There's a 5px difference in mediaControls.css as well to account for the extra 5px padding, but someone overlooked that when making the Android stylesheet, which is why there wasn't any padding at the bottom before. It should now match Desktop, except that the controls are bigger.
https://codereview.chromium.org/17382003/diff/12001/Source/core/css/mediaCont... File Source/core/css/mediaControlsAndroid.css (left): https://codereview.chromium.org/17382003/diff/12001/Source/core/css/mediaCont... Source/core/css/mediaControlsAndroid.css:57: border-radius: 0; I don't think this aligns with desktop, desktop chrome has rounded corners on the media control panel. On 2013/09/22 08:11:48, philipj wrote: > On 2013/09/20 14:52:09, qinmin wrote: > > why this? no rounded corners? > > I don't know why this was here, as you've probably noticed there are no rounded > corners in current Chrome or Opera for Android. This is fixing that to align > with Desktop.
https://codereview.chromium.org/17382003/diff/12001/Source/core/css/mediaCont... File Source/core/css/mediaControlsAndroid.css (left): https://codereview.chromium.org/17382003/diff/12001/Source/core/css/mediaCont... Source/core/css/mediaControlsAndroid.css:57: border-radius: 0; On 2013/09/22 15:51:45, qinmin wrote: > I don't think this aligns with desktop, desktop chrome has rounded corners on > the media control panel. > > On 2013/09/22 08:11:48, philipj wrote: > > On 2013/09/20 14:52:09, qinmin wrote: > > > why this? no rounded corners? > > > > I don't know why this was here, as you've probably noticed there are no > rounded > > corners in current Chrome or Opera for Android. This is fixing that to align > > with Desktop. Yes, and now Android does too. "border-radius: 0" *removes* the rounded corners, removing this line adds them back.
Aaron, Adam, could you have a look at this as well?
lgtm
https://codereview.chromium.org/17382003/diff/12001/Source/core/css/mediaCont... File Source/core/css/mediaControlsAndroid.css (left): https://codereview.chromium.org/17382003/diff/12001/Source/core/css/mediaCont... Source/core/css/mediaControlsAndroid.css:57: border-radius: 0; This looks like a bug we missed earlier when changing all the css files, thanks for removing this.
lgtm, rs=me
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/philipj@opera.com/17382003/12001
Message was sent while issue was closed.
Change committed as 158344 |
