95650 2012-09-01 18:01:42 -0700 REGRESSION (r123837): Full screen transition is broken at apple.com 2013-04-09 16:56:17 -0700 1 1 1 Unclassified WebKit CSS 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED http://www.apple.com/macbook-pro/#macbookpro InRadar, Regression P1 Major --- 1 mitz jer.noble esprehn+autocc jer.noble jonlee macpherson menard mikelawther ojan.autocc simon.fraser webkit-bug-importer webkit.review.bot oldest_to_newest 710300 0 mitz 2012-09-01 18:01:42 -0700 To reproduce: navigate to the URL. When the video starts playing, click the full screen arrows in the playback controls. Observe the transition into full-screen playback. Press Esc to exit full screen mode and observe the transition. Rather than animate smoothly, the video content scale changes instantaneously, while remaining clipped to its original bounds. Its position the animates to the center of the screen, without resizing. Then the size changes abruptly. This was caused by <http://trac.webkit.org/r123837>, the fix for bug 92220. 710302 1 mitz 2012-09-01 18:02:20 -0700 <rdar://problem/12222130> 710441 2 mikelawther 2012-09-03 00:07:43 -0700 I can reproduce this, but only with Safari. I tested with the shipping Safari 6.0 (7536.25) on MacOS 10.7, and with a locally built WebKit synced past r123837. The transition to fullscreen is transitioning from a fixed width,height of 848,480 to a a percent based width,height of 100%,100%. I have not dived into the fullscreen or video player code, but I'm assuming 100% in this case is the extent of the monitor size. From what I see, the blend of these is performed correctly - my monitor is 2560x1600 and the values appear correct, eg: eval'd blending of (type Fixed, value 848.000000) to (type Percent, value 100.000000) at 13.0802% = 1071.932983px eval'd blending of (type Fixed, value 480.000000) to (type Percent, value 100.000000) at 13.0802% = 626.498230px [...] eval'd blending of (type Fixed, value 848.000000) to (type Percent, value 100.000000) at 84.5118% = 2294.842529px eval'd blending of (type Fixed, value 480.000000) to (type Percent, value 100.000000) at 84.5118% = 1426.532471px Going back out of fullscreen, the numbers don't look the same, eg: eval'd blending of (type Percent, value 100.000000) to (type Fixed, value 848.000000) at 45.7933% = 848.000000px eval'd blending of (type percent, value 100.000000) to (type Fixed, value 480.000000) at 45.7933% = 480.000000px [...] eval'd blending of (type Percent, value 100.000000) to (type Fixed, value 848.000000) at 78.0345% = 848.000000px eval'd blending of (type Percent, value 100.000000) to (type Fixed, value 480.000000) at 78.0345% = 480.000000px It appears the container has already been resized to 848x480, and so the blended value is always 848,480. 732764 3 mitz 2012-10-02 10:03:29 -0700 Mike, do you have any thoughts on how to address this regression? Should we just revert r123837? 733214 4 mikelawther 2012-10-02 16:41:04 -0700 From what I can see in the digging I did above, the smooth transition itself is working correctly. My gut feel is that there is a workaround in the existing fullscreen code to make the transition smooth. Now that the underlying transition is actually smooth, the assumptions in the workaround are no longer valid, leading to weirdness. @jer.noble - does that sound likely? If not, I suggest we: - revert r123837 (it's a dead simple patch) - someone helps me write a test for the existing fullscreen transition, and we land this - re-land the smooth transition patch, making sure fullscreen doesn't break How does that sound? 733251 5 jer.noble 2012-10-02 17:21:53 -0700 (In reply to comment #4) > From what I can see in the digging I did above, the smooth transition itself is working correctly. > > My gut feel is that there is a workaround in the existing fullscreen code to make the transition smooth. Now that the underlying transition is actually smooth, the assumptions in the workaround are no longer valid, leading to weirdness. > > @jer.noble - does that sound likely? The blending is happening at the window server level. We resize the view to take up the entire screen, then ask for the element's screen rect. The window server then interpolates from the original screen rect to the final one. Before r123837, the full screen element's renderer's contentsBox() returned the full screen rect. After r123837, the full screen element's renderer's contentsBox() returns the original box. > If not, I suggest we: > > - revert r123837 (it's a dead simple patch) > - someone helps me write a test for the existing fullscreen transition, and we land this There are some helper functions in Internals that let you do processing between when full screen starts and ends. That would be a good time to check the bounds of the full screen element. 733296 6 mikelawther 2012-10-02 18:25:55 -0700 > The blending is happening at the window server level. We resize the view to take up the entire screen, then ask for the element's screen rect. The window server then interpolates from the original screen rect to the final one. > > Before r123837, the full screen element's renderer's contentsBox() returned the full screen rect. > After r123837, the full screen element's renderer's contentsBox() returns the original box. This sounds like the assumption that has been violated. Prior to r123837, the transition between a fixed size and a percent would 'snap' instantly to the final state. Is the fix here is to simply remove the CSS transition upon the view resize? It sounds like you actually want it to be instantaneous. 756709 7 mikelawther 2012-11-01 16:21:19 -0700 Hey Dan, Jer - did you try removing the CSS transition on the view resize? It really feels like this is the right solution here, as Jer points out in this case the blending is happening at the window server level. 756718 8 jer.noble 2012-11-01 16:30:52 -0700 (In reply to comment #7) > Hey Dan, Jer - did you try removing the CSS transition on the view resize? It really feels like this is the right solution here, as Jer points out in this case the blending is happening at the window server level. I'm confused. Is there a CSS transition which is being added to the page through a style sheet? 756746 9 mikelawther 2012-11-01 17:16:52 -0700 There is definitely a CSS transition happening there, evidenced both by what you describe, and the measurements I took in comment #2. It's a transition from a fixed size (eg a pixel size of 848x480) to a percentage size (100%x100%). This is the assumption that I reckon has been violated - prior to r123837, a mixed unit transition would instantly 'snap' from begin to end, with no interpolation. And so the window server code provides the interpolation instead (I'm guessing animateFromRect in Source/WebCore/platform/mac/WebVideoFullscreenController.mm). Now, the CSS transition actually interpolates as well, and there are too many cooks in the animation kitchen. I'm afraid I don't know where the CSS transition is coming from - I had a quick dig around in likely sounding culprits such as Source/WebCore/css/fullscreenQuickTime.css. My Mac dev machine is currently hosed, so I can't check any of this stuff :( 870073 10 196681 jer.noble 2013-04-05 14:17:27 -0700 Created attachment 196681 Patch 872367 11 jer.noble 2013-04-09 16:56:17 -0700 Committed r148065: <http://trac.webkit.org/changeset/148065> 196681 2013-04-05 14:17:27 -0700 2013-04-05 14:43:06 -0700 Patch bug-95650-20130405141714.patch text/plain 1489 jer.noble U3VidmVyc2lvbiBSZXZpc2lvbjogMTQ3NjYzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNDk0MGRkYzc3ZWIwNTYx ZTUzOTE3NzcyODNkZTU2NmU1MzRiZmY0NS4uYTFlM2ViNjE5YjgzOGEyYzM1YTZmZWEyYzdmYzQ1 ZGNjZDY5NzE1MyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSw1ICsxLDE5IEBACiAyMDEzLTA0LTA1ICBKZXIg Tm9ibGUgIDxqZXIubm9ibGVAYXBwbGUuY29tPgogCisgICAgICAgIFJFR1JFU1NJT04gKHIxMjM4 MzcpOiBGdWxsIHNjcmVlbiB0cmFuc2l0aW9uIGlzIGJyb2tlbiBhdCBhcHBsZS5jb20KKyAgICAg ICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTk1NjUwCisKKyAgICAg ICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgQ2FuY2VsIGFueSBvdXRz dGFuZGluZyBhbmltYXRpb25zIG9uIDx2aWRlbz4gZWxlbWVudHMgYXMgdGhleSBlbnRlciBmdWxs IHNjcmVlbiwgc28gYXMgdG8KKyAgICAgICAgbm90IGNvbmZ1c2UgdGhlIFdlYktpdC9XZWJLaXQy IGZ1bGwgc2NyZWVuIHdpbmRvdyBhbmltYXRpb24gYWJvdXQgdGhlIHN0YXJ0aW5nIGFuZCBkZXN0 aW5hdGlvbgorICAgICAgICBzY3JlZW4gcmVjdHMuCisKKyAgICAgICAgKiBjc3MvZnVsbHNjcmVl bi5jc3M6CisgICAgICAgICh2aWRlbzotd2Via2l0LWZ1bGwtc2NyZWVuLCBhdWRpbzotd2Via2l0 LWZ1bGwtc2NyZWVuKToKKworMjAxMy0wNC0wNSAgSmVyIE5vYmxlICA8amVyLm5vYmxlQGFwcGxl LmNvbT4KKwogICAgICAgICBoYW5nIGluIG1lZGlhU2VsZWN0aW9uR3JvdXBGb3JNZWRpYUNoYXJh Y3RlcmlzdGljCiAgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9p ZD0xMTQwNTQKIApkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvY3NzL2Z1bGxzY3JlZW4uY3Nz IGIvU291cmNlL1dlYkNvcmUvY3NzL2Z1bGxzY3JlZW4uY3NzCmluZGV4IGY2YzJiM2Y3MzUxYzVl OTllZDBhMTAyMzY2Y2E3M2YzZTQ1OGQ2YjUuLjYwN2JiMzViMDNlZDU3NzQ3NjYwOTEwZTkzMTJl MjUyMGFjYzI4NzIgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL2Nzcy9mdWxsc2NyZWVuLmNz cworKysgYi9Tb3VyY2UvV2ViQ29yZS9jc3MvZnVsbHNjcmVlbi5jc3MKQEAgLTI5LDYgKzI5LDcg QEAgdmlkZW86LXdlYmtpdC1mdWxsLXNjcmVlbiwgYXVkaW86LXdlYmtpdC1mdWxsLXNjcmVlbiB7 CiAgICAgd2lkdGg6IDEwMCUgIWltcG9ydGFudDsKICAgICAtd2Via2l0LWZsZXg6IDEgIWltcG9y dGFudDsKICAgICBkaXNwbGF5OiBibG9jayAhaW1wb3J0YW50OworICAgIC13ZWJraXQtdHJhbnNp dGlvbjogbm9uZSAhaW1wb3J0YW50OwogfQogCiA6LXdlYmtpdC1mdWxsLXNjcmVlbiB2aWRlbzpo b3ZlciB7Cg==