Closed Bug 1346983 Opened 7 years ago Closed 6 years ago

Create ColumnSetWrapper to be used as a parent for nsColumnSetFrames and spanners. (nsColumnSetFrames will be split on the basis of column-span elements)

Categories

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

Product:
Core
Component:
Layout: Columns
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla64
Tracking Flags:
Tracking Status
firefox64 --- fixed

People

(Reporter: neerja, Assigned: TYLin)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 1 obsolete file)

Bug 1346983 - Add a new nsColumnSetWrapperFrame class to the build to be used as a wrapper for columnset and column span frames.
59 bytes, text/x-review-board-request
neerja
: review?
dbaron
Details
Testcase
3.13 KB, text/html
Details
examples of multicol breaking tests
1.69 KB, text/html
Details
Bug 1346983 - Add ColumnSetWrapperFrame to wrap nsColumnSetFrames and column span frames.
46 bytes, text/x-phabricator-request
dbaron
: review+
Details | Review 
      No description provided.
Blocks: column-span
Assignee: nobody → npancholi
That sounds weird to me.  Why can't you just keep the current frame tree structure,
just making the inner anonymous frame that represents the column span wider?
Has the suggested design here been discussed somewhere?
Flags: needinfo?(npancholi)
(In reply to Mats Palmgren (:mats) from comment #1)
> That sounds weird to me.  Why can't you just keep the current frame tree
> structure, just making the inner anonymous frame that represents the column span wider?

I'm not sure how that would work.  Mind hopping in #layout on IRC & we can talk it over?  (if it's a good time for you)

> Has the suggested design here been discussed somewhere?

Neerja's discussed possible designs in real life with me & dbaron, though I'm not sure we've captured the final plan anywhere.  But if you look at the top half of attachment 577766 [details] (above the <hr>), it's pretty clear (to me at least) that we need to split the multicol element into two different nsColumnSetFrames, with the spanning thing being between them.  And then we need a container for all of those things, to draw borders/outlines & to manage their reflow / height-balancing.  So this is a step in that direction, I think.
Flags: needinfo?(npancholi)
Comment on attachment 8846875 [details]
Bug 1346983 - Add a new nsColumnSetWrapperFrame class to the build to be used as a wrapper for columnset and column span frames.

https://reviewboard.mozilla.org/r/119868/#review121754

First-pass review for the boilerplate stuff:

::: commit-message-7b19a:1
(Diff revision 1)
> +Bug 1346983 - Part1:Add a new nsColumnSetWrapperFrame class to the build to be used as parent of nsColumnSetFrame. r?dholbert

Add a space after the colon, please ("Part1: Add"), not "Part1:Add"

::: layout/generic/nsColumnSetWrapperFrame.h:5
(Diff revision 1)
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

This file needs the same one-line summary comment that you've got in the .cpp file.

::: layout/generic/nsColumnSetWrapperFrame.h:11
(Diff revision 1)
> +#ifndef nsColumnSetWrapperFrame_h___
> +#define nsColumnSetWrapperFrame_h___
> +
> +#include "nsContainerFrame.h"
> +
> +class nsColumnSetWrapperFrame final : public nsContainerFrame

Please include a comment before this "class" line, to explain the purpose of this class.

(This is kind of similar to the one-liner comment at the top of the file, except you can include a bit more detail here if you like.  This comment should at least explain the difference between this class & nsColumnSetFrame.)

(You may find that you're adding code-comments that aren't technically true until later patches have landed.  If you like, you can add an XXX/TODO line at the end of such comments, saying e.g. "This part is coming in a later patch in this series" to be clearer about that.  (or "...coming in bug N", for whatever bug is adding the thing).  And then you'd remove that comment in the patch that adds the thing.

::: layout/generic/nsColumnSetWrapperFrame.h:11
(Diff revision 1)
> +class nsColumnSetWrapperFrame final : public nsContainerFrame
> +{
> +  public:
> +  NS_DECL_FRAMEARENA_HELPERS

"public" should be deindented here -- see sample code at:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Classes

::: layout/generic/nsColumnSetWrapperFrame.h:16
(Diff revision 1)
> +class nsColumnSetWrapperFrame final : public nsContainerFrame
> +{
> +  public:
> +  NS_DECL_FRAMEARENA_HELPERS
> +
> +  explicit nsColumnSetWrapperFrame(nsStyleContext* aContext);

A few more boilerplate functions that you should add:
- GetType()
- GetFrameName() (inside of #ifdef DEBUG_FRAME_DUMP)

::: layout/generic/nsColumnSetWrapperFrame.cpp:12
(Diff revision 1)
> +
> +
> +#include "nsColumnSetWrapperFrame.h"
> +
> +using namespace mozilla;
> +using namespace mozilla::layout;

You probably don't need the mozilla::layout namespace here. We don't use it for much. (And this file isn't using it for anything right now.)
Attachment #8846875 - Flags: review?(dholbert) → review-
Design outline for implementing Column-Span -

The implementation for multicolumn properties currently uses nsColumnSetFrame
as the parent container for all columns and this manages the borders, reflow,
balancing etc of all its child columns. When looking at the expected result in 
attachment 577766 [details], we can see that adding a column-span seems to divide the 
original set of columns (without column-span enabled) into two different sets of 
columns. Opening this attachment in a browser that supports column-span, 
we can see that the content of these sets of columns is never shared between 
each other even on resizing. Therefore, for this implementation we can look at
them as independent nsColumnSetFrames managing their own set of columns 
interspersed with block level elements representing spanning elements. The
number of nsColumnSetFrames will depend on the number of spanning elements
because each new spanning element will cause the content (if any) to be split 
across different nsColumnSetFrames.
The wrapper class that will contain these nsColumnSetFrames and spanning elements
will be the new class nsColumnSetWrapperFrame and we will apply pseudo-styles
to this class to get the correct behavior for borders etc.
Comment on attachment 8846876 [details]
Bug 1346983 - Part2: Insert the new nsColumnSetWrapperFrame into the frame hierarchy and reflow children.

https://reviewboard.mozilla.org/r/119870/#review121774

::: commit-message-5b86e:1
(Diff revision 1)
> +Bug 1346983 - Part2: Insert the new nsColumnSetWrapperFrame into the frame heirarchy and reflow children. r?dholbert

Typo: "hierarchy" (i before e)

::: layout/generic/nsColumnSetWrapperFrame.h:17
(Diff revision 1)
>  {
>    public:
>    NS_DECL_FRAMEARENA_HELPERS
>  
>    explicit nsColumnSetWrapperFrame(nsStyleContext* aContext);
> -  ~nsColumnSetWrapperFrame();
> +  virtual ~nsColumnSetWrapperFrame();

Please revert this change in this patch. (We'll want to make a slightly different version of this change, and we'll want to do it back in part 1. More thoughts coming on that, as an additional note for part 1.)

::: layout/generic/nsColumnSetWrapperFrame.cpp:32
(Diff revision 1)
> +void
> +nsColumnSetWrapperFrame::Reflow(nsPresContext*     aPresContext,
> +                                ReflowOutput&      aDesiredSize,

It doesn't look like you're adding a declaration for this function in the .h file yet -- I think you need to do that.

You'll also need to add declarations & definitions for GetMinISize & GetPrefISize functions, too -- those are associated with Reflow (and pretty much every frame class needs them)  I suspect you should hoist some/most of the already-existing nsColumnSetFrame implementation for these functions (GetMinISize/GetPrefISize) up into this wrapper frame...  It looks like that basically defers to the child's ISize, and then enforces that we're at least as wide as however many columns we know we must have.

::: layout/generic/nsColumnSetWrapperFrame.cpp:42
(Diff revision 1)
> +  // Initialize OUT parameter
> +  aStatus.Reset();

This Reset() call is unnecessary, though we do have this sprinkled around due to an automated int-->smarter-class conversion which wanted to preserve old semantics.

Please replace this with something like:
MOZ_ASSERT(aStatus.IsEmpty(), "Expecting outparam to be initially empty");

::: layout/generic/nsColumnSetWrapperFrame.cpp:47
(Diff revision 1)
> +  // Initialize OUT parameter
> +  aStatus.Reset();
> +
> +  WritingMode wm = GetWritingMode();
> +  nsSize containerSize = aReflowInput.ComputedSizeAsContainerIfConstrained();
> +  for (nsFrameList::Enumerator e(mFrames); !e.AtEnd(); e.Next()) {

Nowadays we can just iterate like so:
  for (nsIFrame* childFrame : mFrames) {

::: layout/generic/nsColumnSetWrapperFrame.cpp:48
(Diff revision 1)
> +    nsColumnSetFrame* child = static_cast<nsColumnSetFrame*>(e.get());
> +    child->Reflow(aPresContext, aDesiredSize, aReflowInput, aStatus);
> +

* Do you need this static_cast? It doesn't look like you do any nsColumnSetFrame-specific stuff here, so I suspect a nsIFrame* is just fine (and you don't need to static_cast).
* You probably want to use ReflowChild rather than child->Reflow.  (ReflowChild takes care of a few more things for you. See callers for sample usage.)
* You shouldn't just pass the same args through that we received -- you need to set up a local nsHTMLReflowState and a nsReflowStatus to pass to the child.

::: layout/generic/nsColumnSetWrapperFrame.cpp:51
(Diff revision 1)
> +    LogicalMargin borderPadding = aReflowInput.ComputedLogicalBorderPadding();
> +    LogicalPoint childOrigin(wm,
> +                             borderPadding.IStart(wm),
> +                             borderPadding.BStart(wm));
> +    FinishReflowChild(child, aPresContext, aDesiredSize, &aReflowInput,
> +                      child->GetWritingMode(), childOrigin, containerSize, 0);

I imagine right now the child tries to manage its own borderPadding, too. So this probably applies the borderPadding twice now (for positioning at least, since that's what this code is doing -- not for painting).

You probably do want to apply the borderPadding here, but in the patch where that happens [this patch for now], you also need to *remove* it from nsColumnSetFrame::Reflow so that it doesn't get applied twice.  (And you'll also need to add a BuildDisplayList method to this frame class in that same patch, too, and hoist some of nsColumnSetFrame::BuildDisplayList's background/border-painting code into this wrapper frame's implementation.)
Attachment #8846876 - Flags: review?(dholbert) → review-
Comment on attachment 8846875 [details]
Bug 1346983 - Add a new nsColumnSetWrapperFrame class to the build to be used as a wrapper for columnset and column span frames.

https://reviewboard.mozilla.org/r/119868/#review121782

::: layout/generic/nsColumnSetWrapperFrame.h:16
(Diff revision 1)
> +  explicit nsColumnSetWrapperFrame(nsStyleContext* aContext);
> +  ~nsColumnSetWrapperFrame();

Two notes on the constructor/destructor:
 - Please put "private:" before them. (Or "protected:".  Of course private/protected are equivalent here since this is a 'final' frame class).  By hiding them, we reduce the possibility that someone can create an instance of this class in a way that we don't expect. (Should only be created via NS_New*** & destroyed via nsFrame::DestroyFrom.  You don't want anybody to be able to just declare a temporary local variable of this type -- they should only exist as part of the frame tree.)

 - Please declare ~nsColumnSetWrapperFrame as "override".  (This is strictly better than declaring it 'virtual' as you initially did in "part 2".  It ensures that the *superclass* has declared it as virtual. We're not good about doing this for destructors in frame classes -- largely because they predated our usage of the "override" keyword at all, I think.)
Another think that I just want to point out here that the scope of this bug is to just add a new wrapper class for (a single) nsColumnSetFrame and not change the behavior at all. It's an intermediate step towards achieving the design in Comment #6 and I've broken it up into a separate bug just for organization purposes.
More details of the implementations will come as part of successive bugs. 
Thanks!
Comment on attachment 8846875 [details]
Bug 1346983 - Add a new nsColumnSetWrapperFrame class to the build to be used as a wrapper for columnset and column span frames.

https://reviewboard.mozilla.org/r/119868/#review121782

> Two notes on the constructor/destructor:
>  - Please put "private:" before them. (Or "protected:".  Of course private/protected are equivalent here since this is a 'final' frame class).  By hiding them, we reduce the possibility that someone can create an instance of this class in a way that we don't expect. (Should only be created via NS_New*** & destroyed via nsFrame::DestroyFrom.  You don't want anybody to be able to just declare a temporary local variable of this type -- they should only exist as part of the frame tree.)
> 
>  - Please declare ~nsColumnSetWrapperFrame as "override".  (This is strictly better than declaring it 'virtual' as you initially did in "part 2".  It ensures that the *superclass* has declared it as virtual. We're not good about doing this for destructors in frame classes -- largely because they predated our usage of the "override" keyword at all, I think.)

I had to make the constructor public because I realized that NS_NewColumnSetWrapperFrame is not a member of nsColumnSetWrapperFrame so a private constructor would mean no way to make an object. Also, now that you mentioned this, I thought it would be better if I removed the default destructor I had made before because that seems to go with the creation/deletion pattern that other frame classes (nsColumnSetFrame in particular) are following. To me it didn't make sense to have a public constructor and private destructor. Let me know what you think, thanks!
Comment on attachment 8846875 [details]
Bug 1346983 - Add a new nsColumnSetWrapperFrame class to the build to be used as a wrapper for columnset and column span frames.

https://reviewboard.mozilla.org/r/119868/#review121782

> I had to make the constructor public because I realized that NS_NewColumnSetWrapperFrame is not a member of nsColumnSetWrapperFrame so a private constructor would mean no way to make an object. Also, now that you mentioned this, I thought it would be better if I removed the default destructor I had made before because that seems to go with the creation/deletion pattern that other frame classes (nsColumnSetFrame in particular) are following. To me it didn't make sense to have a public constructor and private destructor. Let me know what you think, thanks!

> NS_NewColumnSetWrapperFrame is not a member
You should declare NS_NewColumnSetWrapperFrame as "friend" of your frame class -- this gives it the ability to call private methods (including the constructor).  Here's where we do that for nsBlockFrame, for reference: https://dxr.mozilla.org/mozilla-central/rev/6d38ad302429c98115c354d643e81987ecec5d3c/layout/generic/nsBlockFrame.h#102-103

> removed the default destructor
You should also still declare the destructor (in the "private:" section), even if its implementation is empty -- this helps ensure that code doesn't arbitrarily call "delete" on a nsColumnSetWrapperFrame.
Comment on attachment 8846875 [details]
Bug 1346983 - Add a new nsColumnSetWrapperFrame class to the build to be used as a wrapper for columnset and column span frames.

https://reviewboard.mozilla.org/r/119868/#review121782

> > NS_NewColumnSetWrapperFrame is not a member
> You should declare NS_NewColumnSetWrapperFrame as "friend" of your frame class -- this gives it the ability to call private methods (including the constructor).  Here's where we do that for nsBlockFrame, for reference: https://dxr.mozilla.org/mozilla-central/rev/6d38ad302429c98115c354d643e81987ecec5d3c/layout/generic/nsBlockFrame.h#102-103
> 
> > removed the default destructor
> You should also still declare the destructor (in the "private:" section), even if its implementation is empty -- this helps ensure that code doesn't arbitrarily call "delete" on a nsColumnSetWrapperFrame.

Aah, that makes sense!(Sorry, I guess I didn't think much of it when I saw that the constructor in nsColumnSetFrame was public and that it had no default destructor.)
Comment on attachment 8846875 [details]
Bug 1346983 - Add a new nsColumnSetWrapperFrame class to the build to be used as a wrapper for columnset and column span frames.

https://reviewboard.mozilla.org/r/119868/#review121782

> Aah, that makes sense!(Sorry, I guess I didn't think much of it when I saw that the constructor in nsColumnSetFrame was public and that it had no default destructor.)

I see -- looks like nsColumnSetFrame & a few other frames are just lagging on this convention.  (We should perhaps spread this convention across more of our frame subclasses... If you feel like addressing this in a helper bug, feel free, though also no pressure since it's not super high-priority.)
Comment on attachment 8846875 [details]
Bug 1346983 - Add a new nsColumnSetWrapperFrame class to the build to be used as a wrapper for columnset and column span frames.

https://reviewboard.mozilla.org/r/119868/#review122698

r=me for part 1
Attachment #8846875 - Flags: review?(dholbert) → review+
Attached file Testcase — 
Thank you both for the explanations.  I initially thought 'column-span' also
accepted a <number> value to span just a few columns, but I see the spec only
allows none/all.  That makes it simpler and I think the proposed design here
will work for that case.  It's going to be a little tricky to handle
continuations though.  See this example in Chrome and note how it handles
the end border/padding/margin/opacity.  I suspect that the inner columnset
frames needs to be continuations.  I assume you plan to make the outer
wrapper frame the primary frame.  Watch out for code that currently checks
for "GetType() == nsGkAtoms::columnSetFrame" - you might need to change
some of those to check for columnSetWrapperFrame instead.

(Fwiw, I find it quite disappointing that column-span:<number> isn't
in the spec since that's a very common layout in newspapers.)
Attachment #8846876 - Attachment is obsolete: true
Priority: -- → P3
Attachment #8846875 - Flags: review?(dbaron)
Blocks: 1418029
No longer blocks: 1418029
Depends on: 1418029
Blocks: 1418029
No longer depends on: 1418029
No longer blocks: 1418029
Comment on attachment 8846875 [details]
Bug 1346983 - Add a new nsColumnSetWrapperFrame class to the build to be used as a wrapper for columnset and column span frames.

https://reviewboard.mozilla.org/r/119868/#review211620

A few preliminary comments/questions:

::: layout/generic/nsColumnSetWrapperFrame.h:6
(Diff revision 9)
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/* rendering wrapper object for css3 multi-column layout */

I'd say "frame for css multi-column layout that contains column sets and column spans"

::: layout/generic/nsColumnSetWrapperFrame.h:22
(Diff revision 9)
> + * spanning elements.
> + * This wrapper is necessary for implementing column-span as it allows us to
> + * maintain each nsColumnSetFrame as an independent set of columns and each
> + * spanning element then becomes just a block level element.
> + */
> +class nsColumnSetWrapperFrame final : public nsContainerFrame

I thought we talked about making this inherit from nsBlockFrame rather than nsContainerFrame because that would make fragmentation easier.  (Or am I confusing this with the column-span wrapper?)

Also, are there issues with this containing absolutely-positioned elements when it's position:relative?


If you did that, I suspect you wouldn't need to implement a bunch of the methods that you implement here.

(Also, do the MOZ_CRASH() methods get implemented in a later patch?  If so, where?  If not, you should add a comment explaining why they won't be called.)
(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #26)
> > +/* rendering wrapper object for css3 multi-column layout */
> 
> I'd say "frame for css multi-column layout that contains column sets and
> column spans"
Ok, will change this.
I basically copied this language from the comments in nsColumnSetFrame ->
https://searchfox.org/mozilla-central/source/layout/generic/nsColumnSetFrame.h#10

> ::: layout/generic/nsColumnSetWrapperFrame.h:22
> (Diff revision 9)
> > + * spanning elements.
> > + * This wrapper is necessary for implementing column-span as it allows us to
> > + * maintain each nsColumnSetFrame as an independent set of columns and each
> > + * spanning element then becomes just a block level element.
> > + */
> > +class nsColumnSetWrapperFrame final : public nsContainerFrame
> 
> I thought we talked about making this inherit from nsBlockFrame rather than
> nsContainerFrame because that would make fragmentation easier.  (Or am I
> confusing this with the column-span wrapper?)
Yes, we did. But, I kept the inheritance from nsContainerFrame because all the fragmentation issues that I previously talked to you about got fixed without me having to duplicated pagination code from BlockFrame and I assumed that reftest-print would eventually catch something like this but the try run with my patch is already clean so I couldn't justify changing this.
Maybe the printing tests are not exhaustive enough? Also, ColumnSetFrame doesn't inherit from BlockFrame so I feel ColumnSetWrapper shouldn't need to either. 

> Also, are there issues with this containing absolutely-positioned elements
> when it's position:relative?
Not that I know of at this time. The try run is clean (Save one stray assertion not related to abspos). 


> If you did that, I suspect you wouldn't need to implement a bunch of the
> methods that you implement here.
> 
> (Also, do the MOZ_CRASH() methods get implemented in a later patch?  If so,
> where?  If not, you should add a comment explaining why they won't be
> called.)
Sure, I will add clearer comments but the MOZ_CRASH() methods will not be implemented because they are invalid operations for CSWrapper, same as CSFrame ->
https://searchfox.org/mozilla-central/source/layout/generic/nsColumnSetFrame.cpp#1324-1345
 
Basically any append, insert or remove is disallowed because we must recreate the entire frame hierarchy under CSWrapper in case the added/removed element changes the breaking of frames across column-spans.
Comment on attachment 8846875 [details]
Bug 1346983 - Add a new nsColumnSetWrapperFrame class to the build to be used as a wrapper for columnset and column span frames.

https://reviewboard.mozilla.org/r/119868/#review216362

::: layout/generic/nsColumnSetWrapperFrame.h:21
(Diff revision 13)
> + * spanning elements.
> + * This wrapper is necessary for implementing column-span as it allows us to
> + * maintain each nsColumnSetFrame as an independent set of columns and each
> + * spanning element then becomes just a block level element.
> + */
> +class nsColumnSetWrapperFrame final : public nsContainerFrame

So I'm still a little worried about inheriting from nsContainerFrame rather than nsBlockFrame in terms of what's going to happen when inside of page/column breaking.  The Reflow code that you've written doesn't appear to handle that, whereas if this inherited from nsBlockFrame, I think you'd get most or all of that for "free".  I guess I said this before in comment 26, and you replied in comment 27.

I think you need to test explicitly that page and column breaking of a multicol, either with or with spans, works reasonably.  This should probably involve writing explicit tests -- it's probably easiest to test using column breaking, and using 'column-fill: auto' and an explicit height on the outer multicol to make things simpler.

::: layout/generic/nsColumnSetWrapperFrame.cpp:19
(Diff revision 13)
> +                            nsStyleContext* aContext,
> +                            nsFrameState    aStateFlags)
> +{
> +  nsColumnSetWrapperFrame* frame =
> +    new (aPresShell) nsColumnSetWrapperFrame(aContext);
> +  frame->AddStateBits(aStateFlags | NS_BLOCK_MARGIN_ROOT);

So using NS_BLOCK_MARGIN_ROOT isn't actually valid if a frame doesn't inherit from nsBlockFrame.

Though it's also implicit, since it's a frame type that doesn't support margin collapsing.

So you'd need this if you switch to inheriting from nsBlockFrame, but right now you don't.

::: layout/generic/nsColumnSetWrapperFrame.cpp:38
(Diff revision 13)
> +    nsLayoutUtils::IntrinsicForContainer(aRenderingContext, childFrame,
> +                                         nsLayoutUtils::MIN_ISIZE);

please indent these 2 lines an additional 2 spaces (and same in GetPrefISize)

::: layout/generic/nsColumnSetWrapperFrame.cpp:66
(Diff revision 13)
> +nsColumnSetWrapperFrame::BuildDisplayList(nsDisplayListBuilder*   aBuilder,
> +                                          const nsDisplayListSet& aLists)
> +{
> +  DisplayBorderBackgroundOutline(aBuilder, aLists);
> +
> +  // Our children won't have backgrounds so it doesn't matter where we put them.

I can understand that the nsColumnSetFrame children won't have backgrounds, but can't the column-span children have backgrounds?  And it seems like their backgrounds ought to be in the block border background list (which means using BuildDisplayListForNonBlockChildren wouldn't be appropriate).

Is it possible to assert that the display lists are already set up correctly for that (with BorderBackground items going on BlockBorderBackgrounds()), or do you need to do something to ensure it?  I think there should be either an assertion or some work to make that happen.

::: layout/generic/nsColumnSetWrapperFrame.cpp:72
(Diff revision 13)
> +void
> +nsColumnSetWrapperFrame::Reflow(nsPresContext*     aPresContext,
> +                                ReflowOutput&      aDesiredSize,
> +                                const ReflowInput& aReflowInput,
> +                                nsReflowStatus&    aStatus)
> +{

This reflow method seems like it's not going to handle breaking.  As I said above, I think you need to write tests.

Given that, I haven't reviewed it very carefully.

(And I think it would start breaking things once you start constructing column set wrapper frames around all multicols.  I'm not sure what patch/bug that happens in.)

But if it helps to get some of this work into the tree, it may make sense to land with some of these issues unfixed as long as there are bugs on file, blocking the bug to enable the feature (or in this case, to enable using column-span wrappers... which may or may not be separate).  But I'd still like to have a better understanding of the state of things, and whether my understanding that breaking isn't going to work is correct.
Attachment #8846875 - Flags: review?(dbaron)
(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #32)
> I think you need to test explicitly that page and column breaking of a
> multicol, either with or with spans, works reasonably.  This should probably
> involve writing explicit tests -- it's probably easiest to test using column
> breaking, and using 'column-fill: auto' and an explicit height on the outer
> multicol to make things simpler.

Here are a few examples of the sort of tests I was thinking of.

Note that our current behavior shows some bugs:
 * bad column-rule drawing on the inner multicol
 * bad layout of the last inner multicol

But my assumption is that your patch would substantially change the behavior of these sort of tests.

Note that these are all in one file, whereas they should really have one test per file so that they could be reftests.
Comment on attachment 8846875 [details]
Bug 1346983 - Add a new nsColumnSetWrapperFrame class to the build to be used as a wrapper for columnset and column span frames.

https://reviewboard.mozilla.org/r/119868/#review226280

I think this should be landable once you fix the following issues, but marking as review- for now since I'd like to see some of the revisions.

::: layout/generic/nsColumnSetWrapperFrame.h:8
(Diff revision 16)
> +#ifndef nsColumnSetWrapperFrame_h___
> +#define nsColumnSetWrapperFrame_h___

Please use one _ at the end rather than three.  Technically C++ reserves names with two or more consecutive underscores for the implementation.

::: layout/generic/nsColumnSetWrapperFrame.h:30
(Diff revision 16)
> +  virtual nsContainerFrame* GetContentInsertionFrame() override {
> +    nsIFrame* frame = PrincipalChildList().FirstChild();
> +
> +    if (!frame)
> +      return nullptr;
> +
> +    return frame->GetContentInsertionFrame();
> +  }

This seems wrong.

If you should override GetContentInsertionFrame at all, it should be to assert that it's not called.

This override is wrong because it will do the wrong thing for column-spans, and probably also the wrong thing if the frame *has* column-spans.

I'd suggest replacing the body with a MOZ_ASSERT_UNREACHABLE("should not not be called because we don't know whether we're inserting a column-span or not");
or an equivalent MOZ_CRASH().

If that ends up firing, we can discuss what to do about it.

::: layout/generic/nsColumnSetWrapperFrame.h:45
(Diff revision 16)
> +  nscoord GetMinISize(gfxContext* aRenderingContext) override;
> +  nscoord GetPrefISize(gfxContext* aRenderingContext) override;

I don't think you should need these overrides.  Remove them.

::: layout/generic/nsColumnSetWrapperFrame.cpp:30
(Diff revision 16)
> +nscoord
> +nsColumnSetWrapperFrame::GetMinISize(gfxContext* aRenderingContext)
> +{
> +  nscoord minISize = 0;
> +  DISPLAY_MIN_WIDTH(this, minISize);
> +
> +  for (nsIFrame* childFrame : mFrames) {
> +    nscoord childMinISize =
> +      nsLayoutUtils::IntrinsicForContainer(aRenderingContext, childFrame,
> +                                           nsLayoutUtils::MIN_ISIZE);
> +    minISize = std::max(minISize, childMinISize);
> +  }
> +  return minISize;
> +}
> +
> +nscoord
> +nsColumnSetWrapperFrame::GetPrefISize(gfxContext* aRenderingContext)
> +{
> +  nscoord prefISize = 0;
> +  DISPLAY_PREF_WIDTH(this, prefISize);
> +
> +  for (nsIFrame* childFrame : mFrames) {
> +    nscoord childPrefISize =
> +    nsLayoutUtils::IntrinsicForContainer(aRenderingContext, childFrame,
> +                                         nsLayoutUtils::PREF_ISIZE);
> +    prefISize = std::max(prefISize, childPrefISize);
> +  }
> +  return prefISize;
> +}

likewise, remove this

::: layout/generic/nsColumnSetWrapperFrame.cpp:60
(Diff revision 16)
> +void
> +nsColumnSetWrapperFrame::AppendDirectlyOwnedAnonBoxes(nsTArray<OwnedAnonBox>& aResult)
> +{
> +  if (mFrames.NotEmpty()) {
> +    aResult.AppendElement(OwnedAnonBox(mFrames.FirstChild()));
> +  }
> +}

I don't understand how this compiles; it's not declared in the header file.  (Maybe that's in another patch?)

I also don't understand why it appends only the first child.  Shouldn't it need to append all of the  nsColumnSetFrame children?  (Does it need to append the column-spans or not?)  There should probably be a comment explaining the decision.
Attachment #8846875 - Flags: review?(dbaron) → review-
Blocks: 1418029
No longer blocks: 1421105
(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #38)
> Comment on attachment 8846875 [details]
> Bug 1346983 - Add a new nsColumnSetWrapperFrame class to the build to be
> used as a wrapper for columnset and column span frames.
> 
> https://reviewboard.mozilla.org/r/119868/#review226280
> 
> I think this should be landable once you fix the following issues, but
> marking as review- for now since I'd like to see some of the revisions.
> 
> ::: layout/generic/nsColumnSetWrapperFrame.h:8
> (Diff revision 16)
> > +#ifndef nsColumnSetWrapperFrame_h___
> > +#define nsColumnSetWrapperFrame_h___
> 
> Please use one _ at the end rather than three.  Technically C++ reserves
> names with two or more consecutive underscores for the implementation.
> 
> ::: layout/generic/nsColumnSetWrapperFrame.h:30
> (Diff revision 16)
> > +  virtual nsContainerFrame* GetContentInsertionFrame() override {
> > +    nsIFrame* frame = PrincipalChildList().FirstChild();
> > +
> > +    if (!frame)
> > +      return nullptr;
> > +
> > +    return frame->GetContentInsertionFrame();
> > +  }
> 
> This seems wrong.
> 
> If you should override GetContentInsertionFrame at all, it should be to
> assert that it's not called.
> 
> This override is wrong because it will do the wrong thing for column-spans,
> and probably also the wrong thing if the frame *has* column-spans.
> 
> I'd suggest replacing the body with a MOZ_ASSERT_UNREACHABLE("should not not
> be called because we don't know whether we're inserting a column-span or
> not");
> or an equivalent MOZ_CRASH().
> 
> If that ends up firing, we can discuss what to do about it.
> 
> ::: layout/generic/nsColumnSetWrapperFrame.h:45
> (Diff revision 16)
> > +  nscoord GetMinISize(gfxContext* aRenderingContext) override;
> > +  nscoord GetPrefISize(gfxContext* aRenderingContext) override;
> 
> I don't think you should need these overrides.  Remove them.
> 
> ::: layout/generic/nsColumnSetWrapperFrame.cpp:30
> (Diff revision 16)
> > +nscoord
> > +nsColumnSetWrapperFrame::GetMinISize(gfxContext* aRenderingContext)
> > +{
> > +  nscoord minISize = 0;
> > +  DISPLAY_MIN_WIDTH(this, minISize);
> > +
> > +  for (nsIFrame* childFrame : mFrames) {
> > +    nscoord childMinISize =
> > +      nsLayoutUtils::IntrinsicForContainer(aRenderingContext, childFrame,
> > +                                           nsLayoutUtils::MIN_ISIZE);
> > +    minISize = std::max(minISize, childMinISize);
> > +  }
> > +  return minISize;
> > +}
> > +
> > +nscoord
> > +nsColumnSetWrapperFrame::GetPrefISize(gfxContext* aRenderingContext)
> > +{
> > +  nscoord prefISize = 0;
> > +  DISPLAY_PREF_WIDTH(this, prefISize);
> > +
> > +  for (nsIFrame* childFrame : mFrames) {
> > +    nscoord childPrefISize =
> > +    nsLayoutUtils::IntrinsicForContainer(aRenderingContext, childFrame,
> > +                                         nsLayoutUtils::PREF_ISIZE);
> > +    prefISize = std::max(prefISize, childPrefISize);
> > +  }
> > +  return prefISize;
> > +}
> 
> likewise, remove this
> 
> ::: layout/generic/nsColumnSetWrapperFrame.cpp:60
> (Diff revision 16)
> > +void
> > +nsColumnSetWrapperFrame::AppendDirectlyOwnedAnonBoxes(nsTArray<OwnedAnonBox>& aResult)
> > +{
> > +  if (mFrames.NotEmpty()) {
> > +    aResult.AppendElement(OwnedAnonBox(mFrames.FirstChild()));
> > +  }
> > +}
> 
> I don't understand how this compiles; it's not declared in the header file. 
> (Maybe that's in another patch?)
> 
> I also don't understand why it appends only the first child.  Shouldn't it
> need to append all of the  nsColumnSetFrame children?  (Does it need to
> append the column-spans or not?)  There should probably be a comment
> explaining the decision.

@Dbaron - I previously fixed all of these problems you mentioned and I see that on MozReview this patch shows up r+ed but on Bugzilla it is still r-? Not sure why :(
Assignee: neerjapancholi → aethanyc
Blocks: 1421105
No longer blocks: enable-column-span-nightly
Component: Layout → Layout: Columns
Summary: Create a new wrapper class that will be used as a parent for multiple nsColumnSetFrames. (nsColumnSetFrames will be split on the basis of column-span elements) → Create ColumnSpanWrapper to be used as a parent for nsColumnSetFrames and spanners. (nsColumnSetFrames will be split on the basis of column-span elements)
Summary: Create ColumnSpanWrapper to be used as a parent for nsColumnSetFrames and spanners. (nsColumnSetFrames will be split on the basis of column-span elements) → Create ColumnSetWrapper to be used as a parent for nsColumnSetFrames and spanners. (nsColumnSetFrames will be split on the basis of column-span elements)
ColumnSetWrapperFrame is needed to implement column-span. Patches in bug
1421105 will utilize this frame.

Co-authored-by: Neerja Pancholi <npancholi@mozilla.com>
Comment on attachment 9006720 [details]
Bug 1346983 - Add ColumnSetWrapperFrame to wrap nsColumnSetFrames and column span frames.

David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-7 has approved the revision.
Attachment #9006720 - Flags: review+
Pushed by aethanyc@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2ffd781dd9a1
Add ColumnSetWrapperFrame to wrap nsColumnSetFrames and column span frames. r=dbaron
https://hg.mozilla.org/mozilla-central/rev/2ffd781dd9a1
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox64: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: