163504 - marquee should be inline-block rather than block

Status: RESOLVED FIXED

Description

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it)]

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.

Comment 1

[Doron Rosenberg (IBM)]

http://msdn.microsoft.com/workshop/author/dhtml/reference/objects/marquee.asp

"This element is a block element."

Comment 2

[kinmoz]

Created attachment 95893 [details]
Test Case (based on attachment 95560 [details])

Load this test case in Mozilla and IE 6 to see the difference.

Comment 3

[Erik Arvidsson]

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

[Boris Zbarsky [:bzbarsky, bz on IRC]]

To doron, but feel free to punt this to default owner....

Comment 5

[Martijn Wargers (zombie)]

*** Bug 244561 has been marked as a duplicate of this bug. ***

Comment 6

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it)]

Created attachment 252806 [details]
a few more examples

Comment 7

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it)]

Created attachment 252807 [details]
a few more examples

Comment 8

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it)]

Created attachment 252809 [details]
a few more examples

Comment 9

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it)]

Created attachment 252811 [details] [diff] [review]
patch

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...)

Comment 10

[Martijn Wargers (zombie)]

+  <!-- 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.

Comment 11

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it)]

-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

[Martijn Wargers (zombie)]

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.

Comment 13

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it)]

Created attachment 252882 [details] [diff] [review]
patch

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.

Comment 14

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it)]

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.

Comment 15

[Boris Zbarsky [:bzbarsky, bz on IRC]]

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

Comment 16

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Oh, and it would be good to add tests for bug 28811....

Comment 17

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it)]

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.

Comment 18

[Jeff Walden [:Waldo]]

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