Closed Bug 391559 Opened 13 years ago Closed 12 years ago

Incorrect ordered-list numbering within -moz-column-* (and/or numbering changes when clicked)

Categories

(Core :: Layout, defect, minor)

1.8 Branch
defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla1.9beta4

People

(Reporter: brian, Assigned: craig.topper)

References

()

Details

(Keywords: crash)

Attachments

(5 files, 7 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.6) Gecko/20070725 Firefox/2.0.0.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.6) Gecko/20070725 Firefox/2.0.0.6

An ordered list of links that has been styled with the value of -moz-column-count greater than one will exhibit a renumbering bug if the list contains links. Clicking on a link in the last column (on the far right) will result in the list item numbering changing for that column. Clicking two links some other column (or clicking a link then clicking elsewhere on the page) will correct the numbering.

Reproducible: Always

Steps to Reproduce:
1. Visit page an ordered list of links that has -moz-column-count applied to it
2. Click on a link in the last column

Actual Results:  
Clicking on a link in the last column of the list causes the numbering to change.

Expected Results:  
List numbering should not change.

The bug seems to express itself onmousedown. If you return false on this event the renumbering will not occur, providing a workaround of sorts.
On Mac trunk, what I see is that the numbers are initially incorrect (e.g. some numbers are repeated) and clicking sometimes fixes the numbers.
OS: Windows XP → All
Hardware: PC → All
Summary: last columns of multi-column ordered list containing links is renumbered when clicked → Incorrect ordered-list numbering within -moz-column-* (and/or numbering changes when clicked)
Severity: trivial → minor
Status: UNCONFIRMED → NEW
Ever confirmed: true
I believe the root of the problem is that RenumberListsInBlock doesn't handle overflow lines. This patch fixes that.
Attachment #303773 - Attachment is patch: true
Attachment #303773 - Attachment mime type: application/octet-stream → text/plain
This is the right idea but nsBlockInFlowLineIterator is really a lot better to use than TryAllLines.
Changed RenumberListsInBlock to use nsBlockInFlowLineIterator. Pretty sure there's a bug in this if there are no lines to start with, but I wasn't sure what the best way to handle that was.
Attachment #303773 - Attachment is obsolete: true
Attached patch Patch that will actually compile (obsolete) — Splinter Review
Previous patch wouldn't compile. Oop.s
Attachment #303933 - Attachment is obsolete: true
This patch should properly find the first valid line anywhere in the next in flow chain and start iterating from there.
Attachment #303934 - Attachment is obsolete: true
This is basically good, but I'd like to avoid having nsBlockInFlowLineIterator have a "bad state" in which you can only call FindValidLine.

How about making FindValidLine private and adding a new constructor that takes an nsBlockFrame* parameter and calls FindValidLine()? It could also take a PRBool* aFoundValidLine out-parameter since constructors can't return booleans.

Thanks!!!
Attachment #303939 - Attachment is obsolete: true
+      PRBool kidRenumberedABullet = RenumberListsFor(aPresContext, kid, aOrdinal, aDepth);

Shouldn't we be calling RenumberListsFor on bifLineIter.GetContainer(), not 'this'?

Other than that, looks great.

We really want a reftest for this, if you can come up with one.
BTW you should be requesting review on these patches. People don't always catch all their bugmail.
Couldn't RenumberListsFor be static? I don't think it uses any instance variables. Also shouldn't RenumberListsInBlock either be static or be called on the first in flow rather than taking the block as a parameter?
> Couldn't RenumberListsFor be static?

Sure, make it so! In fact it should probably be a regular static function outside of nsBlockFrame.

> Also shouldn't RenumberListsInBlock either be static or be called on
> the first in flow rather than taking the block as a parameter?

Make it a static function too. And assert that its parameter is the first-in-flow (i.e. NS_ASSERTION(!aBlockFrame->GetPrevInFlow())).
RenumberListsInBlock calls RenumbersListFor which itself calls RenumberListsInBlock. So if I take them out of the class I need a prototype for at least one of them. Where should I put the prototype? I'm pretty new to the firefox source so I don't much about the coding methodologies yet.
Since these are local static functions, just put the prototype for one just above the other.
Just out of curiosity what's the reasoning for removing them from the class anyway?
I made them static, but couldn't remove them from the class because RenumberListsFor accesses private member mBullet.
Attachment #303946 - Attachment is obsolete: true
This html file seems to crash my builds of the attached patch if the window is resized too small horizontally. I ran it with Xcode and got and got an EXC_BAD_ACCESS in the GetContentInsertionFrame call in RenumberListsFor. I don't know how to debug further though.
Attachment #304060 - Flags: review?(roc)
(In reply to comment #15)
> Just out of curiosity what's the reasoning for removing them from the class
> anyway?

Then we can remove them from nsBlockFrame.h, which is good because hiding implementation details is always good.

Compilers sometimes do better optimizations with file-level static functions, too.
Oh, in this patch, these functions aren't file-level static, they're just static in nsBlockFrame.h. But that's OK.

Can you see what type of frame it is that causes the crash? Which platform/debugger are you using? Maybe go up some levels in the stack and call nsIFrameDebug::DumpFrameTree(frame) on some valid frame pointer, to get a frame dump.
I couldn't make them file level static because mBullet is referenced in nsRenumberListsInBlock and its a private member variable. So it either needs an accessor method or the function needs to stay in the class. Should RenumberLists and its 2 friends be private in the class?

I'm on Mac OS X 10.5 using Xcode 3.0.
I'd have made a GetBullet accessor, but don't worry about this issue, it's small potatoes.

Welcome to the delights of gdb! Do you know how to drive it? I suppose you can use XCode to control it but I've never done that, gdb command line is the only thing I know.
Xcode has a gdb console window that I can type gdb commands in. Problem is I never learned much gdb. Can you help me out?
Sure can. We might find it easier on IRC than communicating through Bugzilla comments :-). irc.mozilla.org #developers, I'm 'roc'
Stack trace from failure
Frame dump
Attached patch Version that fixes the crash (obsolete) — Splinter Review
Attachment #304060 - Attachment is obsolete: true
Attachment #304155 - Flags: review?(roc)
Attachment #304060 - Flags: review?(roc)
The code is just fine, but could you add a comment to nsIFrame::GetContentInsertionFrame that this can return null during Reflow?
Attachment #304155 - Attachment is obsolete: true
Attachment #304662 - Flags: review?(roc)
Attachment #304155 - Flags: review?(roc)
Comment on attachment 304662 [details] [diff] [review]
Patch that add's the requested comment to nsIFrame.h

requesting approval. This not only fixes list numbering but also a crasher we can probably hit even without the list numbering fix. Risk is pretty low.
Attachment #304662 - Flags: review?(roc)
Attachment #304662 - Flags: review+
Attachment #304662 - Flags: approval1.9?
Comment on attachment 304662 [details] [diff] [review]
Patch that add's the requested comment to nsIFrame.h

a=beltzner for 1.9
Attachment #304662 - Flags: approval1.9? → approval1.9+
Assignee: nobody → craig.topper
Checking in layout/generic/nsBlockFrame.cpp;
/cvsroot/mozilla/layout/generic/nsBlockFrame.cpp,v  <--  nsBlockFrame.cpp
new revision: 3.934; previous revision: 3.933
done
Checking in layout/generic/nsBlockFrame.h;
/cvsroot/mozilla/layout/generic/nsBlockFrame.h,v  <--  nsBlockFrame.h
new revision: 3.266; previous revision: 3.265
done
Checking in layout/generic/nsColumnSetFrame.cpp;
/cvsroot/mozilla/layout/generic/nsColumnSetFrame.cpp,v  <--  nsColumnSetFrame.cpp
new revision: 3.50; previous revision: 3.49
done
Checking in layout/generic/nsIFrame.h;
/cvsroot/mozilla/layout/generic/nsIFrame.h,v  <--  nsIFrame.h
new revision: 3.396; previous revision: 3.395
done
Status: NEW → RESOLVED
Closed: 12 years ago
Keywords: checkin-neededcrash
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta4
Version: unspecified → 1.8 Branch
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.