Closed Bug 163504 Opened 21 years ago Closed 16 years ago

marquee should be inline-block rather than block


(Core :: Layout, defect, P5)






(Reporter: dbaron, Assigned: dbaron)


(Depends on 1 open bug)


(Whiteboard: [patch])


(2 files, 4 obsolete files)

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.
Priority: -- → P5
Attached file Test Case (based on attachment 95560) (obsolete) —
Load this test case in Mozilla and IE 6 to see the difference.
QA Contact: petersen → amar
Target Milestone: --- → Future
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
To doron, but feel free to punt this to default owner....
Assignee: attinasi → doron
Depends on: inline-block
*** Bug 244561 has been marked as a duplicate of this bug. ***
Attached file a few more examples (obsolete) —
Attachment #252806 - Attachment mime type: text/plain; charset=iso-8859-1 → text/html; charset=iso-8859-1
Attached file a few more examples (obsolete) —
Attachment #95893 - Attachment is obsolete: true
Attachment #252806 - Attachment is obsolete: true
Attached file a few more examples
Attachment #252807 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
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
Attachment #252811 - Flags: superreview?(bzbarsky)
Attachment #252811 - Flags: review?(martijn.martijn)
QA Contact: amar → layout
Whiteboard: [patch]
Target Milestone: Future → mozilla1.9beta
+  <!-- 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:

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.
-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.)
There is one reflow bug with the patch.
This is more a general reflow bug caused by the use of inline-block.
Attached patch patchSplinter Review
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)
Attachment #252882 - Flags: review?(bzbarsky)
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.
Attachment #252882 - Flags: review?(martijn.martijn) → review+
Comment on attachment 252882 [details] [diff] [review]

>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+
Oh, and it would be good to add tests for bug 28811....
Depends on: 368461
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.
Closed: 16 years ago
Resolution: --- → FIXED
Summary: should marquee be inline-block rather than block → marquee should be inline-block rather than block
Depends on: 370422
Depends on: 370757
Depends on: 384297
Depends on: 405263
You need to log in before you can comment on or make changes to this bug.