Summary: | Web Inspector: Debugger buttons positioned incorrectly, align-content default value is unexpected | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Joseph Pecoraro <joepeck> | ||||||||||
Component: | CSS | Assignee: | Javier Fernandez <jfernandez> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | commit-queue, darin, hyatt, jfernandez, joepeck, lewis, nvasilyev, rego, simon.fraser, svillar | ||||||||||
Priority: | P2 | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
See Also: | https://bugs.chromium.org/p/chromium/issues/detail?id=647694 | ||||||||||||
Attachments: |
|
Description
Joseph Pecoraro
2016-10-17 16:41:38 PDT
Created attachment 291896 [details]
[IMAGE] Issue
I feel like this may have been caused by: <https://webkit.org/b/161303> [css-align] Initial values are parsed as invalid for some Alignment properties <https://trac.webkit.org/r205807> It looks like before then there were some valid properties outside of grid for flex box, that might not work unless CSSGridLayout is enabled. From that change:
> The CSS Box Alignment specification defines a new syntax for
> align-self/align-items, justify-self/justify-items and
> align-content/justify-content CSS properties. This new syntax include new
> values, some of them not valid following the old syntax defined in the CSS
> Flexible box specification for some of these properties.
>
> Due to the patch landed for fixing bug #151591 we are now sure that the
> parsing logic for the new syntax is only used when the CSS Grid Layout is
> enabled, since this new layout model defines its alignment procedures based
> on the mentioned CSS properties.
However, in testing, it seems I now have to provide:
align-content: center;
To override the default value of:
align-content: flex-start;
when CSSGridLayout is disabled.
That sounds like a backwards compatibility issue. Is that intentional?
Note that previously the default value of `align-content` was `normal`. That now doesn't appear to be a valid option unless CSSGridLayout is enabled. That at least does appear to be a backwards incompatible change. I think I know what's the problem, but I'd need to know exactly the expected behavior. This bug has been already fixed in Blink (see crbug.com/647694) so it's just a matter of porting my patch to WebKit. Created attachment 291934 [details]
Patch
Comment on attachment 291934 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=291934&action=review > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2441 > +#endif Wondering if we could simplify this a bit. What about: CSSValueID valueID = data.position() == ContentPositionNormal ? normalBehaviorValueID : data.position(); #if ENABLE(CSS_GRID_LAYOUT) // Perhaps some comment here explaining that grid layout requires the new alignment behavior if (RuntimeEnabledFeatures::sharedFeatures().isCSSGridLayoutEnabled()) valueID = data.position(); #endif result.get().append(cssValuePool.createValue(valueID); > LayoutTests/css3/flexbox/flexbox-lines-must-be-stretched-by-default.html:1 > +<!doctype html> DOCTYPE in capital Comment on attachment 291934 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=291934&action=review >> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2441 >> +#endif > > Wondering if we could simplify this a bit. What about: > > CSSValueID valueID = data.position() == ContentPositionNormal ? normalBehaviorValueID : data.position(); > #if ENABLE(CSS_GRID_LAYOUT) > // Perhaps some comment here explaining that grid layout requires the new alignment behavior > if (RuntimeEnabledFeatures::sharedFeatures().isCSSGridLayoutEnabled()) > valueID = data.position(); > #endif > result.get().append(cssValuePool.createValue(valueID); It looks much better, indeed. >> LayoutTests/css3/flexbox/flexbox-lines-must-be-stretched-by-default.html:1 >> +<!doctype html> > > DOCTYPE in capital Done. Comment on attachment 291934 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=291934&action=review >>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2441 >>> +#endif >> >> Wondering if we could simplify this a bit. What about: >> >> CSSValueID valueID = data.position() == ContentPositionNormal ? normalBehaviorValueID : data.position(); >> #if ENABLE(CSS_GRID_LAYOUT) >> // Perhaps some comment here explaining that grid layout requires the new alignment behavior >> if (RuntimeEnabledFeatures::sharedFeatures().isCSSGridLayoutEnabled()) >> valueID = data.position(); >> #endif >> result.get().append(cssValuePool.createValue(valueID); > > It looks much better, indeed. Umm, not that easy because ContentPositionNormal is just an enum while normalBehaviorValueID is a CSSValueID. However, there should be a way to perform the simplification you suggests, so I'll do it in the patch for landing. Created attachment 291955 [details]
Patch
Patch for landing.
Created attachment 292059 [details]
Patch
Patch for landing.
Comment on attachment 292059 [details] Patch Clearing flags on attachment: 292059 Committed r207535: <http://trac.webkit.org/changeset/207535> All reviewed patches have been landed. Closing bug. Things look much better now! Thanks for the quick fix! *** Bug 163700 has been marked as a duplicate of this bug. *** *** Bug 163382 has been marked as a duplicate of this bug. *** |