105488 2012-12-19 18:13:06 -0800 [BlackBerry] Fullscreen video fixed position container horizontal position is wrong 2013-01-08 12:25:11 -0800 1 1 1 Unclassified WebKit WebKit BlackBerry 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 mfeil mfeil jkjiang mifenton rwlbuis tonikitoo webkit.review.bot yong.li.webkit oldest_to_newest 795082 0 mfeil 2012-12-19 18:13:06 -0800 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. 795086 1 180258 mfeil 2012-12-19 18:24:24 -0800 Created attachment 180258 Patch 795087 2 180258 mfeil 2012-12-19 18:26:01 -0800 Comment on attachment 180258 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); 795377 3 180258 staikos 2012-12-20 05:09:01 -0800 Comment on attachment 180258 Patch Looks ok to me. 795399 4 180258 tonikitoo 2012-12-20 06:18:14 -0800 Comment on attachment 180258 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? 795442 5 jkjiang 2012-12-20 07:53:43 -0800 (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)); ... } 795444 6 tonikitoo 2012-12-20 07:57:57 -0800 > ... > 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 :) 795636 7 mfeil 2012-12-20 12:05:13 -0800 (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... 795684 8 mfeil 2012-12-20 12:46:18 -0800 (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). 796364 9 mfeil 2012-12-21 10:29:56 -0800 Ping... I need commit queue approval on this. Thanks! Max 796381 10 180258 webkit.review.bot 2012-12-21 10:58:54 -0800 Comment on attachment 180258 Patch Clearing flags on attachment: 180258 Committed r138389: <http://trac.webkit.org/changeset/138389> 796382 11 webkit.review.bot 2012-12-21 10:58:58 -0800 All reviewed patches have been landed. Closing bug. 798001 12 mfeil 2012-12-28 14:38:39 -0800 (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? 798026 13 tonikitoo 2012-12-28 19:54:01 -0800 (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. 802476 14 tonikitoo 2013-01-08 12:09:31 -0800 *** Bug 91748 has been marked as a duplicate of this bug. *** 802502 15 jkjiang 2013-01-08 12:25:11 -0800 *** Bug 94601 has been marked as a duplicate of this bug. *** 180258 2012-12-19 18:24:24 -0800 2012-12-21 10:58:54 -0800 Patch bug-105488-20121219212140.patch text/plain 3306 mfeil U3VidmVyc2lvbiBSZXZpc2lvbjogMTM4MjEyCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L2Js YWNrYmVycnkvQ2hhbmdlTG9nIGIvU291cmNlL1dlYktpdC9ibGFja2JlcnJ5L0NoYW5nZUxvZwpp bmRleCBlZGRjMzQ0OWQ0ZTA5MzdlMTM3ODcwZTFkZTEwOTRiYjRjMjU5MTc5Li40MmEwNGRjZWM0 NjJiYjY3N2YyNGQ1NmFlMzBhYWMzZTM5NTNjMTRjIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0 L2JsYWNrYmVycnkvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XZWJLaXQvYmxhY2tiZXJyeS9DaGFu Z2VMb2cKQEAgLTEsMyArMSwyMSBAQAorMjAxMi0xMi0xOSAgTWF4IEZlaWwgIDxtZmVpbEByaW0u Y29tPgorCisgICAgICAgIFtCbGFja0JlcnJ5XSBGdWxsc2NyZWVuIHZpZGVvIGZpeGVkIHBvc2l0 aW9uIGNvbnRhaW5lciBob3Jpem9udGFsIHBvc2l0aW9uIGlzIHdyb25nCisgICAgICAgIGh0dHBz Oi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMDU0ODgKKworICAgICAgICBSZXZp ZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBUaGUgZml4IGZvciBodHRwczovL2J1 Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTA1MzMzIGhhcworICAgICAgICBicm9rZW4g ZnVsbHNjcmVlbiB2aWRlbywgd2hpY2ggd2FzIGNvbXBlbnNhdGluZyBieSBkb2luZyBpdHMKKyAg ICAgICAgb3duIHBvc2l0aW9uaW5nIGluIHguIE15IHBhdGNoIGZpeGVzIHRoaW5ncyBieSBtYWtp bmcgdmVydGljYWwKKyAgICAgICAgYW5kIGhvcml6b250YWwgaGFuZGxpbmcgc3ltbWV0cmljYWwu CisgICAgICAgIE5PVEU6IFRoaXMgZnVuY3Rpb24gd2FzIG5vdCB1cHN0cmVhbWVkIGJlZm9yZSwg c28geW91IGNhbid0CisgICAgICAgIGFjdHVhbGx5IHNlZSBteSBkaWZmLiBTZWUgdGhlIGJ1Zy4K KworICAgICAgICAqIEFwaS9XZWJQYWdlLmNwcDoKKyAgICAgICAgKFdlYktpdCk6CisgICAgICAg IChCbGFja0JlcnJ5OjpXZWJLaXQ6OldlYlBhZ2VQcml2YXRlOjphZGp1c3RGdWxsU2NyZWVuRWxl bWVudERpbWVuc2lvbnNJZk5lZWRlZCk6CisKIDIwMTItMTItMTggIE5pbWEgR2hhbmF2YXRpYW4g IDxuZ2hhbmF2YXRpYW5AcmltLmNvbT4KIAogICAgICAgICBbQmxhY2tCZXJyeV0gQ2FsY3VsYXRl IGNvcnJlY3Qgd29yZCBvZmZzZXRzIGZvciBmb3JtIGVsZW1lbnRzLgpkaWZmIC0tZ2l0IGEvU291 cmNlL1dlYktpdC9ibGFja2JlcnJ5L0FwaS9XZWJQYWdlLmNwcCBiL1NvdXJjZS9XZWJLaXQvYmxh Y2tiZXJyeS9BcGkvV2ViUGFnZS5jcHAKaW5kZXggMDdhODRmODZiNDU1MzZiZjQ1MmI3YTUyMDFh MzIyYTEzMGY5NzU4Ny4uY2VkMDQ5Mzg0MmVkZmMwZGQ1NTFlY2Y5YTBhN2ZlMThlNWEzZGM3YSAx MDA2NDQKLS0tIGEvU291cmNlL1dlYktpdC9ibGFja2JlcnJ5L0FwaS9XZWJQYWdlLmNwcAorKysg Yi9Tb3VyY2UvV2ViS2l0L2JsYWNrYmVycnkvQXBpL1dlYlBhZ2UuY3BwCkBAIC01ODM5LDYgKzU4 MzksNDAgQEAgdm9pZCBXZWJQYWdlUHJpdmF0ZTo6ZXhpdEZ1bGxTY3JlZW5Gb3JFbGVtZW50KEVs ZW1lbnQqIGVsZW1lbnQpCiAgICAgfQogI2VuZGlmCiB9CisKKy8vIEZJWE1FOiBNb3ZlIHRoaXMg bWV0aG9kIHRvIFdlYkNvcmUuCit2b2lkIFdlYlBhZ2VQcml2YXRlOjphZGp1c3RGdWxsU2NyZWVu RWxlbWVudERpbWVuc2lvbnNJZk5lZWRlZCgpCit7CisgICAgLy8gSWYgd2UgYXJlIGluIGZ1bGxz Y3JlZW4gdmlkZW8gbW9kZSwgYW5kIHdlIGNoYW5nZSB0aGUgRnJhbWVWaWV3Ojp2aWV3cG9ydFJl Y3QsCisgICAgLy8gd2UgbmVlZCB0byBhZGp1c3QgdGhlIG1lZGlhIGNvbnRhaW5lciB0byB0aGUg bmV3IHNpemUuCisgICAgaWYgKCFtX2Z1bGxzY3JlZW5WaWRlb05vZGUgfHwgIW1fZnVsbHNjcmVl blZpZGVvTm9kZS0+cmVuZGVyZXIoKQorICAgICAgICB8fCAhbV9mdWxsc2NyZWVuVmlkZW9Ob2Rl LT5kb2N1bWVudCgpIHx8ICFtX2Z1bGxzY3JlZW5WaWRlb05vZGUtPmRvY3VtZW50KCktPmZ1bGxT Y3JlZW5SZW5kZXJlcigpKQorICAgICAgICByZXR1cm47CisKKyAgICBBU1NFUlQobV9mdWxsc2Ny ZWVuVmlkZW9Ob2RlLT5pc0VsZW1lbnROb2RlKCkpOworICAgIEFTU0VSVChzdGF0aWNfY2FzdDxF bGVtZW50Kj4obV9mdWxsc2NyZWVuVmlkZW9Ob2RlLmdldCgpKS0+aXNNZWRpYUVsZW1lbnQoKSk7 CisKKyAgICBEb2N1bWVudCogZG9jdW1lbnQgPSBtX2Z1bGxzY3JlZW5WaWRlb05vZGUtPmRvY3Vt ZW50KCk7CisgICAgUmVuZGVyU3R5bGUqIGZ1bGxTY3JlZW5TdHlsZSA9IGRvY3VtZW50LT5mdWxs U2NyZWVuUmVuZGVyZXIoKS0+c3R5bGUoKTsKKyAgICBBU1NFUlQoZnVsbFNjcmVlblN0eWxlKTsK KworICAgIC8vIEluIG9yZGVyIHRvIGNvbXBlbnNhdGUgcG9zc2libGUgcm91bmQgZXJyb3JzIHdo ZW4gd2Ugc2NhbGUgdGhlIGZ1bGxzY3JlZW4KKyAgICAvLyBjb250YWluZXIgZWxlbWVudCB0byBm aXQgdG8gdGhlIHZpZXdwb3J0LCBsZXRzIG1ha2UgdGhlIGZ1bGxzY3JlZW4gMXB4IHdpZGVyCisg ICAgLy8gdGhhbiB0aGUgdmlld3BvcnQgc2l6ZSBvbiB0aGUgcmlnaHQsIGFuZCBvbmUgcGl4ZWwg bG9uZ2VyIGF0IHRoZSBib3R0b20KKyAgICAvLyBvZiB0aGUgc2Nyb2xsIHBvc2l0aW9uLgorICAg IEludFJlY3Qgdmlld3BvcnRSZWN0ID0gbV9tYWluRnJhbWUtPnZpZXcoKS0+dmlzaWJsZUNvbnRl bnRSZWN0KCk7CisgICAgaW50IHZpZXdwb3J0V2lkdGggPSB2aWV3cG9ydFJlY3Qud2lkdGgoKSAr IDE7CisgICAgaW50IHZpZXdwb3J0SGVpZ2h0ID0gdmlld3BvcnRSZWN0LmhlaWdodCgpICsgMTsK KworICAgIGZ1bGxTY3JlZW5TdHlsZS0+c2V0V2lkdGgoTGVuZ3RoKHZpZXdwb3J0V2lkdGgsIFdl YkNvcmU6OkZpeGVkKSk7CisgICAgZnVsbFNjcmVlblN0eWxlLT5zZXRIZWlnaHQoTGVuZ3RoKHZp ZXdwb3J0SGVpZ2h0LCBXZWJDb3JlOjpGaXhlZCkpOworICAgIGZ1bGxTY3JlZW5TdHlsZS0+c2V0 TGVmdChMZW5ndGgoMCwgV2ViQ29yZTo6Rml4ZWQpKTsKKyAgICBmdWxsU2NyZWVuU3R5bGUtPnNl dFRvcChMZW5ndGgoMCwgV2ViQ29yZTo6Rml4ZWQpKTsKKyAgICBmdWxsU2NyZWVuU3R5bGUtPnNl dEJhY2tncm91bmRDb2xvcihDb2xvcjo6YmxhY2spOworCisgICAgZG9jdW1lbnQtPmZ1bGxTY3Jl ZW5SZW5kZXJlcigpLT5zZXROZWVkc0xheW91dEFuZFByZWZXaWR0aHNSZWNhbGMoKTsKKyAgICBk b2N1bWVudC0+cmVjYWxjU3R5bGUoTm9kZTo6Rm9yY2UpOworfQogI2VuZGlmCiAKIHZvaWQgV2Vi UGFnZVByaXZhdGU6OmRpZENoYW5nZVNldHRpbmdzKFdlYlNldHRpbmdzKiB3ZWJTZXR0aW5ncykK