User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/43.0.2357.81 Safari/537.36 Steps to reproduce: style the ordered list: ol { display: flex; flex-direction: column; } demo: http://jsbin.com/yadoqi/1/edit?css,output Actual results: list counter is all 0s now, like 0. foo 0. bar 0. baz Expected results: list counter should not be affected 1. foo 2. bar 3. baz

So this is the same as bug 1161825 or bug 1129490... but I'm not sure it's exactly the same thing as bug 4522 anymore. Looking at the frame dump, the parent is of course an "ol" here. It's just that the parent of the list items is not a blockframe. It's a FlexContainer instead. I'm not sure whether it makes sense to make the list handling work with those in addition to blocks, pending the bigger rewrite of list counters that bug 4522 requires.

David, thoughts on comment 1?

I don't think I have strong opinions either way. I guess it's probably pretty quick to copy/share code from nsBlockFrame into nsFlexContainerFrame (which we'd likely have to repeat in other places in short order), though I also think it's not a particularly huge undertaking to really fix the problem (but it is at least a few weeks of work).

Just thought I'd note that this is also broken for Grid. http://labs.jensimmons.com/examples/image-gallery-grid-1-bug-demo.html :dholbert: said it's due to: https://bugzilla.mozilla.org/show_bug.cgi?id=4522

I'm working on implementing this... https://treeherder.mozilla.org/#/jobs?repo=try&revision=ced19dd6fb97

Daniel, should I ask someone else for review here in case you're overloaded? FYI, it's small and simple patches that mostly just refactor existing code, moving the list numbering stuff from nsBlockFrame to nsContainerFrame.

That would be great, thank you! (and sorry for the wait.) I am indeed a bit overloaded at the moment. I think xidorn would make a great substitute-reviewer here, since he's worked with reflow & nsContainerFrame IIRC, as well as our css-generated-lists code. I just checked on IRC, too, and he's cool with it. (Thanks xidorn!)

The patches mostly look good to me. I get confused in the method names for a while. Given you have renamed some of the methods, could you probably also rename others and add some comment to make their function more clear? My proposal is something like: * RenumberLists -> RenumberList (We are really only renumbering one list) Renumber the list of the counter scope started by this frame, if any. * RenumberChildListItems -> RenumberChildFrames (or RenumberDescendentListItems, not just child) * RenumberListItems -> RenumberFrame Renumber this frame (if itself is a list item) and its descendent frames. Does it make sense to you?

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #20) > * RenumberLists -> RenumberList (We are really only renumbering one list) OK > * RenumberChildListItems -> RenumberChildFrames (or > RenumberDescendentListItems, not just child) RenumberChildFrames works for me. > * RenumberListItems -> RenumberFrame > Renumber this frame (if itself is a list item) and its descendent frames. RenumberFrame sounds a bit like it's only renumbering that frame, but it also calls RenumberChildFrames to recurse on the children. So, RenumberFrameAndDescendants or RenumberFrameRecursive ? BTW, thanks for picking up the review!

(In reply to Mats Palmgren (:mats) from comment #21) > RenumberFrame sounds a bit like it's only renumbering that frame, > but it also calls RenumberChildFrames to recurse on the children. > So, RenumberFrameAndDescendants or RenumberFrameRecursive ? Yeah, RenumberFrameAndDescendants sounds better.

Comment on attachment 8786929 [details] [diff] [review] part 1 - Move some list-item numbering code from nsBlockFrame to nsContainerFrame. Review of attachment 8786929 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsBlockFrame.cpp @@ +6978,1 @@ > Could you also remove the tailing whitespace in this line?

Comment on attachment 8783503 [details] [diff] [review] part 6 - Move nsBlockFrame::AttributeChanged <ol> handling to nsContainerFrame so that it works not just for blocks. Review of attachment 8783503 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsContainerFrame.cpp @@ +1980,5 @@ > > return renumbered; > } > > +nsresult nits: please remove this tailing space. ::: layout/generic/nsContainerFrame.h @@ +68,5 @@ > virtual bool IsLeaf() const override; > virtual FrameSearchResult PeekOffsetNoAmount(bool aForward, int32_t* aOffset) override; > virtual FrameSearchResult PeekOffsetCharacter(bool aForward, int32_t* aOffset, > bool aRespectClusters = true) override; > And could you also remove this tailing space?

Comment on attachment 8783507 [details] [diff] [review] part 11 - Reftests for grid/flex list-item numbering. Review of attachment 8783507 [details] [diff] [review]: ----------------------------------------------------------------- I wonder whether these tests should be put inside web-platform-test rather than our internal test set.

> I wonder whether these tests should be put inside web-platform-test rather than our internal test set. Grid isn't preffed on by default yet so that seems premature.

Pushed by mpalmgren@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/df3331607674 part 1 - Move some list-item numbering code from nsBlockFrame to nsContainerFrame. r=xidorn https://hg.mozilla.org/integration/mozilla-inbound/rev/851a6b413bfc part 2 - Remove useless aPresContext param from RenumberLists. r=xidorn https://hg.mozilla.org/integration/mozilla-inbound/rev/f7f7acf36ba8 part 3 - Override RenumberFrameAndDescendants in nsPlaceholderFrame instead and deal with it there rather than nsContainerFrame. r=xidorn https://hg.mozilla.org/integration/mozilla-inbound/rev/2deec338fc88 part 4 - Move nsBlockFrame::RenumberLists to nsContainerFrame and generalize it. r=xidorn https://hg.mozilla.org/integration/mozilla-inbound/rev/db84cab5859b part 5 - Implement nsContainerFrame::RenumberChildFrames and make RenumberFrameAndDescendants deal with flex/grid containers too. r=xidorn https://hg.mozilla.org/integration/mozilla-inbound/rev/098badb45dee part 6 - Move nsBlockFrame::AttributeChanged <ol> handling to nsContainerFrame so that it works not just for blocks. r=xidorn https://hg.mozilla.org/integration/mozilla-inbound/rev/469a5ca9c661 part 7 - Make nsBlockFrame::AttributeChanged look for flex/grid ancestors too, not just blocks. r=xidorn https://hg.mozilla.org/integration/mozilla-inbound/rev/a47028aa4b59 part 8 - Implement list-item numbering for grid containers. r=xidorn https://hg.mozilla.org/integration/mozilla-inbound/rev/b403df416db1 part 9 - Implement list-item numbering for flex containers. r=xidorn https://hg.mozilla.org/integration/mozilla-inbound/rev/04d649b88650 part 10 - Rename RenumberLists() to RenumberList(). r=xidorn https://hg.mozilla.org/integration/mozilla-inbound/rev/28b4b2e50fb0 part 11 - Reftests for grid/flex list-item numbering.

(In reply to Mats Palmgren (:mats) from comment #33) > > I wonder whether these tests should be put inside web-platform-test rather than our internal test set. > > Grid isn't preffed on by default yet so that seems premature. That doesn't really matter. We can specify pref needed for a test.

https://hg.mozilla.org/mozilla-central/rev/df3331607674 https://hg.mozilla.org/mozilla-central/rev/851a6b413bfc https://hg.mozilla.org/mozilla-central/rev/f7f7acf36ba8 https://hg.mozilla.org/mozilla-central/rev/2deec338fc88 https://hg.mozilla.org/mozilla-central/rev/db84cab5859b https://hg.mozilla.org/mozilla-central/rev/098badb45dee https://hg.mozilla.org/mozilla-central/rev/469a5ca9c661 https://hg.mozilla.org/mozilla-central/rev/a47028aa4b59 https://hg.mozilla.org/mozilla-central/rev/b403df416db1 https://hg.mozilla.org/mozilla-central/rev/04d649b88650 https://hg.mozilla.org/mozilla-central/rev/28b4b2e50fb0