Closed Bug 508495 Opened 11 years ago Closed 11 years ago

Embedded YouTube video's controls render as clipped

Categories

(Core :: Layout, defect, P2, major)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9.3a1
Tracking Status
status1.9.2 --- beta1-fixed

People

(Reporter: stephend, Assigned: roc)

References

()

Details

(Keywords: regression, testcase, verified1.9.2)

Attachments

(3 files)

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.
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.
OS: Mac OS X → All
Hardware: x86 → All
We need a regression range.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
Flags: blocking1.9.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.
Regression from bug 339548.
Blocks: 339548
The EMBED has borders and padding. I guess I need to fix borders and padding on plugins properly now instead of just ignoring them.
Assignee: nobody → roc
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P2
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.)
Attached patch fixSplinter Review
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.
Attachment #399176 - Flags: review?(dbaron)
The nsViewManager change is to avoid an assert when you set bounds with nonzero origin when creating a view.
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.
Attachment #399177 - Flags: review?(dbaron)
Whiteboard: [needs review]
(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?).
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.
(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 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
Attachment #399176 - Flags: review?(dbaron) → review+
Attachment #399177 - Flags: review?(dbaron) → review+
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.
Whiteboard: [needs review] → [needs landing]
(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.
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
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 192 landing]
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?
Target Milestone: --- → mozilla1.9.3a1
I don't know, file a new bug for that.
No longer depends on: 520436
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.
Status: RESOLVED → VERIFIED
(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?
Whiteboard: [needs 192 landing]
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
Keywords: verified1.9.2
You need to log in before you can comment on or make changes to this bug.