Plugin placeholders should be inline-block

VERIFIED FIXED

Status

()

Core
Layout
VERIFIED FIXED
12 years ago
12 years ago

People

(Reporter: Martijn Wargers (zombie), Assigned: dbaron)

Tracking

({testcase})

Trunk
testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patch])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

12 years ago
Created attachment 251791 [details]
testcase

See testcase.
It seems to me that plugin placeholders should have a inline-block layout, because the embed element is a replaced element.
Also, it seems to me that the embed element should have a default width/height when no width/height is set and the plugin placeholder is rendered.

In Firefox2, the default width/height for plugin placeholders seems to be 240px by 200px. But maybe the same as for iframe should be used, eg 300px by 150px?

Inline-block is implemented in bug 9458.
(Reporter)

Updated

12 years ago
(Assignee)

Comment 1

12 years ago
Yeah, they were intended to be in bug 309521; I'm surprised nobody filed a followup bug then.
Blocks: 309521
Depends on: 9458
(Assignee)

Comment 2

12 years ago
Created attachment 252817 [details] [diff] [review]
patch
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
Attachment #252817 - Flags: superreview?(bzbarsky)
Attachment #252817 - Flags: review?(cbiesinger)
(Assignee)

Updated

12 years ago
Whiteboard: [patch]
(Assignee)

Updated

12 years ago
OS: Windows XP → All
Hardware: PC → All
(Assignee)

Comment 3

12 years ago
Actually, it occurs to me that I probably want vertical-align to still work, which means I want vertical-align: inherit and overflow:hidden (the latter to cause vertical-align:baseline to do bottom-to-baseline alignment rather than baseline-to-baseline alignment).
(Assignee)

Updated

12 years ago
Attachment #252817 - Flags: superreview?(bzbarsky)
Attachment #252817 - Flags: review?(cbiesinger)
(Assignee)

Comment 4

12 years ago
Created attachment 252844 [details] [diff] [review]
patch

This fixes inline-blocks so they support overflow, and adds some helpers on nsStyleDisplay that I've had in my tree for ages.
Attachment #252817 - Attachment is obsolete: true
Attachment #252844 - Flags: superreview?(bzbarsky)
Attachment #252844 - Flags: review?(cbiesinger)
Comment on attachment 252844 [details] [diff] [review]
patch

>diff -r 88ad983e4579 layout/style/nsStyleStruct.h
>+  PRBool IsBlockInside() const {
>+    // Should TABLE_CELL and TABLE_CAPTION go here?  They have
>+    // block frames nested inside of them.

If we add captions here, we'll need to change the frame ctor code that builds scrollframes and then a block for things that are block-inside and have scrollable overflow...

Add a note about that to this comment?

Add tests for the scrolling, if possible?

sr=bzbarsky.
Attachment #252844 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Comment 6

12 years ago
Note that what gets checked in for this bug will be addressing one of the review comments in bug 9458: see bug 9458 comment 92.
Comment on attachment 252844 [details] [diff] [review]
patch

r=biesi
Attachment #252844 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 8

12 years ago
Patch checked in (in 2 pieces) 2007-01-27 10:44 -0800 and 2007-01-27 10:46 -0800.

Change mentioned in comment 6 checked in 2007-01-27 10:45 -0800 and partially backed out 2007-01-27 11:35 -0800.

Additional tests per comment 5 checked in 2007-01-31 18:13 -0800.

Note also that the comment I added was more general, and just said to look at callers, which really ought to be obvious when changing a method like that...
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Reporter)

Comment 9

12 years ago
I filed bug 368972 on the issue that not the correct width/height is set for the plugin placeholder.

Verified fixed with 2007-01-31 trunk build.
Status: RESOLVED → VERIFIED
(Assignee)

Updated

12 years ago
Blocks: 323457
You need to log in before you can comment on or make changes to this bug.