Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2829303002:
[css-flexbox] Merge align-content and justify-content handling (Closed)
|
Created:
3 years, 8 months ago by cbiesinger Modified:
3 years, 7 months ago Reviewers:
jfernandez CC:
chromium-reviews, pdr+renderingwatchlist_chromium.org, zoltan1, blink-reviews-layout_chromium.org, szager+layoutwatch_chromium.org, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, blink-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[css-flexbox] Merge align-content and justify-content handling
The functions are practically identical and this can be better handled with
an if statement.
R=jfernandez@igalia.com
Review-Url: https://codereview.chromium.org/2829303002
Cr-Commit-Position: refs/heads/master@{#469475}
Committed: https://chromium.googlesource.com/chromium/src/+/047294d8a76953fb5d19b9fc20e7980f2641bc45
Patch Set 1 #
Total comments: 5
Patch Set 2 : helper function #
Messages
Total messages: 23 (11 generated)
The CQ bit was checked by cbiesinger@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2829303002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp (right): https://codereview.chromium.org/2829303002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp:1523: bool is_align_content, I don't like too much the bool argument to change the behavior of the function. Also, I don't understand why this function should be dependent on the specific property it handles. It's not that justify-content has a different behavior for 'stretch', but it's an invalid value for that property. We can simply define the behavior of 'stretch' here and let the caller pass the appropriated value. If justify-content can't have 'stretch' we shouldn't need this conditional clause. https://codereview.chromium.org/2829303002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp:1531: (is_align_content && ditto. https://codereview.chromium.org/2829303002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp:1887: false, available_free_space, distribution, children.size()); Ditto. https://codereview.chromium.org/2829303002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp:1950: false, available_free_space, distribution, children.size()); Ditto. https://codereview.chromium.org/2829303002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp:2000: true, available_cross_axis_space, distribution, line_contexts.size()); Ditto.
If you like, I could change resolvedJustifyContentDistribution to resolve
normal/stretch to flex-start for display:{inline-,}flex.
FWIW, stretch is a valid value for justify-content:
https://drafts.csswg.org/css-align/#distribution-flex
On 2017/04/21 20:46:58, cbiesinger wrote:
> If you like, I could change resolvedJustifyContentDistribution to resolve
> normal/stretch to flex-start for display:{inline-,}flex.
>
Instead, couldn't we resolve it at Flexbox level. I don't think is a
good idea to use the display value to determine the value, since there
are Flexbox objects that are created during layout without that display
value, like Buttons.
In Grid we already have a similar helper function.
> FWIW, stretch is a valid value for justify-content:
> https://drafts.csswg.org/css-align/#distribution-flex
oh, sorry, my fault.
On 2017/04/21 21:00:59, jfernandez wrote:
> On 2017/04/21 20:46:58, cbiesinger wrote:
> > If you like, I could change resolvedJustifyContentDistribution to resolve
> > normal/stretch to flex-start for display:{inline-,}flex.
> >
>
> Instead, couldn't we resolve it at Flexbox level. I don't think is a
> good idea to use the display value to determine the value, since there
> are Flexbox objects that are created during layout without that display
> value, like Buttons.
>
> In Grid we already have a similar helper function.
Fair enough, I'll do that.
> In Grid we already have a similar helper function. Could you point me to the function? I couldn't find it...
On 2017/04/21 21:25:33, cbiesinger wrote: > > In Grid we already have a similar helper function. > > Could you point me to the function? I couldn't find it... StyleSelfAlignmentData LayoutGrid::AlignSelfForChild StyleSelfAlignmentData LayoutGrid::JustifySelfForChild We have it for Self-Alignment, but it makes sense for Content Distribution as well.
On 2017/04/21 21:30:12, jfernandez wrote: > On 2017/04/21 21:25:33, cbiesinger wrote: > > > In Grid we already have a similar helper function. > > > > Could you point me to the function? I couldn't find it... > > StyleSelfAlignmentData LayoutGrid::AlignSelfForChild > StyleSelfAlignmentData LayoutGrid::JustifySelfForChild > > We have it for Self-Alignment, but it makes sense for > Content Distribution as well. The idea is to leave the ComputedStyle resolve functions as generic as possible, and if additional specific behavior is needed implement it as helper functions at layout level.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by cbiesinger@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Done, please take another look
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
lgtm
The CQ bit was checked by cbiesinger@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493925546715780,
"parent_rev": "aeb1abc9b5177c2817b21fe3a366fc0f23450607", "commit_rev":
"047294d8a76953fb5d19b9fc20e7980f2641bc45"}
Message was sent while issue was closed.
Description was changed from ========== [css-flexbox] Merge align-content and justify-content handling The functions are practically identical and this can be better handled with an if statement. R=jfernandez@igalia.com ========== to ========== [css-flexbox] Merge align-content and justify-content handling The functions are practically identical and this can be better handled with an if statement. R=jfernandez@igalia.com Review-Url: https://codereview.chromium.org/2829303002 Cr-Commit-Position: refs/heads/master@{#469475} Committed: https://chromium.googlesource.com/chromium/src/+/047294d8a76953fb5d19b9fc20e7... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/047294d8a76953fb5d19b9fc20e7... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
