Closed Bug 842158 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla21

People

(Reporter: dholbert, Assigned: jwatt)

References

Details

Attachments

(1 file, 1 obsolete file)

patch + test
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.
Blocks: 344618
No longer blocks: 838256
Depends on: 838256
Definitely want to fix this before shipping.
Assignee: nobody → jwatt
Attached patch patch (obsolete) — — Splinter Review 

        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.

    
    

    
  
(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.

    
    

    
  
If you think that's not how things should work, can you file a separate bug and explain why?

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch + test
        — 
          — Splinter Review
      

    


  

        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+

    
    

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

    
    

    
  
(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
Closed: 10 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.