763399 - Remove NS_CSS_MINMAX in favour of mozilla::clamped

Status: RESOLVED WONTFIX

(Core :: Layout, defect)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

In the places where we use NS_CSS_MINMAX:

http://mxr.mozilla.org/mozilla-central/ident?i=NS_CSS_MINMAX&filter=

we should use mozilla::clamped (from <http://mxr.mozilla.org/mozilla-central/source/xpcom/string/public/nsAlgorithm.h>) instead.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm not so sure; NS_CSS_MINMAX has the advantage that it tells you something about the expected order of the arguments.

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

That's a fair point; maybe we should just rename mozilla::clamped first.

Comment 3

[Jignesh Kakadiya [:jhk]]

Attachment: Patch(v1)

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 633236 [details] [diff] [review]
Patch(v1)

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

Clearing f? until it's clear we want it.

Comment 5

[Mats Palmgren (inactive)]

(In reply to David Baron [:dbaron] from comment #1)
> I'm not so sure; NS_CSS_MINMAX has the advantage that it tells you something
> about the expected order of the arguments.

It seems to me that the order min then max is common sense, just like x
always comes before y, width always comes before height, etc.

Fwiw, mozilla::clamped does have:
  NS_ABORT_IF_FALSE(max >= min, "clamped(): max must be greater than or equal to min");

(In reply to :Ms2ger from comment #2)
> That's a fair point; maybe we should just rename mozilla::clamped first.

I wonder why we didn't follow our naming convention that method/function names
start with a capital letter.

My 2 cents: leave mozilla::clamped as is, and replace NS_CSS_MINMAX with it

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I don't have a strong opinion about the exact name, but having one function instead of two would be nice

Comment 7

[joseph]

Hello guys,

May I ask what is the final decision about NS_CSS_MINMAX / mozilla::clamped?

Cheers,

Comment 8

[Amod [:AMoz]]

I am interested to solve the bug further

Comment 9

[Amod [:AMoz]]

Attachment: Initial Patch !

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

One comment on the patch:
 * we should probably be |using namespace mozilla;| rather than sprinkling mozilla:: everywhere

I guess, given that std::minmax() is something different (returns a pair<> with min and max), we should probably stick with either mozilla::clamp() or mozilla::clamped().  (Where did the -ed come from, anyway?)

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 703231 [details] [diff] [review]
Initial Patch !

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

I think this is an improvement over what we have now, at least, so I think we should take it.

::: layout/forms/nsHTMLButtonControlFrame.cpp
@@ +232,2 @@
>                                          aReflowState.mComputedMinHeight,
>                                          aReflowState.mComputedMaxHeight);

You'll want to fix the indentation here and below.

::: layout/generic/nsHTMLReflowState.h
@@ +38,5 @@
>      result = aMaxValue;
>    if (aMinValue > result)
>      result = aMinValue;
>    return result;
>  }

Just remove the implementation

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 703231 [details] [diff] [review]
Initial Patch !

Could you update the patch with the above comments and request review on the new patch?

Comment 13

[Amod [:AMoz]]

sure... i will upload the patch and give you for review.

Comment 14

[Mats Palmgren (inactive)]

(In reply to David Baron [:dbaron] from comment #10)
> (Where did the -ed come from, anyway?)

bug 695303 comment 4 and 5

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Mats Palmgren [:mats] from comment #5)
> Fwiw, mozilla::clamped does have:
>   NS_ABORT_IF_FALSE(max >= min, "clamped(): max must be greater than or
> equal to min");

Er, actually, doesn't that mean this entire patch isn't going to work?  We actually depend on NS_CSS_MINMAX working when max < min, and doing the correct CSS behavior in that case.

Comment 16

[Mats Palmgren (inactive)]

Yeah, good point.  I think we should fix the consumers to not rely on that.

In nsLayoutUtils::ComputeHeightDependentValue right about here:
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp#3146
we should add something like what ComputeAutoSizeWithIntrinsicDimensions does:
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp#3220

I think the consumers that use mComputedMin/MaxHeight should be OK --
nsHTMLReflowState should have taken care of it.

It appears min/max main- and cross-sizes are ordered, but I'm not familiar
enough with nsFlexContainerFrame.cpp to say for sure.

So it appears nsLayoutUtils.cpp#3146 is the one place we need to fix.

Comment 17

[Amod [:AMoz]]

Attachment: updated patch !

Comment 18

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 703259 [details] [diff] [review]
updated patch !

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

::: layout/generic/nsFlexContainerFrame.cpp
@@ +1581,2 @@
>                                  aItem.GetCrossMinSize(),
>                                  aItem.GetCrossMaxSize());

Fix the indentation here

@@ +2060,1 @@
>                        minCrossSize, maxCrossSize);

and here

::: layout/generic/nsHTMLReflowState.h
@@ +38,5 @@
>      result = aMaxValue;
>    if (aMinValue > result)
>      result = aMinValue;
>    return result;
>  }

and remove this

::: layout/xul/base/src/nsLeafBoxFrame.cpp
@@ +287,2 @@
>                                        aReflowState.mComputedMinHeight,
>                                        aReflowState.mComputedMaxHeight);

and the indentation again here.

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Mats Palmgren [:mats] from comment #16)
> Yeah, good point.  I think we should fix the consumers to not rely on that.

But the entire *point* of NS_CSS_MINMAX was to encapsulate that.

Thus I think this bug should be WONTFIX.

Comment 20

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to David Baron [:dbaron] from comment #15)
> (In reply to Mats Palmgren [:mats] from comment #5)
> > Fwiw, mozilla::clamped does have:
> >   NS_ABORT_IF_FALSE(max >= min, "clamped(): max must be greater than or
> > equal to min");
> 
> Er, actually, doesn't that mean this entire patch isn't going to work?  We
> actually depend on NS_CSS_MINMAX working when max < min, and doing the
> correct CSS behavior in that case.

How is this not documented anywhere? I don't think it makes sense to have a generic helper function that follows a different contract than what you'd reasonably expect, and hides that fact too. If there's only one caller that actually relies on this behaviour, we should inline the check.

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Mats Palmgren [:mats] from comment #16)
> I think the consumers that use mComputedMin/MaxHeight should be OK --
> nsHTMLReflowState should have taken care of it.

If mComputedMax{Height,Width} are intended to reflect the computed values of the CSS properties in question, then the code in nsHTMLReflowState that does this adjustment is buggy.

In any case, I'm comfortable with WONTFIXing this bug at this point; CSS has a clear rule for handling min and max values, where min overrides max.  NS_CSS_MINMAX is designed to encapsulate that rule, allowing its input max to be smaller than its input min (in which case the result must be exactly min).  Any replacement for it must have the same behavior.


Sorry for wasting the time of the people preparing patches for this.

Comment 22

[Amod [:AMoz]]

felt very much sad to see this :( :( :(

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to :Ms2ger from comment #20)
> How is this not documented anywhere? I don't think it makes sense to have a
> generic helper function that follows a different contract than what you'd
> reasonably expect, and hides that fact too. If there's only one caller that
> actually relies on this behaviour, we should inline the check.

It's documented pretty clearly in the relevant CSS specs; while the code is clear, I agree that a comment pointing out that the min-overrides-max behavior is what is required by the CSS specs would be useful.

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The relevant spec citations are:
http://www.w3.org/TR/CSS21/visudet.html#min-max-widths
http://www.w3.org/TR/CSS21/visudet.html#min-max-heights

Comment 25

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Add a comment

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 703269 [details] [diff] [review]
Add a comment

r=dbaron  Thanks.

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bb5400c00877

Comment 28

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/bb5400c00877