Bug 56851 – Media controls must use full screen style when in new full screen mode.

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Jer Noble]

The new full screen API support will require the media element controls to change styles when in full screen mode.

[Jer Noble]

Created attachment 86491 [details]
Patch

[Eric Carlson]

Comment on attachment 86491 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=86491&action=review

> Source/WebCore/css/CSSStyleSelector.cpp:1433
> +#if ENABLE(FULLSCREEN_API)
> +    static bool loadedFullScreenStyleSheet;
> +    if (!loadedFullScreenStyleSheet && e->document()->webkitIsFullScreen()) {
> +        loadedFullScreenStyleSheet = true;
> +        String fullscreenRules = String(fullscreenUserAgentStyleSheet, sizeof(fullscreenUserAgentStyleSheet)) + RenderTheme::defaultTheme()->extraFullScreenStyleSheet();
> +        CSSStyleSheet* fullscreenSheet = parseUASheet(fullscreenRules);
> +        defaultStyle->addRulesFromSheet(fullscreenSheet, screenEval());
> +        defaultQuirksStyle->addRulesFromSheet(fullscreenSheet, screenEval());
> +    }
> +#endif

Because this style should only be used for <video> fullscreen, can't you further restrict this loading to when e->hasTagName(videoTag)? Or should the video controls style change when in document fullscreen mode too?

> Source/WebCore/html/shadow/MediaControls.cpp:60
> +#if ENABLE(FULLSCREEN_API)
> +    if (mediaElement->document()->webkitIsFullScreen() && mediaElement->document()->webkitCurrentFullScreenElement() == mediaElement)
> +        return true;
> +#endif

Are these tests necessary, or do we want to use different default controls when in document fullscreen?

> Source/WebCore/rendering/MediaControlElements.cpp:291
> +    if (overrides == m_overridesPosition)
> +        return;
> +    m_overridesPosition = overrides;

The check seems unnecessary since you only change the instance variable.

> Source/WebCore/rendering/RenderThemeMac.mm:1992
>      case MediaToggleClosedCaptionsButtonPart:
>          // We rely on QTKit to render captions so don't enable the button unless it will be able to do so.
>          if (!element->hasTagName(videoTag))
>              return false;

Need a "break" here.

> Source/WebCore/rendering/RenderThemeMac.mm:1995
> +    case MediaRewindButtonPart:
> +        return mediaElement->movieLoadType() == MediaPlayer::LiveStream 
> +            || mediaElement->movieLoadType() == MediaPlayer::StoredStream;

Doesn't this change the display of inline elements, which currently show the rewind button for everything except live streams?

> Source/WebCore/rendering/RenderThemeMac.mm:1997
> +    case MediaReturnToRealtimeButtonPart:
> +        return mediaElement->movieLoadType() == MediaPlayer::LiveStream;

The base class already does this.

[Jer Noble]

Comment on attachment 86491 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=86491&action=review

>> Source/WebCore/css/CSSStyleSelector.cpp:1433
>> +#endif
> 
> Because this style should only be used for <video> fullscreen, can't you further restrict this loading to when e->hasTagName(videoTag)? Or should the video controls style change when in document fullscreen mode too?

Technically, there are styles in the fullScreen.css file which affect non <video> elements, such as <img> elements, as well as the suggested rules from the Mozilla Full Screen API document.

>> Source/WebCore/html/shadow/MediaControls.cpp:60
>> +#endif
> 
> Are these tests necessary, or do we want to use different default controls when in document fullscreen?

When going into full screen mode using the new, generic full screen api, HTMLMediaElement will continue to return "false" for "isFullscreen()", which is why we need these additional checks.  But the current behavior should probably have a bug filed against it.

>> Source/WebCore/rendering/MediaControlElements.cpp:291
>> +    m_overridesPosition = overrides;
> 
> The check seems unnecessary since you only change the instance variable.

Indeed.  I was just following the file's convention, but as you point out, the convention is unnecessary. :)

>> Source/WebCore/rendering/RenderThemeMac.mm:1992
>>              return false;
> 
> Need a "break" here.

Whoops!  Added.

>> Source/WebCore/rendering/RenderThemeMac.mm:1995
>> +            || mediaElement->movieLoadType() == MediaPlayer::StoredStream;
> 
> Doesn't this change the display of inline elements, which currently show the rewind button for everything except live streams?

You're right, this should have a isFullscreen check added.

>> Source/WebCore/rendering/RenderThemeMac.mm:1997
>> +        return mediaElement->movieLoadType() == MediaPlayer::LiveStream;
> 
> The base class already does this.

Removed.

> Source/WebCore/rendering/RenderThemeMac.mm:2000
> +        return mediaElement->isFullscreen() 

And this needs to be changed as well, given the buggy isFullscreen behavior described above.

[Jer Noble]

I'll make the suggested changes and re-upload after rebasing the patch.

[Maciej Stachowiak]

Comment on attachment 86491 [details]
Patch

Marking r- since Jer plans to upload a new patch.

[Jer Noble]

Created attachment 87016 [details]
Patch

[Jer Noble]

Created attachment 87017 [details]
Patch

fullscreenQuickTime.css was missing from the last patch.

[Eric Carlson]

Comment on attachment 87017 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=87017&action=review

Nice, this is *much* cleaner than the earlier version!

> Source/WebCore/html/shadow/MediaControls.cpp:150
> +    m_fullScreenMinVolumeButton = MediaControlFullscreenVolumeMinButtonElement::create(mediaElement);
> +    m_fullScreenMinVolumeButton->attachToParent(m_panel.get());
> +    
> +    m_fullScreenVolumeSlider = MediaControlFullscreenVolumeSliderElement::create(mediaElement);
> +    m_fullScreenVolumeSlider->attachToParent(m_panel.get());
> +    
> +    m_fullScreenMaxVolumeButton = MediaControlFullscreenVolumeMaxButtonElement::create(mediaElement);
> +    m_fullScreenMaxVolumeButton->attachToParent(m_panel.get());
> +    

It would be nice if it was possible to create these controls only when the element goes into fullscreen, maybe via an explicit call from the element? I think it can be done in a follow-up patch, but it would be a nice optimization because most <video> elements will never go fullscreen, and *no* <audio> element ever will. It might be good to have a FIXME here with a bug number for the enhancement so we don't forget to do it.

[Jer Noble]

(In reply to comment #8)
> (From update of attachment 87017 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=87017&action=review
> 
> Nice, this is *much* cleaner than the earlier version!

Thanks!

> > Source/WebCore/html/shadow/MediaControls.cpp:150
> > +    m_fullScreenMinVolumeButton = MediaControlFullscreenVolumeMinButtonElement::create(mediaElement);
> > +    m_fullScreenMinVolumeButton->attachToParent(m_panel.get());
> > +    
> > +    m_fullScreenVolumeSlider = MediaControlFullscreenVolumeSliderElement::create(mediaElement);
> > +    m_fullScreenVolumeSlider->attachToParent(m_panel.get());
> > +    
> > +    m_fullScreenMaxVolumeButton = MediaControlFullscreenVolumeMaxButtonElement::create(mediaElement);
> > +    m_fullScreenMaxVolumeButton->attachToParent(m_panel.get());
> > +    
> 
> It would be nice if it was possible to create these controls only when the element goes into fullscreen, maybe via an explicit call from the element? I think it can be done in a follow-up patch, but it would be a nice optimization because most <video> elements will never go fullscreen, and *no* <audio> element ever will. It might be good to have a FIXME here with a bug number for the enhancement so we don't forget to do it.

We could do a lot more for most of the shadow media controls: only create the Closed Caption button when the media has CCs, for example.  Filed: https://bugs.webkit.org/show_bug.cgi?id=57163

[Jer Noble]

Committed r82053: <http://trac.webkit.org/changeset/82053>

[WebKit Review Bot]

http://trac.webkit.org/changeset/82053 might have broken SnowLeopard Intel Release (Tests)
The following tests are not passing:
accessibility/media-element.html
media/audio-controls-rendering.html
media/audio-repaint.html
media/controls-after-reload.html
media/controls-styling.html
media/controls-without-preload.html
media/media-document-audio-repaint.html

[Jer Noble]

Looks like the "skip back" button is mising.

[Jer Noble]

Comment on attachment 87017 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=87017&action=review

> Source/WebCore/rendering/RenderThemeMac.mm:1997
> +        return mediaElement->isFullscreen()
> +            && (mediaElement->movieLoadType() == MediaPlayer::LiveStream 
> +            || mediaElement->movieLoadType() == MediaPlayer::StoredStream);

Aha, this should really be: "if (mediaElement->isFullscreen())"

[Jer Noble]

Committed r82057: <http://trac.webkit.org/changeset/82057>