1418029 - Augment IBSplitting to handle blocks (inside inlines) that are or contain column span children.

Status: RESOLVED DUPLICATE of bug 1421105

(Core :: Layout: Columns, enhancement, P3)

Description

[Neerja Pancholi[:neerja]]

This is a piece of the column-span implementation patch (See Bug 616436) that adds a flag to nsIFrame.h and this flag indicates if the current frame is a child of a valid multi-column ancestor.

Comment 1

[Neerja Pancholi[:neerja]]

Attachment: Bug 1418029 - Part1:Add pseudo styles for column span wrappers and ColumnSetFrames since they are now children of ColumnSetWrapperFrame.

Review commit: https://reviewboard.mozilla.org/r/200416/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/200416/

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8929123 [details]
Bug 1418029 - Part1:Add pseudo styles for column span wrappers and ColumnSetFrames since they are now children of ColumnSetWrapperFrame.

https://reviewboard.mozilla.org/r/200416/#review205698

So in terms of good ways to split patches out -- I don't think it's a good idea to add a flag like this without adding the code that makes it correct.  I'd include the code that sets the flag in this bug, so that you're not adding a flag that doesn't do what is documented.

Furthermore, I think the flag that you want to add for column-span should be a flag that says whether there's a multicol ancestor **in the same block formatting context** (to use the spec's terminology), and it should be documented as such.  Perhaps in a clearer way, given that the spec's wording is a little bit ambiguous given that the block formatting context might be established by the columns rather than the multicol...

Comment 3

[Neerja Pancholi[:neerja]]

(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #2)

Ok, the usage of this flag is spread out in many places so I guess this is going to be part of a bigger patch that will come later. I will also update the comments as you described. My intention behind splitting it this way was to get pieces of the column-span patch reviewed that I know would not change and would still be safe to land as I am working through some of the try failures.  
Thanks!

Comment 4

[Jet Villegas (inactive)]

You can also hang multiple patches on bug 616436 and get reviews on the parts that are ready.

Comment 5

[Neerja Pancholi[:neerja]]

Attachment: Bug 1418029 - Part3:Augment IBSplitting so that a block inside an inline can now be split into runs of blocks with and without column-span, generating multiple block children instead of just one.

This change is necessary for supporting column-span in the case when there is a block containing a descendent column-span inside an inline. In this case, the column-span will cause the block to
split up intil the inline parent is encountered. Now, when IB splitting happens, the inline parent must wrap its block children with column-span inside a column span wrapper anonymous box so
that the inline's block ancestor can continue column-span splitting, if needed.

Review commit: https://reviewboard.mozilla.org/r/201370/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/201370/

Comment 6

[Neerja Pancholi[:neerja]]

Attachment: Bug 1418029 - Part2:Add a flag in nsIFrame that determines if a frame has an ancestor with column-count or column-width set.

Review commit: https://reviewboard.mozilla.org/r/201372/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/201372/

Comment 7

[Neerja Pancholi[:neerja]]

Comment on attachment 8929123 [details]
Bug 1418029 - Part1:Add pseudo styles for column span wrappers and ColumnSetFrames since they are now children of ColumnSetWrapperFrame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200416/diff/1-2/

Comment 8

[Neerja Pancholi[:neerja]]

(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #2)
> Comment on attachment 8929123 [details]

> So in terms of good ways to split patches out -- I don't think it's a good
> idea to add a flag like this without adding the code that makes it correct. 
> I'd include the code that sets the flag in this bug, so that you're not
> adding a flag that doesn't do what is documented.

I've broadened the original bug description and this patch now contains a valid use of the HasMulticolAncestor flag. 


> Furthermore, I think the flag that you want to add for column-span should be
> a flag that says whether there's a multicol ancestor **in the same block
> formatting context** (to use the spec's terminology), and it should be
> documented as such.  Perhaps in a clearer way, given that the spec's wording
> is a little bit ambiguous given that the block formatting context might be
> established by the columns rather than the multicol...

I thought about your comment above and I realized that this flag doesn't actually check for the same BFC and this is intentional, which is why I never updated the comments. The only thing that this flag does is guarantee that the column-span is a descendant of a frame with column-count or column-width set. The BFC part happens in other ways, eg: Floats and AbsPos create a new BFC but any column-spans inside them are ignored for the purpose of splitting (although the column-spans themselves still create BFCs)
But this is out of the scope of the current patch.

Comment 9

[Neerja Pancholi[:neerja]]

The try run for these patches is clean - 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=885a339595167ac09071c784931333ad571a18b0

Comment 10

[Neerja Pancholi[:neerja]]

Comment on attachment 8929123 [details]
Bug 1418029 - Part1:Add pseudo styles for column span wrappers and ColumnSetFrames since they are now children of ColumnSetWrapperFrame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200416/diff/2-3/

Comment 11

[Neerja Pancholi[:neerja]]

Comment on attachment 8930220 [details]
Bug 1418029 - Part3:Augment IBSplitting so that a block inside an inline can now be split into runs of blocks with and without column-span, generating multiple block children instead of just one.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201370/diff/1-2/

Comment 12

[Neerja Pancholi[:neerja]]

Comment on attachment 8930221 [details]
Bug 1418029 - Part2:Add a flag in nsIFrame that determines if a frame has an ancestor with column-count or column-width set.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201372/diff/1-2/

Comment 13

[Neerja Pancholi[:neerja]]

Comment on attachment 8929123 [details]
Bug 1418029 - Part1:Add pseudo styles for column span wrappers and ColumnSetFrames since they are now children of ColumnSetWrapperFrame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200416/diff/3-4/

Comment 14

[Neerja Pancholi[:neerja]]

Comment on attachment 8930220 [details]
Bug 1418029 - Part3:Augment IBSplitting so that a block inside an inline can now be split into runs of blocks with and without column-span, generating multiple block children instead of just one.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201370/diff/2-3/

Comment 15

[Neerja Pancholi[:neerja]]

Comment on attachment 8930221 [details]
Bug 1418029 - Part2:Add a flag in nsIFrame that determines if a frame has an ancestor with column-count or column-width set.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201372/diff/2-3/

Comment 16

[Neerja Pancholi[:neerja]]

Comment on attachment 8929123 [details]
Bug 1418029 - Part1:Add pseudo styles for column span wrappers and ColumnSetFrames since they are now children of ColumnSetWrapperFrame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200416/diff/4-5/

Comment 17

[Neerja Pancholi[:neerja]]

Comment on attachment 8930220 [details]
Bug 1418029 - Part3:Augment IBSplitting so that a block inside an inline can now be split into runs of blocks with and without column-span, generating multiple block children instead of just one.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201370/diff/3-4/

Comment 18

[Neerja Pancholi[:neerja]]

Comment on attachment 8930221 [details]
Bug 1418029 - Part2:Add a flag in nsIFrame that determines if a frame has an ancestor with column-count or column-width set.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201372/diff/3-4/

Comment 19

[Neerja Pancholi[:neerja]]

Comment on attachment 8929123 [details]
Bug 1418029 - Part1:Add pseudo styles for column span wrappers and ColumnSetFrames since they are now children of ColumnSetWrapperFrame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200416/diff/5-6/

Comment 20

[Neerja Pancholi[:neerja]]

Comment on attachment 8930220 [details]
Bug 1418029 - Part3:Augment IBSplitting so that a block inside an inline can now be split into runs of blocks with and without column-span, generating multiple block children instead of just one.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201370/diff/4-5/

Comment 21

[Neerja Pancholi[:neerja]]

Comment on attachment 8930221 [details]
Bug 1418029 - Part2:Add a flag in nsIFrame that determines if a frame has an ancestor with column-count or column-width set.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201372/diff/4-5/

Comment 22

[Neerja Pancholi[:neerja]]

Comment on attachment 8929123 [details]
Bug 1418029 - Part1:Add pseudo styles for column span wrappers and ColumnSetFrames since they are now children of ColumnSetWrapperFrame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200416/diff/6-7/

Comment 23

[Neerja Pancholi[:neerja]]

Comment on attachment 8930220 [details]
Bug 1418029 - Part3:Augment IBSplitting so that a block inside an inline can now be split into runs of blocks with and without column-span, generating multiple block children instead of just one.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201370/diff/5-6/

Comment 24

[Neerja Pancholi[:neerja]]

Comment on attachment 8930221 [details]
Bug 1418029 - Part2:Add a flag in nsIFrame that determines if a frame has an ancestor with column-count or column-width set.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201372/diff/5-6/

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8929123 [details]
Bug 1418029 - Part1:Add pseudo styles for column span wrappers and ColumnSetFrames since they are now children of ColumnSetWrapperFrame.

https://reviewboard.mozilla.org/r/200416/#review216390

r=dbaron with the following changes

::: layout/style/nsCSSAnonBoxList.h:65
(Diff revision 7)
> +CSS_ANON_BOX(mozColumnSpanWrapper, ":-moz-column-span-wrapper")
> +CSS_ANON_BOX(mozColumnSet, ":-moz-column-set")

This should have brief comments describing them (i.e., that the column span wrapper exists for each of the element between a column-span: all element and its nearest ancestor multicol (but with a single wrapper for multiple consecutive column-spans??), and a column set is a set of columns inside of a multicol, not containing a column-span).

::: layout/style/res/ua.css:183
(Diff revision 7)
> +/* This is used for styling wrappers for one of more frames with column-span set
> + * during splitting frames due to the presence of a column-span. The parent of
> + * the topmost column span wrapper is the ColumnSetWrapperFrame.
> + */

I think the existing convention is to put the comments describing what the anonymous boxes are elsewhere, rather than in the style sheet.  So I think this comment should be removed (see above).  Same for the next comment.

::: layout/style/res/ua.css:188
(Diff revision 7)
> +/* This is used for styling wrappers for one of more frames with column-span set
> + * during splitting frames due to the presence of a column-span. The parent of
> + * the topmost column span wrapper is the ColumnSetWrapperFrame.
> + */
> +*|*::-moz-column-span-wrapper {
> +  opacity: unset;

I don't see what this does.  It seems like opacity should already have its initial value.  I think this should be removed.

::: layout/style/res/ua.css:196
(Diff revision 7)
> +  /* Prevent inheriting of borders on ColumnSetFrames since they are now
> +     inherited by ColumnSetWrapper frame instead*/

I'm not sure what this comment means.  Borders don't normally inherit, and there are no styles here relating to borders.  I think this should be removed.

Comment 26

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8930221 [details]
Bug 1418029 - Part2:Add a flag in nsIFrame that determines if a frame has an ancestor with column-count or column-width set.

https://reviewboard.mozilla.org/r/201372/#review216394

::: layout/generic/nsIFrame.h:4376
(Diff revision 6)
>  protected:
> +  /**
> +   * True if the frame has an ancestor that has either column-count or
> +   * column-width defined.
> +   */
> +  bool mHasMulticolAncestor : 1;

Just FYI, this may be more easily maintained as a style-context bit. Actually Servo layout has a bit for exactly the same purpose, so all the code to handle dynamic changes to it and such is there already, see:

  https://github.com/servo/servo/pull/18893

Comment 27

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8930221 [details]
Bug 1418029 - Part2:Add a flag in nsIFrame that determines if a frame has an ancestor with column-count or column-width set.

https://reviewboard.mozilla.org/r/201372/#review216394

> Just FYI, this may be more easily maintained as a style-context bit. Actually Servo layout has a bit for exactly the same purpose, so all the code to handle dynamic changes to it and such is there already, see:
> 
>   https://github.com/servo/servo/pull/18893

(Well, I guess if you reframe each time the condition changes you get some sort of dynamic change handling for it for free to some extent)

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8930220 [details]
Bug 1418029 - Part3:Augment IBSplitting so that a block inside an inline can now be split into runs of blocks with and without column-span, generating multiple block children instead of just one.

https://reviewboard.mozilla.org/r/201370/#review216406

Here are some initial comments, but I still need to do more review of this next week.

::: layout/base/nsCSSFrameConstructor.h:1935
(Diff revision 6)
> +   * non-column-span wrapper is either given the parent's style context or
> +   * aNonSpannerSC if it exists. It also sets IB-split bits here.
> +   * We refer to this process as 'splitting'. The list of wrapped children
> +   * is returned as aSplitChildItems.
> +   */
> +  void SplitBlocks(nsFrameConstructorState& aState,

I wonder if this should be called SplitAndWrapBlocks or maybe even just WrapBlocks, since I think the key thing this does is create the block wrapper(s).

::: layout/base/nsCSSFrameConstructor.cpp:688
(Diff revision 6)
> +static nsFrameList::FrameLinkEnumerator
> +FindFirstColumnSpan(const nsFrameList& aList)
> +{
> +  nsFrameList::FrameLinkEnumerator link(aList);
> +  for (; !link.AtEnd(); link.Next()) {
> +    if (link.NextFrame()->StyleContext()->StyleColumn()->mColumnSpan

Omit the "->StyleContext()" here; you can call StyleColumn() directly on a frame.

Same for the following function.

Also, maybe these should just be a single function that takes the desired column-span value (either the value to skip over or the value to find).

::: layout/base/nsCSSFrameConstructor.cpp:12189
(Diff revision 6)
> +  // Mark if this inline is under a multicol element
> +  newFrame->SetHasMulticolAncestor(aParentFrame->HasMulticolAncestor());
> +

This seems like it might be in the wrong patch.

I haven't yet seen a patch introducing the SetHasMulticolAncestor method.  If I did, then it ought to be in the dependency list of this bug.

(Same thing later on.)

Comment 29

[Neerja Pancholi[:neerja]]

Comment on attachment 8929123 [details]
Bug 1418029 - Part1:Add pseudo styles for column span wrappers and ColumnSetFrames since they are now children of ColumnSetWrapperFrame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200416/diff/7-8/

Comment 30

[Neerja Pancholi[:neerja]]

Comment on attachment 8930220 [details]
Bug 1418029 - Part3:Augment IBSplitting so that a block inside an inline can now be split into runs of blocks with and without column-span, generating multiple block children instead of just one.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201370/diff/6-7/

Comment 31

[Neerja Pancholi[:neerja]]

Comment on attachment 8930221 [details]
Bug 1418029 - Part2:Add a flag in nsIFrame that determines if a frame has an ancestor with column-count or column-width set.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201372/diff/6-7/

Comment 32

[Neerja Pancholi[:neerja]]

Comment on attachment 8929123 [details]
Bug 1418029 - Part1:Add pseudo styles for column span wrappers and ColumnSetFrames since they are now children of ColumnSetWrapperFrame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200416/diff/8-9/

Comment 33

[Neerja Pancholi[:neerja]]

Comment on attachment 8930220 [details]
Bug 1418029 - Part3:Augment IBSplitting so that a block inside an inline can now be split into runs of blocks with and without column-span, generating multiple block children instead of just one.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201370/diff/7-8/

Comment 34

[Neerja Pancholi[:neerja]]

Comment on attachment 8930221 [details]
Bug 1418029 - Part2:Add a flag in nsIFrame that determines if a frame has an ancestor with column-count or column-width set.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201372/diff/7-8/

Comment 35

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So I think I'm going to keep running into issues where bits are in the wrong patches, given that I've hit that both in comment 28 and in bug 1346983 comment 38.  I think it would help if you check that things compile at each step of the patch sequence (i.e., compile with just the first patch, and then with the first two, etc.) and fix the things that are wrong (i.e., in the wrong patches) to make that work.  Having bits in the wrong patches makes them much harder to review.

It would also help if you clarify via the dependencies what the order of the patches across bugs is.

Comment 36

[Neerja Pancholi[:neerja]]

Comment on attachment 8929123 [details]
Bug 1418029 - Part1:Add pseudo styles for column span wrappers and ColumnSetFrames since they are now children of ColumnSetWrapperFrame.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200416/diff/9-10/

Comment 37

[Neerja Pancholi[:neerja]]

Comment on attachment 8930221 [details]
Bug 1418029 - Part2:Add a flag in nsIFrame that determines if a frame has an ancestor with column-count or column-width set.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201372/diff/8-9/

Comment 38

[Neerja Pancholi[:neerja]]

Comment on attachment 8930220 [details]
Bug 1418029 - Part3:Augment IBSplitting so that a block inside an inline can now be split into runs of blocks with and without column-span, generating multiple block children instead of just one.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201370/diff/8-9/

Comment 39

[Neerja Pancholi[:neerja]]

Comment on attachment 8930220 [details]
Bug 1418029 - Part3:Augment IBSplitting so that a block inside an inline can now be split into runs of blocks with and without column-span, generating multiple block children instead of just one.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201370/diff/9-10/

Comment 40

[Neerja Pancholi[:neerja]]

(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #35)
> So I think I'm going to keep running into issues where bits are in the wrong
> patches, given that I've hit that both in comment 28 
The SetHasMulticolAncestor function you mentioned in comment 28 was in the next patch for this same bug. I have re-ordered the patches in this bug to make that clearer. (Bugzilla does not show this reordering in the 'Attachments' section but it correctly shows this in the 'MozReview Requests' section)

> and in bug 1346983 comment 38.  
The AppendDirectlyOwnedAnonBoxes function was in the wrong patch. I have removed it here and made sure that each patch builds.


> It would also help if you clarify via the dependencies what the order of the
> patches across bugs is.

The following bug dependency was added for others to easily discover related bugs from the meta bug, so I am not removing this even though this does not specify the order of patches -
Bug 616436 (META) -> Bug 1346983
Bug 616436 (META) -> Bug 1418029
Bug 616436 (META) -> Bug 1421105

I am removing the dependency changed by you for Bug 1421105 since it breaks the original order and it can be more simply represented as -
Bug 616436 (META) -> Bug 1346983 -> Bug 1418029 -> Bug 1421105
(But, this makes it more confusing to see all the patches in the dependency chain. This is also the order in which the patches were originally intended to be reviewed.) 

So, the final order of the patches is:
Bug 616436 (META) -> Bug 1346983 -> Bug 1418029 -> Bug 1421105

Comment 41

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

The patches in this bug have been rewritten, and I'll use Bug 1421105 to land them. I'll close this.

Comment 42

[Daniel Holbert [:dholbert]]

Perhaps cleanest to dupe forward to bug 1421105 then.

WORKSFORME implies that the bug has already been addressed somehow but we're not entirely sure where:
  https://wiki.mozilla.org/BMO/UserGuide/BugStatuses

Also: would you mind canceling the still-open review requests here, if that makes sense? (I assume it does)

Comment 43

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

(In reply to Daniel Holbert [:dholbert] from comment #42)
> Perhaps cleanest to dupe forward to bug 1421105 then.
> 
> WORKSFORME implies that the bug has already been addressed somehow but we're
> not entirely sure where:
>   https://wiki.mozilla.org/BMO/UserGuide/BugStatuses

Thanks! 

> Also: would you mind canceling the still-open review requests here, if that
> makes sense? (I assume it does)

The review requests are no longer needed. I'll mark those attachments obsolete.