Bug 163572

Summary: Web Inspector: Debugger buttons positioned incorrectly, align-content default value is unexpected
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: CSSAssignee: 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 Flags
[IMAGE] Issue
none
Patch
none
Patch
none
Patch none

Joseph Pecoraro
Reported 2016-10-17 16:41:38 PDT
Summary: Web Inspector's Debugger buttons positioned incorrectly, align-items does not appear to be taking affect. This appears to be an issue with the Runtime setting exposed via bug 163432 <http://trac.webkit.org/changeset/207402> which disabled the runtime feature.
Attachments
[IMAGE] Issue (118.16 KB, image/png)
2016-10-17 16:42 PDT, Joseph Pecoraro 		no flags
Details
Patch (11.92 KB, patch)
2016-10-18 03:15 PDT, Javier Fernandez 		no flags
DetailsFormatted DiffDiff
Patch (11.78 KB, patch)
2016-10-18 10:24 PDT, Javier Fernandez 		no flags
DetailsFormatted DiffDiff
Patch (11.78 KB, patch)
2016-10-19 03:20 PDT, Javier Fernandez 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Joseph Pecoraro
Comment 1 2016-10-17 16:42:24 PDT
Created attachment 291896 [details] [IMAGE] Issue
Joseph Pecoraro
Comment 2 2016-10-17 17:14:30 PDT
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.
Joseph Pecoraro
Comment 3 2016-10-17 17:23:35 PDT
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?
Joseph Pecoraro
Comment 4 2016-10-17 17:34:06 PDT
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.
Javier Fernandez
Comment 5 2016-10-18 02:49:29 PDT
I think I know what's the problem, but I'd need to know exactly the expected behavior.
Javier Fernandez
Comment 6 2016-10-18 03:11:59 PDT
This bug has been already fixed in Blink (see crbug.com/647694) so it's just a matter of porting my patch to WebKit.
Javier Fernandez
Comment 7 2016-10-18 03:15:24 PDT
Created attachment 291934 [details] Patch
Sergio Villar Senin
Comment 8 2016-10-18 08:12:44 PDT
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
Javier Fernandez
Comment 9 2016-10-18 08:30:51 PDT
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.
Javier Fernandez
Comment 10 2016-10-18 08:45:47 PDT
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.
Javier Fernandez
Comment 11 2016-10-18 10:24:13 PDT
Created attachment 291955 [details] Patch Patch for landing.
Javier Fernandez
Comment 12 2016-10-19 03:20:08 PDT
Created attachment 292059 [details] Patch Patch for landing.
WebKit Commit Bot
Comment 13 2016-10-19 06:45:26 PDT
Comment on attachment 292059 [details] Patch Clearing flags on attachment: 292059 Committed r207535: <http://trac.webkit.org/changeset/207535>
WebKit Commit Bot
Comment 14 2016-10-19 06:45:31 PDT
All reviewed patches have been landed. Closing bug.
Joseph Pecoraro
Comment 15 2016-10-19 10:53:43 PDT
Things look much better now! Thanks for the quick fix!
Joseph Pecoraro
Comment 16 2016-10-19 16:17:43 PDT
*** Bug 163700 has been marked as a duplicate of this bug. ***
Ryosuke Niwa
Comment 17 2017-01-26 22:26:01 PST
*** Bug 163382 has been marked as a duplicate of this bug. ***
Note You need to log in before you can comment on or make changes to this bug.