marquee should be inline-block rather than block

RESOLVED FIXED in mozilla1.9alpha8

Status

()

defect
P5
normal
RESOLVED FIXED
17 years ago
12 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

(Depends on 1 bug)

Trunk
mozilla1.9alpha8
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patch])

Attachments

(2 attachments, 4 obsolete attachments)

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 2

17 years ago
Posted file Test Case (based on attachment 95560) (obsolete) —
Load this test case in Mozilla and IE 6 to see the difference.

Updated

17 years ago
QA Contact: petersen → amar
Target Milestone: --- → Future

Comment 3

17 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.
To doron, but feel free to punt this to default owner....
Assignee: attinasi → doron
*** Bug 244561 has been marked as a duplicate of this bug. ***
Attachment #252806 - Attachment mime type: text/plain; charset=iso-8859-1 → text/html; charset=iso-8859-1
Posted file a few more examples (obsolete) —
Attachment #95893 - Attachment is obsolete: true
Attachment #252806 - Attachment is obsolete: true
Posted 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
Status: NEW → ASSIGNED
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:
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.
-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.
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.
Posted patch patch — — Splinter 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)
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]
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+
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED

Updated

13 years ago
Summary: should marquee be inline-block rather than block → marquee should be inline-block rather than block
Depends on: 370757

Updated

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