-
Notifications
You must be signed in to change notification settings - Fork 699
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[css-flexbox][css-align][css-grid] abspos flex/grid item "align-self: auto behaves as start" spec-text is too vague #440
Comments
On 09/03/2016 01:49 PM, Daniel Holbert wrote:
IIRC we had backported this behavior in Flex and Grid from Align, The intention is that absolutely-positioned items never take their There's two ways to deal with this: I don't think we have a strong opinion on this matter. Computing ~fantasai |
Regarding fantasai's suggested solutions. I think 1) has the same performance problems as we reported here:
Currently, this is equivalent to:
With 2) we would instead get the computed values:
B would have the used value 'left'. It seems wrong that B and C ends up having different used values I think this solution might work better: Grid/Flexbox can then use this flag to indicate 'start' for the static I think this would be easy to implement in Gecko (it's similar to [1] |
I have no opinion on this issue... up to the council of implementers and @dbaron . :) |
Looks like this was discussed at https://lists.w3.org/Archives/Public/www-style/2016Sep/0041.html It was resolved as "RESOLVED: Don't have the computed value dependency", pending any objections. I think I'm on-board with that, but @fantasai, could you clarify what that means? Is the plan for |
Yes, I believe that is the intent. :) |
OK -- that sounds good to me, I think. Thanks! |
Was there a pressing need to change this in a way that's incompatible with all shipping implementations (at the time)? |
@cbiesinger Yes. When we only had Flexbox, it didn't matter what it computed to on abspos elements, unless they also happened to be children of a flex container. But when we extended Lemme know if that makes sense. @yisibl I am not sure what's going on, given you didn't post any source code, but I'm pretty sure it's unrelated to this bug report... |
@fantasai Demo: http://jsbin.com/cenanahovu/edit?html I figured it out, Firefox is right. |
@fantasai -- why is it an action at a distance? the static position is always influenced by the parent. But more importantly, this may break existing content -- I am hesitant to change the abspos behavior again. |
@cbiesinger This isn't about just static position, tho - align/justify-* affect the abspos even when t/r/b/l are all specified. (The old "t+b means you're stretched" behavior is now explicitly keyed to "justify-self: stretch"; otherwise you just align within the box defined by t+b; same for r+l and align-self.) It is quite weird if an abspos isn't relying on static position at all, and is attached to a containing block somewhere high in the tree, but it still gets aligned by its parent element using "*-items". This is the action-at-a-distance @fantasai is talking about. However! The behavior change for static position is, I think, unintentional. We can deploy a more targeted fix to preserve that, so that "auto" continues to look at the parent's *-items value. Would that work for you? |
Yes, that sounds reasonable. |
The flex spec was updated a while ago to use the following language for align-items: > align-items sets the default alignment for all of the flex container’s items, > including anonymous flex items. align-self allows this default alignment to > be overridden for individual flex items. After that, the CSSWG resolved that align-self: auto computes to itself[1][2]. Thus, this test is invalid. [1]: w3c/csswg-drafts#440 (comment) [2]: https://lists.w3.org/Archives/Public/www-style/2016Sep/0041.html
The language introduced in f5d5826 seems problematic to me:
What is the parent? The DOM parent? Or the parent box? How should the <style>
#t4 { display: flex; align-items: center }
#t4 .contents { display: contents; align-items: baseline }
#t4 span { align-self: auto }
</style>
<div id="t4"><div class="contents"><span></span></div></div> It seems to me that the intention is just to use the parent box
|
Fixed @emilio‘s issue. Agenda+ to discuss #440 (comment) |
The flex spec was updated a while ago to use the following language for align-items: > align-items sets the default alignment for all of the flex container’s items, > including anonymous flex items. align-self allows this default alignment to > be overridden for individual flex items. After that, the CSSWG resolved that align-self: auto computes to itself[1][2]. Thus, this test is invalid. [1]: w3c/csswg-drafts#440 (comment) [2]: https://lists.w3.org/Archives/Public/www-style/2016Sep/0041.html Build from revision af7d6f4901eab45cdeaee531daac8c4d360d4a5d
…to and abspos static position; Align now defines this correctly. #440.
@cbiesinger @dholbert Does the updated text look OK to you? 9d16e5f |
Looks good to me, thanks! |
The CSS Box Alignment specification has been changed recently so that now all the propeties have the specificed value as computed value. The rationale of this change are at the associated W3C github issue [1]. This change implies that we don't need to execute the StyleAdjuter logic we implemented specifically for supporting 'auto' values resolution for computed style. We can live now with resolution at layout time only. [1] w3c/csswg-drafts#440 BUG=725489 Review-Url: https://codereview.chromium.org/2455093002 Cr-Commit-Position: refs/heads/master@{#475400}
The CSS Box Alignment specification has been changed recently so that now all the propeties have the specificed value as computed value. The rationale of this change are at the associated W3C github issue [1]. This change implies that we don't need to execute the StyleAdjuter logic we implemented specifically for supporting 'auto' values resolution for computed style. We can live now with resolution at layout time only. [1] w3c/csswg-drafts#440 BUG=725489 Review-Url: https://codereview.chromium.org/2455093002 Cr-Commit-Position: refs/heads/master@{#475400}
The CSS Box Alignment specification has been changed recently so that now all the propeties have the specificed value as computed value. The rationale of this change are at the associated W3C github issue [1]. This change implies that we don't need to execute the StyleAdjuter logic we implemented specifically for supporting 'auto' values resolution for computed style. We can live now with resolution at layout time only. [1] w3c/csswg-drafts#440 BUG=725489 Review-Url: https://codereview.chromium.org/2455093002 Cr-Commit-Position: refs/heads/master@{#475400}
… (patchset #12 id:210001 of https://codereview.chromium.org/2455093002/ ) Reason for revert: This patch breaks DevTools toolbars (Console gear icon, checkbox labels are centered), it also seems to break the Welcome to Chrome page layout. Original issue's description: > [css-align] Don't resolve 'auto' values for computed style. > > The CSS Box Alignment specification has been changed recently so that > now all the propeties have the specificed value as computed value. The > rationale of this change are at the associated W3C github issue [1]. > > This change implies that we don't need to execute the StyleAdjuter > logic we implemented specifically for supporting 'auto' values > resolution for computed style. We can live now with resolution at > layout time only. > > [1] w3c/csswg-drafts#440 > > BUG=725489 > > Review-Url: https://codereview.chromium.org/2455093002 > Cr-Commit-Position: refs/heads/master@{#475400} > Committed: https://chromium.googlesource.com/chromium/src/+/5389373c6dec30d783eb46b4c8190720f411a8a7 TBR=cbiesinger@chromium.org,cbiesinger@google.com,meade@chromium.org,rego@igalia.com,svillar@igalia.com,jfernandez@igalia.com # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=725489 Review-Url: https://codereview.chromium.org/2913093002 Cr-Commit-Position: refs/heads/master@{#475689}
… (patchset #12 id:210001 of https://codereview.chromium.org/2455093002/ ) Reason for revert: This patch breaks DevTools toolbars (Console gear icon, checkbox labels are centered), it also seems to break the Welcome to Chrome page layout. Original issue's description: > [css-align] Don't resolve 'auto' values for computed style. > > The CSS Box Alignment specification has been changed recently so that > now all the propeties have the specificed value as computed value. The > rationale of this change are at the associated W3C github issue [1]. > > This change implies that we don't need to execute the StyleAdjuter > logic we implemented specifically for supporting 'auto' values > resolution for computed style. We can live now with resolution at > layout time only. > > [1] w3c/csswg-drafts#440 > > BUG=725489 > > Review-Url: https://codereview.chromium.org/2455093002 > Cr-Commit-Position: refs/heads/master@{#475400} > Committed: https://chromium.googlesource.com/chromium/src/+/5389373c6dec30d783eb46b4c8190720f411a8a7 TBR=cbiesinger@chromium.org,cbiesinger@google.com,meade@chromium.org,rego@igalia.com,svillar@igalia.com,jfernandez@igalia.com # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=725489 Review-Url: https://codereview.chromium.org/2913093002 Cr-Commit-Position: refs/heads/master@{#475689}
… (patchset #12 id:210001 of https://codereview.chromium.org/2455093002/ ) Reason for revert: This patch breaks DevTools toolbars (Console gear icon, checkbox labels are centered), it also seems to break the Welcome to Chrome page layout. Original issue's description: > [css-align] Don't resolve 'auto' values for computed style. > > The CSS Box Alignment specification has been changed recently so that > now all the propeties have the specificed value as computed value. The > rationale of this change are at the associated W3C github issue [1]. > > This change implies that we don't need to execute the StyleAdjuter > logic we implemented specifically for supporting 'auto' values > resolution for computed style. We can live now with resolution at > layout time only. > > [1] w3c/csswg-drafts#440 > > BUG=725489 > > Review-Url: https://codereview.chromium.org/2455093002 > Cr-Commit-Position: refs/heads/master@{#475400} > Committed: https://chromium.googlesource.com/chromium/src/+/5389373c6dec30d783eb46b4c8190720f411a8a7 TBR=cbiesinger@chromium.org,cbiesinger@google.com,meade@chromium.org,rego@igalia.com,svillar@igalia.com,jfernandez@igalia.com # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=725489 Review-Url: https://codereview.chromium.org/2913093002 Cr-Commit-Position: refs/heads/master@{#475689}
Fixed the regression caused by the previous patch and provided a proper regression test. Reason for revert: This patch breaks DevTools toolbars (Console gear icon, checkbox labels are centered), it also seems to break the Welcome to Chrome page layout. Original issue's description: > [css-align] Don't resolve 'auto' values for computed style. > > The CSS Box Alignment specification has been changed recently so that > now all the propeties have the specificed value as computed value. The > rationale of this change are at the associated W3C github issue [1]. > > This change implies that we don't need to execute the StyleAdjuter > logic we implemented specifically for supporting 'auto' values > resolution for computed style. We can live now with resolution at > layout time only. > > [1] w3c/csswg-drafts#440 > > BUG=725489 > > Review-Url: https://codereview.chromium.org/2455093002 > Cr-Commit-Position: refs/heads/master@{#475400} > Committed: https://chromium.googlesource.com/chromium/src/+/5389373c6dec30d783eb46b4c8190720f411a8a7 TBR=cbiesinger@chromium.org,cbiesinger@google.com,meade@chromium.org,rego@igalia.com,svillar@igalia.com,jfernandez@igalia.com NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=725489 Review-Url: https://codereview.chromium.org/2913093002 Cr-Commit-Position: refs/heads/master@{#475689} Committed: https://chromium.googlesource.com/chromium/src/+/8e4d09ab6b864897d8399ccda555a61b030ceb84 BUG=725489 Review-Url: https://codereview.chromium.org/2915773002 Cr-Commit-Position: refs/heads/master@{#476024}
Fixed the regression caused by the previous patch and provided a proper regression test. Reason for revert: This patch breaks DevTools toolbars (Console gear icon, checkbox labels are centered), it also seems to break the Welcome to Chrome page layout. Original issue's description: > [css-align] Don't resolve 'auto' values for computed style. > > The CSS Box Alignment specification has been changed recently so that > now all the propeties have the specificed value as computed value. The > rationale of this change are at the associated W3C github issue [1]. > > This change implies that we don't need to execute the StyleAdjuter > logic we implemented specifically for supporting 'auto' values > resolution for computed style. We can live now with resolution at > layout time only. > > [1] w3c/csswg-drafts#440 > > BUG=725489 > > Review-Url: https://codereview.chromium.org/2455093002 > Cr-Commit-Position: refs/heads/master@{#475400} > Committed: https://chromium.googlesource.com/chromium/src/+/5389373c6dec30d783eb46b4c8190720f411a8a7 TBR=cbiesinger@chromium.org,cbiesinger@google.com,meade@chromium.org,rego@igalia.com,svillar@igalia.com,jfernandez@igalia.com NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=725489 Review-Url: https://codereview.chromium.org/2913093002 Cr-Commit-Position: refs/heads/master@{#475689} Committed: https://chromium.googlesource.com/chromium/src/+/8e4d09ab6b864897d8399ccda555a61b030ceb84 BUG=725489 Review-Url: https://codereview.chromium.org/2915773002 Cr-Commit-Position: refs/heads/master@{#476024}
Fixed the regression caused by the previous patch and provided a proper regression test. Reason for revert: This patch breaks DevTools toolbars (Console gear icon, checkbox labels are centered), it also seems to break the Welcome to Chrome page layout. Original issue's description: > [css-align] Don't resolve 'auto' values for computed style. > > The CSS Box Alignment specification has been changed recently so that > now all the propeties have the specificed value as computed value. The > rationale of this change are at the associated W3C github issue [1]. > > This change implies that we don't need to execute the StyleAdjuter > logic we implemented specifically for supporting 'auto' values > resolution for computed style. We can live now with resolution at > layout time only. > > [1] w3c/csswg-drafts#440 > > BUG=725489 > > Review-Url: https://codereview.chromium.org/2455093002 > Cr-Commit-Position: refs/heads/master@{#475400} > Committed: https://chromium.googlesource.com/chromium/src/+/5389373c6dec30d783eb46b4c8190720f411a8a7 TBR=cbiesinger@chromium.org,cbiesinger@google.com,meade@chromium.org,rego@igalia.com,svillar@igalia.com,jfernandez@igalia.com NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=725489 Review-Url: https://codereview.chromium.org/2913093002 Cr-Commit-Position: refs/heads/master@{#475689} Committed: https://chromium.googlesource.com/chromium/src/+/8e4d09ab6b864897d8399ccda555a61b030ceb84 BUG=725489 Review-Url: https://codereview.chromium.org/2915773002 Cr-Commit-Position: refs/heads/master@{#476024}
Looks good to me too - thanks! |
…fy-self must not be resolved https://bugs.webkit.org/show_bug.cgi?id=172707 Reviewed by Antti Koivisto. LayoutTests/imported/w3c: This change makes all the cases of the test below to pass now, hence updated expectations accordingly. * web-platform-tests/css/css-align-3/self-alignment/place-self-shorthand-006-expected.txt: Source/WebCore: The CSS Box Alignment specification has been changed recently so that now all the propeties have the specificed value as computed value. The rationale of this change are at the associated W3C github issue [1]. This change implies that we don't need to execute the StyleAdjuter logic we implemented specifically for supporting 'auto' values resolution for computed style. We can live now with resolution at layout time only. [1] w3c/csswg-drafts#440 No new tests, just updating the already defined tests. * css/CSSComputedStyleDeclaration.cpp: (WebCore::ComputedStyleExtractor::propertyValue): * css/StyleResolver.cpp: (WebCore::StyleResolver::adjustRenderStyle): Removed * css/StyleResolver.h: * html/shadow/TextControlInnerElements.cpp: (WebCore::TextControlInnerElement::resolveCustomStyle): * rendering/RenderBox.cpp: (WebCore::RenderBox::columnFlexItemHasStretchAlignment): (WebCore::RenderBox::hasStretchedLogicalWidth): * rendering/RenderFlexibleBox.cpp: (WebCore::RenderFlexibleBox::styleDidChange): Added (WebCore::RenderFlexibleBox::alignmentForChild): * rendering/RenderFlexibleBox.h: LayoutTests: Updated layout tests so that resolved value is as specified, even for 'auto' values. * TestExpectations: * css3/flexbox/css-properties-expected.txt: * css3/flexbox/css-properties.html: * css3/flexbox/new-alignment-values-invalid-if-grid-not-enabled-expected.txt: * css3/flexbox/new-alignment-values-invalid-if-grid-not-enabled.html: * css3/parse-align-self.html: * css3/parse-alignment-of-root-elements-expected.txt: * css3/parse-alignment-of-root-elements.html: * css3/parse-place-items.html: * css3/parse-place-self.html: * fast/css/ensure-flexbox-compatibility-with-initial-values-expected.txt: * fast/css/parse-justify-self.html: git-svn-id: http://svn.webkit.org/repository/webkit/trunk@219315 268f45cc-cd09-0410-ab3c-d52691b4dbfc
…fy-self must not be resolved https://bugs.webkit.org/show_bug.cgi?id=172707 Reviewed by Antti Koivisto. LayoutTests/imported/w3c: This change makes all the cases of the test below to pass now, hence updated expectations accordingly. * web-platform-tests/css/css-align-3/self-alignment/place-self-shorthand-006-expected.txt: Source/WebCore: The CSS Box Alignment specification has been changed recently so that now all the propeties have the specificed value as computed value. The rationale of this change are at the associated W3C github issue [1]. This change implies that we don't need to execute the StyleAdjuter logic we implemented specifically for supporting 'auto' values resolution for computed style. We can live now with resolution at layout time only. [1] w3c/csswg-drafts#440 No new tests, just updating the already defined tests. * css/CSSComputedStyleDeclaration.cpp: (WebCore::ComputedStyleExtractor::propertyValue): * css/StyleResolver.cpp: (WebCore::StyleResolver::adjustRenderStyle): Removed * css/StyleResolver.h: * html/shadow/TextControlInnerElements.cpp: (WebCore::TextControlInnerElement::resolveCustomStyle): * rendering/RenderBox.cpp: (WebCore::RenderBox::columnFlexItemHasStretchAlignment): (WebCore::RenderBox::hasStretchedLogicalWidth): * rendering/RenderFlexibleBox.cpp: (WebCore::RenderFlexibleBox::styleDidChange): Added (WebCore::RenderFlexibleBox::alignmentForChild): * rendering/RenderFlexibleBox.h: LayoutTests: Updated layout tests so that resolved value is as specified, even for 'auto' values. * TestExpectations: * css3/flexbox/css-properties-expected.txt: * css3/flexbox/css-properties.html: * css3/flexbox/new-alignment-values-invalid-if-grid-not-enabled-expected.txt: * css3/flexbox/new-alignment-values-invalid-if-grid-not-enabled.html: * css3/parse-align-self.html: * css3/parse-alignment-of-root-elements-expected.txt: * css3/parse-alignment-of-root-elements.html: * css3/parse-place-items.html: * css3/parse-place-self.html: * fast/css/ensure-flexbox-compatibility-with-initial-values-expected.txt: * fast/css/parse-justify-self.html: Canonical link: https://commits.webkit.org/191158@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@219315 268f45cc-cd09-0410-ab3c-d52691b4dbfc
The css-align spec (the canonical definition for
align-self
) is pretty unequivocal about how to handlealign-self:auto
. Quoting:But, the flexbox & grid specs attempt to define some special-case behavior for this
auto
value on absolutely-positioned elements -- and IMO they're too vague about how/where that special case behavior is supposed to be triggered, and how broadly its effects spread.The language I'm concerned about is here in the flexbox spec:
...and here in the grid spec:
Superficially, the phrase "a value of" is too vague. (Is that "a specified value"? "a computed value"? something in between?) And of course, "behaves as" is vague too.
I have these specific questions/concerns:
Are the flex/grid specs are trying to redefine the computed value of
align-self:auto
for these abspos elements? Or isalign-self:auto
still intended to compute the way that css-align defines it (to the parent's computedalign-items
) and then its computed value (e.g.center
if the parent hasalign-items:center
) should somehow produce different layout behavior than it normally would?If the intent for (1) is the latter (i.e. if
auto
computes away but then ends up somehow producing different results in layout), how is the different behavior intended to be triggered? Layout uses computed/used values of css properties -- so ifauto
gets computed away, then I don't see how layout would be able to realize that it needs to apply this special-case behavior. Maybe e.g. whenauto
computes tocenter
, it's expected to really compute to a specialcenter-which-really-came-from-auto
value, which is expected to behave just likecenter
except in this special abspos case?It's not clear to me what should happen here when an
align-self:auto
value is inherited to an abspos element. For example: suppose an abspos element inside of a flex container hasalign-self:inherit
, and its parent hasalign-self:auto
, and its grandparent hasalign-items:center
. I'm pretty sure this abspos element should end up with a computedalign-self
ofcenter
, BUT: is that reallycenter
, or is it thecenter-which-came-from-auto
scenario discussed above in (2)? (This matters for layout -- if we inherit the purecenter
value, the static position would be center-aligned. But if we inherit some magically taintedcenter-which-came-from-auto
value, then the static position would be start-aligned.)The text was updated successfully, but these errors were encountered: