Open Bug 616436 (column-span) Opened 14 years ago Updated 7 days ago

[meta] Implement column-span (from CSS3 multicolumn)

Categories

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

Product:
Core
Component:
Layout: Columns
Type:
enhancement

Tracking

()

People

(Reporter: odin.omdal, Unassigned)

References

(Depends on 5 open bugs, Blocks 2 open bugs,
URL
)

Details

(8 keywords, Whiteboard: [css3-multicol][DevRel:P2] [layout:p1])

Attachments

(6 files, 15 obsolete files)

testcase
1.53 KB, text/html
Details
TEST CASE 1 (v4)
254.55 KB, text/html
Details
TEST CASE 2 (v4)
33.89 KB, text/html
Details
TEST CASE 3 (v4)
84.43 KB, text/html
Details
VisitedColorReference.jpg
68.40 KB, image/jpeg
Details
VisitedColorFailure.jpg
66.02 KB, image/jpeg
Details 
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; nn-NO; rv:1.9.2.12) Gecko/20101027 Ubuntu/10.10 (maverick) Firefox/3.6.12
Build Identifier: 

Not implemented:

http://www.w3.org/TR/css3-multicol/#column-span0

Very useful, started looking for it because I needed it. WebKit got it in march:
https://bugzilla.mozilla.org/show_bug.cgi?id=418036

Reproducible: Always
Attached file testcase — 

    
  
Keywords: css3
Whiteboard: [css3-multicol]

    
    

    
  
Bah, wrong url, this is the webkit one:

https://bugs.webkit.org/show_bug.cgi?id=15550
Assignee: nobody → sjohnson
Blocks: css-multicol-1
Status: UNCONFIRMED → NEW
Ever confirmed: true
No longer blocks: css-multicol-1

    
  
Keywords: dev-doc-needed
Blocks: 698783

    
    

    
  

      
      
      
      

        Attached file
          
        TEST CASE 1 (obsolete)
        — 
      

    


  

    
    

    
  

      
      
      
      

        Attached file
          
        TEST CASE 2 (obsolete)
        — 
      

    


  

    
    

    
  

      
      
      
      

        Attached file
          
        TEST CASE 3 (obsolete)
        — 
      

    


  

    
    

    
  
I just added three test cases. Here are results of running competitors' browsers in each of the three test cases (note, IE9 does not implement any multicolumn support, so they were omitted from this):

NAME   | CASE 1 |                     CASE 2                     |           CASE 3            |
------------------------------------------------------------------------------------------------
Safari |  PASS  | Works as expected, but text is crunched (PASS) |            PASS             |
Opera  |  PASS  | Makes the containing div larger (FAIL)         | Places on a new line (FAIL) |
Chrome |  PASS  | Makes the containing div larger (FAIL)         |            PASS             |

So, I think, given this, we should probably look at implementing this in a way similar to how pages are split for printing. If we split the frames in the frame constructor. IIUC, Roc thinks that if we do the splitting in nsCSSFrameConstructor, then we're going to be unable to correctly handle case 3. Given that webkit handles this correctly, we should probably avoid not complying with the spec on this aspect.

Thoughts?

    
    

    
  

      
      
      
      

        Attached file
          
        Chart displaying test results (obsolete)
        — 
      

    


  
My previous chart was messed up, so if you can't read it, I'm posting a new chart that is in HTML for better viewing.

    
    

    
  
latest IE10 Platform Preview implements columns, so you should test that too.

    
    

    
  
Can you include in your test results what the expected rendering is and what the test is supposed to be testing?

    
    

    
  
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #8)
> latest IE10 Platform Preview implements columns, so you should test that too.

Yep, I'm installing it on a VM right now.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9)
> Can you include in your test results what the expected rendering is and what
> the test is supposed to be testing?

Sure. I'll add that information.

    
    

    
  


  

        Attachment #576005 -
        Attachment is obsolete: true

    
    

    
  


  

        Attachment #576003 -
        Attachment is obsolete: true

    
    

    
  


  

        Attachment #576004 -
        Attachment is obsolete: true

    
    

    
  
I've updated the test cases. I will work on updating the chart so that it has images of what I see when I run the test on the given platform.

    
    

    
  
Oops, it looks like test case #2 is messed up in firefox. Let me tweak it a bit.

    
    

    
      
  

    
    

    
  
Ok, test case 2 is working in FF again now.

    
    

    
  

      
      
      
      

        Attached file
          
        TEST CASE 1, updated to be well-formed (obsolete)
        — 
      

    


  

        Attachment #576043 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached file
          
        TEST CASE 2, updated to be well-formed (obsolete)
        — 
      

    


  

        Attachment #576052 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached file
          
        TEST CASE 3, updated to be well-formed (obsolete)
        — 
      

    


  

        Attachment #576040 -
        Attachment is obsolete: true

    
    

    
  
The test results needed to be placed at: http://www.glasstowerstudios.com/~scottj/test-results-b616436.html as bugzilla wouldn't let me post an attachment greater than 4096 bytes.

I've updated the chart of test results to include IE10. I found that IE10 developer preview is *very* picky when it comes to the syntax of html files that include multicolumn CSS properties. Specifically, if I didn't include the "<!DOCTYPE html PUBLIC "">", it would not display ANY multicolumn formatting. Further, behavior was different at times, depending on whether I added "<html>", "<body", and "<head>" tags.

There were two instances when the "<H2>" element wasn't even displayed. My thinking is that, for some reason, rather than placing a column-span: all element into the overflow, and treating it as column-span: none, when there isn't enough room for it, as the spec says to do, IE10 seems to just not display the element. Either than, or there is an issue with how I am specifying it. 

I also found a few errors with my previous tests, which have since been fixed. This caused some of the Safari and Chrome tests to result in expected values, so I changed them to PASS.

        Attachment #576010 -
        Attachment is obsolete: true

    
    

    
  
(In reply to Scott Johnson (:jwir3) from comment #22)
> I've updated the chart of test results to include IE10. I found that IE10
> developer preview is *very* picky when it comes to the syntax of html files
> that include multicolumn CSS properties. Specifically, if I didn't include
> the "<!DOCTYPE html PUBLIC "">", it would not display ANY multicolumn
> formatting.

That's because IE only supports new CSS features in its "standards mode".

    
    

    
  
Testcase #3 has only 'column-span' on h2, no vendor prefixes. It needs the vendor prefixes.

I don't see the red box in testcase #2.

    
    

    
  


  
> Testcase #3 has only 'column-span' on h2, no vendor prefixes. It needs the
> vendor prefixes.

Fixed now.

        Attachment #577328 -
        Attachment is obsolete: true

    
    

    
  
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #24)
 
> I don't see the red box in testcase #2.

Hmm... so it apparently doesn't execute javascript when I use the "<!DOCTYPE html PUBLIC "">". Is there some way I can specify specific parts of an html document to be used only with certain browsers?

    
    

    
  
Actually I do see the box. (Why does JS matter? You're not using it.) It just looks pink because of the opacity:0.4. It's a bit confusing that the box extends below the element though. Also, it might disrupt the layout since it actually does take up space.

I'd just make it an absolutely positioned child of the div-with-columns, top:150px; width:100%, border-top:2px dashed red.

    
    

    
  


  
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #27)
> Actually I do see the box. (Why does JS matter? You're not using it.) It
> just looks pink because of the opacity:0.4. It's a bit confusing that the
> box extends below the element though. Also, it might disrupt the layout
> since it actually does take up space.

Ah... yes, I was using javascript in a previous iteration to conditionally position the box on all platforms. But, I can achieve the same effect with what you suggested. Thanks.

        Attachment #577327 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached file
          
        TEST CASE 2, updated for IE 10 (obsolete)
        — 
      

    


  
Made one last tweak for IE10.

        Attachment #577407 -
        Attachment is obsolete: true

    
    

    
  
> <!DOCTYPE html PUBLIC "">

That's a senseless doctype. Just use HTML5 doctype to trigger standards mode in any Browser:

<!DOCTYPE html>

Ans validate your HTML files:

http://validator.w3.org/
or
http://validator.nu/

(and you can omit  type="text/css")

    
    

    
  

      
      
      
      

        Attached file
          
        TEST CASE 1 (v4)
        — 
      

    


  

        Attachment #577326 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached file
          
        TEST CASE 2 (v4)
        — 
      

    


  

        Attachment #577423 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached file
          
        TEST CASE 3 (v4)
        — 
      

    


  
(In reply to j.j. from comment #30)
> > <!DOCTYPE html PUBLIC "">
> 
> That's a senseless doctype. Just use HTML5 doctype to trigger standards mode
> in any Browser:
> 
> <!DOCTYPE html>
> 
> Ans validate your HTML files:
> 
> http://validator.w3.org/
> or
> http://validator.nu/
> 
> (and you can omit  type="text/css")

Thanks for the recommendation. I've validated these and removed the text/css part of the style tag.

        Attachment #577398 -
        Attachment is obsolete: true
Priority: -- → P2
No longer blocks: 698783

    
  
Blocks: css3test
Assignee: sjohnson → nobody

    
    

    
  
I am trying to translate study in PDF to HTML. Its layout features two-columns with some of the tables spanning across both columns, causing a break in the text of the second column.

The layout of the study requires the tables span across both columns and since CSS3 column-span is not implemented, it is impossible to make the layout as shown in the PDF.


The study:
http://igmoris.nic.in/files/Biosafety_data/Biosafety/NK603%20-%20Monsanto/Hammond%20et%20al%202004.pdf

If we can make a proper hand-translation to HTML (and Adobe Acrobat Pro "Save As Webpage" doesn't even come close), then we can better explain boggy science to the public. Generally, such things are boring and hard. I want to highlight some of the problems in this study by Monsanto and I want to do it using dynamic effects in HTML. But I can't properly mark it up into HTML because column-span isn't implemented.

In general, it's important to have this feature for other scientific studies and the PDF alternative has many downsides.

Also, I noticed that this bug is four years old.
Alias: dhtmlkitchen@gmail.com
Alias: dhtmlkitchen@gmail.com

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Would it be possible to take the part of Gecko used to render tables and re-purpose it to render columns? They seem like an analogous problem.

Requesting information from Scott Johnson about whatever progress he made while he was the bug's assignee.

Also suggesting the dev team raise this to P1 given the time it has spent open combined with the heightened importance of parity with WebKit as a result of the Chrome wars.
Flags: needinfo?(jaywir3)

    
    

    
  
My apologies for the late reply.

(In reply to ipatrol from comment #39)
> Would it be possible to take the part of Gecko used to render tables and
> re-purpose it to render columns? They seem like an analogous problem.

No. Tables and column sets function pretty differently. Namely, the table code doesn't allow for content to flow between table cells as in column sets. The way the column set code works is as follows - depending on how the columns are setup in CSS, there is a single nsColumnSetFrame that has anonymous block-level frames within it (the number are determined by the CSS rules). Then, during the reflow of the column set frame, the height of each of these anonymous frames is determined using a binary search over the set of constraints to determine the "optimal" height.

The reason this matters is that, given a spanning element, we somehow need it to span across these multiple anonymous block frames. However, this shouldn't affect the binary search algorithm, other than the content needs to flow around the new spanning element in all child frames of the column set.
 
> Requesting information from Scott Johnson about whatever progress he made
> while he was the bug's assignee.

So, my original intent was to split the column set frame into two column set frames, with another anonymous block frame in between (the spanning element). The problem is with the binary search, and flowing content between these two column set frames given a change in width (or other property that would require a new reflow) of the column set frame.

I think that we might be able to do something akin to what's been done with absolute/relative positioning elements - that is, make a placeholder frame, leave room for the spanning element within each column, and then add it back in visually after the reflow has completed.

I'm simply not sure how to go about this at the moment. I can give you insight into the nsColumnSetFrame code if it would be helpful, if you're interested in starting on a new patch.
Flags: needinfo?(jaywir3)
Whiteboard: [css3-multicol] → [css3-multicol][DevRel:P2]
Flags: platform-rel?
platform-rel: --- → ?
Summary: column-span not implemented (css3 multicolumn) → Implement column-span (from CSS3 multicolumn)
platform-rel: ? → ---
Assignee: nobody → npancholi
Depends on: 1339298

    
    

    
  
The URL for the test case is not reachable, so I replaced it with the one to the spec.

Sebastian
URL: http://www.glasstowerstudios.com/~sco... → https://drafts.csswg.org/css-multicol...

    
    

    
  
Creates Web Compatibility issue on https://boards.greenhouse.io/robinhood
See https://webcompat.com/issues/4902
See Also:  → https://webcompat.com/issues/4902
Whiteboard: [css3-multicol][DevRel:P2] → [css3-multicol][DevRel:P2] [webcompat]
Depends on: 1346983

    
    

    
  
column-span: all always create new block formatting contexts, whether the element is a descendent of a multicol or not, chrome is not currently support.

    
    

    
  
(In reply to Sebastian Zartner [:sebo] from comment #41)
> The URL for the test case is not reachable, so I replaced it with the one to
> the spec.

CSS Multi-column Layout Module, Section 6.1 'column-span', Example 21

http://www.gtalbot.org/BrowserBugsSection/CSS3Multi-Columns/Opera/NewTests/column-span-example-21.xht

Notes
-----
- The example 21 in section 6.1 in the specification uses and specifies a font-size of 14px for the text. So, if your preferences in your browser has a set minimum font size greater than 14px, then you need to reset it accordingly
- All mainstream browsers' user agent style sheet have preset vertical margins like this:
h2 {font-size: 1.5em; margin: 0.83em 0;} 
(now it's margin-block-start: 0.83em; margin-block-end: .83em; in Gecko; it's -webkit-margin-before: 0.83em; -webkit-margin-after: 0.83em; in Blink)
so, the column-spanning <h2> element, which creates a new block formatting context, has to render such vertical margins and they should be transparent in the test. This is where my expected results image of the test is more precise than the example in section 6.1. I will eventually ask Håkon Wium Lie to confirm this.
- The h2 element uses bolder glyphs in IE11 and presumably in Edge 12+; that's not relevant in the test.
- I use a small script to identify browser in use: this is useful when using browser shots service for browsers (like Edge 16 only available under Windows 10 and Safari 10.x only available in Mac OS X) not available on a platform (eg Linux).

    
    

    
  
4 'column-span' tests and 12 'column-span: all' tests of the CSS3 Multi-column test suite and their results in Firefox 57.0a1 buildID=20170820221144:

http://test.csswg.org/harness/results/css-multicol-1_dev/grouped/browser/Firefox/browser_version/57.0/#s6

http://test.csswg.org/harness/results/css-multicol-1_dev/grouped/browser/Firefox/browser_version/57.0/#s6.1

Nota bene: you need to get the Ahem font 
{
[   ] AHEM____.TTF            2017-01-31 20:55   22K  
[   ] ahem.ttf                2017-01-31 20:55   22K 
downloadable from
https://www.w3.org/Style/CSS/Test/Fonts/Ahem/
}
and then install it in your operating system before taking those reftests.

    
  
Keywords: testcase

    
    

    
      
  


  

        Attachment #8907285 -
        Flags: feedback?(dbaron)

    
    

    
      
  

        Attachment #8907285 -
        Flags: feedback?(dholbert)

        Attachment #8907285 -
        Flags: feedback?(dbaron)

    
    

    
      
  

    
    

    
  
What feedback in particular were you looking for?  (I don't think we have a clear common understanding of what "feedback?" means, so I think "feedback?" requests should come with a description.)
Flags: needinfo?(npancholi)

    
    

    
  
(In reply to David Baron :dbaron: ⌚️UTC-7 (catching up on backlog from vacation) from comment #50)
> What feedback in particular were you looking for?  (I don't think we have a
> clear common understanding of what "feedback?" means, so I think "feedback?"
> requests should come with a description.)

Okay, will keep that in mind. 
The reason why I sent you a feedback request for this is I implemented this according to the IB-split related discussions we've had in the past. So I'm wondering if you have the time to look at this and see if it is on the right track/this is what you expected. Some of the things that I am unsure of here are if the pseudo-styles I created are correct because I mainly came about that with trial and error. (I think that might be one reason why I have alignment related bugs so maybe there is something I am missing there). I am also not sure if the changes I made to WipeContainingBlock are correct because right now I reconstruct whenever there is any change to the multicol and that might be less than ideal but I'm not sure if there is a way around that.
Flags: needinfo?(npancholi)

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Fixed the crash happening when the inspector was invoked and updated the patch.

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

        Attachment #8907285 -
        Flags: feedback?(dbaron)

    
    

    
      
  

        Attachment #8907285 -
        Flags: feedback?(dbaron)

    
    

    
  
At first glance this seems to look overall like it has the pieces I was expecting, although it would have been better (in hindsight) to split it into multiple patches; it's probably too messy to try to do that now.

One thing I noticed so far is that I'm curious what the style setup is.  It looks like you have two new anonymous boxes, ::-moz-column-set and ::-moz-column-span-wrapper.  I guess I was expecting only one, although two does make sense in some ways in that you want each to have only a subset of the styles.  This seems like they both inherit from the element's style, but no particular frame *has* the style associated with the element.  However, I'm not sure if that setup works well with recomputation of style, which I thought relied on a frame having each style -- though perhaps this is less of a problem with stylo than with the old style system.  Have you tested dynamic style changes, particularly on the element with the column styles, but perhaps also inherited properties on an ancestor?

    
    

    
  
OK, after discussion with Neerja it turns out I was misunderstanding the style bits; the primary style is on the outermost frame, which is the ColumnSetWrapperFrame.  Then there are ColumnSetFrame inside of that, which are ::-moz-columnset, and ::-moz-column-span-wrapper, which wrap column-span frames (like ::-moz-block-inside-inline-wrapper, I guess).  So that matches the setup we use for similar things, which is good.

I guess it leaves the question of whether there are properties that we then need to make not work on the ColumnSetWrapperFrame.
Depends on: 1411422

    
    

    
      
  
Depends on: 1417725
Depends on: 1418029
No longer depends on: 1418029
No longer depends on: 1411422
No longer depends on: 1417725
Depends on: 1418029
Depends on: 1421105

        Attachment #8907285 -
        Attachment is obsolete: true

    
    

    
  
[Asking question here because time zones make IRC difficult]

Hi Bz,
I have a couple of questions about anon box styling for servo. Some context on why - I have a mochitest failure (https://treeherder.mozilla.org/#/jobs?repo=try&revision=127fce6713ebd5dc53c8775edb52efa9fcae696b&selectedJob=150386691) happening with my column-span patches only on Servo, Gecko is ok.
(Attached correct and failure output if you would like to see.)

The reason this happens is that GetStyleIfVisited() (here: https://searchfox.org/mozilla-central/source/layout/style/nsStyleContext.cpp#471) always returns false when being called by nsColumnSetFrame for the column-rule color (here: https://searchfox.org/mozilla-central/source/layout/generic/nsColumnSetFrame.cpp#254-256) with my patch. 

I believe its because, with my patches, columnSetFrame is now an anon box inside a wrapper called 'ColumnSetFrameWrapper' and it now uses an anonymous box style called "-moz-column-set" -

*|*::-moz-column-set {
  /* Prevent inheriting of borders on ColumnSetFrames since they are now
     inherited by ColumnSetWrapper frame instead*/
  display: block !important;
  columns: inherit;
  column-gap: inherit;
  column-rule: inherit;
  column-fill: inherit;
  height: 100%; //xxxNeerja - remove this workaround after fix for Bug 1411422
  width: 100%; //xxxNeerja - remove this workaround after fix for Bug 1411422
}

I see that GetStyleIfVisited() is pulling this visited style out of ServoStyleContext's computed data (link below) but I have no idea how this style is being calculated by servo.
https://searchfox.org/mozilla-central/source/layout/style/ServoStyleContext.h#37-40

I also don't understand why nsIFrame::UpdateStyleOfChildAnonBox seems to only handle a subset of pseudo-style updates only for blocks (https://searchfox.org/mozilla-central/source/layout/generic/nsFrame.cpp#10722-10725) and columnSetFrame is an nsContainerFrame. I couldn't find a place where this :visited style was being updated for Stylo. Can you please point me to it?

Do you have a hunch as to what might be wrong here? Anything that you can tell me about this style calculation would really help!
Thanks!
Flags: needinfo?(bzbarsky)

    
    

    
  
Not sure whether comment 66 was supposed to go in bug 1421105...

jryans is the expert on how servo's visited bits work, so might be worth checking with him, once you make sure all your anonymous box restyling is correct.

That said, the patches in bug 1421105 are a bit unclear to me.  Why is nsBlockFrame::UpdateStyleOfOwnedAnonBoxesForColumnSpanSplit only doing work in the ib-split case?  Why is there any interaction at all between ib splits and the column-span stuff?  Chances are, your changes to the anon box restyling aren't quite right and then when we determine the link has a visited style we don't restyle all the bits properly, which the test is picking up.

What would probably help the most is if bug 1421105 had a description of what the frame structure looks like after the patches.  The commit message would be a reasonable place for that description, or comments in the frame constructor...  At that point we can talk about what the restyling code should look like given that frame structure.

> I also don't understand why nsIFrame::UpdateStyleOfChildAnonBox seems to only handle a subset
> of pseudo-style updates only for blocks

Because only blocks can have ::first-letter and ::first-line pseudo-elements, and list bullets.  That code you link to is for the case when a block (1) has those pseudo-elements and (2) is itself an anonymous box.  One example would in fact be something like a list bullet (or ::first-letter/::first-line) on a columnated block: the block box is anonymous in that case.

> I couldn't find a place where this :visited style was being updated for Stylo.

Assuming we end up doing ResolveInheritingAnonymousBoxStyle for the relevant box, Servo_ComputedValues_GetForAnonymousBox gets called, which calls precomputed_values_for_pseudo_with_rule_node, which calls properties::cascade, which calls apply_declarations, etc.  Note that precomputed_values_for_pseudo_with_rule_node passes None as the "visited_style".  Maybe that's actually an issue; again worth checking with jryans or emilio...
Flags: needinfo?(bzbarsky)

    
    

    
  
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #67)
> Assuming we end up doing ResolveInheritingAnonymousBoxStyle for the relevant
> box, Servo_ComputedValues_GetForAnonymousBox gets called, which calls
> precomputed_values_for_pseudo_with_rule_node, which calls
> properties::cascade, which calls apply_declarations, etc.  Note that
> precomputed_values_for_pseudo_with_rule_node passes None as the
> "visited_style".  Maybe that's actually an issue; again worth checking with
> jryans or emilio...

This is actually an issue afaict, https://github.com/servo/servo/pull/19551 should fix that.

    
    

    
  
(In reply to Emilio Cobos Álvarez [:emilio] from comment #68)
> (In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from
> comment #67)
> > Assuming we end up doing ResolveInheritingAnonymousBoxStyle for the relevant
> > box, Servo_ComputedValues_GetForAnonymousBox gets called, which calls
> > precomputed_values_for_pseudo_with_rule_node, which calls
> > properties::cascade, which calls apply_declarations, etc.  Note that
> > precomputed_values_for_pseudo_with_rule_node passes None as the
> > "visited_style".  Maybe that's actually an issue; again worth checking with
> > jryans or emilio...
> 
> This is actually an issue afaict, https://github.com/servo/servo/pull/19551
> should fix that.

Thanks for looking at this Emilio!

    
    

    
  
Thanks bz! Here are the answers to your questions - 

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #67)
> That said, the patches in bug 1421105 are a bit unclear to me.  Why is
> nsBlockFrame::UpdateStyleOfOwnedAnonBoxesForColumnSpanSplit only doing work
> in the ib-split case?  Why is there any interaction at all between ib splits
> and the column-span stuff?

Column-span reuses some of the IB-splitting code. Basically the block that is split by a column-span is treated as the same case as an inline being split by a block so that we can take advantage or stuff like drawing fragmented borders between these blocks that IB-splitting already does. 
So, when servo tries to style the anonymous boxes generated by the column-span splitting, even though they are IB-split siblings of each other, they need to be handled differently which is why we need UpdateStyleOfOwnedAnonBoxesForColumnSpanSplit. 

> What would probably help the most is if bug 1421105 had a description of
> what the frame structure looks like after the patches.  The commit message
> would be a reasonable place for that description, or comments in the frame
> constructor...  At that point we can talk about what the restyling code
> should look like given that frame structure.

I updated my patches with descriptive comments and an example of what the expected frame tree should look like.

    
    

    
  
> Column-span reuses some of the IB-splitting code.

Hmm... I guess the "just reframe if anything under here changes" bits in <https://reviewboard.mozilla.org/r/203318/diff/7#index_header> make sure we don't try to actually construct frames under there via the "normal" ib-related codepaths?  But it also means that any append to a columnset with spanners (like during pageload) reframes the whole columnset...

> I updated my patches with descriptive comments and an example of what the expected
> frame tree should look like.

Thank you.  Just to make sure we're on the same page, that's this part:

  //  <div style="-moz-column-count: 2;">
  //    <div style="-moz-column-span: all">a</div>
  //    <div>b</div>
  //    <div>c</div>
  //    <div>
  //      d
  //      <div style="-moz-column-span: all">e</div>
  //      <div style="-moz-column-span: all">f</div>
  //      g
  //    </div>
  //    <div>h</div>
  //  </div>
  //  <div style="-moz-column-span: all">i</div>
  //
  //01  ColumnSetWrapperFrame (original sc)
  //02    Block (a, spanner(new BFC))
  //03    ColumnSetFrame (anonymous, -moz-column-set)
  //04      Block (anonymous, -moz-column-content)
  //05        Block (b)
  //06        Block (c)
  //07        Block (d)
  //08    Block (anonymous, spanner(new BFC), -moz-column-span-wrapper)
  //09      Block(e, spanner(new BFC))
  //10      Block(f, spanner(new BFC))
  //11    ColumnSetFrame (anonymous, -moz-column-set)
  //12      Block (anonymous, -moz-column-content)
  //13        Block (g)
  //14        Block (h)
  //15  Block (i, spanner(new BFC)) <-This is outside of the multicol parent
  //
  // Note: Here, 07, 08 and 13 are marked IBSplit siblings of each other
  // because we reuse some of the ib-split logic between blocks split by
  // a column-span for things like breaking borders correctly between these

Given the frame tree shown, I would expect the following:

1)  nsColumnSetWrapperFrame::AppendDirectlyOwnedAnonBoxes should append its child anonymous ColumnSetFrame boxes (two of them, in this case) to the list.  But https://reviewboard.mozilla.org/r/203322/diff/7#index_header only shows it appending the first thing in mFrames, which would be the "Block (a, spanner(new BFC))" in this case.  Looks fishy.

2)  I would expect that after we update the style on (d) (which happens normally during stylo post-traversal, since it's reachable from the DOM node) we go and update the style on the wrapper around (e)/(f) and also update the style on (g).  I don't see that happening in <https://reviewboard.mozilla.org/r/203322/diff/7#index_header> unless I'm really missing something.  The code added in nsIFrame::DoUpdateStyleOfOwnedAnonBoxes calls UpdateStyleOfOwnedAnonBoxesForColum if the anon box that it's looking at has mColumnSpan == NS_STYLE_COLUMN_SPAN_ALL.  It seems to be assuming that this box would be the wrapper around (e)/(f) in this case.  But I don't see how that frame would even end up in that code, given item 1.  And even if we did, that would be at the point when we're processing the anon kids of the ColumnSetWrapperFrame, which is before we've updated the style on (d), which means we'll get the wrong style on the (e)/(f) wrapper.  The right way to handle this, I would think, is to model the behavior on what we do for inlines.  That is, if we land in nsIFrame::DoUpdateStyleOfOwnedAnonBoxes for a blockframe that is part of an ib split, call a method (like your UpdateStyleOfOwnedAnonBoxesForColumnSpanSplit) which starts at the block _before_ the split (in this case your (d)) and walks its ib-split chain updating the styles.

Again, this is all assuming I understand the frame structure correctly...

    
  
Component: CSS Parsing and Computation → Layout
Summary: Implement column-span (from CSS3 multicolumn) → Implement column-span (from CSS3 multicolumn) and land it pref'd off.

    
  
Depends on: 1426372
Blocks: 1374134
Whiteboard: [css3-multicol][DevRel:P2] [webcompat] → [css3-multicol][DevRel:P2] [webcompat][parity-blink][parity-webkit][parity-edge]

    
    

    
  
Mass bug change to replace various 'parity' whiteboard flags with the new canonical keywords. (See bug 1443764 comment 13.)
Keywords: parity-chrome, 
          
            parity-edge, 
          
            parity-safari
Whiteboard: [css3-multicol][DevRel:P2] [webcompat][parity-blink][parity-webkit][parity-edge] → [css3-multicol][DevRel:P2] [webcompat]
Assignee: neerjapancholi → aethanyc
Status: NEW → ASSIGNED

    
    

    
  
Change this bug to a meta bug that collects all the bugs related to column-span.
Alias: column-span
Assignee: aethanyc → nobody
Status: ASSIGNED → NEW
Component: Layout → Layout: Columns
Summary: Implement column-span (from CSS3 multicolumn) and land it pref'd off. → [meta] Implement column-span (from CSS3 multicolumn)

    
  
Keywords: meta
Whiteboard: [css3-multicol][DevRel:P2] [webcompat] → [css3-multicol][DevRel:P2] [webcompat][layout:p1]

    
    

    
  
Migrating Webcompat whiteboard priorities to project flags. See bug 1547409.


Webcompat Priority: --- → ?

    
    

    
  
See bug 1547409. Migrating whiteboard priority tags to program flags.


No longer depends on: 1348839
Depends on: 1571239
No longer depends on: 1571239
No longer depends on: 1487927
Webcompat Priority: ? → revisit
Depends on: 1710883
Depends on: 1758723
Webcompat Priority: revisit → ---

    
  
Severity: normal → S3

    
    

    
  
boards.greenhouse.io no longer has an issue with this, and since we don't have any other known breakage at the moment, let's unset the webcompat flag for now.


Whiteboard: [css3-multicol][DevRel:P2] [webcompat][layout:p1] → [css3-multicol][DevRel:P2] [layout:p1]

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: