Bug 141439 – Scroll to make the video element visible when exiting fullscreen.

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

[Jeremy Jones]

Scroll element visible when exiting fullscreen.

[Jeremy Jones]

Created attachment 246333 [details]
Patch

[WebKit Commit Bot]

Attachment 246333 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/Cocoa/UIDelegate.h:94:  Please declare enum bitfields as unsigned integral types.  [runtime/enum_bitfields] [5]
Total errors found: 1 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jeremy Jones]

Created attachment 246335 [details]
Patch

[WebKit Commit Bot]

Attachment 246335 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/Cocoa/UIDelegate.h:94:  Please declare enum bitfields as unsigned integral types.  [runtime/enum_bitfields] [5]
Total errors found: 1 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

"Scroll element visible when exiting fullscreen." is so hard to parse.

Do you mean "the scroll element is visible" or "scroll the element to be visible"?

[Jeremy Jones]

(In reply to comment #5)
> "Scroll element visible when exiting fullscreen." is so hard to parse.
> 
> Do you mean "the scroll element is visible" or "scroll the element to be
> visible"?

Second one. Scroll is a verb. I'll make that less ambiguous.

[Jeremy Jones]

Created attachment 247560 [details]
Patch

[WebKit Commit Bot]

Attachment 247560 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/Cocoa/UIDelegate.h:94:  Please declare enum bitfields as unsigned integral types.  [runtime/enum_bitfields] [5]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jeremy Jones]

Created attachment 247712 [details]
Patch

[WebKit Commit Bot]

Attachment 247712 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/Cocoa/UIDelegate.h:94:  Please declare enum bitfields as unsigned integral types.  [runtime/enum_bitfields] [5]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jeremy Jones]

Created attachment 247964 [details]
Patch

[Jeremy Jones]

UIDelegate::UIClient::isPageVisible no longer delegates to WKUIDelegatePrivate and instead implements it based on if the WKView is hidden and is in a window.

[Simon Fraser (smfr)]

Comment on attachment 247964 [details]
Patch

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

> Source/WebCore/dom/Element.h:194
>      void scrollIntoView(bool alignToTop = true);
>      void scrollIntoViewIfNeeded(bool centerIfNeeded = true);
> +    WEBCORE_EXPORT void scrollIntoViewIfNotVisible(bool centerIfNotVisible = true);

Shame we have all these variants. Maybe one with some bit flags would be better long-term.

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:1156
> +void WebVideoFullscreenInterfaceAVKit::preparedToReturnToInline(bool visible, IntRect inlineRect)

const IntRect&

> Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:260
> +bool UIDelegate::UIClient::isPageVisible(WebKit::WebPageProxy*)
> +{
> +    return ![m_uiDelegate.m_webView isHidden] && [m_uiDelegate.m_webView window] != nil;
> +}

We already have PageClient which has isViewVisible() and isViewInWindow(). Not sure why we need new code here.

[Jeremy Jones]

Created attachment 248004 [details]
Patch

[Jeremy Jones]

(In reply to comment #13)
> Comment on attachment 247964 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=247964&action=review
> 
> > Source/WebCore/dom/Element.h:194
> >      void scrollIntoView(bool alignToTop = true);
> >      void scrollIntoViewIfNeeded(bool centerIfNeeded = true);
> > +    WEBCORE_EXPORT void scrollIntoViewIfNotVisible(bool centerIfNotVisible = true);
> 
> Shame we have all these variants. Maybe one with some bit flags would be
> better long-term.

I'll file a bug for this.

> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:1156
> > +void WebVideoFullscreenInterfaceAVKit::preparedToReturnToInline(bool visible, IntRect inlineRect)
> 
> const IntRect&

Still need to fix this one....

> 
> > Source/WebKit2/UIProcess/Cocoa/UIDelegate.mm:260
> > +bool UIDelegate::UIClient::isPageVisible(WebKit::WebPageProxy*)
> > +{
> > +    return ![m_uiDelegate.m_webView isHidden] && [m_uiDelegate.m_webView window] != nil;
> > +}
> 
> We already have PageClient which has isViewVisible() and isViewInWindow().
> Not sure why we need new code here.

Removed.

[Simon Fraser (smfr)]

Comment on attachment 248004 [details]
Patch

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

> Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.h:62
> +    void preparedToReturnToInline(bool visible, WebCore::IntRect inlineRect) override;

const IntRect&

> Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:126
> +void WebVideoFullscreenManagerProxy::preparedToReturnToInline(bool visible, WebCore::IntRect inlineRect)

const IntRect&

> Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.mm:265
> +    m_page->send(Messages::WebVideoFullscreenManagerProxy::PreparedToReturnToInline(true, clientRectForElement(m_videoElement.get())), m_page->pageID());

Not clear what 'true' means here.

[Jeremy Jones]

(In reply to comment #16)
> Comment on attachment 248004 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=248004&action=review
> 
> > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.h:62
> > +    void preparedToReturnToInline(bool visible, WebCore::IntRect inlineRect) override;
> 
> const IntRect&

Fixed.

> 
> > Source/WebKit2/UIProcess/ios/WebVideoFullscreenManagerProxy.mm:126
> > +void WebVideoFullscreenManagerProxy::preparedToReturnToInline(bool visible, WebCore::IntRect inlineRect)
> 
> const IntRect&

Fixed. And several locations.

> 
> > Source/WebKit2/WebProcess/ios/WebVideoFullscreenManager.mm:265
> > +    m_page->send(Messages::WebVideoFullscreenManagerProxy::PreparedToReturnToInline(true, clientRectForElement(m_videoElement.get())), m_page->pageID());
> 
> Not clear what 'true' means here.

'true' will be replaced by something more meaningful in a later bug fix.

[Jeremy Jones]

Created attachment 248020 [details]
Patch for landing.

[Brent Fulgham]

Still breaking Windows:

ScrollBehavior.cpp(50): error C2370: 'alignToEdgeIfNotVisible' : redefinition; different storage class (..\rendering\RenderingAllInOne.cpp)

[Brent Fulgham]

(In reply to comment #19)
> Still breaking Windows:
> 
> ScrollBehavior.cpp(50): error C2370: 'alignToEdgeIfNotVisible' :
> redefinition; different storage class (..\rendering\RenderingAllInOne.cpp)

If you declare it as WEBCORE_EXPORT in the implementation file, it needs to have the same declaration in the header file (or else Windows gets mad). I think it's usually sufficient to just add it in the header file and not do so in the implementation file.

[Jeremy Jones]

Created attachment 248027 [details]
Patch for landing.

[Jeremy Jones]

(In reply to comment #20)
> (In reply to comment #19)
> > Still breaking Windows:
> > 
> > ScrollBehavior.cpp(50): error C2370: 'alignToEdgeIfNotVisible' :
> > redefinition; different storage class (..\rendering\RenderingAllInOne.cpp)
> 
> If you declare it as WEBCORE_EXPORT in the implementation file, it needs to
> have the same declaration in the header file (or else Windows gets mad). I
> think it's usually sufficient to just add it in the header file and not do
> so in the implementation file.

I removed that WEBCORE_EXPORT as it was unnecessary.

[WebKit Commit Bot]

Comment on attachment 248027 [details]
Patch for landing.

Clearing flags on attachment: 248027

Committed r181173: <http://trac.webkit.org/changeset/181173>