Make <input type="range"> respond to 'direction' property

RESOLVED FIXED in mozilla21

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: dholbert, Assigned: jwatt)

Tracking

Trunk
mozilla21
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

HTML5 spec says:

> User agents are expected to use the used value of the 'direction' property on
> the element to determine the direction in which the slider operates. Typically,
> a left-to-right ('ltr') horizontal control would have the lowest value on the
> left and the highest value on the right, and vice versa.

http://www.whatwg.org/specs/web-apps/current-work/multipage/rendering.html#the-input-element-as-a-range-control

I don't think the <input type="range"> work landed so far (in e.g. bug 838256) pays attention to 'direction'.

Filing this bug on implementing that part of the spec.
Blocks: 344618
No longer blocks: 838256
Depends on: 838256
(Assignee)

Comment 1

6 years ago
Definitely want to fix this before shipping.
Assignee: nobody → jwatt
(Assignee)

Comment 2

6 years ago
Created attachment 714958 [details] [diff] [review]
patch
Attachment #714958 - Flags: review?(dholbert)
Comment on attachment 714958 [details] [diff] [review]
patch

># HG changeset patch
># Parent f59bdcd8476919aab9136bb1397cc96eed69194a
># User Jonathan Watt <jwatt@jwatt.org>
>Bug 842158 - Make <input type=range> honor the 'direction' property. r=dholbert.
>
>diff --git a/layout/forms/nsRangeFrame.cpp b/layout/forms/nsRangeFrame.cpp
>--- a/layout/forms/nsRangeFrame.cpp
>+++ b/layout/forms/nsRangeFrame.cpp
>@@ -264,16 +264,19 @@ nsRangeFrame::ReflowAnonymousContent(nsP
> 
>     // Find the x/y position of the thumb frame such that it will be positioned
>     // as described above. These coordinates are with respect to the
>     // nsRangeFrame's border-box.
>     nscoord thumbX, thumbY;
> 
>     if (isHorizontal) {
>       thumbX = NSToCoordRound(rangeFrameContentBoxWidth * valueAsFraction);
>+      if (StyleVisibility()->mDirection == NS_STYLE_DIRECTION_RTL) {
>+        thumbX = rangeFrameContentBoxWidth - thumbX;

You need to subtract the width of the thumb frame, too, right?  thumbX is the upper-left corner of the thumb.  When valueAsFraction == 0, I'm pretty sure this current patch would position the thumb barely-outside of the range frame (instead of barely-inside, which is what we want) when we're at the minimum value.

Also, needs a testcase.
(Assignee)

Comment 4

6 years ago
(In reply to Daniel Holbert [:dholbert] from comment #3)
> You need to subtract the width of the thumb frame, too, right?

Not as things stand currently, no. For values of 0 and 100 the center of the thumb is placed directly on the edge of the nsRangeFrame's content box.
(Assignee)

Comment 5

6 years ago
If you think that's not how things should work, can you file a separate bug and explain why?
(Assignee)

Comment 6

6 years ago
Created attachment 714964 [details] [diff] [review]
patch + test
Attachment #714958 - Attachment is obsolete: true
Attachment #714958 - Flags: review?(dholbert)
Attachment #714964 - Flags: review?(dholbert)
Attachment #714964 - Attachment is patch: true
(In reply to Jonathan Watt [:jwatt] from comment #4)
> Not as things stand currently, no. For values of 0 and 100 the center of the
> thumb is placed directly on the edge of the nsRangeFrame's content box.

Ah, right -- that makes sense.

That does mean that (w/out padding) the thumb extends outside the input element's border-box, which seems a bit odd to me. We should probably add some padding to fix that. I'll mention that over in bug 842021, which is probably where that should happen.
Comment on attachment 714964 [details] [diff] [review]
patch + test

>+++ b/layout/reftests/forms/input/range/input-range-direction-1.html
>@@ -0,0 +1,7 @@
>+<!DOCTYPE html>
>+<html>
>+  <!-- Test: that direction:rtl behaves connectly -->
>+  <body>
>+    <input type='range' value=70 style="-moz-appearance:none; direction:rtl;">
>+  </body>
>+</html>

It's probably worth adding a copy of this test without "-moz-appearance:none" in the testcase/reference, because once we support native styling for these elements, we'll want to make sure that this works there, too. (and in the meantime, it'll just re-test the existing behavior, I think)

Maybe that's premature, though, because we'll probably want to make native-themed versions of all of our /range reftests, and maybe we'll want to just copy them all into a new "range-themed" directory or something.

Anyway, r=me
Attachment #714964 - Flags: review?(dholbert) → review+
(Assignee)

Comment 9

6 years ago
(In reply to Daniel Holbert [:dholbert] from comment #8)
> It's probably worth adding a copy of this test without
> "-moz-appearance:none" in the testcase/reference

That doesn't really work at the moment because we get some generic 'input' styling. Once -moz-appearance:range et. al. are added (later patches) and that's the default set in forms.css, then it will make more sense.

> Maybe that's premature, though, because we'll probably want to make
> native-themed versions of all of our /range reftest

Right. That's what I'm planning on doing (and why I've named all the existing tests with the 'unthemed' label).
(In reply to Daniel Holbert [:dholbert] from comment #7)
> That does mean that (w/out padding) the thumb extends outside the input
> element's border-box, which seems a bit odd to me. We should probably add
> some padding to fix that. I'll mention that over in bug 842021

(I ended up filing a helper-bug -- bug 842179 -- so as not to clutter up bug 842021 w/ that specific piece of discussion right off the bat.)
Comment on attachment 714964 [details] [diff] [review]
patch + test

>+++ b/layout/reftests/forms/input/range/input-range-direction-1.html
>@@ -0,0 +1,7 @@
>+<!DOCTYPE html>
>+<html>
>+  <!-- Test: that direction:rtl behaves connectly -->

late-breaking nit: s/n/r/ in "connectly"

(You could land that as a DONTBUILD followup)
(In reply to Jonathan Watt [:jwatt] from comment #9)
> That doesn't really work at the moment because we get some generic 'input'
> styling.

(Are you sure? It looks like the first declaration in forms.css for input[type="range"] is actually "-moz-appearance: none"* so I'd expect that you could remove that from the testcase and it'd still render identically.)

* https://hg.mozilla.org/mozilla-central/annotate/5e660787834c/layout/style/forms.css#l720
(not advocating that you do that, just clarifying. The end of comment 9 sounds good.)
(Assignee)

Comment 15

6 years ago
(In reply to Daniel Holbert [:dholbert] from comment #12)
> late-breaking nit: s/n/r/ in "connectly"

https://hg.mozilla.org/integration/mozilla-inbound/rev/16ff7211d7b2

(In reply to Daniel Holbert [:dholbert] from comment #13)
> (Are you sure? It looks like the first declaration in forms.css for
> input[type="range"] is actually "-moz-appearance: none"

Oh, right. I forgot I did that to avoid that default 'input' styling. Anyway, I'd rather land the native themed tests later once we support native theming.
https://hg.mozilla.org/mozilla-central/rev/841077d27df8
https://hg.mozilla.org/mozilla-central/rev/16ff7211d7b2
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.