Bug 144235 - [CSS Grid Layout] overflow-position keyword for align and justify properties.
Summary: [CSS Grid Layout] overflow-position keyword for align and justify properties.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Javier Fernandez
URL:
Keywords:
Depends on:
Blocks: 60731 91512 111616
  Show dependency treegraph
 
Reported: 2015-04-26 15:15 PDT by Javier Fernandez
Modified: 2015-04-30 18:15 PDT (History)
9 users (show)

See Also:


Attachments
Patch (22.30 KB, patch)
2015-04-30 03:26 PDT, Javier Fernandez
no flags Details | Formatted Diff | Diff
Patch (22.12 KB, patch)
2015-04-30 14:36 PDT, Javier Fernandez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Javier Fernandez 2015-04-26 15:15:58 PDT
When the alignment subject is larger than the alignment container, it will overflow. Some alignment modes, if honored in this situation, may cause data loss; an overflow alignment mode can be explicitly specified to avoid this.
Comment 1 Javier Fernandez 2015-04-30 03:26:27 PDT
Created attachment 252049 [details]
Patch
Comment 2 Sergio Villar Senin 2015-04-30 07:20:34 PDT
Comment on attachment 252049 [details]
Patch

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

Awesome fix+test. I've some comments though

> Source/WebCore/rendering/RenderGrid.cpp:1241
> +static LayoutUnit computeOverflowAlignmentOffset(OverflowAlignment overflow, LayoutUnit startOfTrack, LayoutUnit endOfTrack, LayoutUnit childBreadth)

You better pass directly the trackBreadth here as neither endOfTrack nor startOfTrack are used for anything else.

> Source/WebCore/rendering/RenderGrid.cpp:1254
> +    return offset;

I prefer to have a switch here with all the potential values for OverflowAlignment as it's safer against future additions/removals in the enum. Something like

switch(overflow) {
case Safe:
return std::max(...)
case True:
case Default:
return trackBreadth - childBreath;
}
ASSERT_NOT_REACHED()
return 0;

> Source/WebCore/rendering/style/RenderStyle.cpp:-39
> -#include "StyleInheritedData.h"

Why are you removing this?

> LayoutTests/fast/css-grid-layout/grid-align-justify-overflow-expected.txt:10
> +PASS

It's a pity that a gorgeous set of test cases as the ones you're adding have this miserable output :(. Anyway I guess we could eventually replace some of this checkLayout() tests.

> LayoutTests/fast/css-grid-layout/grid-align-justify-overflow.html:120
> +

Nit: perhaps we can write all these in a single line each so the test is more compact.

.justifySelfEnd { justify-self: end; } 

etc....
Comment 3 Javier Fernandez 2015-04-30 09:56:46 PDT
Comment on attachment 252049 [details]
Patch

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

>> Source/WebCore/rendering/RenderGrid.cpp:1241
>> +static LayoutUnit computeOverflowAlignmentOffset(OverflowAlignment overflow, LayoutUnit startOfTrack, LayoutUnit endOfTrack, LayoutUnit childBreadth)
> 
> You better pass directly the trackBreadth here as neither endOfTrack nor startOfTrack are used for anything else.

the same would appliy to trackBreadh and childBreadth; the thing is that if I pass that, I end up computing the same in 2 functions computeOverflowAlignmentOffset is kind of helpers function to clearly describe how the overflow is handled  such logic can be moved to both, row-axis and column-axis positioning logic but I thought haveing a single function with the shared code was clearer.

If you don't mind, I'll keep the function as it is.

>> Source/WebCore/rendering/RenderGrid.cpp:1254
>> +    return offset;
> 
> I prefer to have a switch here with all the potential values for OverflowAlignment as it's safer against future additions/removals in the enum. Something like
> 
> switch(overflow) {
> case Safe:
> return std::max(...)
> case True:
> case Default:
> return trackBreadth - childBreath;
> }
> ASSERT_NOT_REACHED()
> return 0;

Ok

>> Source/WebCore/rendering/style/RenderStyle.cpp:-39
>> -#include "StyleInheritedData.h"
> 
> Why are you removing this?

Wow, good catch !!! it was a mistake.

>> LayoutTests/fast/css-grid-layout/grid-align-justify-overflow-expected.txt:10
>> +PASS
> 
> It's a pity that a gorgeous set of test cases as the ones you're adding have this miserable output :(. Anyway I guess we could eventually replace some of this checkLayout() tests.

Agree.

>> LayoutTests/fast/css-grid-layout/grid-align-justify-overflow.html:120
>> +
> 
> Nit: perhaps we can write all these in a single line each so the test is more compact.
> 
> .justifySelfEnd { justify-self: end; } 
> 
> etc....

ok.
Comment 4 Javier Fernandez 2015-04-30 14:36:52 PDT
Created attachment 252089 [details]
Patch
Comment 5 WebKit Commit Bot 2015-04-30 18:15:37 PDT
Comment on attachment 252089 [details]
Patch

Clearing flags on attachment: 252089

Committed r183660: <http://trac.webkit.org/changeset/183660>
Comment 6 WebKit Commit Bot 2015-04-30 18:15:41 PDT
All reviewed patches have been landed.  Closing bug.