Default width/height for plugin placeholders should be 240px/200px

RESOLVED FIXED

Status

()

RESOLVED FIXED
12 years ago
10 years ago

People

(Reporter: martijn.martijn, Assigned: martijn.martijn)

Tracking

Trunk
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments)

(Assignee)

Description

12 years ago
This is a follow-up from bug 367247, the testcase is from that bug.
The last example of the testcase has no width/height set.
In that case the width/height of the plugin placeholder should be 240px/200px.
I guess this could be fixed in missingPlugin.css, but biesi mentioned that the embed element should get this width/height from:
http://lxr.mozilla.org/seamonkey/source/layout/generic/nsObjectFrame.cpp#596
Yeah, after thinking more about that I was wrong. I broke that when I rewrote pluginfinder. This should be sized explicitly now.
OS: Windows XP → All
Hardware: PC → All
> This should be sized explicitly now.

meaning: CSS should be added somewhere to do that sizing.
(Assignee)

Comment 3

12 years ago
Created attachment 254825 [details]
testcase1
(Assignee)

Comment 4

12 years ago
Created attachment 254826 [details]
testcase2
(Assignee)

Comment 5

12 years ago
Created attachment 254830 [details] [diff] [review]
patch?

I think this fixes it for most cases, except it doesn't fix the last case of testcase1, where width="xxx".
Basically, I need a selector(s) that fo all plugin elements that don't have a (valid) width/height set. It seems to me that isn't possible with css.
(Assignee)

Updated

12 years ago
Attachment #254830 - Flags: review?(cbiesinger)
Comment on attachment 254830 [details] [diff] [review]
patch?

hm, if this works r=biesi
Attachment #254830 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 7

12 years ago
Comment on attachment 254830 [details] [diff] [review]
patch?

Well, yes, it works, except for the width="xxx" case.
But I guess that's probably not a big deal.
Attachment #254830 - Flags: superreview?(dbaron)
(In reply to comment #5)
> Basically, I need a selector(s) that fo all plugin elements that don't have a
> (valid) width/height set. It seems to me that isn't possible with css.

Or you need a way of making your style rules apply at a level of the cascade lower than the width/height attributes.
Comment on attachment 254830 [details] [diff] [review]
patch?

sr=dbaron -- good enough for now, but please file a bug on doing it right (so it applies to any invalid height/width attribute by being at a lower level of the cascade)
Attachment #254830 - Flags: superreview?(dbaron) → superreview-
Attachment #254830 - Flags: superreview- → superreview+
(Assignee)

Updated

11 years ago
Attachment #254830 - Flags: approval1.9?
(Assignee)

Updated

11 years ago
Assignee: nobody → martijn.martijn
Comment on attachment 254830 [details] [diff] [review]
patch?

a19=dbaron
Attachment #254830 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 11

11 years ago
Checking in missingPlugin.css;
/cvsroot/mozilla/toolkit/themes/winstripe/mozapps/plugins/missingPlugin.css,v  <
--  missingPlugin.css
new revision: 1.4; previous revision: 1.3

Checked into trunk.

I filed bug 390700 as a follow-up for a better solution as was suggested by dbaron in comment 9.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Created attachment 337869 [details] [diff] [review]
pinstripe patch and mochitest

Apparently this fix wasn't applied to pinstripe. This fixes that and adds a mochitest for it using the example testcases.
Attachment #337869 - Flags: superreview?
Attachment #337869 - Flags: review?

Updated

10 years ago
Attachment #337869 - Flags: superreview?(dbaron)
Attachment #337869 - Flags: superreview?
Attachment #337869 - Flags: review?(cbiesinger)
Attachment #337869 - Flags: review?
I'll fix the alignment oddity in Makefile.in on checkin.
Attachment #337869 - Flags: superreview?(dbaron)
Attachment #337869 - Flags: superreview+
Attachment #337869 - Flags: review?(cbiesinger)
Attachment #337869 - Flags: review+
Comment on attachment 337869 [details] [diff] [review]
pinstripe patch and mochitest

r+sr=dbaron
You need to log in before you can comment on or make changes to this bug.