Closed
Bug 163504
Opened 19 years ago
Closed 14 years ago
marquee should be inline-block rather than block
Categories
(Core :: Layout, defect, P5)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla1.9alpha8
People
(Reporter: dbaron, Assigned: dbaron)
References
(Depends on 1 open bug)
Details
(Whiteboard: [patch])
Attachments
(2 files, 4 obsolete files)
1.22 KB,
text/html; charset=UTF-8
|
Details | |
15.84 KB,
patch
|
martijn.martijn
:
review+
bzbarsky
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
In bug 163031 comment 7, kin pointed out that, in MSIE, marquee acts more like an inline-block rather than a block. I haven't tested this, but someone may want to investigate whether this is consistently the case, and, if so, what we should do about it.
Assignee | ||
Updated•19 years ago
|
Priority: -- → P5
Comment 1•19 years ago
|
||
http://msdn.microsoft.com/workshop/author/dhtml/reference/objects/marquee.asp "This element is a block element."
Updated•19 years ago
|
QA Contact: petersen → amar
Updated•19 years ago
|
Target Milestone: --- → Future
Comment 3•19 years ago
|
||
That's very odd. Alerting currentStyle.display gives "block" even though IE uses the "inhline-block" rendering model when the width is set. I recommend not to follow MS weird and buggy ways. Be consistent even though MS isn't.
![]() |
||
Comment 4•18 years ago
|
||
To doron, but feel free to punt this to default owner....
Assignee: attinasi → doron
![]() |
||
Updated•18 years ago
|
Depends on: inline-block
Comment 5•17 years ago
|
||
*** Bug 244561 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 6•14 years ago
|
||
Assignee | ||
Updated•14 years ago
|
Attachment #252806 -
Attachment mime type: text/plain; charset=iso-8859-1 → text/html; charset=iso-8859-1
Assignee | ||
Comment 7•14 years ago
|
||
Attachment #95893 -
Attachment is obsolete: true
Attachment #252806 -
Attachment is obsolete: true
Assignee | ||
Comment 8•14 years ago
|
||
Attachment #252807 -
Attachment is obsolete: true
Assignee | ||
Comment 9•14 years ago
|
||
This switches to using inline-block, sets the correct vertical alignment, and changes the vertical marquee slightly so that it still has infinitely large preferred width (otherwise it would shrink-wrap its contents). (We should probably implement width: -moz-container to suppress shrink-wrapping for things where width:auto means shrink wrap. And we should probably also implement width: shrink-wrap...)
Assignee: doronr → dbaron
Status: NEW → ASSIGNED
Assignee | ||
Updated•14 years ago
|
Attachment #252811 -
Flags: superreview?(bzbarsky)
Attachment #252811 -
Flags: review?(martijn.martijn)
Assignee | ||
Updated•14 years ago
|
QA Contact: amar → layout
Whiteboard: [patch]
Target Milestone: Future → mozilla1.9beta
Comment 10•14 years ago
|
||
+ <!-- XXX why no inheritstyle="false"? --> Yeah, the vertical binding also should have inheritstyle="false", so I think you should add it. The 'mother' binding has also inheritstyle=""false", but that doesn't seem to work, so I think that one can be removed: http://lxr.mozilla.org/seamonkey/source/layout/style/xbl-marquee/xbl-marquee.xml#47 I tried your patch on various marquee websites, they all seem to work just fine. Why do you change overflow: -moz-scrollbars-none to overflow: hidden? From what I heard (at least that's what my memory tells me), the former was a little bit faster, because it doesn't add (hidden) scrollbars or something like that.
Assignee | ||
Comment 11•14 years ago
|
||
-moz-scrollbars-none and hidden are the same thing. 'hidden' used to be something different, but the CSS spec was clarified to make 'hidden' mean what we'd in the past called -moz-scrollbars-none. (Scrollable, but without scrollbars visible.)
Comment 12•14 years ago
|
||
There is one reflow bug with the patch. http://wargers.org/mozilla/bug163504_marquee_inline_block/marquee_inline_block.htm This is more a general reflow bug caused by the use of inline-block.
Assignee | ||
Comment 13•14 years ago
|
||
So, I tossed in the fix for the bug Martijn points out in comment 12. This requires a little bit of explanation. nsLineBox has bits on it that indicate that something in the line depends on the width of the containing block. However, there were two separate bits and the bits were set in three separate places. This patch merges the two separate bits and the three separate setters: * it combines HasPercentageAwareChild into ResizeReflowOptimizationDisabled * it combines the code to set those bits in nsBlockFrame, nsInlineFrame and nsLineLayout into only nsLineLayout::ReflowFrame. I think that's the right place (rather than VerticalAlignFrames) because I think if we try to fit a frame on a line, it's percentage-awareness should affect that line even if it doesn't currently fit. * the code now used is largely the function from nsBlockFrame. However, I made a few changes: + I added the skip-text-frames optimization from one of the others + I removed the checking of percentages on 'top' and 'bottom' since those are relative to the containing block height, and instead checked those in nsHTMLReflowState + I added code checking for auto-width inline-blocks, inline-tables, and (since bz says so) buttons I believe that if we reflow a line, we reflow everything that's "inline" within the line (i.e., nsInlineFrame doesn't make dirtyness optimizations), so I think just checking in ReflowFrame should be sufficient. That said, this is messing with enough that I'll be surprised if it doesn't break something. Martijn -- the review request to you is really just for the marquee changes, and it's an r+sr request to bzbarsky for the rest of the patch.
Attachment #252811 -
Attachment is obsolete: true
Attachment #252882 -
Flags: superreview?(bzbarsky)
Attachment #252882 -
Flags: review?(martijn.martijn)
Attachment #252811 -
Flags: superreview?(bzbarsky)
Attachment #252811 -
Flags: review?(martijn.martijn)
Assignee | ||
Updated•14 years ago
|
Attachment #252882 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 14•14 years ago
|
||
And now that I think about it, InitResizeFlags should check mMinHeight and mMaxHeight too. And, for reviewers, I also removed the mHeight, mMinHeight, and mMaxHeight checks from the function I moved from nsBlockFrame.
Updated•14 years ago
|
Attachment #252882 -
Flags: review?(martijn.martijn) → review+
![]() |
||
Comment 15•14 years ago
|
||
Comment on attachment 252882 [details] [diff] [review] patch >diff -r 99154f949330 layout/generic/nsLineLayout.cpp >+HasPercentageUnitSide(const nsStyleSides& aSides) Maybe worth using NS_FOR_CSS_SIDES here? Either way. >+IsPercentageAware(const nsIFrame* aFrame) >+ fType == nsGkAtoms::HTMLButtonControlFrame || >+ fType == nsGkAtoms::gfxButtonControlFrame) { I just checked and we should add fieldsets here. With those added, r+sr=bzbarsky
Attachment #252882 -
Flags: superreview?(bzbarsky)
Attachment #252882 -
Flags: superreview+
Attachment #252882 -
Flags: review?(bzbarsky)
Attachment #252882 -
Flags: review+
![]() |
||
Comment 16•14 years ago
|
||
Oh, and it would be good to add tests for bug 28811....
Assignee | ||
Comment 17•14 years ago
|
||
Patch checked in (in 2 pieces), 2007-01-27 10:40 -0800. Additional tests per comment 16 checked in 2007-01-31 18:14 -0800. It's also worth noting that (based on the test I wrote) fieldsets don't do shrink-wrapping -- they just size based on the available width -- or perhaps the available width and the min width.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Comment 18•14 years ago
|
||
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/layout/reftests&command=DIFF_FRAMESET&root=/cvsroot&file=reftest.list&rev1=1.33&rev2=1.34
Flags: in-testsuite+
Updated•14 years ago
|
Summary: should marquee be inline-block rather than block → marquee should be inline-block rather than block
You need to log in
before you can comment on or make changes to this bug.
Description
•