Closed Bug 841942 Opened 12 years ago Closed 2 years ago

Consider displaying tick marks for <input type=range> when @list/<datalist> is used

Categories

(Core :: Layout: Form Controls, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
109 Branch
Accessibility Severity s4
Tracking Status
firefox109 --- fixed

People

(Reporter: jwatt, Assigned: zrhoffman, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: access, dev-doc-complete, Whiteboard: , [wptsync upstream])

Attachments

(2 files, 1 obsolete file)

We should figure out what we want to do for @list support for <input type=range>, if anything.
Blocks: 841950
Blocks: 344618
Everyone should feel free to comment on what we should be doing for @list/<datalist> with <input type=range> and how much content authors would care about having this bug fixed relative to other things we could be doing. Also please do CC anyone you think might have an opinion.

I don't think fixing this should block us shipping <input type=range>. (To my mind it would be better to have this as an enhancement at some later date, assuming it's even worth working on this rather than other things on our list of high priority work.) If anyone disagrees, please do speak up.
My concern is that there is no way to feature-detect this for a specific type. Except that, I agree it iss more a nice to have than a must have. I think we should at least have a look and see how hard implementing this would be.
What does "implementing this" involve? What is the desired behavior in your opinion?
(In reply to Jonathan Watt [:jwatt] from comment #3)
> What does "implementing this" involve? What is the desired behavior in your
> opinion?

Would say that the minimum is:
- showing a tick for each valid values in the associated datalist;

Maybe there could be:
- have a "magnet effect" when the user is around those values;
- show the values (the numbers in the UI).

I would be perfectly fine with simply the ticks being shown.
This sounds like it could get very involved.

* We'd need UI design for each individual native theming backend,
  and -moz-appearance:none, for major and minor ticks, and rendering
  of the data point labels.

* We'd (presumably) need to provide some sort of pseudo elements for
  long and short ticks, yet those elements wouldn't really be
  reflowed and rendered themselves, but rather would have to be used
  as templates for repeating, somehow.

* We'd need to decide what should happen when some of the data points
  in the list are too close together for the available space. We'd
  need logic to measure tick metrics for the various themes and for
  unthemed styled pseudo elements, detect when this is the case and
  act "appropriately", whatever that means.

* Laying out and painting data point labels at the various ticks
  would make the above even more complex given they would add to
  the metrics, layout and fitting issue.

* We'd have to take transforms into account when figuring out when
  ticks and labels are "too close" to visually make sense, and
  acting on that.

* If we have a "magnet effect", how does that interact with @step
  and the clamp-to-values that @step introduces, especially when
  the @step and @list data points conflict?

* Throw vertical vs horizontal range into the mix for even more
  fun.

* A range with ticks [and labels] is going to take up quite a bit
  of space - what are the layout rules for positioning and alignment
  of the whole thing, and how does that fit into the flow of a page?

Those are just the issues I can think of off the top of my head. Even without including labels in the UI, this sounds like it could take quite a lot of time to hash out the details for, and then another significant chunk of time to implement.

It's worth mentioning that none of the other browsers seem to do anything with datalist for type=range (probably because of the complexities listed above, or maybe because sliders with ticks look ugly).

I've also not seen anyone asking for this when I was researching type=range before starting this project FWIW.
Lack of feature-detection does not strike me as a major problem. Others shipped this without <datalist> support too. And not having that support degrades gracefully.
Ok, you win :)
No longer blocks: 841950
Blocks: input-range
No longer blocks: 344618
Severity: normal → enhancement
Summary: Figure out what we want to do for @list support for <input type=range> → Consider displaying tick marks for <input type=range> when @list/<datalist> is used
Happy 2017 all!

Speaking from the perspective of creating data visualizations - a basic slider with tick marks would really enhance the usability of the range <input>.  In the interim years since this bug came up, looks like gecko/webkit as well as ie/edge show tick marks in this use case.

In my view, tick marks don't need to be major/minor -- simply rendering a tick mark for every <option> in the <datalist> would be tremendously helpful and consistent with other browsers.
Currently, HTML range inputs are not very useful because of the lack of any kind of gradiation. I think it's pretty rare that these tick marks aren't a beneficial feature; indeed, having at least some tick marks which are also labeled is a big help. Although type "range" is meant to be used when precision isn't important, realistically, there are times when users are going to be trying to achieve something resembling a specific value. Such as trying to hit 50% volume so that your track is the same volume as the others. Or aiming for about 60% transparency when adjusting an rgba color using a set of sliders in a graphic editor. You might get sort of in the area, but knowing you're on the dot for key or common values is a big help.

Anyway -- the real reason I'm commenting: I've set dev-doc-needed on this bug since if it's ever fixed, docs will need updating to note that Firefox supports it.
Keywords: dev-doc-needed
I haven't seen this mentioned, but Chrome supports this now. I don't know for how long.
Priority: -- → P3
Edge/Chrome/Safari all support this now.  I'm moving this to Layout since
I suspect it's primarily Layout work to implement this.
Component: DOM: Core & HTML → Layout: Form Controls
Attached file Testcase #1
No UA seems to support labels yet though.  (The URL I mentioned earlier
uses a polyfill so it's misleading for testing support for this feature.)

(In reply to Mats Palmgren (:mats) from comment #14)

No UA seems to support labels yet though. (The URL I mentioned earlier
uses a polyfill so it's misleading for testing support for this feature.)

MDN suggests Chrome 66 supports labels if the datalist is styled:
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/range#A_range_control_with_hash_marks_and_labels

Version 66 (66.0.3359.181) of Chrome supports labels but the <datalist> tag has to be styled with CSS as its display property is set to none by default, hiding the labels.

This impacts accessibility because it's much easier to ensure good a11y if authors use native widgets. The longer we don't support things like this, the more custom (and probably inaccessible) implementations will proliferate.

Note that if/when the layout part of this is implemented, we'll need code in the a11y module to support this as well.

Related Twitter thread: https://twitter.com/AmeliasBrain/status/1225270470372556801

Keywords: access
Whiteboard: [access-p3]

Updating the Accessibility Team's impact assessment to conform with the new triage guidelines. See https://wiki.mozilla.org/Accessibility/Triage for descriptions of these whiteboard flags.

Whiteboard: [access-p3] → [access-s4]

Any updates on this? As a developer this is pretty frustrating, since I have to build a long workaround just to get this to work similarly on Firefox

It would be useful.

Severity: normal → S3

(In reply to one.snapp8 from comment #18)

Any updates on this? As a developer this is pretty frustrating, since I have to build a long workaround just to get this to work similarly on Firefox

Agreed 100%

Chrome at least seems to tie this to their native appearance code. So this should be reasonably fixable for someone that knows C++ by changing Theme::PaintRange here

Mentor: emilio

I'd like to pick up bug 841942. Can I please be assigned it?

Feel free to send a patch, you should be auto-assigned afterwards.

Attached patch 841942-tick-marks.patch (obsolete) — Splinter Review

Thanks, patch sent!

What test coverage would be appropriate? Since Chrome and WebKit's tick marks are on different sides of the range input, a WPT reftest might need to be user agent-specific.

Attachment #9304960 - Flags: review?(emilio)
Comment on attachment 9304960 [details] [diff] [review]
841942-tick-marks.patch

Review of attachment 9304960 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this looks on the right track, see comments below.

Can you send the patch using Phabricator? See https://firefox-source-docs.mozilla.org/devtools/contributing/making-prs.html

Regarding testing, at least a couple "negative" reftests (so testing that `<input type=range list=mylist>` renders differently to `<input type=range>`, that two different lists render differently) should be doable. We can use `<link rel="mismatch" href="should-render-differently.html">` for that.

Thanks again!

::: layout/forms/nsRangeFrame.h
@@ +126,5 @@
> +   * minimum and its maximum (i.e. returns 0.0 when the value is the same as the
> +   * input's minimum, and returns 1.0 when the value is the same as the input's
> +   * maximum).
> +   */
> +  double GetDoubleAsFractionOfRange(mozilla::Decimal value);

We could consider to add an `mozilla::dom::HTMLInputElement& InputElement();` helper here, to avoid having so many static_cast<>s  spread around. See https://searchfox.org/mozilla-central/rev/05f486c76f84830041e51b7c68e3e697dc768454/layout/forms/nsComboboxControlFrame.h#211 for a reference.

::: widget/Theme.cpp
@@ +840,5 @@
>    }
>  
> +  nsIContent* mContent = rangeFrame->GetContent();
> +  MOZ_ASSERT(mContent->IsHTMLElement(nsGkAtoms::input), "bad cast");
> +  dom::HTMLInputElement* input =

nit: `auto* input =` probably.

@@ +842,5 @@
> +  nsIContent* mContent = rangeFrame->GetContent();
> +  MOZ_ASSERT(mContent->IsHTMLElement(nsGkAtoms::input), "bad cast");
> +  dom::HTMLInputElement* input =
> +      static_cast<dom::HTMLInputElement*>(rangeFrame->GetContent());
> +  auto* list = static_cast<HTMLDataListElement*>(input->GetList());

Let's fix `GetList()` so that it returns the right type if possible. Just changing the return type and changing this code:

https://searchfox.org/mozilla-central/rev/05f486c76f84830041e51b7c68e3e697dc768454/dom/html/HTMLInputElement.cpp#1659-1664

By:

return HTMLDataListElement::FromNodeOrNull(docOrShadow->GetElementById(dataListId));


should do.

@@ +854,5 @@
> +        continue;
> +      }
> +      nsAutoString str;
> +      option->GetValue(str);
> +      nsresult ec;

nit: `rv` is the usual naming for this. Also, can we move all this block of code to its own function that either returns an nsTArray<double>, or takes an `nsTArray<double>&` and fills it up?

@@ +855,5 @@
> +      }
> +      nsAutoString str;
> +      option->GetValue(str);
> +      nsresult ec;
> +      double tickMark = PromiseFlatString(str).ToDouble(&ec);

Why do you need `PromiseFlatString`?

@@ +859,5 @@
> +      double tickMark = PromiseFlatString(str).ToDouble(&ec);
> +      if (NS_FAILED(ec)) {
> +        continue;
> +      }
> +      tickMarks.AppendElement(tickMark);

Hmm, we should invalidate paint when options change... That might be a bit tricky, might be worth doing it as a follow-up.

@@ +928,5 @@
> +                                 Decimal::fromDouble(tickMark));
> +    tickMarkRects.AppendElement(
> +        LayoutDeviceRect(scaledTickMark.X() - tickMarkBox.X() / 2,
> +                         scaledTickMark.Y() - tickMarkBox.Y() / 2,
> +                         tickMarkBox.X(), tickMarkBox.Y()));

This is turning CSS pixels into device pixels without multiplying by the appropriate scale. That seems wrong? Let's use the right units in tickMarkOffsets etc.

::: widget/Theme.h
@@ +117,5 @@
>                                                           OutlineCoversBorder);
>    std::pair<sRGBColor, sRGBColor> ComputeRangeProgressColors(
>        const ElementState&, const Colors&);
> +  std::tuple<sRGBColor, sRGBColor, sRGBColor> ComputeRangeTrackColors(
> +      const ElementState&, const Colors&);

Let's document the new color :)
Attachment #9304960 - Flags: review?(emilio)
Assignee: nobody → zach
Status: NEW → ASSIGNED
Attachment #9304960 - Attachment is obsolete: true
Attachment #9304967 - Attachment description: Bug 841942 - Display tick marks for <input type=range> when <datalist> is used r=emilio! → Bug 841942 - Display tick marks for <input type=range> when @list/<datalist> is used r=emilio!
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/79dd2b51b7b6
Display tick marks for <input type=range> when @list/<datalist> is used r=emilio,credential-management-reviewers,sgalich
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/37216 for changes under testing/web-platform/tests
Whiteboard: [access-s4] → [access-s4], [wptsync upstream]
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 109 Branch
Regressions: 1803108
Upstream PR merged by moz-wptsync-bot
Blocks: 1803118
Blocks: 1803303

Docs updates can be tracked here https://github.com/mdn/content/issues/22892

While documenting feature I came across an keyboard errro which I reported here

Whiteboard: [access-s4], [wptsync upstream] → , [wptsync upstream]
Accessibility Severity: --- → s4
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: