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

RESOLVED FIXED in mozilla21

Status

()

Core
Layout
RESOLVED FIXED
5 years ago
5 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)

(Reporter)

Description

5 years ago
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.
(Reporter)

Updated

5 years ago
Blocks: 344618
No longer blocks: 838256
(Reporter)

Updated

5 years ago
Depends on: 838256
(Assignee)

Comment 1

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

Comment 2

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

Comment 3

5 years ago
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

5 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

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

Comment 6

5 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)
(Reporter)

Updated

5 years ago
Attachment #714964 - Attachment is patch: true
(Reporter)

Comment 7

5 years ago
(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.
(Reporter)

Comment 8

5 years ago
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

5 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).
(Reporter)

Comment 11

5 years ago
(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.)
(Reporter)

Comment 12

5 years ago
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)
(Reporter)

Comment 13

5 years ago
(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
(Reporter)

Comment 14

5 years ago
(not advocating that you do that, just clarifying. The end of comment 9 sounds good.)
(Assignee)

Comment 15

5 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: 5 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.