Closed Bug 1176782 Opened 9 years ago Closed 9 years ago

[css-grid][css-align] Implement full support for the CSS Box Alignment spec in the style system.

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla45
Tracking Flags:
Tracking Status
firefox45 --- fixed

People

(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)

References

(Blocks 3 open bugs)

Details

(Keywords: dev-doc-needed)

Attachments

(11 files, 14 obsolete files)

GetUniqueStyleData(eStyleStruct_Position)
1.61 KB, patch
Details | Diff | Splinter Review
errors with previous patch
3.59 KB, text/plain
Details
part 8 - [css-align] Update three tests that assumed that the computed value of 'align-items' is 'stretch' on all types of boxes which is no longer true.
4.71 KB, patch
Details | Diff | Splinter Review
part 1 - [css-align] Implement the 'justify-items' property in the style system.
34.46 KB, patch
SimonSapin
: review+
Details | Diff | Splinter Review
part 2 - [css-align] Implement the 'justify-self' property in the style system.
19.22 KB, patch
SimonSapin
: review+
Details | Diff | Splinter Review
part 7 - [css-align] Update the flexbox layout code to use the new align/justify style constants and remove the old in the style system.
21.13 KB, patch
heycam
: review+
Details | Diff | Splinter Review
part 4 - [css-align] Implement additional syntax and values for the 'align-items' property in the style system.
22.57 KB, patch
heycam
: review+
Details | Diff | Splinter Review
part 5 - [css-align] Implement additional syntax and values for the 'align-self' property in the style system.
19.86 KB, patch
heycam
: review+
Details | Diff | Splinter Review
part 1 to 7 as one patch
90.16 KB, patch
Details | Diff | Splinter Review
part 6 - [css-align] Implement additional syntax and values for the 'align-content' property in the style system.
18.51 KB, patch
heycam
: review+
Details | Diff | Splinter Review
part 3 - [css-align] Implement additional syntax and values for the 'justify-content' property in the style system.
28.71 KB, patch
heycam
: review+
Details | Diff | Splinter Review 
The spec https://drafts.csswg.org/css-align-3/#content-distribution
says that the computed values for justify-content/align-content are:
For blocks: auto/auto
For multicol: auto/auto
Flexbox: flex-start/stretch
Grid: start/start

We currently set the initial values to flex-start/stretch:
http://mxr.mozilla.org/mozilla-central/source/layout/style/nsRuleNode.cpp?rev=443625267e00#7800
http://mxr.mozilla.org/mozilla-central/source/layout/style/nsRuleNode.cpp?rev=443625267e00#7695

I assume this is because these properties are only implemented in Flexbox
so far.  I need to change this somehow to implement it for Grid.

What is the correct way to implement this?
I'm thinking adding 'auto' to both props, set the initial values to that,
then detect flex/grid 'display' values in nsStyleContext::ApplyStyleFixups
and overwrite the values justify-content/align-content there.  Having to do
GetUniqueStyleData(eStyleStruct_Position) kinda sucks though.
Is there a better way?
Flags: needinfo?(cam)
Note: we also need to add 'stretch' to 'justify-content' because it's currently
missing in the table there:
http://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSProps.cpp?rev=443625267e00#1200
I assume this is because the spec says that for flexbox 'stretch' computes
to 'flex-start':
https://drafts.csswg.org/css-align-3/#propdef-justify-content
Hmm, I get all sorts of weird errors when I do
GetUniqueStyleData(eStyleStruct_Position)
This patch should be functionally idempotent, right?
Assignee: nobody → mats
Attached file errors with previous patch — 
... but it results in these errors when I run:
mach mochitest -f plain layout/style/test/
BTW, for 'justify-items' the spec says:

"Computed value: specified value, except for 'auto' (see prose) 
...
If the inherited value of 'justify-items' includes the 'legacy' keyword,
'auto' computes to the inherited value. Otherwise, 'auto' computes to:
    'stretch' for flex containers and grid containers
    'start' for everything else"
https://drafts.csswg.org/css-align-3/#propdef-justify-items

Which also seems to require GetUniqueStyleData(eStyleStruct_Position)
to be able to modify the values in ApplyStyleFixups based on display type.
David, do you have any suggestions on how to implement this? or what the error
might be with GetUniqueStyleData(eStyleStruct_Position)?  (see comment 2, 4, 5)
Flags: needinfo?(dbaron)
I think the error you should focus on is the:
  should be testing with values that compute to different things
one, since that seems likely to lead to the "should not get initial value" error.

One possible cause could be broken copy-constructors (in terms of all members of nsStylePosition being copied correctly).  mAlignContent's copy constructor looks ok, though (but the other is complicated and could be broken).

Another cause could be memory being overwritten.

Beyond that, I'm not sure if I have any suggestions for what to debug.  You could see what the relevant variables look like around the relevant GetUniqueStyleData call.
Flags: needinfo?(dbaron)
(And I'm not sure what sort of other suggestions you were looking for -- what's interesting about these properties that requires suggestions?)
(In reply to David Baron [:dbaron] UTC-7 from comment #8)
> One possible cause could be broken copy-constructors (in terms of all
> members of nsStylePosition being copied correctly).

Right, I already checked that and it looked OK to me, and just now I added
code to check a->member == b->member before and after the GetUniqueStyleData
call just to verify they are equal at this point, and they were.

>  mAlignContent's copy
> constructor looks ok, though (but the other is complicated and could be
> broken).

Yeah, mAlignContent is an integral type with a trivial copy-ctor, so that
indicates to me that the error occurs later...

> Another cause could be memory being overwritten.

Seems very likely to me.

> Beyond that, I'm not sure if I have any suggestions for what to debug.  You
> could see what the relevant variables look like around the relevant
> GetUniqueStyleData call.

OK, I'll try... I'm not very familiar with how this style-struct caching and
sharing and stuff works though, which is why I asked you guys.
(In reply to David Baron [:dbaron] UTC-7 from comment #9)
> (And I'm not sure what sort of other suggestions you were looking for --
> what's interesting about these properties that requires suggestions?)

Well, the css-align spec is littered with ... uhm creative solutions
where the computed value of one property depends on the box type and
whatnot.  So, from a Style System architectural point of view, it seems
to me we should have a better mechanism than GetUniqueStyleData / 
ApplyStyleFixups to implement stuff like that.  At least this comment
strongly suggests we should not use it other than in exceptional cases:
http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleContext.cpp?rev=6229c2f38ea9#437

Implementing the css-align properties will make this a lot more common.
For grid/flexbox containers it's going to occur 100% of the time.
I suspect there is a memory/performance penalty associated with that.
FWIW, when I run the test in a debugger I also get:

layout/style/test/test_initial_computation.html | should be testing with values that compute to different things for 'animation-name'
layout/style/test/test_initial_computation.html | should be testing with values that compute to different things for 'padding-block-start'

etc, so it's not just errors in the nsStylePosition struct.
FWIW, testing this in an ASAN build didn't find anything so it's not arbitrary memory
corruption (which is a pity since that would be easier to diagnose).

I'm able to replay the test failure using 'rr', but it's pretty hard to know what to
look for specifically...
I debugged the align-content failure to the point of understanding it.

The bug is that nsStylePosition::CalcDifference is capable of returning a 0 hint when align-content is different.  This leads us to using nsStyleContext::SwapStyleData to exchange an old (and different) struct with the new one.

I believe that replacing the code:

  if (mFlexWrap != NS_STYLE_FLEX_WRAP_NOWRAP &&
      mAlignContent != aOther.mAlignContent) {
    NS_UpdateHint(hint, nsChangeHint_NeedReflow);
  }

with:

  if (mAlignContent != aOther.mAlignContent) {
    if (mFlexWrap != NS_STYLE_FLEX_WRAP_NOWRAP) {
      NS_UpdateHint(hint, nsChangeHint_NeedReflow);
    } else {
      NS_UpdateHint(hint, nsChangeHint_NeutralChange);
    }
  }

will fix the problem, although I haven't actually tested this.


I suspect there is a similar problem with the other property.  And we should probably audit for missing uses of nsChangeHint_NeutralChange since the patch that introduced it.



Leaving the needinfo so that I can reply to comment 11 later.
(And this bug will cause misbehavior with that property in existing code, not just with your GetUniqueStyleData patch.)
(In reply to David Baron [:dbaron] ⌚UTC-7 from comment #14)
> I debugged the align-content failure to the point of understanding it.

Thanks a lot for your help debugging this!  Good that you warned about
this in dev.layout too.

I filed bug 1203472 about 'grid-template'.  I'll deal with 'align-content'
in this bug.  (I was planning to remove that mFlexWrap optimization anyway
since it's only valid for flexbox containers.)

> And we should
> probably audit for missing uses of nsChangeHint_NeutralChange ...

I was about to file a bug on that when I found bug 1026820 --
perhaps we can deal with it there?
Flags: needinfo?(cam)
(In reply to Mats Palmgren (:mats) from comment #16)
> (In reply to David Baron [:dbaron] ⌚UTC-7 from comment #14)
> > And we should
> > probably audit for missing uses of nsChangeHint_NeutralChange ...
> 
> I was about to file a bug on that when I found bug 1026820 --
> perhaps we can deal with it there?

Well, that's about auditing something different -- making sure that none of the places where we have eChangeHint_NeutralChange shouldn't be returning a real change hint -- as opposed to making sure there aren't places where there's a data difference returning no hint at all.  So probably better in a separate bug.
OK, filed bug 1203571 for the audit.
landing of part 1 is documented in bug 1151214 (not surprisingly, given the commit message)
(In reply to Mats Palmgren (:mats) from comment #11)
> (In reply to David Baron [:dbaron] UTC-7 from comment #9)
> > (And I'm not sure what sort of other suggestions you were looking for --
> > what's interesting about these properties that requires suggestions?)
> 
> Well, the css-align spec is littered with ... uhm creative solutions
> where the computed value of one property depends on the box type and
> whatnot.  So, from a Style System architectural point of view, it seems
> to me we should have a better mechanism than GetUniqueStyleData / 
> ApplyStyleFixups to implement stuff like that.  At least this comment
> strongly suggests we should not use it other than in exceptional cases:
> http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleContext.
> cpp?rev=6229c2f38ea9#437
> 
> Implementing the css-align properties will make this a lot more common.
> For grid/flexbox containers it's going to occur 100% of the time.
> I suspect there is a memory/performance penalty associated with that.

So GetUniqueStyleData() isn't really much worse than conditions.SetUncacheable(), except that using GetUniqueStyleData() in ApplyStyleFixups() also defeats the laziness of struct computation and forces that struct to be computed eagerly.  (Then again, I'm not even sure the laziness is a win -- it might be faster to do all the struct computation eagerly, which would probably be better for memory locality.)

Using GetUniqueStyleData() rather than conditions.SetUncacheable() actually has the advantage that we don't need to make the style data uncacheable in all cases because we need to do something weird for some cases.  For example, in the code handling text-align: -moz-center and -moz-right, we only have to call GetUniqueStyleData when disp->mDisplay is NS_STYLE_DISPLAY_TABLE.  If we were doing the same with conditions.SetUncacheable(), we'd have to call SetUncacheable() anytime mTextAlign had one of those values, whether or not disp->mDisplay is NS_STYLE_DISPLAY_TABLE.  So in that case, GetUniqueStyleData is much cheaper since it defeats storage in the rule tree in much fewer cases.

So, for example, with 'align-content' and 'justify-content', you'd call GetUniqueStyleData for the struct with mAlignContent/mJustifyContent only when (a) the element is a flex container or grid container (non-default, and relatively rare) and (b) align-content / justify-content is auto (the default).  Since condition (a) is a non-default condition, it wouldn't be a big deal for performance or memory.  (This is a similar case where GetUniqueStyleData is a lot cheaper than conditions.SetUncacheable().)

The other four properties are a bigger problem, though.  I'm not sure what to do about them.
Flags: needinfo?(dbaron)
(In reply to David Baron [:dbaron] ⌚UTC-7 (busy, returning September 22) from comment #21)
> The other four properties are a bigger problem, though.  I'm not sure what
> to do about them.

Actually, other than the 'legacy' values, I do have an idea.  We could internally treat 'auto' as the computed value all the time, have a method on the style context that gets the "real" computed value of the property, and then use that method (preferably lazily) when handling eCSSValue_Inherit in nsRuleNode::ComputeDisplayData, and add a CheckDisplayCallback for nsRuleNode::CheckSpecifiedProperties and remember to change Inherited to Mixed when we hit the relevant cases.
... although the idea in comment 22 wouldn't be right for animations, but I'm not sure anyone would care.
(In reply to David Baron [:dbaron] ⌚UTC-7 (busy, returning September 22) from comment #23)
> ... although the idea in comment 22 wouldn't be right for animations, but
> I'm not sure anyone would care.

er, by animations, I meant transitions.  And, actually, we could work around that with eStyleAnimType_Custom.

There's probably yet something else that would be wrong, but maybe nobody will notice.  I guess it's pretty similar to what we do for currentColor border colors today (except that that's now changed so that it is what the spec says to do, and we haven't yet fixed things to match).
FWIW, I had some trouble with the justify-items 'legacy' values because
a child with 'auto' doesn't get a call ComputePositionData at all!
(I'm guessing due to the laziness you mentioned.)

So I'll handle 'auto' for both justify-items/self in ApplyStyleFixups
for now - which unfortunately means calling GetUniqueStyleData() for
nsStylePosition in the common case.  I suggest we handle any new
mechanisms to improve the performance here as a separate bug.
(also pending any feedback from www-style)
I think that might be an unacceptable speed and memory use regression.
Attachment #8659210 - Attachment is obsolete: true
(In reply to David Baron [:dbaron] ⌚UTC-7 from comment #27)
> I think that might be an unacceptable speed and memory use regression.

I wonder if creating a separate struct for these values would help.
I note that the six properties align/justify-contents/items/self is
just 64 bytes together.  It seems like it would be pretty cheap even
if we always made a copy in ApplyStyleFixups of such a small struct.

Of course, mutating the used values instead of the computed values
would make the performance issue moot.
I'm widening the scope of this bug to implement all of the css-align spec
in the style system.  I originally intended to just add the missing
property values and skip implementing parsing for the new syntax, but
I figured I should just do it all rather have to revisit this later...
Summary: [css-grid][css-align] Add the missing property values for align/justify-self and align/justify-content → [css-grid][css-align] Implement full support for the CSS Box Alignment spec in the style system.
(In reply to Mats Palmgren (:mats) from comment #28)
> just 64 bytes together.

er, I meant 64 bits in total.
Blocks: css-align-3
Blocks: 1207698
Simon, feel free to skip reviewing any changes to ApplyStyleFixups if you want.
I think the CSSWG is considering changing the spec for resolving 'auto' to
used values instead of computed values, so I may have to revisit that part.
https://lists.w3.org/Archives/Public/www-style/2015Sep/0099.html
Do you think a separate style struct would be acceptable from a performance POV?
(64 *bits* in total) (see comment 28)
Flags: needinfo?(dbaron)
(In reply to Mats Palmgren (:mats) from comment #41)
> Do you think a separate style struct would be acceptable from a performance
> POV?
> (64 *bits* in total) (see comment 28)

I think it's probably not.  I'd prefer to take the approach of manipulating used values **if** you feel that the feature the spec provides by doing so is actually important and necessary.

That said, you should be carefully judging the value of everything specified here before implementing; the spec's editors don't think it's ready for implementation yet:
http://krijnhoetmer.nl/irc-logs/css/20150928#l-212

(I think a bunch of it is clearly very useful, but I'm not convinced that the implementation complexity required by the current spec draft is needed to get those benefits.)
Flags: needinfo?(dbaron)
To be clear:  I think we should be pushing on the part that is important to make it ready to implement -- and one of the ways to do that is by implementing it -- but it requires judgment about what is important and what the tradeoffs are.  (I don't think we should stop working on this just because the spec's authors don't think it's ready.)
Comment on attachment 8664963 [details] [diff] [review]
part 1 - [css-align] Implement the 'justify-items' property in the style system.

Review of attachment 8664963 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the two cases merged if they can be. (See inline comment.)

::: layout/style/nsStyleContext.cpp
@@ +825,5 @@
> +      }
> +      break;
> +    }
> +    case NS_STYLE_DISPLAY_GRID:
> +    case NS_STYLE_DISPLAY_INLINE_GRID: {

Is this case identical to the one above? Could they be merged?
Attachment #8664963 - Flags: review?(simon.sapin) → review+
Attachment #8664964 - Flags: review?(simon.sapin) → review+
I'm attaching a new batch of patches that fixes the performance problem
with the ApplyStyleFixups approach.  The patches are mostly the same
as before, but the 'inherit' stuff in nsRuleNode, and nsComputedDOMStyle
methods now use nsStylePosition::ComputedJustifyItems etc to resolve the
computed value dynamically. (I'm also making mJustifyItems etc private
to avoid using them directly by mistake).  The new patches have no
changes to ApplyStyleFixups.

The new approach, that dbaron suggested, is that we store 'auto'
on the style struct and resolve it later when needed.  We need the
parent's value for 'inherit' directly but at this point we do have
'display' for the parent so the value can be resolved correctly.
The new nsStylePosition::Computed* methods are also used in
nsComputedDOMStyle and later in Layout when they need the values.
Attachment #8664963 - Attachment is obsolete: true
Attachment #8673039 - Flags: review?(simon.sapin)
This has an XXX comment in nsStylePosition::MapLeftRightToStart.
It's about "If the property’s axis is not parallel with the inline axis, this value computes to 'start'."
https://drafts.csswg.org/css-align-3/#valdef-self-position-left
which is trivial for all box types except Flexbox where it depends on
some flex properties.  I expect dholbert will fill in this bit later :-)
Attachment #8664964 - Attachment is obsolete: true
Attachment #8673042 - Flags: review?(simon.sapin)
As before, most of the code is the same, but ComputedJustifyContent()
is new and nsComputedDOMStyle::DoGetJustifyContent() and the 'inherit'
code now use that.  A minor tweak to Flexbox layout code here too
to make it compile: mJustifyContent is now private it must use
the new ComputedJustifyContent method instead.
Attachment #8664965 - Attachment is obsolete: true
Attachment #8664965 - Flags: review?(simon.sapin)
Attachment #8673044 - Flags: review?(simon.sapin)
Similar changes...
Attachment #8664966 - Attachment is obsolete: true
Attachment #8664966 - Flags: review?(simon.sapin)
Attachment #8673045 - Flags: review?(simon.sapin)
Similar changes as previous patches.

A slight tweak to the Flexbox code though that I should explain:

+  if (MOZ_UNLIKELY(mAlignSelf == NS_STYLE_ALIGN_AUTO)) {
+    // Happens in rare edge cases when 'position' were ignored.
+    mAlignSelf = NS_STYLE_ALIGN_STRETCH;
+  }

Since the new Computed<property> methods use style values alone
there is a potential mismatch between that and the box type
that the frame constructor actually created.  Here's an example:
layout/generic/crashtests/973701-1.xhtml
For MathML we ignore 'position' so the <munderover> ends up
being a flex item.  But ComputedAlignSelf() uses 
IsAbsolutelyPositionedStyle() which looks at the 'position'
value so it computes 'align-self' to 'auto' because that's what
the spec says for "Absolutely-positioned boxes", although in
this case the box type really isn't an abs.pos. of course.

I don't think anyone cares that much that the align/justify
values are wrong in these edge cases though, so my suggested
work around is to map 'auto' to some suitable value in Layout.
We could of course write a separate set of Computed methods
for Layout that looks at the frame type etc, but that's
a) more code, b) probably slower (virtual calls etc), and
c) there would be a mismatch with the value 'inherit' etc
use anyway; so it doesn'tseem worth it.
Attachment #8664967 - Attachment is obsolete: true
Attachment #8664967 - Flags: review?(simon.sapin)

        Attachment #8664969 -
        Attachment is obsolete: true

        Attachment #8664969 -
        Flags: review?(simon.sapin)

        Attachment #8673052 -
        Flags: review?(simon.sapin)

    
    

    
  
Looks green with some slightly updated Layout patches for bug 1151214 / bug 1151213.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=955193cb6d9c

    
    

    
  
Comment on attachment 8673042 [details] [diff] [review]
part 2 - [css-align] Implement the 'justify-self' property in the style system.

BTW, I'm not entirely sure that piercing through display:contents
ancestors in GetAlignmentContainer is the right thing to do.
I'm still contemplating this case and might ask www-style@ later.
(The spec doesn't say anything on the matter.)

        Attachment #8673039 -
        Flags: review?(simon.sapin) → review+

        Attachment #8673042 -
        Flags: review?(simon.sapin) → review+

    
  

        Attachment #8673051 -
        Flags: review?(simon.sapin)

    
    

    
  
Cam, I wonder if you can take over the remaining reviews here please?
Flags: needinfo?(cam)

    
    

    
  


  
A couple of minor changes to how we're mapping 'left'/'right' to 'start'
in ComputedJustifyItems and ComputedJustifySelf (from the first two
patches).  I think we should simply map the member value and when
justify-self copies it's value from the parent it should map the
value it "inherits" according to its own display value.

I'm removing the special display:contents handling for now.
I'll contemplate that a bit more and then propose something
to www-style and deal with it in a later bug.
It's not particularly important at this point anyway.

        Attachment #8673044 -
        Attachment is obsolete: true

        Attachment #8673044 -
        Flags: review?(simon.sapin)

        Attachment #8677575 -
        Flags: review?(cam)

    
    

    
  


  
Similar tweaks to left/right -> start mapping.

        Attachment #8673045 -
        Attachment is obsolete: true

        Attachment #8673045 -
        Flags: review?(simon.sapin)

        Attachment #8677576 -
        Flags: review?(cam)

    
    

    
  


  

        Attachment #8673051 -
        Attachment is obsolete: true

        Attachment #8673051 -
        Flags: review?(simon.sapin)
Flags: needinfo?(cam)

        Attachment #8677578 -
        Flags: review?(cam)

    
  

        Attachment #8673052 -
        Flags: review?(simon.sapin) → review?(cam)

    
  

        Attachment #8673053 -
        Flags: review?(simon.sapin) → review?(cam)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        part 1 to 7 as one patch
        — 
          — Splinter Review
      

    


  

    
    

    
  
Comment on attachment 8677575 [details] [diff] [review]
part 3 - [css-align] Implement additional syntax and values for the 'justify-content' property in the style system.

Review of attachment 8677575 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsCSSParser.cpp
@@ +9435,5 @@
>  
> +// auto | <baseline-position> | [ <content-distribution> ||
> +//   [ <overflow-position>? && <content-position> ] ]
> +bool
> +CSSParserImpl::ParseAlignJustifyContent(nsCSSProperty aPropID)

This method doesn't seem to be parsing <overflow-position>?

@@ +9444,5 @@
> +      nsCSSValue fallbackValue;
> +      if (!ParseEnum(value, nsCSSProps::kAlignContentDistribution)) {
> +        if (!ParseAlignJustifyPosition(fallbackValue,
> +                                       nsCSSProps::kAlignContentPosition) ||
> +            fallbackValue.GetUnit() == eCSSUnit_Null) {

Can fallbackValue be null here if ParseAlignJustifyPosition returned true?

::: layout/style/nsCSSPropList.h
@@ +1705,5 @@
> +    "",
> +    0,
> +    nullptr,
> +    CSS_PROP_NO_OFFSET,
> +    eStyleAnimType_None)

I see we're changing this to be non-animatable.  Is the plan to just handle this once we support non-interpolable animation of all properties, rather than implement custom animation for this one, and figure out how to deal with the fact that mJustifyContent is private then?

::: layout/style/nsCSSValue.cpp
@@ +1320,5 @@
> +    case eCSSProperty_justify_content: {
> +      AppendAlignJustifyValueToString(intValue & NS_STYLE_ALIGN_ALL_BITS, aResult);
> +      auto fallback = intValue >> NS_STYLE_ALIGN_ALL_SHIFT;
> +      if (fallback) {
> +        aResult.Append(' ');

Since the fallback should only be one of a limited set of values, maybe assert that here?  You could also assert in nsCSSValue::AppendAlignJustifyValueToString that a <baseline-position> or "auto" doesn't have any flags.

Also, I think the serialization should follow the order of the property's grammar.  So, in the case of a

  <content-distribution> || [ <overflow-position>? && <content-position> ]

value, we should serialize the <content-distribution> first, if specified, then the <overflow-position> flag, if specified, then the <content-position>, if specified.

Same goes for nsComputedDOMStyle::DoGetJustifyContent.  It might make sense to have a single utility method that can serialize the value+fallback so we don't have to repeat this logic in nsComputedDOMStyle::DoGetJustifyContent, too.

::: layout/style/nsStyleStruct.cpp
@@ +1681,5 @@
> +{
> +  switch (aDisplay->mDisplay) {
> +    case NS_STYLE_DISPLAY_FLEX:
> +    case NS_STYLE_DISPLAY_INLINE_FLEX:
> +      if (mJustifyContent == NS_STYLE_JUSTIFY_AUTO ||

The spec says that "auto" behaves like "stretch", but I don't think that means that it computes to the same value that "stretch" does (i.e., "flex-start").  Instead we should be storing "auto" in the value, yes?

The "storing auto in the style struct but later computing it to another value" behavior should only apply to {align,justify}-{self,items}, right?

@@ +1689,5 @@
> +      break;
> +    case NS_STYLE_DISPLAY_GRID:
> +    case NS_STYLE_DISPLAY_INLINE_GRID:
> +      if (mJustifyContent == NS_STYLE_JUSTIFY_AUTO) {
> +        return NS_STYLE_JUSTIFY_START;

Similarly here; "auto" behaves like "start" for grid containers, but shouldn't compute to it.

::: layout/style/test/property_database.js
@@ +4016,5 @@
> +    initial_values: [ "auto" ],
> +    other_values: [ "start", "end", "flex-start", "flex-end", "center", "left",
> +                    "right", "space-between", "space-around", "space-evenly",
> +                    "baseline", "last-baseline", "stretch", "start safe",
> +                    "true end", "true end stretch", "end safe space-evenly" ],

Since we don't seem to be parsing <overflow-position> values in CSSParserImpl::ParseAlignJustifyContent, I wonder why these values with "true" and "safe" in them here aren't causing failures?  Am I missing something?

        Attachment #8677575 -
        Flags: review?(cam) → review-

    
    

    
  
(In reply to Cameron McCormack (:heycam) (away Oct 28) from comment #59)
> Also, I think the serialization should follow the order of the property's
> grammar.  So, in the case of a
> 
>   <content-distribution> || [ <overflow-position>? && <content-position> ]
> 
> value, we should serialize the <content-distribution> first, if specified,
> then the <overflow-position> flag, if specified, then the
> <content-position>, if specified.

Actually this might already be the case.  Please ignore if so.

        Attachment #8673053 -
        Flags: review?(cam) → review+

    
    

    
  
(In reply to Cameron McCormack (:heycam) (away Oct 28) from comment #59)
> The spec says that "auto" behaves like "stretch", but I don't think that
> means that it computes to the same value that "stretch" does (i.e.,
> "flex-start").  Instead we should be storing "auto" in the value, yes?

https://drafts.csswg.org/css-align-3/#content-distribution
Under Flexbox it says:
"The justify-content [...] 'stretch' computes to 'flex-start'.
'auto' behaves as 'stretch'."
and then ISSUE 8:
"The Flexbox spec's legacy definition has "stretch" as the initial value,
so this is technically a behavior change. We can instead "auto" compute
to "stretch" for Flexbox if we decide it's necessary for compat."

So I think you're right that 'auto' computes to itself and then we'll
have to map it in Layout to 'flex-start', but it seems a bit silly
since we already have to compute 'stretch' to 'flex-start' anyway
so we might as well compute 'auto' to 'flex-start' directly.

I don't have a strong opinion either way though.
Daniel, what do you think?
Flags: needinfo?(dholbert)

    
    

    
  
I'd prefer to avoid magic "display"-dependent property-computation behavior.  So, I'd lean towards doing what heycam suggests and simply interpreting non-flex-relevant "justify-content" values in layout, and keeping them at their specified values in the style-struct.

(As you note, this represents a behavior change, exposed to web content via getComputedStyle on a default-styled flex item. This is fine, I think; there have been other more-significant flexbox spec-changes that the spec authors have taken in recent history, so this one doesn't concern me too much. I'd be surprised if any sites are impacted by this.)
Flags: needinfo?(dholbert)

    
    

    
  
OK, let's go with what the spec currently says, until ISSUE 8 is resolved one
way or the other.  I've added a XXX comment in the code for now.

    
    

    
  


  
(In reply to Cameron McCormack (:heycam) from comment #59)
> > +// auto | <baseline-position> | [ <content-distribution> ||
> > +//   [ <overflow-position>? && <content-position> ] ]
> > +bool
> > +CSSParserImpl::ParseAlignJustifyContent(nsCSSProperty aPropID)
> 
> This method doesn't seem to be parsing <overflow-position>?

It calls ParseAlignJustifyPosition which parses it (optionally).

> > +        if (!ParseAlignJustifyPosition(fallbackValue,
> > +                                       nsCSSProps::kAlignContentPosition) ||
> > +            fallbackValue.GetUnit() == eCSSUnit_Null) {
> 
> Can fallbackValue be null here if ParseAlignJustifyPosition returned true?

Yes, ParseAlignJustifyPosition parses an *optional* alignment value.
It returns false only when a parse error occurs, the caller have to
check for eCSSUnit_Null to know if we got a value or not.  This is
because this method is re-used by several places and this part
may be optional/mandatory in different cases.

Note: the ParseAlignJustifyPosition isn't necessarily the fallback
value.  It's the main value if only the
[ <overflow-position>? && <content-position> ]
part is specified.  It's the fallback when both parts of the ||
are specified.

> > +    eStyleAnimType_None)
> 
> I see we're changing this to be non-animatable.  Is the plan to just handle
> this once we support non-interpolable animation of all properties, rather
> than implement custom animation for this one, and figure out how to deal
> with the fact that mJustifyContent is private then?

All these properties have "Animatable: no" in the spec:
https://drafts.csswg.org/css-align-3/#property-index
I thought this is what eStyleAnimType_None is for?
(It's certainly a misnomer if it's not :-) )

(some of them might have been animatable in earlier an spec
version though?)

> Since the fallback should only be one of a limited set of values, maybe
> assert that here?  You could also assert in
> nsCSSValue::AppendAlignJustifyValueToString that a <baseline-position> or
> "auto" doesn't have any flags.

OK, I've added an assertion for that.

> we should serialize the <content-distribution> first, if specified,
> then the <overflow-position> flag, if specified, then the
> <content-position>, if specified.

Yes, that's what the code does.

> > +    case NS_STYLE_DISPLAY_GRID:
> > +    case NS_STYLE_DISPLAY_INLINE_GRID:
> > +      if (mJustifyContent == NS_STYLE_JUSTIFY_AUTO) {
> > +        return NS_STYLE_JUSTIFY_START;
> 
> Similarly here; "auto" behaves like "start" for grid containers, but
> shouldn't compute to it.

Fair enough.  (I wouldn't be surprised if this changes to "computes to"
in a future spec though, if ISSUE 8 is resolved in favor ;-) )

        Attachment #8677575 -
        Attachment is obsolete: true

        Attachment #8678349 -
        Flags: review?(cam)

    
    

    
  


  
Made 'auto' compute to itself for align-content too.

        Attachment #8673052 -
        Attachment is obsolete: true

        Attachment #8673052 -
        Flags: review?(cam)

        Attachment #8678350 -
        Flags: review?(cam)

    
    

    
  


  
Tweaked the AppendAlignJustifyValueToString assertion a little bit.

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

        Attachment #8678349 -
        Attachment is obsolete: true

        Attachment #8678349 -
        Flags: review?(cam)

        Attachment #8678581 -
        Flags: review?(cam)

    
    

    
  
(In reply to Mats Palmgren (:mats) from comment #64)
> > I see we're changing this to be non-animatable.  Is the plan to just handle
> > this once we support non-interpolable animation of all properties, rather
> > than implement custom animation for this one, and figure out how to deal
> > with the fact that mJustifyContent is private then?
> 
> All these properties have "Animatable: no" in the spec:
> https://drafts.csswg.org/css-align-3/#property-index
> I thought this is what eStyleAnimType_None is for?
> (It's certainly a misnomer if it's not :-) )
> 
> (some of them might have been animatable in earlier an spec
> version though?)

I'm not sure.  Do you think it's a problem dbaron if we stop supporting animation of justify-content until we support it for Animatable:no properties in general?
Flags: needinfo?(dbaron)

    
    

    
  
Comment on attachment 8678581 [details] [diff] [review]
part 3 - [css-align] Implement additional syntax and values for the 'justify-content' property in the style system.

Review of attachment 8678581 [details] [diff] [review]:
-----------------------------------------------------------------

r=me if David is OK with the loss of animatability of the property.

::: layout/style/nsCSSParser.cpp
@@ +9490,5 @@
> +          value = fallbackValue;
> +          fallbackValue.Reset();
> +        }
> +      } else {
> +        // any optional <content-position> is a fallback value

Where you mention <content-position> in the comments here, can you make it clear that you're also including the optional <overflow-position>?  I think that's what led me astray to think that you weren't parsing it.

        Attachment #8678581 -
        Flags: review?(cam) → review+

    
    

    
  
Comment on attachment 8677576 [details] [diff] [review]
part 4 - [css-align] Implement additional syntax and values for the 'align-items' property in the style system.

Review of attachment 8677576 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the same proviso on the animatibility of the property.

FYI, if you do need/want to keep the animatibility, you could probably leave the members and private and just make whatever class does the animation a friend.

::: layout/style/nsCSSProps.cpp
@@ +1306,1 @@
>  // Note: 'align-self' takes the same keywords as 'align-items', plus 'auto'.

Please update this comment now that align-items also takes 'auto'.  Maybe just make it say that this table of keywords is for both align-self and align-items.

::: layout/style/nsComputedDOMStyle.cpp
@@ +3880,5 @@
>      // "align-self: auto" needs to compute to parent's align-items value.
>      nsStyleContext* parentStyleContext = mStyleContext->GetParent();
>      if (parentStyleContext) {
>        computedAlignSelf =
> +        parentStyleContext->StylePosition()->ComputedAlignItems(parentStyleContext->StyleDisplay());

Nit: Break somewhere before 80 columns?

::: layout/style/nsRuleNode.cpp
@@ +7796,5 @@
> +  } else {
> +    SetDiscrete(alignItemsValue,
> +                pos->mAlignItems, conditions,
> +                SETDSC_ENUMERATED | SETDSC_UNSET_INITIAL,
> +                parentPos->mAlignItems,

Maybe add a comment on this line saying that this value isn't used, since we handle 'inherit' above?  (And similarly in the other patches.)  Otherwise it looks like we might incorrectly inherit an 'auto' value.

        Attachment #8677576 -
        Flags: review?(cam) → review+

    
    

    
  
Comment on attachment 8677578 [details] [diff] [review]
part 5 - [css-align] Implement additional syntax and values for the 'align-self' property in the style system.

Review of attachment 8677578 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsFlexContainerFrame.cpp
@@ +1577,5 @@
> +  mAlignSelf = aFlexItemReflowState.mStylePosition->ComputedAlignSelf(
> +                 aFlexItemReflowState.mStyleDisplay,
> +                 mFrame->StyleContext()->GetParent());
> +  if (MOZ_UNLIKELY(mAlignSelf == NS_STYLE_ALIGN_AUTO)) {
> +    // Happens in rare edge cases when 'position' were ignored.

Nit: "is" or "was" rather than "were".

Can you expand on this situation a bit further in the comment?  The spec says auto computes to itself for absolutely positioned elements.  Is there a situation where flex items can have position:absolute on them, which is ignored for layout, but which still causes align-self to leave auto computed as auto?

::: layout/style/nsCSSProps.cpp
@@ -1302,5 @@
>    eCSSKeyword_stretch,       NS_STYLE_ALIGN_CONTENT_STRETCH,
>    eCSSKeyword_UNKNOWN,-1
>  };
>  
> -// Note: 'align-self' takes the same keywords as 'align-items', plus 'auto'.

That comment on the table we're using for all of these properties should be updated again...

::: layout/style/nsRuleNode.cpp
@@ +7805,5 @@
> +  const auto& alignSelfValue = *aRuleData->ValueForAlignSelf();
> +  if (MOZ_UNLIKELY(alignSelfValue.GetUnit() == eCSSUnit_Inherit)) {
> +    if (MOZ_LIKELY(parentContext)) {
> +      nsStyleContext* grandparentContext = parentContext->GetParent();
> +      if (MOZ_LIKELY(grandparentContext)) {

(Aside: I'm wondering how useful these MOZ_LIKELY/MOZ_UNLIKELY annotations are in practice, unless we know for sure that this is a performance critical part of the code where branch mis-predictions are common?)

::: layout/style/nsStyleStruct.cpp
@@ +1699,5 @@
> +  if (MOZ_LIKELY(aParent)) {
> +    auto parentAlignItems = aParent->StylePosition()->
> +      ComputedAlignItems(aParent->StyleDisplay());
> +    MOZ_ASSERT(!(parentAlignItems & NS_STYLE_ALIGN_LEGACY),
> +               "align-items can't have 'legacy'");

The spec says "minus any legacy keywords", but you are right that align-items doesn't allow 'legacy'.  File a bug on the spec to fix that?

        Attachment #8677578 -
        Flags: review?(cam) → review+

    
    

    
  
Comment on attachment 8677578 [details] [diff] [review]
part 5 - [css-align] Implement additional syntax and values for the 'align-self' property in the style system.

Review of attachment 8677578 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsComputedDOMStyle.cpp
@@ +3946,5 @@
>    return val;
>  }
>  
>  CSSValue*
> +nsComputedDOMStyle::DoGetAlignSelf()

Why don't we worry about the fallback value here?

    
    

    
  
By the way, we don't want to land support for the new properties until we actually support them in layout, right?  Or we can, but only if we put them behind a pref.

    
    

    
  
Comment on attachment 8678350 [details] [diff] [review]
part 6 - [css-align] Implement additional syntax and values for the 'align-content' property in the style system.

Review of attachment 8678350 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsRuleNode.cpp
@@ +7794,5 @@
>                parentPos->mBoxSizing,
>                NS_STYLE_BOX_SIZING_CONTENT, 0, 0, 0, 0);
>  
>    // align-content: enum, inherit, initial
> +  const auto& alignContentValue = *aRuleData->ValueForAlignContent();

This pattern of handling inherit and auto values might be common enough to factor out into a helper function.

        Attachment #8678350 -
        Flags: review?(cam) → review+

    
    

    
  
(In reply to Cameron McCormack (:heycam) (away Nov 3) from comment #67)
> (In reply to Mats Palmgren (:mats) from comment #64)
> > > I see we're changing this to be non-animatable.  Is the plan to just handle
> > > this once we support non-interpolable animation of all properties, rather
> > > than implement custom animation for this one, and figure out how to deal
> > > with the fact that mJustifyContent is private then?
> > 
> > All these properties have "Animatable: no" in the spec:
> > https://drafts.csswg.org/css-align-3/#property-index
> > I thought this is what eStyleAnimType_None is for?
> > (It's certainly a misnomer if it's not :-) )
> > 
> > (some of them might have been animatable in earlier an spec
> > version though?)
> 
> I'm not sure.  Do you think it's a problem dbaron if we stop supporting
> animation of justify-content until we support it for Animatable:no
> properties in general?

I believe eStyleAnimType_EnumU8 is used only by SMIL animation and not by CSS Animation and Transitions.  (CSS Animations and Transitions always test *both* StyleAnimationValue::ExtractComputedValue (via ExtractComputedValueForTransition) *and* StyleAnimationValue::Interpolate; with EnumU8 the ExtractComputedValue should work but the Interpolate will fail.  dholbert and/or birtles would know more.
Flags: needinfo?(dbaron)

    
    

    
  
OK, thanks.  I think it's very unlikely people are relying on these being animatable from SMIL, so Mats let's go ahead with them as unanimatable.

    
    

    
  
(In reply to Cameron McCormack (:heycam) from comment #70)
> > +  if (MOZ_UNLIKELY(mAlignSelf == NS_STYLE_ALIGN_AUTO)) {
> > +    // Happens in rare edge cases when 'position' were ignored.
> 
> Can you expand on this situation a bit further in the comment? 

Sure:

    // Happens in rare edge cases when 'position' was ignored by the frame
    // constructor (and the style system computed 'auto' based on 'position').

> Is there a
> situation where flex items can have position:absolute on them, which is
> ignored for layout, but which still causes align-self to leave auto computed
> as auto?

Yes.  See comment 49.

> ::: layout/style/nsCSSProps.cpp
> > -// Note: 'align-self' takes the same keywords as 'align-items', plus 'auto'.
> 
> That comment on the table we're using for all of these properties should be
> updated again...

There is no table for 'align-self' anymore.  The parser now use
the kAlignSelfPosition table to parse the <self-position> part.

> ::: layout/style/nsRuleNode.cpp
> (Aside: I'm wondering how useful these MOZ_LIKELY/MOZ_UNLIKELY annotations
> are in practice, unless we know for sure that this is a performance critical
> part of the code where branch mis-predictions are common?)

That depends on compiler/platform and is subject to change, of course.
I try to annotate code I think is hot, *just in case* it helps (now or
in the future).

> ::: layout/style/nsStyleStruct.cpp
> > +    MOZ_ASSERT(!(parentAlignItems & NS_STYLE_ALIGN_LEGACY),
> > +               "align-items can't have 'legacy'");
> 
> The spec says "minus any legacy keywords", but you are right that
> align-items doesn't allow 'legacy'.  File a bug on the spec to fix that?

It seems unlikely to me that the spec left out the 'legacy' values on
'align-items' by mistake.

    
    

    
  
(In reply to Cameron McCormack (:heycam) from comment #71)
> > +nsComputedDOMStyle::DoGetAlignSelf()
> 
> Why don't we worry about the fallback value here?

'align-self' doesn't have a fallback value.
https://drafts.csswg.org/css-align-3/#propdef-align-self
(only *-content does)

    
    

    
  
(In reply to Cameron McCormack (:heycam) from comment #72)
> By the way, we don't want to land support for the new properties until we
> actually support them in layout, right?  Or we can, but only if we put them
> behind a pref.

The goal is to have them fully implemented for Grid and Flexbox.
If we miss that somehow before the next uplift (seems unlikely), we can
discuss options at that point.

For other box types, the current situation is that we'll parse and
compute these properties with mostly Flexbox specific values.
On an in-flow block I get:
align-self/items/content: stretch
justify-content: flex-start
(justify-items/self properties are not implemented)
and layout is only implemented for Flexbox.

So for other box types, I think the situation improves with these patches
because they will have correct computed values and be parsed according to
the current spec, albeit still missing layout (status quo).

    
    

    
  
(In reply to Cameron McCormack (:heycam) from comment #73)
> > +  const auto& alignContentValue = *aRuleData->ValueForAlignContent();
> 
> This pattern of handling inherit and auto values might be common enough to
> factor out into a helper function.

There are some differences between the properties here so it
doesn't seem worth it to me.

    
    

    
  
(In reply to Mats Palmgren (:mats) from comment #76)
> > ::: layout/style/nsStyleStruct.cpp
> > > +    MOZ_ASSERT(!(parentAlignItems & NS_STYLE_ALIGN_LEGACY),
> > > +               "align-items can't have 'legacy'");
> > 
> > The spec says "minus any legacy keywords", but you are right that
> > align-items doesn't allow 'legacy'.  File a bug on the spec to fix that?
> 
> It seems unlikely to me that the spec left out the 'legacy' values on
> 'align-items' by mistake.

I wasn't clear, but I was meaning that the spec should remove the "minus any legacy keywords" wording, since there are no legacy keywords for this property, rather than adding the legacy keywords to the property.

    
    

    
  
(In reply to Mats Palmgren (:mats) from comment #78)
> The goal is to have them fully implemented for Grid and Flexbox.
> If we miss that somehow before the next uplift (seems unlikely), we can
> discuss options at that point.

OK, but in the meantime, if authors are trying to feature detect support for these new properties/values, e.g. with

  @supports (justify-items: auto) {
    ...
  }

then it's not going to work out well for them.  Maybe it also violates http://www.w3.org/TR/CSS/#partial to support parsing a property without implementing its behaviour.

    
    

    
  
(In reply to Cameron McCormack (:heycam) from comment #82)
> (In reply to Mats Palmgren (:mats) from comment #78)
> > The goal is to have them fully implemented for Grid and Flexbox.
> > If we miss that somehow before the next uplift (seems unlikely), we can
> > discuss options at that point.
> 
> OK, but in the meantime, if authors are trying to feature detect support for
> these new properties/values, e.g. with
> 
>   @supports (justify-items: auto) {

Since the 'auto' value doesn't have any associated layout I think this
value is actually fully implemented (in that it computes to the right
value for all possible box types etc per spec).

A better example is "@supports (align-self: flex-end)" which tests true
in current *RELEASE* builds, although layout for it is only implemented
for Flexbox.  (This is true for *all* the implemented properties/values
until now.)

> Maybe it also violates
> http://www.w3.org/TR/CSS/#partial to support parsing a property without
> implementing its behaviour.

Yes, but we were already violating that *before* these patches landed,
even in RELEASE builds.

These patches takes us from:
  state A: partial layout + wrong computed values and incomplete syntax
to:
  state B: partial layout + correct computed values and complete syntax

IMHO, state B is strictly better than state A.

The only way to conform to http://www.w3.org/TR/CSS/#partial
would be to:
  1. put all these properties behind a pref, all disabled by default
  2. disable Flexbox by default
  3. disable Grid by default
Then, in a year or two when we have layout for all box types,
we enable all the prefs at once.

I think that would be unreasonable, and it would certainly break
a lot of web sites that already use Flexbox.

IMO, we should instead charge forward and implement layout as quickly
as we can, but admit that we can't do it for all box types at once.
The goal is to implement full support for Flexbox/Grid in v45.
Then add layout for each of the other box types after that (i.e.
those box types will either have no layout (currently) or have full
layout as an atomic increment).

Does that seem like a reasonable plan given the current situation?
Flags: needinfo?(cam)

    
    

    
  
Hmm, why are uplifting this to b2g44?  That certainly wasn't my intention.
I only landed these patches because I thought we were on v45 now.
Flags: needinfo?(cbook)

    
  
Depends on: 1221565

    
    

    
  
(In reply to Mats Palmgren (:mats) from comment #86)
> Hmm, why are uplifting this to b2g44?  That certainly wasn't my intention.
> I only landed these patches because I thought we were on v45 now.

hey mats b2g44 is b2g 2.5 and they needed a merge to their branch, so we had to merge m-c master to that b2g-branch
Flags: needinfo?(cbook)

    
    

    
  
OK, but I don't want these patches anywhere that isn't based on v45.
Please back them out from b2g44 (all the patches I landed in the past
48 hours that were merged).
Flags: needinfo?(cbook)

    
    

    
  
(In reply to Cameron McCormack (:heycam) from comment #81)
> I wasn't clear, but I was meaning that the spec should remove the "minus any
> legacy keywords" wording, since there are no legacy keywords for this
> property, rather than adding the legacy keywords to the property.

Ah, gotcha.  Sent a mail to www-style:
https://lists.w3.org/Archives/Public/www-style/2015Nov/0043.html

    
    

    
  
(In reply to Mats Palmgren (:mats) from comment #85)
> Does that seem like a reasonable plan given the current situation?

OK, I think it's a fair plan.
Flags: needinfo?(cam)

    
    

    
  
(In reply to Mats Palmgren (:mats) from comment #88)
> OK, but I don't want these patches anywhere that isn't based on v45.
> Please back them out from b2g44 (all the patches I landed in the past
> 48 hours that were merged).

backed this out and the others from b2g v2.5 in http://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/0869ace8c965

sorry again mats for the confusion

          status-b2g-v2.5:
          fixed → ---
Flags: needinfo?(cbook)
Depends on: 1223568

    
  
Blocks: 1224804

    
  
Depends on: 1246101

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: