Bug 156254 – [CSS Box Alignment] New CSS Value 'normal' for Self Alignment

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Javier Fernandez]

The Box Alignment specification defines a new value 'normal' to be used as default for the different layout models, which will define the specific behavior for each case. The Self-Alignment properties (align-self, justify-self) must allow this value and the different layout models should implement it accordingly.

[Javier Fernandez]

Created attachment 275990 [details]
Patch

[Build Bot]

Comment on attachment 275990 [details]
Patch

Attachment 275990 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1120090

New failing tests:
fast/events/right-click-focus.html
svg/css/getComputedStyle-basic.xhtml
editing/spelling/spellcheck-input-search-crash.html

[Build Bot]

Created attachment 275994 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 275990 [details]
Patch

Attachment 275990 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1120092

New failing tests:
svg/css/getComputedStyle-basic.xhtml

[Build Bot]

Created attachment 275995 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Javier Fernandez]

Created attachment 275996 [details]
Fixed layout tests failures.

[Build Bot]

Comment on attachment 275996 [details]
Fixed layout tests failures.

Attachment 275996 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1120396

New failing tests:
fast/events/right-click-focus.html
svg/css/getComputedStyle-basic.xhtml
editing/spelling/spellcheck-input-search-crash.html

[Build Bot]

Created attachment 276001 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 275996 [details]
Fixed layout tests failures.

Attachment 275996 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1120403

New failing tests:
svg/css/getComputedStyle-basic.xhtml

[Build Bot]

Created attachment 276002 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Javier Fernandez]

Created attachment 276003 [details]
Fixed layout tests failures.

[Darin Adler]

Comment on attachment 276003 [details]
Fixed layout tests failures.

Can someone help us figure out why the build is failing on Windows?

[Javier Fernandez]

(In reply to comment #12)
> Comment on attachment 276003 [details]
> Fixed layout tests failures.
> 
> Can someone help us figure out why the build is failing on Windows?

The log doesn't provide any useful info to me. I had problems in a similar patch because of the all-in-one compiling (see comment 14 in bug #154282). Perhaps the same problem with this change ?

diff --git a/Source/WebCore/rendering/RenderGrid.cpp b/Source/WebCore/rendering/RenderGrid.cpp
index 66de23022f1dc569cf32b3170fd05068256ef6c2..b6faa65d8a992370edaa3b93f88a171317bd1dbd 100644
--- a/Source/WebCore/rendering/RenderGrid.cpp
+++ b/Source/WebCore/rendering/RenderGrid.cpp
@@ -38,6 +38,7 @@
 namespace WebCore {
 
 static const int infinity = -1;
+static const ItemPosition selfAlignmentNormalBehavior = ItemPositionStretch;


diff --git a/Source/WebCore/rendering/RenderFlexibleBox.cpp b/Source/WebCore/rendering/RenderFlexibleBox.cpp
index 69f3781a6b9327e59dff7e5f9f10d20c7c1a13d4..4ca043a369e7a431b8913622afc6ae7a85a85e0f 100644
--- a/Source/WebCore/rendering/RenderFlexibleBox.cpp
+++ b/Source/WebCore/rendering/RenderFlexibleBox.cpp
@@ -39,6 +39,8 @@
 
 namespace WebCore {
 
+static const ItemPosition selfAlignmentNormalBehavior = ItemPositionStretch;
+



I can send a patch with a different name for that variable.

[Javier Fernandez]

Created attachment 279956 [details]
Patch

Trying to fix the win build

[Javier Fernandez]

Created attachment 279957 [details]
Patch

Trying to fix the win build

[Javier Fernandez]

Created attachment 279972 [details]
Patch

Trying to fix the win build

[Darin Adler]

Comment on attachment 279972 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=279972&action=review

> Source/WebCore/ChangeLog:32
> +        (WebCore::CSSParser::parseItemPositionOverflowPosition): A new value 'normal' is now valid.a

Stray "a" here.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2281
> +        return {data.position(), OverflowAlignmentDefault};

We’ve been using spaces in the braces in this syntax in WebKit coding style:

    return { data.position(), OverflowAlignmentDefault };

It’s not mandatory to match this, but would fit in better with other new code we are writing.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2291
> +    const StyleSelfAlignmentData& inheritedValue = (!parent || !parent->computedStyle()) ? RenderStyle::initialDefaultAlignment() : parent->computedStyle()->justifyItems();

Could use auto& or const auto& here instead to make this line not so long. Not sure it is important to state the type name explicitly.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2296
> +    return {ItemPositionNormal, OverflowAlignmentDefault};

Spaces in braces thing.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2306
> +        return {ItemPositionNormal, OverflowAlignmentDefault};

Spaces in braces thing.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2317
> +        return {ItemPositionNormal, OverflowAlignmentDefault};

Spaces in braces thing.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2401
> +static RefPtr<CSSValueList> valueForItemPositionWithOverflowAlignment(const StyleSelfAlignmentData& data)

This function should return a Ref, not. RefPtr.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2404
>      RefPtr<CSSValueList> result = CSSValueList::createSpaceSeparated();

This should use auto and be a Ref, not a RefPtr.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:42
> +static const ItemPosition selfAlignmentNormalBehaviorFlexibleBox = ItemPositionStretch;

Maybe this can be constexpr instead of const.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:239
> +        const auto& newStyle = style();

Consider just auto& instead of const auto&.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:241
> +            const auto& childStyle = child->style();

Ditto.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:242
> +            ItemPosition previousAlignment = childStyle.resolvedAlignSelf(*oldStyle, selfAlignmentNormalBehaviorFlexibleBox).position();

Consider auto.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1116
> +    ContentPosition position = style().resolvedJustifyContentPosition(contentAlignmentNormalBehaviorFlexibleBox());
> +    ContentDistributionType distribution = style().resolvedJustifyContentDistribution(contentAlignmentNormalBehaviorFlexibleBox());

Consider auto for types.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1200
> +    ContentPosition position = style().resolvedJustifyContentPosition(contentAlignmentNormalBehaviorFlexibleBox());
> +    ContentDistributionType distribution = style().resolvedJustifyContentDistribution(contentAlignmentNormalBehaviorFlexibleBox());

Consider auto for types.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1258
> +    ContentPosition position = style().resolvedAlignContentPosition(contentAlignmentNormalBehaviorFlexibleBox());
> +    ContentDistributionType distribution = style().resolvedAlignContentDistribution(contentAlignmentNormalBehaviorFlexibleBox());

Consider auto for types.

> Source/WebCore/rendering/RenderGrid.cpp:42
>  static const int infinity = -1;
> +static const ItemPosition selfAlignmentNormalBehavior = ItemPositionStretch;

Consider constexpr instead of const.

> Source/WebCore/rendering/RenderGrid.cpp:2047
> +        OverflowAlignment overflow = child.style().resolvedAlignSelf(style(), selfAlignmentNormalBehavior).overflow();
> +        LayoutUnit offsetFromStartPosition = computeOverflowAlignmentOffset(overflow, endOfRow - startOfRow, childBreadth);

Consider auto for types.

> Source/WebCore/rendering/RenderGrid.cpp:2080
> +        OverflowAlignment overflow = child.style().resolvedJustifySelf(style(), selfAlignmentNormalBehavior).overflow();
> +        LayoutUnit offsetFromStartPosition = computeOverflowAlignmentOffset(overflow, endOfColumn - startOfColumn, childBreadth);

Consider auto for types.

> Source/WebCore/rendering/RenderGrid.cpp:2143
> +    ContentPosition position = isRowAxis ? style().resolvedJustifyContentPosition(contentAlignmentNormalBehaviorGrid()) : style().resolvedAlignContentPosition(contentAlignmentNormalBehaviorGrid());
> +    ContentDistributionType distribution = isRowAxis ? style().resolvedJustifyContentDistribution(contentAlignmentNormalBehaviorGrid()) : style().resolvedAlignContentDistribution(contentAlignmentNormalBehaviorGrid());

Consider auto for types.

> Source/WebCore/rendering/style/RenderStyle.cpp:197
> +        return {normalValueBehavior, OverflowAlignmentDefault};

Spaces in braces.

> Source/WebCore/rendering/style/RenderStyle.cpp:221
> +        return {normalValueBehaviour, OverflowAlignmentDefault};

Spaces in braces.

> Source/WebCore/rendering/style/RenderStyle.h:514
> +    const StyleSelfAlignmentData resolvedAlignItems(ItemPosition normalValueBehaviour) const;
> +    const StyleSelfAlignmentData resolvedAlignSelf(const RenderStyle& parentStyle, ItemPosition normalValueBehaviour) const;
> +    const StyleSelfAlignmentData resolvedJustifyItems(ItemPosition normalValueBehaviour) const;
> +    const StyleSelfAlignmentData resolvedJustifySelf(const RenderStyle& parentStyle, ItemPosition normalValueBehaviour) const;

Return types should just be StyleSelfAlignmentData, not const StyleSelfAlignmentData. It’s rarely useful to have a return value be a const; functions typically return "int", not "const int".

> Source/WebCore/rendering/style/RenderStyleConstants.h:257
> +enum ItemPosition {ItemPositionAuto, ItemPositionNormal, ItemPositionStretch, ItemPositionBaseline, ItemPositionLastBaseline, ItemPositionCenter, ItemPositionStart, ItemPositionEnd, ItemPositionSelfStart, ItemPositionSelfEnd, ItemPositionFlexStart, ItemPositionFlexEnd, ItemPositionLeft, ItemPositionRight};

Spaces in braces here. Notice that the older code all has the spaces, but the newer code does not. Seems this code has been done without considering prevailing WebKit style.

[Javier Fernandez]

Created attachment 280039 [details]
Patch

Patch for landing.

[Javier Fernandez]

Created attachment 280041 [details]
Patch

Patch for landing fixing copy-elision issue.

[Javier Fernandez]

(In reply to comment #18)
> Created attachment 280039 [details]
> Patch
> 
> Patch for landing.

This patch doesn't build in the ios-sim bot because the clang version used complains about using WTFMove in the return value of some functions :

/Volumes/Data/EWS/WebKit/Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2411:12: error: moving a local object in a return statement prevents copy elision [-Werror,-Wpessimizing-move]
    return WTFMove(result);
           ^
In file included from /Volumes/Data/EWS/WebKit/Source/WebCore/css/CSSComputedStyleDeclaration.cpp:25:

Apparently, this is issue has been already reported in llvm:

https://llvm.org/bugs/show_bug.cgi?id=23819

There are similar returns in the same class, so I'm not sure what's the correct way to implement it. 

I've submitted a new patch without the WTFMove, but I wonder whether it will cause unnecessary ref/deref ops.

[Darin Adler]

(In reply to comment #20)
> I've submitted a new patch without the WTFMove, but I wonder whether it will
> cause unnecessary ref/deref ops.

It won’t. An explicit std::move is not needed or helpful when the class matches exactly and the return value optimization kicks in without requiring a move. I find remembering the rule annoying when using a compiler without the warning, but there’s no actual reference count churning.

[WebKit Commit Bot]

Comment on attachment 280041 [details]
Patch

Clearing flags on attachment: 280041

Committed r201498: <http://trac.webkit.org/changeset/201498>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.