Closed
Bug 842158
Opened 12 years ago
Closed 12 years ago
Make <input type="range"> respond to 'direction' property
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla21
People
(Reporter: dholbert, Assigned: jwatt)
References
Details
Attachments
(1 file, 1 obsolete file)
|
2.81 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
|
Assignee | |
Comment 1•12 years ago
|
||
Definitely want to fix this before shipping.
Assignee: nobody → jwatt
|
|
Assignee | |
Comment 2•12 years ago
|
||
Attachment #714958 -
Flags: review?(dholbert)
|
|
Reporter | |
Comment 3•12 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•12 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•12 years ago
|
||
If you think that's not how things should work, can you file a separate bug and explain why?
|
|
Assignee | |
Comment 6•12 years ago
|
||
Attachment #714958 -
Attachment is obsolete: true
Attachment #714958 -
Flags: review?(dholbert)
Attachment #714964 -
Flags: review?(dholbert)
|
|
Reporter | |
Updated•12 years ago
|
Attachment #714964 -
Attachment is patch: true
|
|
Reporter | |
Comment 7•12 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•12 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•12 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).
|
|
Assignee | |
Comment 10•12 years ago
|
||
|
|
Reporter | |
Comment 11•12 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•12 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•12 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•12 years ago
|
||
(not advocating that you do that, just clarifying. The end of comment 9 sounds good.)
|
|
Assignee | |
Comment 15•12 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.
|
||
Comment 16•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/841077d27df8
https://hg.mozilla.org/mozilla-central/rev/16ff7211d7b2
Status: NEW → RESOLVED
Closed: 12 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.
Description
•