Bug 61911 – REGRESSION: Page layout messed up after exiting full screen after video ends at jerryseinfeld.com

• Component: Media
• Status: REOPENED

[Jer Noble]

REGRESSION: Page layout messed up after exiting full screen after video ends at jerryseinfeld.com

[Jer Noble]

1. Navigate to <http://jerryseinfeld.com/>
2. Click on one of the video titles
3. When the video starts playing, click the full screen button
4. Wait for the video to end
5. Click the exit full screen button in the HUD


The video (all black) shrinks back into place, then the page appears to relayout (see <https://bugs.webkit.org/show_bug.cgi?id=61897>), ending in a broken state where it’s showing a frame from the middle of the video stretched beyond the original bounds of the video element.

[Jer Noble]

<rdar://problem/9523017>

[Jer Noble]

This is a problem for all users of VideoJS (2.0.2).

[Jer Noble]

We're going to have to lie to the callers of originWidth and originHeight (as well as Left and Top, just to be sure).

[Jer Noble]

Created attachment 95731 [details]
Patch

[Jer Noble]

Created attachment 95733 [details]
Patch

Added radar URL; moved some code between the two patches to allow them to apply cleanly.

[WebKit Review Bot]

Comment on attachment 95733 [details]
Patch

Attachment 95733 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8760065

[Collabora GTK+ EWS bot]

Comment on attachment 95733 [details]
Patch

Attachment 95733 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8758337

[WebKit Review Bot]

Comment on attachment 95733 [details]
Patch

Attachment 95733 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8755788

[WebKit Review Bot]

Comment on attachment 95733 [details]
Patch

Attachment 95733 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8754734

[Eric Seidel (no email)]

Comment on attachment 95733 [details]
Patch

Looks like thsi doesn't build.

[Jer Noble]

Comment on attachment 95733 [details]
Patch

This patch depends on the patch in bug #61897.  Can the EWS bots not track bug dependencies?  Setting back to r?, but leaving the cq-.

[Jeff Miller]

Comment on attachment 95733 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.h:47
> +class RenderBlock;

Why did you need to forward declare RenderBlock? I don't see this class referenced anywhere in your other changes to this header.

[Jer Noble]

Comment on attachment 95733 [details]
Patch

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

>> Source/WebCore/html/HTMLMediaElement.h:47
>> +class RenderBlock;
> 
> Why did you need to forward declare RenderBlock? I don't see this class referenced anywhere in your other changes to this header.

Ah, good catch.  I forgot to take this out before uploading.  I previously had made the elementPlaceholder() function a class-static, but changed it to a file-static function.  When I removed the declaration from the header, I forgot to take out the forward declaration of RenderBlock.  I'll remove this.

[Maciej Stachowiak]

Comment on attachment 95733 [details]
Patch

According to the bots this seems to break lots of builds. Please revise to fix that.

[Jer Noble]

Created attachment 95941 [details]
Patch

Re-uploading the same patch (with Jeff's comments addressed) to get the EWS bots chewing on it now that the fix for bug #61897 has landed.

[Simon Fraser (smfr)]

Comment on attachment 95941 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.h:210
> +    // Override Element implementations
> +    int offsetLeft();
> +    int offsetTop();
> +    int offsetWidth();
> +    int offsetHeight();

This approach seems pretty fragile. What if we add code elsewhere in HTMLMediaElement or subclasses that calls offsetLeft etc?

[Maciej Stachowiak]

Comment on attachment 95941 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.idl:90
> +    // Override Element functions
> +    readonly attribute long offsetLeft;
> +    readonly attribute long offsetTop;
> +    readonly attribute long offsetWidth;
> +    readonly attribute long offsetHeight;
> +

This seems wrong to me. The right way to override is to make the methods virtual and override them at the C++ level. IDL overrides are not needed.

[Maciej Stachowiak]

Comment on attachment 95941 [details]
Patch

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

r- due to previous comment about additions to the IDL.

>> Source/WebCore/html/HTMLMediaElement.h:210
>> +    int offsetHeight();
> 
> This approach seems pretty fragile. What if we add code elsewhere in HTMLMediaElement or subclasses that calls offsetLeft etc?

If these are virtual overrides, then this seems ok unless we have code that specifically needs the "true" offsetLeft/Top/Height/Width. However, I believe these methods exist almost entirely to implement the relevant JS API. So I think this is ok.

[Jer Noble]

Comment on attachment 95941 [details]
Patch

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

>>> Source/WebCore/html/HTMLMediaElement.h:210

>> 
>> This approach seems pretty fragile. What if we add code elsewhere in HTMLMediaElement or subclasses that calls offsetLeft etc?
> 
> If these are virtual overrides, then this seems ok unless we have code that specifically needs the "true" offsetLeft/Top/Height/Width. However, I believe these methods exist almost entirely to implement the relevant JS API. So I think this is ok.

If that's the case; the reason I wrote this patch this way was that the offset functions would continue to return the "truth" unless you ask the media element directly, like the JS API does. But if these functions only exist to feed the JS API, then that's unnecessary.  I'll make these functions virtual instead.

[Maciej Stachowiak]

(In reply to comment #20)
> (From update of attachment 95941 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95941&action=review
> 
> >>> Source/WebCore/html/HTMLMediaElement.h:210
> 
> >> 
> >> This approach seems pretty fragile. What if we add code elsewhere in HTMLMediaElement or subclasses that calls offsetLeft etc?
> > 
> > If these are virtual overrides, then this seems ok unless we have code that specifically needs the "true" offsetLeft/Top/Height/Width. However, I believe these methods exist almost entirely to implement the relevant JS API. So I think this is ok.
> 
> If that's the case; the reason I wrote this patch this way was that the offset functions would continue to return the "truth" unless you ask the media element directly, like the JS API does. But if these functions only exist to feed the JS API, then that's unnecessary.  I'll make these functions virtual instead.

It's really dangerous to have non-virtual overrides that differ significantly in behavior. It means just down casting or upcasting the pointer can change the behavior of methods, which is pretty risky. If the vanilla offsetLeft/Top/Height/Width is really needed somewhere then it would be better to create separate non virtual methods with those names.

[Jer Noble]

Created attachment 96453 [details]
Patch

Made the offset functions in Element and HTMLMediaElement virtual.

[Maciej Stachowiak]

The latest patch looks good to me. Two more requests before I sign off:

1) Please search for all uses of offsetLeft, offsetTop, offsetRight, offsetBottom and see if the new variable-behavior version is reasonable for that case. It looks to me like there are only a few uses (outside of JS code).

2) Let's run through the full-screen video test plan.

[Jer Noble]

(In reply to comment #23)
> The latest patch looks good to me. Two more requests before I sign off:
> 
> 1) Please search for all uses of offsetLeft, offsetTop, offsetRight, offsetBottom and see if the new variable-behavior version is reasonable for that case. It looks to me like there are only a few uses (outside of JS code).

One use:  In SpatialNavigation.cpp.  offsetLeft and offsetTop are used for calculating the absolute position in a node, meaning in full-screen this calculation will be off for media elements and their children.

Second use: In PrintContext.cpp.  pageNumberForElement uses offsetLeft and offsetTop to determine the page number for a given element.  Printing in full-screen mode might lead to wrong page numbers.

[Maciej Stachowiak]

Comment on attachment 96453 [details]
Patch

Given the code inspection and testing, r=me

[Jer Noble]

Committed r88468: <http://trac.webkit.org/changeset/88468>

[Csaba Osztrogonác]

(In reply to comment #26)
> Committed r88468: <http://trac.webkit.org/changeset/88468>

It broke Qt build. :S Could you fix it, or should we roll it out?

Reopen to fix the build.

[Csaba Osztrogonác]

Comment on attachment 96453 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.cpp:2724
> +    RenderFullScreen* fullScreen = parent && parent->isRenderFullScreen() ? toRenderFullScreen(parent) : 0;

isRenderFullScreen is defined inside ENABLE(FULLSCREEN_API) block, 
but it is used outside of ENABLE(FULLSCREEN_API) block, that's why
this patch broke Qt build.

[Csaba Osztrogonác]

Rolled out by http://trac.webkit.org/changeset/88490

[Heff]

I'm trying to figure out if this is related to an issue I'm having. I'm finding the element that requests fullscreen doesn't report zero for offsetLeft while in fullscreen, as I would expect.

The use case is building a draggable currentTime/progress bar. When the user is dragging (mousemove), I need to calculate the event's pageX in relation to the x value of the progress bar, which is inside the fullscreen element. I use the following function to get the x value.

  findPosX: function(obj) {
    var curleft = obj.offsetLeft;
    while(obj = obj.offsetParent) {
      curleft += obj.offsetLeft;
    }
    return curleft;
  }

In the chain of parent objects is the fullscreen element, which compared to the event's x should have an offsetleft of 0, but doesn't.

http://videojs.com/offsetleft/dev.html
If you go to this link, go into fullscreen, click somewhere on the timeline (nothing will happen), exit fullscreen, then pull up the console, you'll see offsetleft is reported as 520. The element's offset left is 285 when not in fullscreen, so I'm not sure where the 520 is coming from. If you play the video, go into fullscreen, and drag on the timeline you can see the result. The progress bar + handle lag 520px behind the cursor.

Is this related?

-Steve Heffernan (VideoJS)

[Heff]

Switched to using getBoundingClientRect which works as expected, so no longer an issue for me.

(In reply to comment #30)
> I'm trying to figure out if this is related to an issue I'm having. I'm finding the element that requests fullscreen doesn't report zero for offsetLeft while in fullscreen, as I would expect.
> 
> The use case is building a draggable currentTime/progress bar. When the user is dragging (mousemove), I need to calculate the event's pageX in relation to the x value of the progress bar, which is inside the fullscreen element. I use the following function to get the x value.
> 
>   findPosX: function(obj) {
>     var curleft = obj.offsetLeft;
>     while(obj = obj.offsetParent) {
>       curleft += obj.offsetLeft;
>     }
>     return curleft;
>   }
> 
> In the chain of parent objects is the fullscreen element, which compared to the event's x should have an offsetleft of 0, but doesn't.
> 
> http://videojs.com/offsetleft/dev.html
> If you go to this link, go into fullscreen, click somewhere on the timeline (nothing will happen), exit fullscreen, then pull up the console, you'll see offsetleft is reported as 520. The element's offset left is 285 when not in fullscreen, so I'm not sure where the 520 is coming from. If you play the video, go into fullscreen, and drag on the timeline you can see the result. The progress bar + handle lag 520px behind the cursor.
> 
> Is this related?
> 
> -Steve Heffernan (VideoJS)

(In reply to comment #30)

I'm having the same problem - offsetTop and offsetLeft are really messed up when in full-screen in Safari, and slightly wrong in Chrome. Firefox doesn't have this problem.