Closed Bug 206602 Opened 22 years ago Closed 21 years ago

[FIXr]list-style-images load only when resizing browser

Categories

(Core :: Layout: Block and Inline, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.8alpha1

People

(Reporter: habere, Assigned: bzbarsky)

References

()

Details

(Keywords: css2, testcase)

Attachments

(2 files, 2 obsolete files)

User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3) Gecko/20030430 Debian/1.3-5 Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3) Gecko/20030430 Debian/1.3-5 in the test url there are a number of custom bullets set with css2 list-style-images. for example: li.aries { list-style: url(aries24.png); } none of these are loaded. the bullets are either entirely missing or appear as some blotched dot graphic. Reproducible: Always Steps to Reproduce: to reproduce: 1. visit: http://home.worldonline.co.za/~astrology/faq/#6 (when the page has finished loading) look at the list items: aries, taurus, etc. now **resize** your browser, and watch the list-style-images suddenly appear. it's as if the page isn't being redrawn for the final rendering. this has happened with every version of mozilla i've ever tried (1.0, 1.1, 1.2, 1.3 and 1.4b)
This may be related to bug 187419. Confirm new 20030516 PC/WinXP OS->all ->Layout B&I
Assignee: dbaron → block-and-inline
Status: UNCONFIRMED → NEW
Component: Style System → Layout: Block & Inline
Ever confirmed: true
OS: Linux → All
Depends on: 187419
Blocks: 210132
Shift-reloading the minimal testcase shows the problem.... It looks like this is a bug with an <li> whose first child is a block. Somewhow we're not reflowing/repainting the bullet frame correctly....
Right on, Boris. I found that adding &nbsp; (non-breaking space) right before the block element is a workaround.
Keywords: css2, testcase
Summary: [css2] list-style-images load only when resizing browser → list-style-images load only when resizing browser
So the problem here is that the initial reflow gives the bullet frame a size of 0x0 (not quite sure why, but it really doesn't matter -- even if it gave it the size of a bullet in the current font the rest of the problem would exist). Now when the bullet discovers its size it tries to reflow itself. It does this by calling ReflowDirtyChild() on the first child of the block of the block frame. that's pretty bogus... What happens here is that the first child is a height-0 textframe or something along those lines. So we dirty the first line in the block (height 0), and eventually end up in nsBlockFrame::PlaceLine, where we skip reflowing the bullet, since the line height is 0 and it's not the last line. In my opinion the right way to fix this is to have the bullet ReflowDirtyChild itself and make blocks compare the child to mBullet and if the two are equal determine which line to dirty itself (the whole "first line unless it's height 0, etc" mess that bullet frame knows about). Even so, I'm not sure what would happen if we have an inline first line of nonzero height and then a block and the reflow makes the first line be 0. Would the block end up getting reflown? If not, then we could still run into this issue of the bullet frame not getting reflown when it needs to be. David? Thoughts?
Hardware: PC → All
Would making our bullet code actually work per spec (i.e. take part in the inline box model for the first line of its block) help at all?
Hmm.. Should it do so even when the first line of the block is a zero-height whitespace-only line? That's what we have here.
What is a zero-height white-space only line? If white-space is normal, you just don't get a line at all: # Whitespace content that would subsequently be collapsed away according to the # 'white-space' property does not generate any anonymous inline boxes. -- http://www.w3.org/TR/CSS21/visuren.html#q7 Anyway, to answer your question, the current internal draft says that the marker box will be vertically aligned with the first line of content in the principal box, as specified by the pseudo-element's 'vertical-align' property. (The principal box is the main one generated for the element with its 'display' property set to list-item, as opposed to the marker box.) The marker box participates in the height calculations of the principal box's first line box. Thus, markers are aligned with the first line of an element's content. The first line of a principal box is the one matched by the box's '::first-line' pseudo-element. It might not be a direct child of the box, but it must be in flow. If no first line box exists in a principal box, the marker box establishes its line box alone, as the first box of the principal block box. HTH.
The issue is that we have this extra line floating around that doesn't actually affect layout (as far as I can tell) but does exist in our internal data structures that happens in cases like this: <div style="display: list-item"> <div style="display: block"> </div> </div In this case, the inner div is actually in the second "line"; the first line is an inline-level line containing no text. This is sort of required if one demands that changing the value of white-space does not lead to box creation (which in Mozilla it does not; changing white-space just reflows, does not reframe). The point is, most of the code involved knows about this situation and correctly attaches the bullet to the right line. The code I pointed out in comment 5 is the exception. Now there may be better ways to architect the whole thing. I don't know. I don't understand block reflow quite well enough yet.
Ah. Well, those lines don't exist per CSS, so no, it wouldn't put it in those lines. (Changing white-space should _so_ just reframe, if that would get around having these random non-lines complicating layout everywhere... no? Seems like changing white-space on the fly is pretty rare outside my test cases.)
No idea, frankly. Like I said, I really don't know block/line layout that well. Reframing is really expensive, though. And dynamic changes will happen more if I get async stylesheet loading working.
Attached patch Fix (obsolete) — Splinter Review
This is the right fix, I think. At the very least, it encapsulates all the "where does the bullet live?" logic in nsBlockFrame, so it can be changed as a unit as needed. This patch fixes the testcase in this bug and the one in bug 210132
Assignee: core.layout.block-and-inline → bzbarsky
Status: NEW → ASSIGNED
Attachment #145868 - Flags: superreview?(dbaron)
Attachment #145868 - Flags: review?(dbaron)
Priority: -- → P1
Summary: list-style-images load only when resizing browser → [FIX]list-style-images load only when resizing browser
Target Milestone: --- → mozilla1.7final
Comment on attachment 145868 [details] [diff] [review] Fix >+ if (aChild == mBullet && HaveOutsideBullet()) { >+ // The bullet lives in the first line, unless the first line has >+ // height 0, in which case it lives in the second line. >+ line_iterator bulletLine = begin_lines(); >+ if (bulletLine != end_lines() && bulletLine->mBounds.height == 0) { >+ MarkLineDirty(bulletLine); why? >+ bulletLine = bulletLine.next(); >+ } >+ >+ if (bulletLine == end_lines()) { >+ // no meaningful lines; reflow all of this block >+ NS_ASSERTION(mParent, "Should really have a parent!"); >+ if (mParent) { >+ mState |= NS_FRAME_IS_DIRTY; >+ mParent->ReflowDirtyChild(aPresShell, this); >+ } Is there a better way? (Perhaps there isn't, but I really dislike ReflowDirtyChild.) >+ } else { >+ MarkLineDirty(bulletLine); >+ } >+ } else { >+ // Mark the line containing the child frame dirty. >+ line_iterator fline = FindLineFor(aChild); >+ if (fline != end_lines()) >+ MarkLineDirty(fline); >+ }
>>+ MarkLineDirty(bulletLine); >why? Hmm... maybe that's not needed, yes. I thought I needed it to handle the case when a reflow would make that line a different height, but that shouldn't happen if all that's dirty is the bullet (and otherwise, the line is already dirty). I'll remove that line. > Is there a better way? (Perhaps there isn't, but I really dislike > ReflowDirtyChild.) Hmmm... Looking through ReflowDirtyLines, that handles the empty-line-list case already (it just reflows the bullet then), so all this code is in fact unnecessary. I'll remove it.
Attachment #145868 - Flags: superreview?(dbaron)
Attachment #145868 - Flags: review?(dbaron)
Attached patch Updated to comments (obsolete) — Splinter Review
Attachment #145868 - Attachment is obsolete: true
Attachment #145945 - Flags: superreview?(dbaron)
Attachment #145945 - Flags: review?(dbaron)
Did you want to include the nsBulletFrame.cpp diffs?
Attachment #145945 - Flags: superreview?(dbaron)
Attachment #145945 - Flags: review?(dbaron)
Attached patch DohSplinter Review
Yes, I did.... those are the same as before. Sorry about that.
Attachment #145945 - Attachment is obsolete: true
Attachment #145949 - Flags: superreview?(dbaron)
Attachment #145949 - Flags: review?(dbaron)
Attachment #145949 - Flags: superreview?(dbaron)
Attachment #145949 - Flags: superreview+
Attachment #145949 - Flags: review?(dbaron)
Attachment #145949 - Flags: review+
David, do you think this is safe enough for 1.7? I suspect it is, but I'm not quite as comfortable with blocks as I'd like to be to be sure here.
Summary: [FIX]list-style-images load only when resizing browser → [FIXr]list-style-images load only when resizing browser
Checked in for 1.8a; leaving open pending David deciding whether this is ok for 1.7.
I don't think it's worth trying to get this into the branch.
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.7final → mozilla1.8alpha
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: