842179 - Keep the thumb for <input type=range> within its content box

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Daniel Holbert [:dholbert]]

Currently, for <input type="range">, the thumb-frame will extend outside of the input element's border-box at values 0 and 100.

(but fortunately it remains inside its (default) margin-box, because we specify a margin)

I think we might want to replace some (maybe all?) of the <input> element's margin with padding, so that the thumb frame remains inside the border box.  Here's the CSS that currently specifies a margin but no padding (as of when bug 838256 landed):
  https://hg.mozilla.org/mozilla-central/annotate/5e660787834c/layout/style/forms.css#l720

Testcase:
data:text/html,<input type="range" value=100 style="border: 1px dashed black">


More extreme testcase (w/ margin explicitly set to 0, which is admittedly a bit of a strawman but I'm doing it for the sake of demonstration):

data:text/html,<input type="range" value=100 style="border: 1px dashed black; margin:0">pleaseDontOverlapMe



If we replaced the margin with padding, then that last testcase would look like this:

data:text/html,<input type="range" value=100 style="border: 1px dashed black; margin:0; padding: 0 0.7em">pleaseDontOverlapMe

...which looks much better IMHO.

Marking as blocking bug 842021, since that covers the UI for this widget.

Comment 1

[Jonathan Watt [:jwatt]]

I think what we should do is keep the thumb border box inside the input's content box. That's what makes sense to me and, now that I check, that's what Chrome does. I'll write a patch to do that.

Comment 2

[Daniel Holbert [:dholbert]]

Makes sense to me.

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 725028 [details] [diff] [review]
patch

>diff --git a/layout/forms/nsRangeFrame.cpp b/layout/forms/nsRangeFrame.cpp
>--- a/layout/forms/nsRangeFrame.cpp
>+++ b/layout/forms/nsRangeFrame.cpp
>@@ -247,63 +247,36 @@ nsRangeFrame::ReflowAnonymousContent(nsP
>     // The idea here is that we want to position the thumb so that the center
>     // of the thumb is on an imaginary line drawn from the middle of one edge
>     // of the range frame's content box to the middle of the opposite edge of
>     // its content box (the opposite edges being the left/right edge if the
>     // range is horizontal, or else the top/bottom edges if the range is
>     // vertical). How far along this line the center of the thumb is placed
>     // depends on the value of the range.
> 
>-    nsSize frameSizeOverride(aDesiredSize.width, aDesiredSize.height);

First observation: This comment ^^ no longer belongs here -- you moved this logic into DoUpdateThumbPosition().  The comment probably wants to move there, too, right?

Maybe add a brief comment saying "We'll adjust the positioning of the frame after we reflow it.

>     nsReflowStatus frameStatus = NS_FRAME_COMPLETE;
>     nsHTMLReflowMetrics thumbDesiredSize;
>     nsresult rv = ReflowChild(thumbFrame, aPresContext, thumbDesiredSize,
>-                              thumbReflowState, thumbX, thumbY, 0, frameStatus);
>+                              thumbReflowState, 0, 0, 0, frameStatus);
>     NS_ENSURE_SUCCESS(rv, rv);
>     MOZ_ASSERT(NS_FRAME_IS_FULLY_COMPLETE(frameStatus),
>                "We gave our child unconstrained height, so it should be complete");
>     rv = FinishReflowChild(thumbFrame, aPresContext, &thumbReflowState,
>-                           thumbDesiredSize, thumbX, thumbY, 0);
>+                           thumbDesiredSize, 0, 0, 0);
>     NS_ENSURE_SUCCESS(rv, rv);
>+
>+    DoUpdateThumbPosition(thumbFrame, nsSize(aDesiredSize.width,
>+                                             aDesiredSize.height));

It seems a bit hacky / unnecessary to reflow the thumb frame at 0,0 and then move it.  I'd sort of rather we called it first (& make it return a nsPoint instead of directly setting the position), and then use that point for ReflowChild / FinishReflowChild.

Probably not a huge deal, though. If we keep things the way they are now, you should at least add a brief comment above ReflowChild explaining why you're reflowing the thumb at 0,0.
>+  if (IsHorizontal()) {
>+    nscoord travel = rangeContentRect.width - thumbSize.width;

"travel" seems like an odd name for this variable. Maybe "traversableDistance"?

>+    nscoord posAtStart = rangeContentRect.x + thumbSize.width/2;
>+    nscoord posAtEnd = rangeContentRect.x + rangeContentRect.width - thumbSize.width/2;
>+    nscoord posOfPoint = mozilla::clamped(point.x, posAtStart, posAtEnd);
>+    fraction = (posOfPoint - posAtStart) / double(travel);

I think the posAtStart & posAtEnd values there will introduce rounding error when thumbSize.width is odd, since /2 does a "floor" operation, and we're applying that "floor" to the value we're adding on the lower-end, so we end up with a range that's larger (by 1 nscoord unit) than we'd like.

This would mean we could end up with "fraction" being greater than 1.0, which would be very weird.

For example, suppose rangeContentRect.x = 0, rangeContentRect.width = 10, and thumbSize.width is 1.  Then "travel" will be 9, posAtStart will be 0, and posAtEnd will be 10.  So we could end up with posOfPoint = 10, which would give us fraction = (10 - 0 / 9) = 1.11111.

You can address this (and make the math easier-to-understand IMHO) if you just set posAtEnd as follows:
  posAtEnd = posAtStart + traversableDistance;

>+  } else {
>+    nscoord travel = rangeContentRect.height - thumbSize.height;
>+    if (travel <= 0) {
>+      return minimum;
>+    }
>+    nscoord posAtStart = rangeContentRect.y + thumbSize.height/2;
>+    nscoord posAtEnd = rangeContentRect.y + rangeContentRect.height - thumbSize.height/2;

As above:
 * rename travel to traversableDistance or something.
 * posAtEnd = posAtStart + traversableDistance;

>+    fraction = 1.0 - (posOfPoint - posAtStart) / double(travel);

Add a comment to explain the 1.0 subtraction there.
e.g. something like:
   // For a vertical range frame, the "start" side of the frame is
   // the highest value -- so we subtract the fraction from 1.0
   // to get that polarity correct.


>+  return minimum + fraction * range;
> }

Assert that fraction is between 0.0 and 1.0 before returning here. (to catch rounding bugs like the one I mentioned above)

>+void
>+nsRangeFrame::DoUpdateThumbPosition(nsIFrame* aThumbFrame,
>+                                    const nsSize& aRangeSize)
>+{

Assert that aThumbFrame is non-null here. (since we do have null thumb frames sometimes -- but this function implicitly assumes that its caller has already done the null-check. Let's make that assumption explicit.)

>+  nsSize thumbSize = aThumbFrame->GetSize();
>+
>+  // We are called under Reflow, so we need to pass IsHorizontal a valid rect.
>+  nsSize frameSizeOverride(aRangeSize.width, aRangeSize.height);

I wonder: Should we be passing aRangeSize (the border-box size), or rangeContentBoxSize, for the purposes of determining whether we're horizontal?

I'm inclined to think we should be using the *content-box* size, since that's what determines the traversable distance for the thumb.  I expect authors would be surprised if adding padding around <input> modified whether it rendered as a vertical or a horizontal control.

(I suppose it doesn't matter much until IsHorizontal is sorted out in bug 840820, though.

>   double fraction = GetValueAsFractionOfRange();

Add an assertion here to verify that |fraction| is between 0.0 and 1.0 (like the assert at the other GetValueAsFractionOfRange() call-site.)

>+  if (IsHorizontal(&frameSizeOverride)) {
>+    if (thumbSize.width < rangeContentBoxSize.width) {
>+      nscoord travel = rangeContentBoxSize.width - thumbSize.width;

As above, I think something like "traversableDistance" would be clearer than "travel" here.

>+      if (StyleVisibility()->mDirection == NS_STYLE_DIRECTION_RTL) {
>+        newPosition.x += travel - NSToCoordRound(fraction * travel);
>+      } else {
>+        newPosition.x += NSToCoordRound(fraction * travel);
>+      }

I think the RTL case should be shortened to:
   newPosition.x += NSToCoordRound((1.0 - fraction) * travel);
for symmetry with elsewhere in the file, where we subtract fraction from 1.0 before multiplying.

>+      newPosition.y += (rangeContentBoxSize.height - thumbSize.height)/2;

observation: this will almost certainly be the same as |oldPosition.y| (except on the first reflow, or except when we've moved)

I guess that's OK.

>+    if (thumbSize.height < rangeContentBoxSize.height) {
>+      nscoord travel = rangeContentBoxSize.height - thumbSize.height;
>+      newPosition.x += (rangeContentBoxSize.width - thumbSize.width)/2;
>+      newPosition.y += travel - NSToCoordRound(fraction * travel);

As above, I'd rather this were:
  newPosition.y += NSToCoordRound((1.0 - fraction) * travel);


r=me with the above addressed.

Comment 5

[Daniel Holbert [:dholbert]]

some edits to my review feedback:

> Maybe add a brief comment saying "We'll adjust the positioning of the frame
> after we reflow it.
(disregard this comment -- this is basically saying the same thing as my later suggestion RE "explaining why you're reflowing the thumb at 0,0."

> I think the posAtStart & posAtEnd values there will introduce rounding error
> when thumbSize.width is odd, since /2 does a "floor" operation, and we're
> applying that "floor" to the value we're adding on the lower-end, so we end
> up with a range that's larger (by 1 nscoord unit) than we'd like.

I left out a chunk here -- I meant to say:
  ...we're applying that "floor" to the value we're adding on the lower end, **as well as to the value we're subtracting on the upper end**, so we end up with a range that's larger (by 1 nscoord unit) than we'd like.

Comment 6

[Jonathan Watt [:jwatt]]

(In reply to Daniel Holbert [:dholbert] from comment #4)
> It seems a bit hacky / unnecessary to reflow the thumb frame at 0,0 and then
> move it.  I'd sort of rather we called it first (& make it return a nsPoint
> instead of directly setting the position), and then use that point for
> ReflowChild / FinishReflowChild.

We can't. The position that we give the thumb depends on its size (we subtract the size from the range frame's content box), so we need to do the reflow first to get the correct size.

> Probably not a huge deal, though. If we keep things the way they are now,
> you should at least add a brief comment above ReflowChild explaining why
> you're reflowing the thumb at 0,0.

Done.

> I think the posAtStart & posAtEnd values there will introduce rounding error
> when thumbSize.width is odd, since /2 does a "floor" operation, and we're
> applying that "floor" to the value we're adding on the lower-end, so we end
> up with a range that's larger (by 1 nscoord unit) than we'd like.
> 
> This would mean we could end up with "fraction" being greater than 1.0,
> which would be very weird.

Good catch!

> You can address this (and make the math easier-to-understand IMHO) if you
> just set posAtEnd as follows:
>   posAtEnd = posAtStart + traversableDistance;

Agreed.

> I wonder: Should we be passing aRangeSize (the border-box size), or
> rangeContentBoxSize, for the purposes of determining whether we're
> horizontal?
> 
> I'm inclined to think we should be using the *content-box* size, since
> that's what determines the traversable distance for the thumb.  I expect
> authors would be surprised if adding padding around <input> modified whether
> it rendered as a vertical or a horizontal control.
> 
> (I suppose it doesn't matter much until IsHorizontal is sorted out in bug
> 840820, though.

I think I might agree, but that needs to be fixed in various places. I'll add a comment to bug 840820 and we can make the decision there.

> >+      newPosition.y += (rangeContentBoxSize.height - thumbSize.height)/2;
> 
> observation: this will almost certainly be the same as |oldPosition.y|
> (except on the first reflow, or except when we've moved)

Seems like it may be different on subsequent reflows if the height of the range changes. Left as-is.

Comment 7

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8de291574d1a

Thanks for the review, Daniel!

Comment 8

[Daniel Holbert [:dholbert]]

No problem!

Comment-nit on landed patch:
> +    // Where we position of the thumb depends on its size, so we first reflow
> +    // the thumb at {0,0} to obtain its size, then position it afterwards.

Looks like "of" is a typo, in "Where we position of the thumb"

Comment 9

[Daniel Holbert [:dholbert]]

Might be worth adding a test for this, e.g. with a specified-size <input type="range"> covered up by a <div style="fill:lime"> w/ the same content-box-size, and have the thumb at its minimum position in one test & maximum in another test.

With this bug's fix, I'd expect the thumb not to be visible (it'd be entirely behind the lime div); before the fix, I'd expect the thumb would stick out.  So the reference case could just be a lime div.

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8de291574d1a

Comment 11

[Daniel Holbert [:dholbert]]

I pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/57d5c140a8f1 to address the typo in comment 8, FWIW.

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/57d5c140a8f1