Bug 105488 – [BlackBerry] Fullscreen video fixed position container horizontal position is wrong

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit BlackBerry

[Max Feil]

The fix for https://bugs.webkit.org/show_bug.cgi?id=105333 has broken fullscreen video, which was compensating by doing its own positioning in x. My patch fixes things by making vertical and horizontal handling symmetrical.

[Max Feil]

Created attachment 180258 [details]
Patch

[Max Feil]

Comment on attachment 180258 [details]
Patch

Note: for whatever reason this function was not upstreamed before, so this is my actual diff:

diff --git a/Source/WebKit/blackberry/Api/WebPage.cpp b/Source/WebKit/blackberry/Api/WebPage.cpp
index 1d62d31..e7a9553 100644
--- a/Source/WebKit/blackberry/Api/WebPage.cpp
+++ b/Source/WebKit/blackberry/Api/WebPage.cpp
@@ -5960,19 +5960,15 @@ void WebPagePrivate::adjustFullScreenElementDimensionsIfNeeded()
 
     // In order to compensate possible round errors when we scale the fullscreen
     // container element to fit to the viewport, lets make the fullscreen 1px wider
-    // than the viewport size on the right, and position it 1px position less left
+    // than the viewport size on the right, and one pixel longer at the bottom
     // of the scroll position.
     IntRect viewportRect = m_mainFrame->view()->visibleContentRect();
     int viewportWidth = viewportRect.width() + 1;
-    int viewportHeight = viewportRect.height();
-    // Given the way the BlackBerry port implements fixed position elements,
-    // we needs to set the left position ourselves, which matches the 'x' scroll
-    // position of the main frame.
-    int viewportXScrollPosition = viewportRect.x() - 1;
+    int viewportHeight = viewportRect.height() + 1;
 
     fullScreenStyle->setWidth(Length(viewportWidth, WebCore::Fixed));
     fullScreenStyle->setHeight(Length(viewportHeight, WebCore::Fixed));
-    fullScreenStyle->setLeft(Length(viewportXScrollPosition, WebCore::Fixed));
+    fullScreenStyle->setLeft(Length(0, WebCore::Fixed));
     fullScreenStyle->setTop(Length(0, WebCore::Fixed));
     fullScreenStyle->setBackgroundColor(Color::black);

[George Staikos]

Comment on attachment 180258 [details]
Patch

Looks ok to me.

[Antonio Gomes]

Comment on attachment 180258 [details]
Patch

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

> Source/WebKit/blackberry/Api/WebPage.cpp:5869
> +    fullScreenStyle->setLeft(Length(0, WebCore::Fixed));

this is needed now because blackberry respects X on fixed position now?

[Jacky Jiang]

(In reply to comment #4)
> (From update of attachment 180258 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180258&action=review
> 
> > Source/WebKit/blackberry/Api/WebPage.cpp:5869
> > +    fullScreenStyle->setLeft(Length(0, WebCore::Fixed));
> 
> this is needed now because blackberry respects X on fixed position now?

Looks like that, https://bugs.webkit.org/show_bug.cgi?id=105333.
But if we are doing so, we will also need to get rid of the fullscreen render initial style as well I think.
In WebCore RenderFullScreen.cpp:
static PassRefPtr<RenderStyle> createFullScreenStyle(FrameView* mainFrameView)
{

...
 int viewportWidth = viewportRect.width() + 1;
    int viewportHeight = viewportRect.height();
    // Given the way the BlackBerry port implements fixed position elements,
    // we needs to set the left position ourselves, which matches the 'x' scroll
    // position of the main frame.
    int viewportXScrollPosition = viewportRect.x() - 1;

    fullscreenStyle->setWidth(Length(viewportWidth, WebCore::Fixed));
    fullscreenStyle->setHeight(Length(viewportHeight, WebCore::Fixed));
    fullscreenStyle->setLeft(Length(viewportXScrollPosition, WebCore::Fixed));   // Get rid of this as well.
    fullscreenStyle->setTop(Length(0, WebCore::Fixed));
...
}

[Antonio Gomes]

> ...
>  int viewportWidth = viewportRect.width() + 1;
>     int viewportHeight = viewportRect.height();
>     // Given the way the BlackBerry port implements fixed position elements,
>     // we needs to set the left position ourselves, which matches the 'x' scroll
>     // position of the main frame.
>     int viewportXScrollPosition = viewportRect.x() - 1;
> 
>     fullscreenStyle->setWidth(Length(viewportWidth, WebCore::Fixed));
>     fullscreenStyle->setHeight(Length(viewportHeight, WebCore::Fixed));
>     fullscreenStyle->setLeft(Length(viewportXScrollPosition, WebCore::Fixed));   // Get rid of this as well.
>     fullscreenStyle->setTop(Length(0, WebCore::Fixed));
> ...
> }

Absolutely, but it is downstream code, so easier :)

[Max Feil]

(In reply to comment #5)
> But if we are doing so, we will also need to get rid of the fullscreen render initial style as well I think.

Thanks, I didn't know about that one...

[Max Feil]

(In reply to comment #7)
> (In reply to comment #5)
> > But if we are doing so, we will also need to get rid of the fullscreen render initial style as well I think.
> 
> Thanks, I didn't know about that one...

Would you guys know about any other place this was being compensated for? If you enter fullscreen while zoomed in, the touch coordinates are all shifted and its impossible to press media controls (or the press is in a different location to the left).

[Max Feil]

Ping... I need commit queue approval on this.

Thanks!

Max

[WebKit Review Bot]

Comment on attachment 180258 [details]
Patch

Clearing flags on attachment: 180258

Committed r138389: <http://trac.webkit.org/changeset/138389>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Max Feil]

(In reply to comment #8)
> Would you guys know about any other place this was being compensated for? If you enter fullscreen while zoomed in, the touch coordinates are all shifted and its impossible to press media controls (or the press is in a different location to the left).

Does anybody know the answer to this question?

[Antonio Gomes]

(In reply to comment #12)
> (In reply to comment #8)
> > Would you guys know about any other place this was being compensated for? If you enter fullscreen while zoomed in, the touch coordinates are all shifted and its impossible to press media controls (or the press is in a different location to the left).
> 
> Does anybody know the answer to this question?

That was the only method to be changed.

[Antonio Gomes]

*** Bug 91748 has been marked as a duplicate of this bug. ***

[Jacky Jiang]

*** Bug 94601 has been marked as a duplicate of this bug. ***