Closed Bug 1163435 Opened 9 years ago Closed 9 years ago

[css-grid] percentage lengths for grid item isn't calculated correctly

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla45
Tracking Flags:
Tracking Status
firefox40 --- affected
firefox45 --- fixed
firefox62 --- affected

People

(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)

References

(Blocks 2 open bugs)

Details

(Keywords: testcase)

Attachments

(3 files, 4 obsolete files)

testcase
564 bytes, text/html
Details
fix
7.84 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
tests
12.43 KB, patch
Details | Diff | Splinter Review
Attached file testcase — 
The expected size for the green box is 75x50 pixels.
Attached patch wip (obsolete) — — Splinter Review 

    
    

    
  

      
      
      
      

        Attached patch
          
          
        fix (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8603877 -
        Attachment is obsolete: true

        Attachment #8604050 -
        Flags: review?(dholbert)

    
    

    
  
Comment on attachment 8604050 [details] [diff] [review]
fix

>diff --git a/layout/generic/nsGridContainerFrame.cpp b/layout/generic/nsGridContainerFrame.cpp
>--- a/layout/generic/nsGridContainerFrame.cpp
>+++ b/layout/generic/nsGridContainerFrame.cpp
[...]
>-    nsHTMLReflowState childRS(pc, aReflowState, child, cb.Size(wm));
>+    nsHTMLReflowState childRS(pc, aReflowState, child, cb.Size(wm),
>+                              cb.Width(wm), cb.Height(wm));

Just to make sure I understand -- the idea here is that grid items need to use their *grid area* as their containing block -- not the grid container -- right?  (And by default, nsHTMLReflowState assumes it should use the parent reflow-state's size as the containing block, and that's not what we want.)

If so: please update the documentation for the nsHTMLReflowState constructor that you're invoking -- quoting MXR:
> 618    * @param aContainingBlockWidth An optional width, in app units, that is used
> 619    *        by absolute positioning code to override default containing block
> 620    *        width.
> 621    * @param aContainingBlockHeight An optional height, in app units, that is
> 622    *        used by absolute positioning code to override default containing
> 623    *        block height.
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsHTMLReflowState.h?rev=9f3b7f39745b#618

Right now, this documentation basically says these args are *only relevant for abspos code*. That's no longer the case, it seems.

    
    

    
  
Comment on attachment 8604050 [details] [diff] [review]
fix

Optional nit:
>Bug 1163435 part 1 - Propagate an explicit CB width/height to the reflow state to resolve percentage lengths for grid items properly.  r=dholbert

Maybe worth adding a second line (or just rewording) to clarify what "properly" actually means here. e.g. "Grid items are supposed to use their 'grid area' -- not their grid container -- as their containing block."

r=me with the nsHTMLReflowState documentation updated, anyway.

        Attachment #8604050 -
        Flags: review?(dholbert) → review+

    
    

    
  
Comment on attachment 8604051 [details] [diff] [review]
tests

Drive-by nits on tests:

>+++ b/layout/reftests/css-grid/grid-item-sizing-percent-001-ref.html
>@@ -0,0 +1,96 @@
>+<!DOCTYPE html>
>+<!--
>+     Any copyright is dedicated to the Public Domain.
>+     http://creativecommons.org/publicdomain/zero/1.0/
>+-->
>+<html><head>
>+  <title>CSS Test: Testing grid item 'px' sizes</title>

IMO reference cases should have a different title from the testcase. (Otherwise it's harder to tell which is which when switching between them in tabs)

>+<!--
>+<div id="abs" style="float:left">
>+<div class="grid"><div id="t1" class="item"></div></div>
[...]
>+</div>
>+-->

What's up with this huge commented-out part in each of the test files?

If that's intended to be uncommented, pending some other bugfix, maybe add a comment referencing that bug number? Otherwise it seems like mysterious cruft that'll easily end up abandoned & forgotten.

    
    

    
  
(Also, the reference case has "percent" in its filename, but "px" in its <title> -- that seems like a typo.

Also, since this reference case is used for both a percent testcase and a px testcase, it seems a bit odd to have it declare one or the other in its filename. These might really want to be named something like:
  grid-item-sizing-001-pct.html
  grid-item-sizing-001-px.html
  grid-item-sizing-001-ref.html
or -001a, -001b, -001-ref. Maybe not worth worrying about, though.)

    
  
Blocks: 1000592

    
    

    
  
Looks good. This is the behavior as resolved by the CSSWG at the May NYC f2f. It was re-opened for discussion at the Sept Paris f2f, left at an impasse, however it was widely acknowledged that implementer experience here is encouraged. 

Key thing is that we did get agreement between Mozilla, Microsoft, and explicit abstention from Apple[1] who had agreed with this behavior at the May NYC f2f, and thus we can reasonably move forward with that.

https://lists.w3.org/Archives/Public/www-style/2015Sep/0038.html

    
    

    
  

      
      
      
      

        Attached patch
          
          
        fix
        — 
          — Splinter Review
      

    


  
Note: this is with the fix from bug 1163565 folded in.

This patch makes grid/flex items use the size in the same axis as
the percentage basis.  Also for abs.pos. boxes with a grid/flex
container as their Absolute Containing Block.

(This change passed reftests, so it seems we don't have any Flexbox tests
 for the abs.pos. case that use percentage in the block axis?)

(In reply to Daniel Holbert [:dholbert] from comment #4)
> Just to make sure I understand -- the idea here is that grid items need to
> use their *grid area* as their containing block -- not the grid container --
> right?

Yes.

> (And by default, nsHTMLReflowState assumes it should use the parent
> reflow-state's size as the containing block, and that's not what we want.)

I think the given aAvailableSpace is used(?)

> If so: please update the documentation for the nsHTMLReflowState constructor

Fixed.

        Attachment #8604050 -
        Attachment is obsolete: true

        Attachment #8678153 -
        Flags: review?(dholbert)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        tests
        — 
          — Splinter Review
      

    


  
(In reply to Daniel Holbert [:dholbert] from comment #6)
> What's up with this huge commented-out part in each of the test files?

It was to comment out the tests for bug 1163565 that uncommented
this block.  Not a problem now that the bugs have merged.

(In reply to Daniel Holbert [:dholbert] from comment #7)
> Also, since this reference case is used for both a percent testcase and a px
> testcase, it seems a bit odd to have it declare one or the other in its
> filename.

Meh.  I think it's fine.

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

        Attachment #8604051 -
        Attachment is obsolete: true

    
  
Blocks: 1176775

    
    

    
      
  

    
    

    
      
  


  

    
    

    
      
  

    
    

    
  
OK, I missed that you were talking about percent *margin and padding* w.r.t. changing the flexbox percentage basis here (as opposed to width/height, which is what this bug was originally about).

Marking my confused comment 13 as "obsolete" & thinking about this again, with that in mind.

    
    

    
  
I'm not clear why this & bug 1163565 were merged.  They're related, but I'd somewhat prefer that we landed this bug's original change separately from bug 1163565's changes, since what bug 1163565 covered was contentious (& seems like it could change in the future), as shown in the discussion that tantek linked to in comment 9.

Whereas, this bug's original (1-line) change is uncontroversial & orthogonal to that discussion.

Not a big enough deal for me to "r-", but if you agree, I'd suggest considering re-splitting the patches if not the bugs.

    
    

    
  
Comment on attachment 8678153 [details] [diff] [review]
fix

r=me, modulo comment 17 and the following nit:

>+++ b/layout/generic/nsGridContainerFrame.cpp
>@@ -2795,20 +2795,21 @@ nsGridContainerFrame::ReflowChildren(Gri
>     LogicalSize childCBSize = cb.Size(wm).ConvertTo(childWM, wm);
>     // XXX temporary workaround to avoid being INCOMPLETE until we have
>     // support for fragmentation (bug 1144096)
>+    LogicalSize percentBasis(childCBSize);
>     childCBSize.BSize(childWM) = NS_UNCONSTRAINEDSIZE;

Please move this new line up 2 lines -- insert it before the "XXX" comment.  (The XXX comment is referring to the next line -- the NS_UNCONSTRAINEDSIZE assignment -- and your new line splits those two things apart right now.)

        Attachment #8678153 -
        Flags: review?(dholbert) → review+

        Attachment #8678297 -
        Attachment is obsolete: true

    
    

    
  
Two more notes:
 (1) FWIW, this will make us violate (or at least go beyond the requirements of) the current flexbox spec ED, w.r.t. % margins/padding. It only calls for this behavior on "flex items" (in-flow children): https://drafts.csswg.org/css-flexbox-1/#item-margins  So until the spec is updated to call for this behavior on abspos children [not sure if/how-soon that'll happen], we'll be non-conforming on this point.  I'll defer to tantek's judgement that this is fine.

 (2) I expect there to be a small web-compat impact from the flexbox change, with bugs being filed about the behavior-change. (I'm basing this expectation on the handful of bugs that were filed after bug 851379 landed, which I've clustered as dupes of invalid-bug 958714).

    
    

    
  
Right, this is a change for Flexbox abs.pos. items, which previously used the ISize
and now use BSize to resolve vertical percent.  (I think that means we don't have
any tests for that because I haven't noticed any failures from this change.)

    
    

    
  
(In reply to Daniel Holbert [:dholbert] from comment #19)
> Two more notes:
>  (1) FWIW, this will make us violate (or at least go beyond the requirements
> of) the current flexbox spec ED, w.r.t. % margins/padding. It only calls for
> this behavior on "flex items" (in-flow children):

I don't see how this is stating this for "only" in flow flex items. It just states flex items, not in-flow flex items nor does the section regarding abspos specify that they should resolve % margin/padding any differently.

    
    

    
  
(In reply to Greg Whitworth from comment #21)
> > this behavior on "flex items" (in-flow children):
> 
> I don't see how this is stating this for "only" in flow flex items. It just
> states flex items, not in-flow flex items

Flex items are *defined* as being the in-flow children of a flex container.  See first two sentences of https://drafts.csswg.org/css-flexbox-1/#flex-items

(The spec is careful to refer to abspos children as "absolutely-positioned child of a flex container" or "Absolutely-Positioned Flex Children", and avoids calling them "flex items", since they don't flex.)

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/9dbec0f99fd0
https://hg.mozilla.org/mozilla-central/rev/a8b188bf416f
Status: NEW → RESOLVED
Closed: 9 years ago

          status-firefox45:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45

    
    

    
  
Sorry, but I'm not familiar with your release process, when could I expect it to be released to Stable Firefox?

    
  
Blocks: css-grid-1

    
    

    
  
The width of outer grid container is calculated wrong: https://codepen.io/KES777/pen/KxXdgV

it have less width than summ of all columns

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: