Tomasz Stryjewski Reporter
	Description • 9 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 1 • 9 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 2 • 9 years ago

David, thoughts on comment 1?

	David Baron :dbaron:
	Comment 5 • 9 years ago

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

	Jen Simmons [:jensimmons]
	Comment 6 • 8 years ago

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

	Mats Palmgren (inactive) Assignee
	Comment 7 • 8 years ago

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

	Mats Palmgren (inactive) Assignee
	Comment 18 • 8 years ago

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.

	Daniel Holbert [:dholbert]
	Comment 19 • 8 years ago

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

	Xidorn Quan [:xidorn] UTC+11
	Comment 20 • 8 years ago

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?

	Mats Palmgren (inactive) Assignee
	Comment 21 • 8 years ago

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

	Xidorn Quan [:xidorn] UTC+11
	Comment 22 • 8 years ago

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

	Mats Palmgren (inactive) Assignee
	Comment 29 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e36a895a58aa

	Xidorn Quan [:xidorn] UTC+11
	Comment 30 • 8 years ago

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?

	Xidorn Quan [:xidorn] UTC+11
	Comment 31 • 8 years ago

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?

	Xidorn Quan [:xidorn] UTC+11
	Comment 32 • 8 years ago

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.

	Mats Palmgren (inactive) Assignee
	Comment 33 • 8 years ago

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

	Pulsebot
	Comment 34 • 8 years ago

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.

	Xidorn Quan [:xidorn] UTC+11
	Comment 35 • 8 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 36 • 8 years ago
bugherder

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