Bug 61220 – Video looks squished when animating to full screen.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jer Noble]

Video looks squished when animating to full screen.

[Jer Noble]

Created attachment 94290 [details]
Patch

[Jer Noble]

Comment on attachment 94290 [details]
Patch

This is the wrong patch; uploading a new one afresh.

[Jer Noble]

Created attachment 94302 [details]
Patch

[Darin Adler]

Comment on attachment 94302 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        Added a new css pseudo-class: -webkit-animating-full-screen.  During the transition animation, this
> +        pseudo-class will be applied to the current full-screen element.  Styles have been added to 
> +        fullscreenQuickTime.css to hide the video element's built-in controller during the full-screen
> +        animation.

How does removing the controller relate to “video looks squished”? I understand that all this new code makes the controller disappear during the transition to full screen, but I don’t have the slightest idea why that is a good idea, and the change log does not tell me.

> Source/WebCore/css/CSSSelector.cpp:327
> +    DEFINE_STATIC_LOCAL(AtomicString, animatingFullScreen, ("-webkit-animating-full-screen"));

The name “animating full screen” doesn’t seem like clear language to me. That term makes it sound like we’re animating something that’s covering an entire screen. To someone not working on this it does not immediately point to the full screen transition animation. Instead I would prefer a name that is explicitly mentions the transition into or out of full screen rather than simply mentioning full screen and also animation. Maybe even two different pseudo-classes, one for the animation into and the other for the animation out of the full screen state.

> Source/WebCore/css/CSSStyleSelector.cpp:2909
>                  if (e != e->document()->webkitCurrentFullScreenElement())
>                      return false;
>                  return true;

I think it's easier to read something like this as a boolean expression:

    return e == e->document()->webkitCurrentFullScreenElement();

> Source/WebCore/css/CSSStyleSelector.cpp:2913
> +                if (e != e->document()->webkitCurrentFullScreenElement())
> +                    return false;
> +                return e->document()->isAnimatingFullScreen();

I think it's easier to read something like this as a boolean expression:

    return e == e->document()->webkitCurrentFullScreenElement() && e->document()->isAnimatingFullScreen();

> Source/WebCore/dom/Document.cpp:4898
> +    m_fullScreenElement->recalcStyle(Force);

Normally when a pseudo-style has changed we don’t call recalcStyle directly, and we certainly don’t call it directly on a single specific element. I’d expect to instead see a call here to scheduleStyleRecalc (or possibly scheduleForcedStyleRecalc, but I think that would be wrong). I’m surprised by this, and I think it’s probably wrong.

> Source/WebKit2/ChangeLog:11
> +        * WebProcess/FullScreen/mac/WebFullScreenManagerMac.mm:
> +        (WebKit::WebFullScreenManagerMac::beginEnterFullScreenAnimation): Set the destination frame
> +            to be the content box of the current full screen element.
> +        (WebKit::WebFullScreenManagerMac::beginExitFullScreenAnimation): Ditto.

I don’t understand why these changes are here, and how they relate to the WebCore changes above. The change log states the change being made, but neither the change log nor the code touch on the *why* aspect. Was there something wrong before? What was the symptom? How does this change help?

[Jer Noble]

(In reply to comment #4)
> (From update of attachment 94302 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94302&action=review
> 
> > Source/WebCore/ChangeLog:13
> > +        Added a new css pseudo-class: -webkit-animating-full-screen.  During the transition animation, this
> > +        pseudo-class will be applied to the current full-screen element.  Styles have been added to 
> > +        fullscreenQuickTime.css to hide the video element's built-in controller during the full-screen
> > +        animation.
> 
> How does removing the controller relate to “video looks squished”? I understand that all this new code makes the controller disappear during the transition to full screen, but I don’t have the slightest idea why that is a good idea, and the change log does not tell me.

Okay, I'll add more to the ChangeLog describing why this is necessary.

> > Source/WebCore/css/CSSSelector.cpp:327
> > +    DEFINE_STATIC_LOCAL(AtomicString, animatingFullScreen, ("-webkit-animating-full-screen"));
> 
> The name “animating full screen” doesn’t seem like clear language to me. That term makes it sound like we’re animating something that’s covering an entire screen. To someone not working on this it does not immediately point to the full screen transition animation. Instead I would prefer a name that is explicitly mentions the transition into or out of full screen rather than simply mentioning full screen and also animation. Maybe even two different pseudo-classes, one for the animation into and the other for the animation out of the full screen state.

I'd be fine with that.  -webkit-animating-enter-full-screen and -webkit-animating-exit-full-screen.

> > Source/WebCore/css/CSSStyleSelector.cpp:2909
> >                  if (e != e->document()->webkitCurrentFullScreenElement())
> >                      return false;
> >                  return true;
> 
> I think it's easier to read something like this as a boolean expression:
> 
>     return e == e->document()->webkitCurrentFullScreenElement();

Sure thing.

> > Source/WebCore/css/CSSStyleSelector.cpp:2913
> > +                if (e != e->document()->webkitCurrentFullScreenElement())
> > +                    return false;
> > +                return e->document()->isAnimatingFullScreen();
> 
> I think it's easier to read something like this as a boolean expression:
> 
>     return e == e->document()->webkitCurrentFullScreenElement() && e->document()->isAnimatingFullScreen();

On this one though, I actually think that the combined return statement is more difficult to parse.

> > Source/WebCore/dom/Document.cpp:4898
> > +    m_fullScreenElement->recalcStyle(Force);
> 
> Normally when a pseudo-style has changed we don’t call recalcStyle directly, and we certainly don’t call it directly on a single specific element. I’d expect to instead see a call here to scheduleStyleRecalc (or possibly scheduleForcedStyleRecalc, but I think that would be wrong). I’m surprised by this, and I think it’s probably wrong.

Okay, I'll try that.

> > Source/WebKit2/ChangeLog:11
> > +        * WebProcess/FullScreen/mac/WebFullScreenManagerMac.mm:
> > +        (WebKit::WebFullScreenManagerMac::beginEnterFullScreenAnimation): Set the destination frame
> > +            to be the content box of the current full screen element.
> > +        (WebKit::WebFullScreenManagerMac::beginExitFullScreenAnimation): Ditto.
> 
> I don’t understand why these changes are here, and how they relate to the WebCore changes above. The change log states the change being made, but neither the change log nor the code touch on the *why* aspect. Was there something wrong before? What was the symptom? How does this change help?

Okay, I'll add more to describe in the ChangeLog why this is necessary.

[Jer Noble]

(In reply to comment #5)
> (In reply to comment #4)
> > > Source/WebCore/css/CSSSelector.cpp:327
> > > +    DEFINE_STATIC_LOCAL(AtomicString, animatingFullScreen, ("-webkit-animating-full-screen"));
> > 
> > The name “animating full screen” doesn’t seem like clear language to me. That term makes it sound like we’re animating something that’s covering an entire screen. To someone not working on this it does not immediately point to the full screen transition animation. Instead I would prefer a name that is explicitly mentions the transition into or out of full screen rather than simply mentioning full screen and also animation. Maybe even two different pseudo-classes, one for the animation into and the other for the animation out of the full screen state.
> 
> I'd be fine with that.  -webkit-animating-enter-full-screen and -webkit-animating-exit-full-screen.

In the end, I decided on -webkit-animating-full-screen-transition, because splitting the pseudo-class in twain would require a lot of record keeping for no functional reason.

> > > Source/WebCore/dom/Document.cpp:4898
> > > +    m_fullScreenElement->recalcStyle(Force);
> > 
> > Normally when a pseudo-style has changed we don’t call recalcStyle directly, and we certainly don’t call it directly on a single specific element. I’d expect to instead see a call here to scheduleStyleRecalc (or possibly scheduleForcedStyleRecalc, but I think that would be wrong). I’m surprised by this, and I think it’s probably wrong.
> 
> Okay, I'll try that.

Yes, all of these new recalcStyle(Force) instances can be replaced with setNeedsStyleRecalc() and scheduleStyleRecalc().

[Jer Noble]

Committed r87102: <http://trac.webkit.org/changeset/87102>