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

Description Joseph Pecoraro 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.
Comment 1 Joseph Pecoraro 2016-10-17 16:42:24 PDT
Created attachment 291896 [details]
[IMAGE] Issue
Comment 2 Joseph Pecoraro 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.
Comment 3 Joseph Pecoraro 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?
Comment 4 Joseph Pecoraro 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.
Comment 5 Javier Fernandez 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.
Comment 6 Javier Fernandez 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.
Comment 7 Javier Fernandez 2016-10-18 03:15:24 PDT
Created attachment 291934 [details]
Patch
Comment 8 Sergio Villar Senin 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
Comment 9 Javier Fernandez 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.
Comment 10 Javier Fernandez 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.
Comment 11 Javier Fernandez 2016-10-18 10:24:13 PDT
Created attachment 291955 [details]
Patch

Patch for landing.
Comment 12 Javier Fernandez 2016-10-19 03:20:08 PDT
Created attachment 292059 [details]
Patch

Patch for landing.
Comment 13 WebKit Commit Bot 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>
Comment 14 WebKit Commit Bot 2016-10-19 06:45:31 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Joseph Pecoraro 2016-10-19 10:53:43 PDT
Things look much better now! Thanks for the quick fix!
Comment 16 Joseph Pecoraro 2016-10-19 16:17:43 PDT
*** Bug 163700 has been marked as a duplicate of this bug. ***
Comment 17 Ryosuke Niwa 2017-01-26 22:26:01 PST
*** Bug 163382 has been marked as a duplicate of this bug. ***