Closed Bug 1303643 Opened 3 years ago Closed 3 years ago

[css-grid] fix bugs grid item percentage padding/margin calculations

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: mats, Assigned: mats)

References

Details

Attachments

(3 files, 2 obsolete files)

No description provided.
Summary: [css-grid] re-enable reftest grid-auto-min-sizing-definite-001.html → [css-grid] fix bugs grid item percentage padding/margin calculations
Review ping. In case you forgot about this one ;-)
Flags: needinfo?(dholbert)
Comment on attachment 8794464 [details] [diff] [review]
part 1 - Add a flag to control wether percentages should be applied also to min-content contributions

Review of attachment 8794464 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, one observation and then two nits:


Observation (address is you see fit):
It's a bit tragic that AddPercents is becoming less "convenient" (i.e. now we have to wrap all of these 6 calls in an if{...} check, which makes them longer -- and we're having to assign "result" twice instead of once.  I suppose that's kind of a good change, from a clarity perspective -- i.e. now AddPercents() always does actually Add a Percent.   Though -- PERHAPS we could preserve the compactness without hurting clarity if we renamed it to MaybeAddPercent(), and made it take a boolean variable which controls whether or not to actually do the percent-addition?  I think I'd marginally prefer that.... This is fine too, though.)


> Bug 1303643 part 1 - Add a flag to control wether percentages should be applied also to min-content contributions. r=dholbert

Nit #1: Typo in commit message: s/wether/whether/

::: layout/base/nsLayoutUtils.cpp
@@ +4794,5 @@
>    result = NSCoordSaturatingAdd(result, coordOutsideSize);
>    pctTotal += pctOutsideSize;
>  
> +  const bool kAddPercent = aType == nsLayoutUtils::PREF_ISIZE ||
> +                           (aFlags & nsLayoutUtils::ADD_PERCENTS);

Nit #2: I'm pretty sure that the "k" prefix is incorrect here. This should probably be named something like "shouldAddPercent", without any special letter prefix.

(My impression from the coding style guide (and from usage[1]) is that "k" is for *persistent* constants, which always have the same value on calls into the same piece of code -- not for temporary variables whose value may vary on repeated calls into the same function, which happen to be labeled as "const".

The coding style guide unfortunately isn't particularly clear on what it means by "k=constant", but a sampling of usage clarifies how we've interpreted it in the past -- based on a quick skim of the results from:
      grep -r "const.* k[A-Z]" mozilla-central
...it all seems to be string literals, numeric literals, products-of-other-constants, and the like.  And of course, we have tons of "const" references to other variables which are *not* prefixed with "k". So I think "constant" means something more specific than just "const".)
Attachment #8794464 - Flags: review?(dholbert) → review+
Comment on attachment 8794465 [details] [diff] [review]
part 2 - [css-grid] Apply percentages to grid item min-content contributions

Review of attachment 8794465 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsLayoutUtils.cpp
@@ +5129,5 @@
>                  aType == MIN_ISIZE ? "min" : "pref",
>                  aWM.IsVertical() ? "vertical" : "horizontal");
>  #endif
>  
> +  aFlags |= nsLayoutUtils::ADD_PERCENTS;

This tweak (modifying the flags that have been passed in) might deserve a code-comment to explain it... The reason for it is not really self-explanatory.

::: layout/generic/nsGridContainerFrame.cpp
@@ +3625,5 @@
>      size += offsets.hMargin;
> +    if (!aState.mCols.mCanResolveLineRangeSize) {
> +      offsets.hPctMargin += offsets.hPctPadding;
> +    }
> +    size = nsLayoutUtils::AddPercents(size, offsets.hPctMargin);

What's the if-check for?  (I don't see an obvious reason why "mCanResolveLineRangeSize" would determine whether we should  honor hPctPadding here [and what differentiates %-padding from %-margin in this respect])
Flags: needinfo?(dholbert) → needinfo?(mats)
> What's the if-check for?

Good question.  After digging a little deeper the two cases we want to
treat differently is intrinsic sizing vs reflow.  For intrinsic sizing,
we don't have a percent basis so the border-box child block-size we get
from MeasuringReflow doesn't include it.  During reflow though, we
always have percent basis in both axis (the grid area) so any percentage
padding is already baked in to the result.

(I've added a reftest that fails with the earlier if-condition.)
Attachment #8794465 - Attachment is obsolete: true
Attachment #8794465 - Flags: review?(dholbert)
Flags: needinfo?(mats)
Attachment #8796276 - Flags: review?(dholbert)
Comment on attachment 8796276 [details] [diff] [review]
part 2 - [css-grid] Apply percentages to grid item min-content contributions

Review of attachment 8796276 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for investigating/clarifying that!

r=me, one nit:

::: layout/base/nsLayoutUtils.cpp
@@ +5131,5 @@
>                  aWM.IsVertical() ? "vertical" : "horizontal");
>  #endif
>  
> +  // Note: this method is only meant for grid/flex items which always
> +  // includes percentages in their intrinsic size.

typo: s/includes/include/

(Thanks for adding a comment)
Attachment #8796276 - Flags: review?(dholbert) → review+
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/24a382443a0f
part 1 - Add a flag to control whether percentages should be applied also to min-content contributions.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/17341d12497d
part 2 - [css-grid] Apply percentages to grid item min-content contributions.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/32f744b48562
part 3 - [css-grid] Update reftest references.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.