508495 - Embedded YouTube video's controls render as clipped

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Stephen Donner [:stephend] Not actively reading bugmail]

Summary: Embedded YouTube video's controls render as clipped

I've been seeing this for at least the past 3 days, I think.

STR:

1. Load http://lexusenthusiast.com/2009/07/29/twin-turbo-lexus-is-f-video/
2. Look at the embedded YouTube video

Expected Results:

The embedded YouTube video should look as it does elsewhere: controls rendered intact.

Actual Results:

The video-controls bar is rendered clipped.

Comment 1

[Ria Klaassen (not reading all bugmail)]

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090804 Minefield/3.6a1pre

Happens on Windows as well. With Shiretoko it looks normal.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

We need a regression range.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Regressed between the builds 090722 and 090723:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=02f8bf10f441&tochange=684dadc0726e

Eventually bug 502447? I more granular window would be helpful.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

My fault. Regressed between 0721 and 0722:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f2a58ffcd00c&tochange=02f8bf10f441

Looks like it is one of the checkins from Robert.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Regression from bug 339548.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The EMBED has borders and padding. I guess I need to fix borders and padding on plugins properly now instead of just ignoring them.

Comment 7

[(not currently active) Ted Mielczarek]

Attachment: testcase - embed with border

This is a 200x200px flash object with a 10px border. It should render as a solid 200x200px green square. (It would be easy enough to modify this testcase to work with the test plugin, but I figured this was more usable as a testcase for most people.)

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

David, if you want someone else to review this, just say the word.

This patch makes borders and padding work on plugin elements. The plugin fills the content-box.

-- Add an mInnerView for nsObjectFrames to hold the plugin. We need this because the plugin's widget can dispatch events through its associated view, and the view needs to be aligned to the content-box. Adding views sucks, but for now it seems necessary.

-- Size and position the plugin widget based on the content-box.

-- Windowless plugin painting needs to set things up to draw into the content-box.

-- When we translate event coordinates to send to the windowless plugin, we need to subtract off the border+padding.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The nsViewManager change is to avoid an assert when you set bounds with nonzero origin when creating a view.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: addtional fix for printing

Additional fixes to use the right size when printing a plugin with borders or padding.

I'm also removing two chunks of printing code: Unix code that no longer works because we don't support EPS (and is already #if 0), and fallback code that's used if !defined(XP_MACOSX) && !defined(XP_UNIX) && !defined(XP_WIN) && !defined(XP_OS2) --- i.e. it's never really used.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #8)
> -- Add an mInnerView for nsObjectFrames to hold the plugin. We need this
> because the plugin's widget can dispatch events through its associated view,
> and the view needs to be aligned to the content-box. Adding views sucks, but
> for now it seems necessary.

Can we remove nsObjectFrame::NeedsView to compensate?

Also, while investigating that, I think it looks like nsIFrame::CreateWidgetForView and nsObjectFrame::CreateWidgetForView are completely unused; only nsMenuPopupFrame::CreateWidgetForView appears to still be called.  The old calls in nsFrame.cpp and nsObjectFrame.cpp seem to have been removed since 1.9.1; I'd suspect relatively recently (compositor phase 1?).

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Also, might be good to test either an rgba(), dotted, or transparent border in addition to your current tests so that you're testing what's drawn in the border area too.  Though you are testing padding, so it's probably ok.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #11)
> (In reply to comment #8)
> > -- Add an mInnerView for nsObjectFrames to hold the plugin. We need this
> > because the plugin's widget can dispatch events through its associated view,
> > and the view needs to be aligned to the content-box. Adding views sucks, but
> > for now it seems necessary.
> 
> Can we remove nsObjectFrame::NeedsView to compensate?

No, our view-management code would fail to find mInnerView and so it wouldn't get moved due to ancestor frame reflow etc.

> Also, while investigating that, I think it looks like
> nsIFrame::CreateWidgetForView and nsObjectFrame::CreateWidgetForView are
> completely unused; only nsMenuPopupFrame::CreateWidgetForView appears to still
> be called.  The old calls in nsFrame.cpp and nsObjectFrame.cpp seem to have
> been removed since 1.9.1; I'd suspect relatively recently (compositor phase
> 1?).

I just noticed this myself! I'll remove them.

(In reply to comment #12)
> Also, might be good to test either an rgba(), dotted, or transparent border in
> addition to your current tests so that you're testing what's drawn in the
> border area too.  Though you are testing padding, so it's probably ok.

I'll just make my borders in the existing tests dotted, that should cover it.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 399176 [details] [diff] [review]
fix

Is there a reason nsObjectFrame::CreateWidget sets the size of the frame's
view?  Should you do the same for the inner view?

>+  origin += GetContentRect().TopLeft() - GetPosition();

It's a bit less work to just do:
nsMargin bp = GetUsedBorderAndPadding();
origin += nsPoint(bp.top, bp.left);

(Maybe even add a TopLeft() helper to nsMargin?)

Thought that does assume nsObjectFrame::GetSkipSides always returns 0;
I think that's a fair assumption but it's worth commenting.

Same in PaintPrintPlugin.  and nsPluginInstanceOwner::ProcessEventX11Composited
(three times).  Maybe make a function for it?


r=dbaron

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'll make a function for it.

> Is there a reason nsObjectFrame::CreateWidget sets the size of the frame's
> view?  Should you do the same for the inner view?

To make sure that the widget gets created with the right initial geometry. I guess I should do that for the inner view now.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #15)
> To make sure that the widget gets created with the right initial geometry. I
> guess I should do that for the inner view now.

Oops, I already do when it's created.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/29e974a88602
http://hg.mozilla.org/mozilla-central/rev/ac37ce54ddb0

Removing CreateWidgetAndView:
http://hg.mozilla.org/mozilla-central/rev/5cc6bebd0dd9

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Roc, when I want to print the page given by comment 0 the embedded video renders too small and doesn't fit into the surrounding frame on OS X. Is this a flash problem or our fault?

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't know, file a new bug for that.

Comment 20

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED on trunk using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.3a1pre) Gecko/20091006 Minefield/3.7a1pre.

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #18)
> Roc, when I want to print the page given by comment 0 the embedded video
> renders too small and doesn't fit into the surrounding frame on OS X. Is this a
> flash problem or our fault?

Steven, is this the same thing as what we have seen on bug 191046? I know I stumble over it again and again. Is a new bug needed or can't we fix that?

Comment 22

[Chris Pearce [:cpearce (Not reading bugmail)]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/0d343588f1c1

Comment 23

[Chris Pearce [:cpearce (Not reading bugmail)]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/3983225cd60c
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/748e664edc71

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2b1pre) Gecko/20091007 Namoroka/3.6b1pre ID:20091007034618