Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
6 years, 9 months ago by Fredrik Öhrn Modified:
6 years, 5 months ago Reviewers:
acolwell GONE FROM CHROMIUM, qinmin, philipj_slow CC:
blink-reviews, nessy, gasubic, fs, eric.carlson_apple.com, feature-media-reviews_chromium.org, dglazkov+blink, adamk+blink_chromium.org, vcarbune.chromium Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionRemove media controls when not in use.
To avoid ambiguity remove the meda controls when the controls attribute is
false rather than just hiding them.
In addition there is no need to force enable the controls for fullscreen
video on Android.
BUG=347105
Patch Set 1 #
Total comments: 3
Patch Set 2 : Add shouldDisplayControls and fix overlay logic #
Total comments: 1
Patch Set 3 : Remove overlayFullscreenVideoEnabled test #
Total comments: 3
Patch Set 4 : Track visibility in controls. #
Total comments: 6
Created: 6 years, 9 months ago
Messages
Total messages: 26 (0 generated)
Adding qinmin for Android and adamk, abarth becasue it looks like you guys have reviewed changes in HTMLMediaElement before and are owners. This patch fixes the incorrectly shown media controls both if JS toggles the controls attribute or enters fullscreen, see the bug for a more details.
-adams, +acolwell who's the media expert
no lgtm https://codereview.chromium.org/182613006/diff/1/Source/core/html/HTMLMediaEl... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/182613006/diff/1/Source/core/html/HTMLMediaEl... Source/core/html/HTMLMediaElement.cpp:2307: // Always show controls when in full screen mode, except on Android which This is a wrong assumption
not lgtm On 2014/02/27 18:07:40, qinmin wrote: > no lgtm > > https://codereview.chromium.org/182613006/diff/1/Source/core/html/HTMLMediaEl... > File Source/core/html/HTMLMediaElement.cpp (right): > > https://codereview.chromium.org/182613006/diff/1/Source/core/html/HTMLMediaEl... > Source/core/html/HTMLMediaElement.cpp:2307: // Always show controls when in full > screen mode, except on Android which > This is a wrong assumption
There is a fullscreen mode that android will use html5 media controls, currently that is not enabled by default. On 2014/02/27 18:07:56, qinmin wrote: > not lgtm > On 2014/02/27 18:07:40, qinmin wrote: > > no lgtm > > > > > https://codereview.chromium.org/182613006/diff/1/Source/core/html/HTMLMediaEl... > > File Source/core/html/HTMLMediaElement.cpp (right): > > > > > https://codereview.chromium.org/182613006/diff/1/Source/core/html/HTMLMediaEl... > > Source/core/html/HTMLMediaElement.cpp:2307: // Always show controls when in > full > > screen mode, except on Android which > > This is a wrong assumption
https://codereview.chromium.org/182613006/diff/1/Source/core/html/HTMLMediaEl... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/182613006/diff/1/Source/core/html/HTMLMediaEl... Source/core/html/HTMLMediaElement.cpp:2309: if (isFullscreen()) This and the "script disabled" code above seems misplaced. This code causes the JS object attribute to differ from the content attribute, which violates the HTML5 spec (http://www.w3.org/html/wg/drafts/html/master/embedded-content-0.html#dom-medi...) It seems like this logic should be in a shouldDisplayControls() function instead of controls(). https://codereview.chromium.org/182613006/diff/1/Source/core/html/HTMLMediaEl... Source/core/html/HTMLMediaElement.cpp:3603: ensureUserAgentShadowRoot().removeChild(mediaControls()); Why is this needed? What are you trying to solve with this?
On 2014/02/27 18:12:02, qinmin wrote: > There is a fullscreen mode that android will use html5 media controls, currently > that is not enabled by default. > I've updated the patch to use RuntimeEnabledFeatures::overlayFullscreenVideoEnabled() to handle this case.
On 2014/02/27 18:35:22, acolwell wrote: > https://codereview.chromium.org/182613006/diff/1/Source/core/html/HTMLMediaEl... > File Source/core/html/HTMLMediaElement.cpp (right): > > https://codereview.chromium.org/182613006/diff/1/Source/core/html/HTMLMediaEl... > Source/core/html/HTMLMediaElement.cpp:2309: if (isFullscreen()) > This and the "script disabled" code above seems misplaced. This code causes the > JS object attribute to differ from the content attribute, which violates the > HTML5 spec > (http://www.w3.org/html/wg/drafts/html/master/embedded-content-0.html#dom-medi...) > > It seems like this logic should be in a shouldDisplayControls() function instead > of controls(). > OK, fixed. > https://codereview.chromium.org/182613006/diff/1/Source/core/html/HTMLMediaEl... > Source/core/html/HTMLMediaElement.cpp:3603: > ensureUserAgentShadowRoot().removeChild(mediaControls()); > Why is this needed? What are you trying to solve with this? The play button on Android can show itself in response to play/pause events, see MediaControlOverlayPlayButtonElement::updateDisplayType(). So merely hiding when shouldDisplayControls() toggles to false is not enough to make sure the controls stay hidden. Removing the controls makes sure there is no ambiguity.
https://codereview.chromium.org/182613006/diff/20001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/182613006/diff/20001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:2312: if (isFullscreen() && !RuntimeEnabledFeatures::overlayFullscreenVideoEnabled()) overlayFullscreenVideoEnabled is enabled by default for android, the actual flag in chromium is enable-overlay-fullscreen-video-subtitle, which is not passed to blink.
On 2014/02/28 17:12:44, qinmin wrote: > https://codereview.chromium.org/182613006/diff/20001/Source/core/html/HTMLMed... > File Source/core/html/HTMLMediaElement.cpp (right): > > https://codereview.chromium.org/182613006/diff/20001/Source/core/html/HTMLMed... > Source/core/html/HTMLMediaElement.cpp:2312: if (isFullscreen() && > !RuntimeEnabledFeatures::overlayFullscreenVideoEnabled()) > overlayFullscreenVideoEnabled is enabled by default for android, the actual flag > in chromium is enable-overlay-fullscreen-video-subtitle, which is not passed to > blink. OK, I've now removed this part of the patch. I did some testing and the enable-overlay-fullscreen-video-subtitle option doesn't seem to be working as intended, the overlay player is still on top in the z order covering up the subtitles below it, also it's still showing it's own native controls which would collide with the html controls if the z order was correct. acolwell, regarding the rest of the patch, is my explanation about removing the controls clear enough?
Most of the changes look good. Just one remaining issue I'd like you to fix. https://codereview.chromium.org/182613006/diff/40001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/182613006/diff/40001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:3605: userAgentShadowRoot()->removeChild(mediaControls()); This still doesn't seem right to me. These controls could get recreated by createMediaControls() in various ways. Why doesn't the MediaControlsAndroid, just overload show() & hide() to make sure the button doesn't get displayed at the wrong time?
https://codereview.chromium.org/182613006/diff/40001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/182613006/diff/40001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:3605: userAgentShadowRoot()->removeChild(mediaControls()); On 2014/03/05 00:09:50, acolwell wrote: > This still doesn't seem right to me. These controls could get recreated by > createMediaControls() in various ways. Why doesn't the MediaControlsAndroid, > just overload show() & hide() to make sure the button doesn't get displayed at > the wrong time? I can probably patch something together using that method but I think it's ugly. What I wanted to do is get away from the confusion between the controls being enabled/disabled (i.e. the controls attribute is true or false) and the controls being enabled but visible or hidden due to other state like the element having focus or not, faded out out when video is playing etc. Especially since since these "temporary" visibility changes are driven from two directions, both HTMLMediaElement and MediaControlsAndroid, and MediaControls and it's descendants don't have the full picture. I think destroying the controls when they are not needed is the most obvious solution, however if that is deemed too heavy handed, I think it would be better to give the MediaControls class access to call HTMLMediaElement::shouldDisplayControls() so they can query the the enabled/disabled state and avoid showing themselves when not wanted.
https://codereview.chromium.org/182613006/diff/40001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/182613006/diff/40001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:3605: userAgentShadowRoot()->removeChild(mediaControls()); On 2014/03/06 10:20:16, Fredrik Öhrn wrote: > On 2014/03/05 00:09:50, acolwell wrote: > > This still doesn't seem right to me. These controls could get recreated by > > createMediaControls() in various ways. Why doesn't the MediaControlsAndroid, > > just overload show() & hide() to make sure the button doesn't get displayed at > > the wrong time? > > I can probably patch something together using that method but I think it's ugly. I guess, I'm confused as to why this is ugly. > > What I wanted to do is get away from the confusion between the controls being > enabled/disabled (i.e. the controls attribute is true or false) and the controls > being enabled but visible or hidden due to other state like the element having > focus or not, faded out out when video is playing etc. > > Especially since since these "temporary" visibility changes are driven from two > directions, both HTMLMediaElement and MediaControlsAndroid, and MediaControls > and it's descendants don't have the full picture. > > I think destroying the controls when they are not needed is the most obvious > solution, however if that is deemed too heavy handed, I think it would be better > to give the MediaControls class access to call > HTMLMediaElement::shouldDisplayControls() so they can query the the > enabled/disabled state and avoid showing themselves when not wanted. But hide() and show() are how the HTMLMediaElement is signalling whether the controls should be displayed or not. It sounds like the Android controls are not paying attention to these signals. It seems to me that it is the responsibility of HTMLMediaElement to properly call hide/show() when shouldDisplayControls() changes state. If that is not happening, then we should fix that first. It is also still not clear to me that what you have here is the right solution because all the application has to do is add a TextTrack cue and I believe the controls will get recreated and hidden by createMediaControls(). Why doesn't createMediaControls() have to have the same logic as this method? It looks like the controls can just blindly get recreated w/o checking shouldDisplayControls() so I think we are still missing something here.
On 2014/03/06 16:30:56, acolwell wrote: > https://codereview.chromium.org/182613006/diff/40001/Source/core/html/HTMLMed... > File Source/core/html/HTMLMediaElement.cpp (right): > > https://codereview.chromium.org/182613006/diff/40001/Source/core/html/HTMLMed... > Source/core/html/HTMLMediaElement.cpp:3605: > userAgentShadowRoot()->removeChild(mediaControls()); > On 2014/03/06 10:20:16, Fredrik Öhrn wrote: > > On 2014/03/05 00:09:50, acolwell wrote: > > > This still doesn't seem right to me. These controls could get recreated by > > > createMediaControls() in various ways. Why doesn't the MediaControlsAndroid, > > > just overload show() & hide() to make sure the button doesn't get displayed > at > > > the wrong time? > > > > I can probably patch something together using that method but I think it's > ugly. > > I guess, I'm confused as to why this is ugly. > > > > > What I wanted to do is get away from the confusion between the controls being > > enabled/disabled (i.e. the controls attribute is true or false) and the > controls > > being enabled but visible or hidden due to other state like the element having > > focus or not, faded out out when video is playing etc. > > > > Especially since since these "temporary" visibility changes are driven from > two > > directions, both HTMLMediaElement and MediaControlsAndroid, and MediaControls > > and it's descendants don't have the full picture. > > > > I think destroying the controls when they are not needed is the most obvious > > solution, however if that is deemed too heavy handed, I think it would be > better > > to give the MediaControls class access to call > > HTMLMediaElement::shouldDisplayControls() so they can query the the > > enabled/disabled state and avoid showing themselves when not wanted. > > But hide() and show() are how the HTMLMediaElement is signalling whether the > controls should be displayed or not. It sounds like the Android controls are not > paying attention to these signals. It seems to me that it is the responsibility > of HTMLMediaElement to properly call hide/show() when shouldDisplayControls() > changes state. If that is not happening, then we should fix that first. > > It is also still not clear to me that what you have here is the right solution > because all the application has to do is add a TextTrack cue and I believe the > controls will get recreated and hidden by createMediaControls(). Why doesn't > createMediaControls() have to have the same logic as this method? It looks like > the controls can just blindly get recreated w/o checking shouldDisplayControls() > so I think we are still missing something here. I've changed the patch to use show/hide. What I find ugly is the need keep an extra bool deep down in the controls that must be kept in sync with the HTMLMediaElement's idea of visibility. I think it would be better to expose shouldDisplayControls() in MediaControllerInterface, perhaps renaming it to controlsVisible to match the existing closedCaptionsVisible() function used by MediaControlToggleClosedCaptionsButtonElement::updateDisplayType() to solve a similar case.
https://codereview.chromium.org/182613006/diff/60001/Source/core/html/shadow/... File Source/core/html/shadow/MediaControlsAndroid.cpp (right): https://codereview.chromium.org/182613006/diff/60001/Source/core/html/shadow/... Source/core/html/shadow/MediaControlsAndroid.cpp:79: m_overlayPlayButton->updateDisplayType(); nit: It seems like you could unconditionally hide here instead. https://codereview.chromium.org/182613006/diff/60001/Source/core/html/shadow/... Source/core/html/shadow/MediaControlsAndroid.cpp:85: m_overlayPlayButton->updateDisplayType(); It seems like you could make this call and the one below conditionally based on whether m_panel->m_isDisplayed is true. This could be hidden behind a MediaControls::isDisplayed() helper. Doing this avoids the extra bool and allows updates only when the controls as a whole are supposed to be displayed.
On 2014/03/06 17:51:40, Fredrik Öhrn wrote: > On 2014/03/06 16:30:56, acolwell wrote: > > > https://codereview.chromium.org/182613006/diff/40001/Source/core/html/HTMLMed... > > File Source/core/html/HTMLMediaElement.cpp (right): > > > > > https://codereview.chromium.org/182613006/diff/40001/Source/core/html/HTMLMed... > > Source/core/html/HTMLMediaElement.cpp:3605: > > userAgentShadowRoot()->removeChild(mediaControls()); > > On 2014/03/06 10:20:16, Fredrik Öhrn wrote: > > > On 2014/03/05 00:09:50, acolwell wrote: > > > > This still doesn't seem right to me. These controls could get recreated by > > > > createMediaControls() in various ways. Why doesn't the > MediaControlsAndroid, > > > > just overload show() & hide() to make sure the button doesn't get > displayed > > at > > > > the wrong time? > > > > > > I can probably patch something together using that method but I think it's > > ugly. > > > > I guess, I'm confused as to why this is ugly. > > > > > > > > What I wanted to do is get away from the confusion between the controls > being > > > enabled/disabled (i.e. the controls attribute is true or false) and the > > controls > > > being enabled but visible or hidden due to other state like the element > having > > > focus or not, faded out out when video is playing etc. > > > > > > Especially since since these "temporary" visibility changes are driven from > > two > > > directions, both HTMLMediaElement and MediaControlsAndroid, and > MediaControls > > > and it's descendants don't have the full picture. > > > > > > I think destroying the controls when they are not needed is the most obvious > > > solution, however if that is deemed too heavy handed, I think it would be > > better > > > to give the MediaControls class access to call > > > HTMLMediaElement::shouldDisplayControls() so they can query the the > > > enabled/disabled state and avoid showing themselves when not wanted. > > > > But hide() and show() are how the HTMLMediaElement is signalling whether the > > controls should be displayed or not. It sounds like the Android controls are > not > > paying attention to these signals. It seems to me that it is the > responsibility > > of HTMLMediaElement to properly call hide/show() when shouldDisplayControls() > > changes state. If that is not happening, then we should fix that first. > > > > It is also still not clear to me that what you have here is the right solution > > because all the application has to do is add a TextTrack cue and I believe the > > controls will get recreated and hidden by createMediaControls(). Why doesn't > > createMediaControls() have to have the same logic as this method? It looks > like > > the controls can just blindly get recreated w/o checking > shouldDisplayControls() > > so I think we are still missing something here. > > I've changed the patch to use show/hide. > > What I find ugly is the need keep an extra bool deep down in the controls that > must be kept in sync with the HTMLMediaElement's idea of visibility. I don't think you should think about it quite this way. Right now we have a sub-control that is not paying attention to the display state of its parent MediaControls container. The HTMLMediaElement is signalling its intentions to MediaControls, but the sub-control is not paying attention to that. It should be tracking the MediaControl's state and not looking at HTMLMediaElements. > > I think it would be better to expose shouldDisplayControls() in > MediaControllerInterface, perhaps renaming it to controlsVisible to match the > existing closedCaptionsVisible() function used by > MediaControlToggleClosedCaptionsButtonElement::updateDisplayType() to solve a > similar case. At some point we may want to consider moving these visibility decisions into MediaControls instead of having them in HTMLMediaElement. I think our division of responsibilities is slightly off here because ISTM that the controls attribute on the element should simply trigger creation/destruction of the controls and MediaControls should take care of determining when it should be visible or now. It feels like we have these concepts unnecessarily mixed right now, but this patch is definitely taking us in the right direction.
https://codereview.chromium.org/182613006/diff/60001/Source/core/html/shadow/... File Source/core/html/shadow/MediaControlsAndroid.cpp (right): https://codereview.chromium.org/182613006/diff/60001/Source/core/html/shadow/... Source/core/html/shadow/MediaControlsAndroid.cpp:85: m_overlayPlayButton->updateDisplayType(); On 2014/03/06 19:21:54, acolwell wrote: > It seems like you could make this call and the one below conditionally based on > whether m_panel->m_isDisplayed is true. This could be hidden behind a > MediaControls::isDisplayed() helper. > > Doing this avoids the extra bool and allows updates only when the controls as a > whole are supposed to be displayed. Problem is that MediaControlOverlayPlayButtonElement::defaultEventHandler() can also call updateDisplayType() on itself, there MediaControls::isDisplayed() is not available since the buttons don't have a pointer back it.
On 2014/03/07 09:45:44, Fredrik Öhrn wrote: > On 2014/03/06 19:21:54, acolwell wrote: > > It seems like you could make this call and the one below conditionally based > on > > whether m_panel->m_isDisplayed is true. This could be hidden behind a > > MediaControls::isDisplayed() helper. > > > > Doing this avoids the extra bool and allows updates only when the controls as > a > > whole are supposed to be displayed. > > Problem is that MediaControlOverlayPlayButtonElement::defaultEventHandler() can > also call updateDisplayType() on itself, there MediaControls::isDisplayed() is > not available since the buttons don't have a pointer back it. Ping?
On 2014/03/12 12:59:50, Fredrik Öhrn wrote: > On 2014/03/07 09:45:44, Fredrik Öhrn wrote: > > On 2014/03/06 19:21:54, acolwell wrote: > > > It seems like you could make this call and the one below conditionally based > > on > > > whether m_panel->m_isDisplayed is true. This could be hidden behind a > > > MediaControls::isDisplayed() helper. > > > > > > Doing this avoids the extra bool and allows updates only when the controls > as > > a > > > whole are supposed to be displayed. > > > > Problem is that MediaControlOverlayPlayButtonElement::defaultEventHandler() > can > > also call updateDisplayType() on itself, there MediaControls::isDisplayed() is > > not available since the buttons don't have a pointer back it. > > Ping? Hi. Once https://codereview.chromium.org/195473002/ lands you should rebase your changes on top of it. I believe my suggestions will be viable and your concern about defaultEventHandler() can be addressed because MediaControlOverlayPlayButtonElement will have a reference to MediaControls.
I like the shouldDisplayControls() clarification! As for making MediaControls::hide() really hide them, I think the cleanest thing might be to just set hide the HTMLDivElement that is MediaControls itself. Copy paste MediaControlElement::hide() and show(), or even simpler use setBooleanAttribute(hiddenAttr, true/false). https://codereview.chromium.org/182613006/diff/60001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/182613006/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.h:169: // This reflects the DOM attribute. If you add [Reflect] to HTMLMediaElement.idl you can remove both of these instead, since neither are actually needed internally. https://codereview.chromium.org/182613006/diff/60001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.h:174: bool shouldDisplayControls() const; Bikeshed: shouldShowControls() to go with MediaControls::show() and HTMLMediaElement::showingTrackWithSameKind()?
On 2014/03/12 13:52:16, acolwell_OOO_til_3-15 wrote: > > Hi. Once https://codereview.chromium.org/195473002/ lands you should rebase your > changes on top of it. I believe my suggestions will be viable and your concern > about defaultEventHandler() can be addressed because > MediaControlOverlayPlayButtonElement will have a reference to MediaControls. Thanks for the pointer, I'll sort it out together with philipj.
I tried out my own suggestion of hiding the whole controls in <https://codereview.chromium.org/199413008/>, does that seem OK?
acolwell found a pretty big hole in that approach. Until the text track rendering is separated from the controls code I think the next best thing is to just call m_overlayEnclosure->hide() in MediaControls::hide(). By hiding the enclosure instead of the button one doesn't need a m_isDisplayed flag like the panel has. (Ideally it should be m_enclosure that's hidden instead of m_panel as well, but that causes test failures that don't seem worth fiddling with if this is going to change again soonish anyway.)
https://codereview.chromium.org/182613006/diff/60001/Source/core/html/shadow/... File Source/core/html/shadow/MediaControlsAndroid.cpp (right): https://codereview.chromium.org/182613006/diff/60001/Source/core/html/shadow/... Source/core/html/shadow/MediaControlsAndroid.cpp:79: m_overlayPlayButton->updateDisplayType(); On 2014/03/06 19:21:54, acolwell wrote: > nit: It seems like you could unconditionally hide here instead. Yes, when moving this into MediaControls.cpp try just if (m_overlayEnclosure) m_overlayEnclosure->hide().
I've made a new review for the controls attribute fix alone to get it rolling: https://codereview.chromium.org/351213007/
Message was sent while issue was closed.
New simpler solution posted in https://codereview.chromium.org/363563007/ |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
