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

RESOLVED FIXED in mozilla1.9beta4

Status

()

Core
Layout
--
minor
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: Brian Sweeney, Assigned: Craig Topper)

Tracking

({crash})

1.8 Branch
mozilla1.9beta4
crash
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(5 attachments, 7 obsolete attachments)

(Reporter)

Description

11 years ago
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

11 years ago
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)

Updated

11 years ago
Severity: trivial → minor

Updated

11 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 2

10 years ago
Created attachment 303773 [details] [diff] [review]
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.
(Assignee)

Updated

10 years ago
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.
(Assignee)

Comment 4

10 years ago
Created attachment 303933 [details] [diff] [review]
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.
Attachment #303773 - Attachment is obsolete: true
(Assignee)

Comment 5

10 years ago
Created attachment 303934 [details] [diff] [review]
Patch that will actually compile

Previous patch wouldn't compile. Oop.s
Attachment #303933 - Attachment is obsolete: true
(Assignee)

Comment 6

10 years ago
Created attachment 303939 [details] [diff] [review]
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.
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!!!
(Assignee)

Comment 8

10 years ago
Created attachment 303946 [details] [diff] [review]
Patch to address Robert's comments
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.
(Assignee)

Comment 11

10 years ago
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())).
(Assignee)

Comment 13

10 years ago
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.
(Assignee)

Comment 15

10 years ago
Just out of curiosity what's the reasoning for removing them from the class anyway?
(Assignee)

Comment 16

10 years ago
Created attachment 304060 [details] [diff] [review]
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.
Attachment #303946 - Attachment is obsolete: true
(Assignee)

Comment 17

10 years ago
Created attachment 304061 [details]
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.
(Assignee)

Updated

10 years ago
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.
(Assignee)

Comment 20

10 years ago
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.
(Assignee)

Comment 22

10 years ago
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'
(Assignee)

Comment 24

10 years ago
Created attachment 304090 [details]
Stack trace from failure

Stack trace from failure
(Assignee)

Comment 25

10 years ago
Created attachment 304091 [details]
Frame dump from failure

Frame dump
(Assignee)

Comment 26

10 years ago
Created attachment 304092 [details]
Locals from top 4 stack frames
(Assignee)

Comment 27

10 years ago
Created attachment 304155 [details] [diff] [review]
Version that fixes the crash
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?
(Assignee)

Comment 29

10 years ago
Created attachment 304662 [details] [diff] [review]
Patch that add's the requested comment to nsIFrame.h
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+
Keywords: checkin-needed
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
Last Resolved: 10 years ago
Keywords: checkin-needed → crash
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta4
Version: unspecified → 1.8 Branch

Updated

10 years ago
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.