Closed Bug 631388 Opened 9 years ago Closed 9 years ago

Investigate not centering plugin images in their frame if the frame/image sizes mismatch

Categories

(Core :: Plug-ins, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: cjones, Assigned: roc)

References

()

Details

(Whiteboard: [has patch][hardblocker])

Attachments

(4 files, 1 obsolete file)

Spun off from bug 626602.  It's not clear how this helps anything, and Rob alluded to it being a problem, but I didn't get the details there.

If the goal is to minimize the visual difference between sizes-match rendering and a subsequent sizes-mismatch rendering, then there are a few things to consider first.  If we assume that the common case is the <object> top-left staying fixed and width/height changing, then (obviously) the minimum visual difference before/after resize is to leave the image at <0,0>.  I don't know if that's a good assumption.  That's the simplest implementation.
For example, go to http://superpokepets.com/spp, create an account and log in. There's Flash object at the top of the page with a kind of menubar. Hover over the menubar and the Flash object will jump around because we've changed the size of the object and the async rendering hasn't caught up.
I housed my monkey in the sky palace.

Yeah, that's pretty jarring.
Seems to fix superpokepets.

I don't see any risk in taking this patch.
Assignee: nobody → jones.chris.g
Attachment #509644 - Flags: review?(roc)
Attachment #509644 - Flags: approval2.0?
Attachment #509644 - Flags: review?(roc)
Attachment #509644 - Flags: review+
Attachment #509644 - Flags: approval2.0?
Attachment #509644 - Flags: approval2.0+
Blocks: 626691
Whiteboard: [needs landing]
http://hg.mozilla.org/mozilla-central/rev/c3a4a1970f6c
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
Version: unspecified → Trunk
I backed out this because unfortunately it was conflicting with the backout of bug 626602 and I didn't want to merge a code that I don't know.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [needs landing]
So I think here we should get rid of all the centering logic, and furthermore we should avoid changing nsDisplayPlugin::GetBounds in nsObjectFrame::BuildLayer, by a) never changing the result of GetBounds based on the imagecontainer, and b) modifying nsDisplayPlugin::GetOpaqueRegion to return false if the mImageSurface dimensions don't match the display item bounds.
Attached patch fix v2Splinter Review
More comprehensive fix. Needed for patch in bug 630835 to pass tests reliably.

I'm happy to land this after bug 626602; there will be some merging, but it's probably easier to merge this on top of 626602.
Assignee: jones.chris.g → roc
Attachment #509644 - Attachment is obsolete: true
Attachment #512760 - Flags: review?(jones.chris.g)
Comment on attachment 512760 [details] [diff] [review]
fix v2

cjones asked if I wanted to look this over. Seems fine to me.
Attachment #512760 - Flags: feedback+
Comment on attachment 512760 [details] [diff] [review]
fix v2

> I'm happy to land this after bug 626602; there will be some merging, but it's
> probably easier to merge this on top of 626602.

Bless you, kind sir.
Attachment #512760 - Flags: review?(jones.chris.g) → review+
Blocks: 626206
We should get this landed for beta feedback on the whole set.
blocking2.0: --- → betaN+
Whiteboard: [needs landing] → [needs landing][hardblocker]
Confirm this bug.
I tried to land this, but the patch didn't apply cleanly, and it was not easy to merge at all.
Whiteboard: [needs landing][hardblocker] → [needs new patch][hardblocker]
Sorry yes, this needs more work post-626602. However, I don't think this needs beta coverage.
blocking2.0: betaN+ → final+
Blocks: 635158
GetImageSize will be useful in bug 634844 as well.
Attachment #513688 - Flags: review?(jones.chris.g)
nsDisplayPlugin::GetOpaqueRegion is necessarily dependent on async rendering state, but this patch makes it depend on nsIPluginInstance::GetImageSize, which shouldn't change during painting.
Attachment #513689 - Flags: review?(jones.chris.g)
Whiteboard: [needs new patch][hardblocker] → [has patch][hardblocker]
Whiteboard: [has patch][hardblocker] → [has patch][hardblocker][needs review cjones]
Passed reftests on try.
Attachment #513688 - Flags: review?(jones.chris.g) → review+
Attachment #513689 - Flags: review?(jones.chris.g) → review+
Blocks: 635405
Whiteboard: [has patch][hardblocker][needs review cjones] → [has patch][hardblocker][needs landing]
http://hg.mozilla.org/mozilla-central/rev/8b22f4933c31
http://hg.mozilla.org/mozilla-central/rev/9ad79fc34fee
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][hardblocker][needs landing] → [has patch][hardblocker]
Verified. Mozilla/5.0 (Windows NT 6.1; rv:2.0b13pre) Gecko/20110223 Firefox/4.0b13pre ID:20110223030416
Thanks ! Very good job!
Depends on: 636244
No longer depends on: 636244
This has been reported to have fixed the Nike sites on bug 634380, now marked as resolved

thanks to Keldorn on mozillazine
http://forums.mozillazine.org/viewtopic.php?f=23&t=2116361&p=10474673#p10474673
(In reply to comment #19)
> Verified. Mozilla/5.0 (Windows NT 6.1; rv:2.0b13pre) Gecko/20110223
> Firefox/4.0b13pre ID:20110223030416
> Thanks ! Very good job!

Confirming as verified
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.