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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Brian Sweeney]

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.

Comment 1

[Jesse Ruderman]

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.

Comment 2

[Craig Topper]

Attachment: Patch to make RenumberListsInBlock renumber overflow lines

I believe the root of the problem is that RenumberListsInBlock doesn't handle overflow lines. This patch fixes that.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This is the right idea but nsBlockInFlowLineIterator is really a lot better to use than TryAllLines.

Comment 4

[Craig Topper]

Attachment: Second try using nsBlockInFlowLineIterator

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.

Comment 5

[Craig Topper]

Attachment: Patch that will actually compile

Previous patch wouldn't compile. Oop.s

Comment 6

[Craig Topper]

Attachment: Patch that properly handles an empty line list on the first in flow

This patch should properly find the first valid line anywhere in the next in flow chain and start iterating from there.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 8

[Craig Topper]

Attachment: Patch to address Robert's comments

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

BTW you should be requesting review on these patches. People don't always catch all their bugmail.

Comment 11

[Craig Topper]

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?

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 13

[Craig Topper]

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.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Since these are local static functions, just put the prototype for one just above the other.

Comment 15

[Craig Topper]

Just out of curiosity what's the reasoning for removing them from the class anyway?

Comment 16

[Craig Topper]

Attachment: Version that makes RenumberListsInBlock and RenumberListsFor static

I made them static, but couldn't remove them from the class because RenumberListsFor accesses private member mBullet.

Comment 17

[Craig Topper]

Attachment: Test case that crashes the browser during resize

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.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 20

[Craig Topper]

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.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 22

[Craig Topper]

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?

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sure can. We might find it easier on IRC than communicating through Bugzilla comments :-). irc.mozilla.org #developers, I'm 'roc'

Comment 24

[Craig Topper]

Attachment: Stack trace from failure

Stack trace from failure

Comment 25

[Craig Topper]

Attachment: Frame dump from failure

Frame dump

Comment 26

[Craig Topper]

Attachment: Locals from top 4 stack frames

Comment 27

[Craig Topper]

Attachment: Version that fixes the crash

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The code is just fine, but could you add a comment to nsIFrame::GetContentInsertionFrame that this can return null during Reflow?

Comment 29

[Craig Topper]

Attachment: Patch that add's the requested comment to nsIFrame.h

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 31

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 304662 [details] [diff] [review]
Patch that add's the requested comment to nsIFrame.h

a=beltzner for 1.9

Comment 32

[Reed Loden [:reed]]

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