When deciding on an answer for https://github.com/w3c/csswg-drafts/issues/440 (a complex dependency between "align-self:auto" and other properties), the CSSWG decided to just have "auto" compute to itself, and get interpreted when it's used. Meeting notes: https://lists.w3.org/Archives/Public/www-style/2016Sep/0041.html GitHub issue that prompted the change: https://github.com/EFForg/https-everywhere/issues/4241 I'm filing this bug to cover this update -- in particular, removing any special-case code for handling this special value from the style system. (e.g. I think we have some special code to implement inheritance of resolved "auto" values, which can now go -- and nsComputedDOMStyle::DoGetAlignSelf() / DoGetJustifySelf() should be adjusted so they can return "auto".) In layout, we can continue to abstract away the resolve-auto-when-it-gets-used logic, via the Computed{Align|Justify}Self() methods: https://dxr.mozilla.org/mozilla-central/rev/fd0564234eca242b7fb753a110312679020f8059/layout/style/nsStyleStruct.cpp#1693 ...though we might want to rename them with s/Computed/Used/

The github issue is this one I believe: https://github.com/w3c/csswg-drafts/issues/440 Brad, do you have time to take this? I think we want to fix this before shipping Grid. As Daniel suggested, we could just rename the methods and make Grid/Flex layout call Used* instead. Calls from layout/style/ should probably just use the mAlign[Content|Self] fields directly.

> the mAlign[Content|Self] fields directly I meant the m[Align|Justify]Self fields.

The inheritance hacks that we can remove are here for "align-self": https://dxr.mozilla.org/mozilla-central/source/layout/style/nsRuleNode.cpp#8561 ...and similar for "justify-self" below that. Those properties should change to look like all the other enumerated properties' boilerlate in that function. (That to-be-removed code existed for scenarios like this with 3 nested elements: <div id="A" style="align-items:center"> <div id="B style="align-items:inherit"> <div id="C" style="align-self:auto; align-items: end"> <div id="D" style="align-self:inherit"> In this case, according to the old spec-text, "C" was supposed to end up with "align-self:center" in its computed style, and "D" was supposed to inherit that "center" value. According to the new text, "C" now just has "auto" in its computed style (which will be treated as "center" in layout -- its parent's align-items value). And "D" should now just inherit the computed value of "auto" (not "center"), and will end up with a used value of "end" (its parent's align-items value). So this change will be observable via a behavioral (alignment) change for "D" in this case (as well as via impacts on getComputedStyle) but it probably shouldn't affect real-world content much at all.

Just checking my understanding of the guidance already given in this bug report. Please verify these changes are as intended. If correct, these changes do break several tests. This patch updates layout/style/test/test_align_justify_computed_values.html with what I believe is the correct behavior, and my try build shows that these also need updating: layout/reftests/w3c-css/submitted/flexbox/flexbox-align-self-horiz-001-block.xhtml layout/reftests/w3c-css/submitted/flexbox/flexbox-align-self-horiz-001-table.xhtml layout/reftests/w3c-css/submitted/flexbox/flexbox-align-self-vert-001.xhtml layout/reftests/w3c-css/submitted/flexbox/flexbox-align-self-vert-rtl-001.xhtml layout/reftests/flexbox/flexbox-align-self-baseline-horiz-4.xhtml https://treeherder.mozilla.org/#/jobs?repo=try&revision=f0e1278e75e2&selectedJob=27924557

I think the initial value should be auto (in the SetValue call). I think UsedAlignSelf should still call ComputedAlignItems, and there should be no change to ComputedAlignItems (and no rename), (ditto for UsedJustifySelf and ComputedJustifyItems). The spec change only affects *-self as far as I understood.

Comment on attachment 8794419 [details] [diff] [review] usedStyle1.patch Review of attachment 8794419 [details] [diff] [review]: ----------------------------------------------------------------- I'm not sure any of the reftests you mentioned in comment 4 are expected to change behavior in this bug. Perhaps they fail even without your patch, due to subtle font-metrics issues or something like that? (A few of the flexbox reftests are definitely flaky on some platforms due to subtle font-dependent alignment things; these might be in that category.) If this patch really does influence the results of those tests, I'd be interested to see the reftest-analyzer results. (It might be correct that their behavior is changed, depending on the change; but I don't immediately see why they'd be expected to change.) ::: layout/base/nsLayoutUtils.cpp @@ +5515,5 @@ > !aFrame->StyleMargin()->HasBlockAxisAuto(aWM)) { > auto blockAxisAlignment = > !aWM.IsOrthogonalTo(aFrame->GetParent()->GetWritingMode()) ? > + stylePos->UsedAlignSelf(aFrame->StyleContext()->GetParent()) : > + stylePos->UsedJustifySelf(aFrame->StyleContext()->GetParent()); Please perform the s/Computed/Used/ renaming in its own dedicated (non-functional) patch. That renaming seems to make up the majority of this patch, and since it's non-functional, it makes it harder to see the actual functional things that are changing. ::: layout/style/test/test_align_justify_computed_values.html @@ +167,2 @@ > "computed value of 'align-self:inherit' on node when parent has " + > + "'align-self:auto' and grandparent has 'align-items:normal'") The change to the expected-value here is correct, but the change to the message is incorrect. The grandparent still has "align-items:baseline" here -- but now we end up inheriting "align-self:auto" and resolving that against our parent's align-items. (rather than inheriting a pre-resolved align-self value) Really, this test probably needs a good bit of editorial rewriting (and perhaps some chunks of it should just be deleted), since the comments & code are basically all documenting & checking the behavior of this now-removed special inheritance behavior...

BTW, I think the spec has been updated to reflect the change: https://drafts.csswg.org/css-align-3/#propdef-justify-self

And, right -- what mats said, about the Items properties not changing. (I think that also means you shouldn't be removing the nsRuleNode.cpp chunk about justify-items -- only the justify-self & align-self chunks there.)

Trying again with the preferred piece-wise method. This is just a rename patch.

Comment on attachment 8795040 [details] [diff] [review] Part 1: rename nsStyleStruct Computed* functions to Used*. Review of attachment 8795040 [details] [diff] [review]: ----------------------------------------------------------------- r-, because this is renaming too much. It should only be renaming ComputedAlignSelf & ComputedJustifySelf. ::: layout/style/nsStyleStruct.h @@ +1766,5 @@ > > /** > * Return the computed value for 'align-items'. > */ > + uint8_t UsedAlignItems() const { return mAlignItems; } As noted in comment 5, we should only be renaming the accessor for the {align,justify}-self properties. So this one (for align-items) should be left untouched. @@ +1777,5 @@ > > /** > * Return the computed value for 'justify-content'. > */ > + uint16_t UsedJustifyContent() const { return mJustifyContent; } The *-content accessors should be left untouched, too. This change doesn't have anything to do with them. (Also: it would be kinda misleading to rename them, because the presence of a special "Used" accessor implies that the Used value is different from the computed value. That makes sense for the *-self properties, because the used value *is* different there. But there's no difference between used/computed value for *-content, as I understand it. So, a "Used"-specific function would be misleading there.)

Okay, I'm slowly getting it. Here's a new version of the just-the-rename patch that limits itself to renaming the ComputedAlignSelf and ComputedJustifySelf functions.

Thanks! This patch looks a bit better. But (sorry) I'm realizing one other problem here: this renaming patch probably works a bit better as a "part 2", because in some of the places where you're doing the rename, we actually *DO* want the CSS computed value (not the used value). So, changing to invoke UsedAlignSelf/UsedJustifySelf in those places is kinda bogus. In particular: * the nsRuleNode.cpp chunks are dealing with inheritance -- and in CSS, the thing that's inherited is the computed value (not the used value). * the nsComputedDOMStyle chunks are about our getComputedStyle() implementation, which is also computed-style rather than used-style (with a few legacy exceptions like "width" and "height" properties). So, doing a s/ComputedStyle/UsedStyle/ rename in those locations is taking us from something that makes sense (using the computed value according to an old-ish version of the spec) to something that does not make sense (using the used value, in a place where we really want the computed value). I imagine we'd be back to making sense after the "part 2" patch addresses those spots by removing/fixing them -- but it'd be kind of better if we just fixed them up-front and then did the rename as an afterthought, I think. So, I think I'd prefer that we structure this like so: Part 1: clean up the special-case {align,justify}-self inheritance code in nsRuleNode.cpp, and clean up DoGetComputedAlign***Self to use m***Self member-vars instead of Computed***Self(). Part 2: Rename the Computed***Self methods to Used***Self. Does that sound OK? Sorry for not having this more clearly laid out when we set you on this bug. :)

This attempts to follow the guidance in comment 12, with these interpretations: > in some of the places where you're doing the rename, we actually *DO* want > the CSS computed value (not the used value). If that's the case, then it seems like nsStyleStruct should have both ComputedAlignSelf() and UsedAlignSelf(), and used value should just be a passthrough to the mAlignSelf member variable. > clean up the special-case {align,justify}-self inheritance code in > nsRuleNode.cpp, and clean up DoGetComputedAlign***Self to use m***Self > member-vars instead of Computed***Self(). To get those member variables, I made nsComputedDOMStyle::DoGetAlignSelf() call the new UsedAlignSelf() function, which passes that variable through. Still not sure this reflects the intent, but if it does, then it seems the next step is to identify which if any of the following call sites that currently call the computed values should instead call the used values: nsLayoutUtils::ComputeSizeWithIntrinsicDimensions() ReflowInput::InitConstraints() FlexItem::FlexItem() nsFrame::ComputeSize() nsGridContainerFrame::Tracks::Initialize() nsGridContainerFrame::ReflowInFlowChild() nsRuleNode::ComputePositionData()

> If that's the case, then it seems like nsStyleStruct should have both > ComputedAlignSelf() and UsedAlignSelf(), and used value should just be a > passthrough to the mAlignSelf member variable. I think it's the other way around. The field members (normally) stores computed values, which is why we just them directly rather than introducing accessors. The reason we had ComputedAlignSelf() was that the spec had some really complicated rules for computing the value which required us to look at other properties on the parent. The spec change moved that stuff from computed values to used values, so we can go back to just use the field as is for the computed value. Instead, we need to do that logic at used-value time (layout) instead, hence we want Used* methods that does the old thing and use that in layout code. I think everything under layout/style/ should use the m*Self fields directly and everything else should use the Used*Self methods. As a first approximation anyway, I haven't looked in detail. :-)

Comment on attachment 8795424 [details] [diff] [review] Bug 1304012 -- Part 1: distinguish between used and computed values for AlignSelf and JustifySelf. Review of attachment 8795424 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Brad Werth [:bradwerth] from comment #13) > > in some of the places where you're doing the rename, we actually *DO* want > > the CSS computed value (not the used value). > > If that's the case, then it seems like nsStyleStruct should have both > ComputedAlignSelf() and UsedAlignSelf() Perhaps, but you could also just directly grab the member-variables to access the computed versions. Those member-variables are public, after all (and that's normally how we access data from style structs -- just reading the member-var directly). These ComputedAlignSelf() etc. accessor functions because (for now) there's extra resolution to be done. So -- I tend to think it'd be better to avoid having two accessors, because it'd be too easy to accidentally use the wrong one. I think it'd be better to keep one accessor (which will be renamed from Computed to Used), and (informally) require that callers from outside of layout/style/ use that accessor, and callers from inside layout/style can use either the member-var or the accessor as-needed. (Probably usually the member-var.) > and used value should just be a > passthrough to the mAlignSelf member variable. This is wrong -- if we had a Computed() and a Used() accessor alongside each other, the relationship between them should be the other way around. The "used value" is a *derivation* of the "computed value" -- in cases where they differ, the "used value" is based on the computed value *plus* extra contextual information from layout, other elements, etc. See the definitions of these terms here: https://www.w3.org/TR/CSS2/cascade.html#computed-value https://www.w3.org/TR/CSS2/cascade.html#used-value > To get those member variables, I made nsComputedDOMStyle::DoGetAlignSelf() > call the new UsedAlignSelf() function, which passes that variable through. This might technically give us the right behavior, but the naming/semantics is wrong. - nsComputedDOMStyle::DoGetAlignSelf() is producing the result of getComputedStyle(), so it does really want the "computed value" (not the used value), as I said in my earlier comment. - As of this bug, the computed value will just be the direct member-var itself (no need for extra processing) - So "part 1" probably probably wants to just adjust DoGetAlignSelf() to grab the member-var itself. > Still not sure this reflects the intent, but if it does, then it seems the > next step is to identify which if any of the following call sites that > currently call the computed values should instead call the used values: > > nsLayoutUtils::ComputeSizeWithIntrinsicDimensions() > ReflowInput::InitConstraints() > FlexItem::FlexItem() > nsFrame::ComputeSize() > nsGridContainerFrame::Tracks::Initialize() > nsGridContainerFrame::ReflowInFlowChild() These are all in layout, so they should be working with the more-processed value (the one that's had "auto" resolved). So -- I don't think any of those callers need to change, except that they'll ultimately have their call renamed in the "part 2" renaming patch here. > nsRuleNode::ComputePositionData() This one does really want the computed value -- which will now be the raw member-var. I really do think my "Part 1" and "Part 2" suggestions in comment 12 should be what we go with here. Let's chat in #layout if my suggestions there don't make sense, or if anything's not clear at this point.

(sorry, only just saw Mats' response now. I think he and I said the same thing, but he was just more concise I think. :))

(In reply to Daniel Holbert [:dholbert] from comment #15) > Those member-variables are public, after all Oops -- as you noted in IRC, I guess they're private, not public. That was probably to prevent people from accidentally thinking that mAlignSelf/mJustifySelf were actually usable as computed values, back when the spec defined the computed value as being something more complex (as hinted in comment 14). Now that the computed value is simple again, we can just make these members public. (We should perhaps make most of the associated member-vars (mAlignItems, mAlignContent, mJustifyContent, etc) public as well, and remove their trivial ComputedXYZ() accessors, since they only existed for consistency with these Computed***Self() fucntions. I think mJustifyItems/ComputedJustifyItems() is the only one that needs to stay the way it is (private with a public accessor), since it's got some special computed-value behavior around "legacy". Anyway, we can handle this cleanup of the non-"***-self" member-vars in a separate bug, I think.)

Delivers part 1 features as laid out in comment 15. The movement of mAlignSelf and mJustifySelf to public is done piecemeal, with the understanding that a future patch will move the remaining member variables to public (with perhaps the exception of mJustifyItems).

Comment on attachment 8795463 [details] [diff] [review] Bug 1304012 -- Part 1: remove special-case inheritance for align-self and justify-self. Review of attachment 8795463 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! This looks good. r=me with one commit message nit and one code nit addressed. First, the commit message: > Bug 1304012 -- Part 1: remove special-case inheritance for align-self and justify-self. This description only covers half of what the patch does. Please broaden/clarify this to something like this: Bug 1304012 Part 1: Don't bother resolving align-self/justify-self 'auto' value for getComputedStyle & inheritance. ::: layout/style/nsStyleStruct.h @@ +1815,4 @@ > uint16_t mJustifyContent; // [reset] fallback value in the high byte > uint8_t mJustifyItems; // [reset] see nsStyleConsts.h > +public: > + uint8_t mAlignSelf; // [reset] see nsStyleConsts.h This is a nice attempt at making a minimal change here, but this reordering of the member-variables will actually cause a compile warning (treated as an error by our builders on TreeHerder), because this object's constructor has an initialization list: https://dxr.mozilla.org/mozilla-central/rev/c55bcb7c777ea09431b4d16903ed079ae5632648/layout/style/nsStyleStruct.cpp#1424 ...and compilers (clang at least) will warn if the initialization list is in a different order from the member-variable declarations. To avoid introducing this compile warning (/ error), let's just leave the member-variable order as it originally was (all the align things, and then all of the justify things), and just adjust/add public/private wrappers as-needed. Something like: public: uint8_t mAlignSelf; private: ... public: uint8_t mJustifySelf; ...more things...

Addresses issues in comment 19. https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c07e30afde9

Thanks! I'm not concerned with the extra private/public cruft since I anticipate it'll be cleaned up in bug 1305844 (where we'll only leave a "private: [...] public:" wrapper around mJustifyItems, and have everything else be public).

Rename of functions as laid out in comment 12. https://treeherder.mozilla.org/#/jobs?repo=try&revision=445399b23c34

Comment on attachment 8795528 [details] [diff] [review] Bug 1304012 -- Part 2: Rename nsStyleStruct Computed**Self functions to Used**Self. Review of attachment 8795528 [details] [diff] [review]: ----------------------------------------------------------------- r=me, just one nit: ::: layout/style/nsStyleStruct.h @@ +1772,5 @@ > /** > * Return the computed value for 'align-self' given our parent StyleContext > * aParent (or null for the root). > */ > + uint8_t UsedAlignSelf(nsStyleContext* aParent) const; Please fix the documentation comment for both of these functions (s/computed value/used value/)

(Of course, this can't land quite yet, since it'll break test_align_justify_computed_values.html and perhaps other tests as well. I assume you'll have a "part 3" patch coming up which updates tests to reflect the new expected behavior here. You might want to review the end of comment 6 when writing that patch -- and also, feel free to reach out in #layout if you run into any trouble with this, or if any chunks of affected tests don't make sense.)

Comment on attachment 8795505 [details] [diff] [review] Bug 1304012 -- Part 1: Don't bother resolving align-self/justify-self 'auto' value for getComputedStyle & inheritance. r=dholbert (Looks like you put an extra "r=dholbert" in the attachment summary for part 1 --> fixing that up. And I'll mark this explicitly r+, while I'm here, so sheriffs can more-clearly see that this has my blessing. :) )

Fixes up comments as requested in comment 25. Regarding tests, yes another part of this patch is needed before landing. In addition to the failing mochitest layout/style/test/test_align_justify_computed_values.html, these reftests are failing: layout/reftests/w3c-css/submitted/flexbox/flexbox-align-self-horiz-001-table.xhtml layout/reftests/flexbox/flexbox-align-self-baseline-horiz-4.xhtml layout/reftests/w3c-css/submitted/flexbox/flexbox-align-self-horiz-001-table.xhtml

(In reply to Brad Werth [:bradwerth] from comment #28) > Regarding tests [...] these reftests are failing: > > layout/reftests/w3c-css/submitted/flexbox/flexbox-align-self-horiz-001-table.xhtml I can reproduce just this one reftest failure. > layout/reftests/flexbox/flexbox-align-self-baseline-horiz-4.xhtml (I can't reproduce this one. I suspect it might be a subtle font-metrics reliance, which triggers a subtle failure on your machine, with or without the patch -- worth double-checking.) > layout/reftests/w3c-css/submitted/flexbox/flexbox-align-self-horiz-001-table.xhtml (This is the first failure again. :)) So I think there's only one reftest failure. I expect it has something to do with the fact that we have a weird pseudo-element wrapper (& frame) around table elements...

Yeah, so the problem here is that we do this when resolving "align-self" for flex items: mAlignSelf = aFlexItemReflowInput.mStylePosition->UsedAlignSelf( mFrame->StyleContext()->GetParent()); Due to the weird way moz-table-wrapper works, mFrame->StyleContext()->GetParent() actually returns the *table frame's* style context there. And so we're resolving our align-self:auto against the *table frame's" align-items property, instead of the flex container's align-items property. I'm not immediately sure why this wasn't already broken. But I think we really should be using mFrame->GetParent()->StyleContext() there, to be resolving against the parent frame's align-items (the flex container's align-items). That's what we do in nsGridContainerFrame, too (though there, we've got a pointer to the container stashed elsewhere so we don't have to ask the child for it).

(That source quote is from here BTW: https://dxr.mozilla.org/mozilla-central/rev/66a77b9bfe5dcacd50eccf85de7c0e7e15ce0ffd/layout/generic/nsFlexContainerFrame.cpp#1720 Also, I'm tagging comment 30 as "obsolete", since I think mats mistook the nature of the table-wrapper problem. The bug he spun off does cover some real issues, but they're orthogonal to the align-self/justify-self changes here & not related to the reftest failure.

This attempts to update layout/style/test/test_ali_justify_computed_values.html to match the new spec. Mostly it turns a lot of tests into "assert that align-self:auto and justify-self:auto maintains those computed values under all conditions", which are now likely redundant. So two questions for reviewers: 1) How aggressive do we want to be in eliminating items in this test? Is it useful to continue testing all conditions that align-self:auto children don't inherit their parents' values when those parents' values are left/right/end? 2) Lines 400 and 401 test the behavior of justify-self:inherit, and are now marked as todo_is. Would you please clarify whether returning 'auto' or 'center' is correct, per spec?

Updated when I figured out the intent of the tests on line 400 and 401, which was one of the issues in comment 33. Now all that remains is the high degree of redundancy in the tests, and I'd appreciate direction on how much to pare it down.

Comment on attachment 8795935 [details] [diff] [review] Part 3 (draft): mochitest updated to match new spec behavior.usedStyle3.patch Review of attachment 8795935 [details] [diff] [review]: ----------------------------------------------------------------- Commit message nit: right now, the commit message has some "*** cleanup test" trailing lines (from combining two commits, via qfold or histedit) ::: layout/style/test/test_align_justify_computed_values.html @@ +83,1 @@ > * This mochitest tests that situation and a few other similar tricky situations. (The text above this comment probably wants to be edited down quite a bit. It's describing the old behavior & our tricks for implementing it [and those tricks are being removed in this bug]. It should change to say something like: "In a previous revision of the CSS Alignment spec, align-self:auto was required to actually *compute* to the parent's align-items value -- but now, the spec says it simply computes to itself, and it should only get converted into the parent's align-items value when it's used in layout. This test verifies that we do indeed have it compute to itself, regardless of the parent's align-items value." @@ +99,5 @@ > elem.style.alignSelf = ""; // clean up > > // Test value after setting align-self explicitly to "inherit" > elem.style.alignSelf = "inherit"; > + if(elem.parentNode && elem.parentNode.style) { * Add a space between "if" and the open-paren, please. @@ +121,5 @@ > "bug in test -- expecting caller to pass us a node with a parent"); > > // Test initial computed style when "align-items" has been set on our parent. > // (elem's initial "align-self" value should be "auto", which should compute > // to its parent's "align-items" value, which in this case is "center".) The parenthetical here should be removed. @@ +139,5 @@ > elem.parentNode.style.alignItems = ""; // clean up > > // Finally: test computed style after setting "align-self" to "inherit" > // and leaving parent at its initial value (which should be "auto", which > // should compute to "normal") Drop the text |which should compute to "normal"| from the end of this comment. @@ +151,5 @@ > > /* > * Tests that depend on us having a grandparent node: > */ > function testNodeThatHasGrandparent(elem) { I think we should probably just get rid of this whole function. It was useful to test our old behavior, but it doesn't really serve a purpose now, since now there's no way to indirectly inherit values from the grandparent (as there was previously, via align-items->align-self->inherited-align-self). We also should probably get rid of the whole "Test the <div id="display"> node" section, too (i.e. the chunk of main() where we call into this function). It doesn't add much value anymore either, now that grandparent styles don't have as much potential for subtle impacts with these properties. @@ +247,3 @@ > elem.style.alignItems = "end"; > + is(getComputedAlignSelf(child), 'auto', "align-self:auto value persists for block child"); > + is(getComputedAlignSelf(abs), 'auto', "align-self:auto value persists for block container abs.pos. child"); I suspect this where you were asking about how to handle the redundancy. (Here and below this.) I'll defer to mats on that, since he added this chunk. My initial thought: I don't think we benefit much from testing so many align-items values here (and verifying that none of them perturb our computed "align-self:auto" value). I think we could definitely get rid of the other align-items values after "end" here. As for the other checks beyond that, I'll defer to mats I think.

Sorry, I just pushed a patch via mozreview to address comment 31, and I intended to post it on a helper bug, but I used the wrong bug number so it ended up here. Disregard it; I'll repost over on bug 1306213.

Comment on attachment 8795935 [details] [diff] [review] Part 3 (draft): mochitest updated to match new spec behavior.usedStyle3.patch Meh, as long as the test passes I don't mind a little redundancy. Our time is better spent on something that actually helps our users than polishing tests. Just my 2 cents.

Updated version of mochitest that addresses issues in comment 35 and comment 38.

Comment on attachment 8796241 [details] [diff] [review] Bug 1304012 -- Part 3: mochitest updated to match new spec behavior. Review of attachment 8796241 [details] [diff] [review]: ----------------------------------------------------------------- r=me, thanks!

For the two failing reftests, I manually applied the fix for bug 1306213 and that resolved them both. When the patch for that bug lands, I'll post a try build for this and it should be ready for checkin.

Comment on attachment 8795786 [details] [diff] [review] Bug 1304012 -- Part 2: Rename nsStyleStruct Computed**Self functions to Used**Self. r=dholbert ># HG changeset patch ># User Brad Werth <bwerth@mozilla.com> ># Date 1475083928 25200 ># Wed Sep 28 10:32:08 2016 -0700 ># Node ID e9dd5491b6aa533b628a36826b6786d2d4d7bd76 ># Parent 127184a265a8da079757efe6525cbb32477028ad >Bug 1304012 -- Part 2: Rename nsStyleStruct Computed**Self functions to Used**Self. >*** >Updated comment. ^^^^^^^^^^^^^^^ I just noticed this patch-folding cruft in the Try push -- please take that out before requesting checkin. Thanks!

Fixes the comment cruft noted in comment 43.

Sorry, the try in commment 44 was missing the fix from bug 1306213. This one includes it. https://treeherder.mozilla.org/#/jobs?repo=try&revision=3afa5769486d

That try run's M-5 opt / M-9 debug test failures indicate that this file needs an update: https://dxr.mozilla.org/mozilla-central/source/layout/style/test/property_database.js#4402-4407 As part of Part 3, please: - move "normal" from the initial_values list to the beginning of the other_values list - Delete the comment " // (Assuming defaults on the parent, 'auto' will compute to 'normal'.)"

(no need to re-request review when fixing that; r=me still stands)

dholbert, thank you for the insights in comment 46. I was having difficulty figuring out the proper fix to the test. This rolls in changes to the property_database.js file so that the layout/style/test/test_value_computation.html mochitest will pass.

Updated Part 2 to apply cleanly now that bug 1306213 is resolved.

This combines patches 1 through 3 in a form that's hopefully easier for checkin.

Please don't do patch roll-ups for checkin, except when it's needed e.g. to compile correctly & there's no way around it. It's valuable to have smaller atomic changesets -- not only for review, but also for looking at change history. (In particular, the separation between non-functional rename-style changes & functional changes.)

Pushed by dholbert@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ed543800e808 Part 1: Don't bother resolving align-self/justify-self 'auto' value for getComputedStyle & inheritance. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/a0ea382c12ca Part 2: Rename nsStyleStruct Computed**Self functions to Used**Self. r=dholbert https://hg.mozilla.org/integration/mozilla-inbound/rev/1c8ed959072a Part 3: mochitest updated to match new spec behavior, and updated property database. r=dholbert

https://hg.mozilla.org/mozilla-central/rev/ed543800e808 https://hg.mozilla.org/mozilla-central/rev/a0ea382c12ca https://hg.mozilla.org/mozilla-central/rev/1c8ed959072a