Add a ::-moz-range-progress pseudo-element to <input type=range> for Firefox OS

RESOLVED FIXED in mozilla22

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jwatt, Assigned: jwatt)

Tracking

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

20.84 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
(Assignee)

Description

6 years ago
Firefox OS's UI design requires a blue highlight along the range track up to the thumb. In order to support this we'll need to add a ::-moz-range-progress pseudo-element to <input type=range>.
(Assignee)

Comment 1

6 years ago
The UI design for gaia's slider (which is called a seekbar in gaia) can be seen here:

http://telefonicaid.github.com/Gaia-UI-Building-Blocks/index.html#widgets/seekbars/
(Assignee)

Comment 2

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

Review comments, part 1 (the rest coming shortly):

>+++ b/layout/forms/nsRangeFrame.cpp
>+  // Create the progress div:
>+  nodeInfo = doc->NodeInfoManager()->GetNodeInfo(nsGkAtoms::div, nullptr,
>+                                                 kNameSpaceID_XHTML,
>+                                                 nsIDOMNode::ELEMENT_NODE);
>+  NS_ENSURE_TRUE(nodeInfo, NS_ERROR_OUT_OF_MEMORY);
>+  rv = NS_NewHTMLElement(getter_AddRefs(mProgressDiv), nodeInfo.forget(),
>+                         mozilla::dom::NOT_FROM_PARSER);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  // Associate ::-moz-range-progress pseudo-element to the anonymous child.
>+  pseudoType = nsCSSPseudoElements::ePseudo_mozRangeProgress;
>+  newStyleContext =
>+    PresContext()->StyleSet()->ResolvePseudoElementStyle(mContent->AsElement(),
>+                                                         pseudoType,
>+                                                         StyleContext());
>+
>+  if (!aElements.AppendElement(ContentInfo(mProgressDiv, newStyleContext))) {
>+    return NS_ERROR_OUT_OF_MEMORY;
>+  }
>+

So, it looks like this code is copypasted in sequence 3 times now. Can we switch it to use a helper function? :)

(I suggested this back in bug 838256, when this code was copypasted twice, and you said "This is going to be changed to meet some B2G requirements in a later patch".  I imagine this patch here is that later patch? And it looks like this follows the same pattern as the other helper-frames, so it can use the same helper-func, right?

>@@ -260,16 +286,42 @@ nsRangeFrame::ReflowAnonymousContent(nsP
>+  if (progressFrame) { // display:none?
>+    nsHTMLReflowState progressReflowState(aPresContext, aReflowState,
>+                                          progressFrame,
>+                                          nsSize(aReflowState.ComputedWidth(),
>+                                                 NS_UNCONSTRAINEDSIZE));
>+
>+    // We first reflow the thumb at {0,0} to obtain its unadjusted dimensions,
>+    // then we adjust it to so that the appropriate edge ends at the thumb.

This comment looks like it wants to mention "progress frame" instead of "thumb", right?

>+void
>+nsRangeFrame::DoUpdateProgressFrame(nsIFrame* aProgressFrame,
>+                                    const nsSize& aRangeSize)
>+{
>+  MOZ_ASSERT(aProgressFrame);
>+
>+  // The idea here is that we want to position the progress element

It's probably better not to call this "the progress element", since there is already such thing as a <progress> element.

"progress frame" isn't really any better, because there's also nsProgressFrame.

I can't think of a better name at the moment, but we should address this before landing to avoid confusion.  For the rest of this review, I'll use your term "progress element" for convenience though.

>+  // center line running along its length is on the corresponding center line
>+  // of the nsRangeFrame's content box. The position and size of its other
>+  // dimension is handled automatically so that the "start" edge of its border
>+  // box is on the corresponding edge of the nsRangeFrame's content box, and it
>+  // is given a size that is GetValueAsFractionOfRange() times the
>+  // corresponding size of the content box.


So, I'd clarify/tweak a few things in that comment
 - The thing that ends up being GetValueAsFractionOfRange() times that size is the progress element's *border box*
 - It'd be worth specifying that the content-box mentioned in the last line is for the nsRangeFrame
 - Also, I don't think "is handled automatically" really makes sense.

Maybe replace the chunk starting with "The position and size of its other dimension" with something like the following:
  // In the other dimension, we align the "start" edge of the progress
  // element's border-box with the corresponding edge of the nsRangeFrame,
  // and we size the progress element's border-box to have a length of
  // GetValueAsFractionOfRange() times the nsRangeFrame's content-box size.

Or something like that. (modulo a possible "progress element" rename)
Comment on attachment 729616 [details] [diff] [review]
patch

review comments, part 2 of 2:
>+void
>+nsRangeFrame::DoUpdateProgressFrame(nsIFrame* aProgressFrame,
>+                                    const nsSize& aRangeSize)
>+{
[...]
>+  nsMargin borderAndPadding = GetUsedBorderAndPadding();
>+  nsSize progSize = aProgressFrame->GetSize();
>+  nsRect progRect(borderAndPadding.left, borderAndPadding.top,
>+                  progSize.width, progSize.height);
>+
>+  nsSize rangeContentBoxSize(aRangeSize);
>+  rangeContentBoxSize.width -= borderAndPadding.LeftRight();
>+  rangeContentBoxSize.height -= borderAndPadding.TopBottom();
[...]
>+  aProgressFrame->SetRect(progRect);
>+}

I think we should refactor this into a helper-function, too, though it doesn't necessarily have to happen in this bug.

 We now have 3 chunks of code that all basically do:
 (a) Find the child's centered position in the "skinny" axis.
 (b) Find the position/size in the "wide" axis.

Part (a) is identical for all three child frames, AFAIK.
Part (b) is different, but it uses some of the same inputs & logic (i.e. both progress and thumb need to compute the center of the thumb frame for this), so it could potentially share code as well.
Plus, the structure of this code is (or can be) the same for all three children.

So: I'd love for this logic to move to a method like this:

  // Computes the size and position of a child pseudo-element's border-box.
  nsRect
  nsRangeFrame::ComputeChildRect(nsSize aRangeFrameSize,
                                 nsSize aChildTentativeSize,
                                 ChildType aChildType)
  {
    nsRect childRect;

    if (IsHorizontal()) {
      // Center child, vertically, in the range frame.
      childRect.height = aChildComputedSize.height;
      childRect.y = (rangeContentBoxSize.height - childSize.height)/2;

      // Size & position child in horizontal axis. This differs based on aChildType.
      switch(aChildType) {
        ...(childType-specific stuff)...
      }
    } else {
      // Center child, horizontally, in the range frame:
      ...
      // Position child in vertical axis based on aChildType:
      switch(aChildType) {
        ...(childType-specific stuff)
      }
    }
    return childRect;
  }

That doesn't necessarily have to happen in this bug, but if you don't do it here, please file a followup on consolidating this logic, assuming you agree that it's a good idea.  Now that there are chunks of logic that are semi-duplicated 3x here (for the three child frame types), this consolidation is looking fairly attractive / important.

>+++ b/layout/forms/nsRangeFrame.h
>   /**
>    * Helper to reposition the thumb and schedule a repaint when the value of
>    * the range changes. (This does not reflow, since the position and size of
>    * the thumb do not affect the position or size of any other frames.)
>    */
>-  void UpdateThumbPositionForValueChange();
>+  void UpdateForValueChange();

Mention in the comment that the position and size of the progress pseudo-element may change here, as well (not just the thumb frame), and it also doesn't affect the position/size of other frames.

>   /**
>+   * The div used to show the progress pseudo-element.
>+   * @see nsRangeFrame::CreateAnonymousContent

Add another line here explaining what this pseudo-element is for. ("Thumb" and "track" are pretty self-explanatory on a slider, but "progress pseudo-element" is not, particularly given that many sliders don't do anything special to paint the progress.)

Maybe add this after the first line (after a comma):
  * which is used to (optionally) style the specific chunk
  * of track leading up to the thumb's current position.

>+++ b/layout/style/forms.css
>+::-moz-range-progress {
>+  /* Prevent styling that would change the type of frame we construct. */
>+  display: inline-block !important;
>+  float: none !important;
>+  position: static !important;
>+  /* Since one of width/height will be ignored, this just sets the "other"
>+     dimension.
>+   */
>+  width: 0.2em;
>+  height: 0.2em;
>+  /* Prevent nsFrame::HandlePress setting mouse capture to this element. */
>+  -moz-user-select: none ! important;
>+  /* This pseudo-element is hidden by default. We use 'visibility' to hide it
>+     since we want display to always be inline-block.
>+   */
>+  visibility: hidden;

If you turn off "visibility:hidden", does this paint anything? It looks like it won't.  If not, then we should drop it -- it'll just be one additional barrier to styling this element, for those that want to style it.
Comment on attachment 729616 [details] [diff] [review]
patch

(r=me with the above all addressed)
Attachment #729616 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] from comment #4)
> So: I'd love for this logic to move to a method like this:
> 
>   // Computes the size and position of a child pseudo-element's border-box.
>   nsRect
>   nsRangeFrame::ComputeChildRect(nsSize aRangeFrameSize,
>                                  nsSize aChildTentativeSize,
>                                  ChildType aChildType)
>   {
[...]
>       childRect.height = aChildComputedSize.height;

(Sorry, I meant "aChildTentativeSize" there (not aChildComputedSize). I renamed that param in my pseudo-code without renaming its usage.)
(Assignee)

Comment 7

6 years ago
Created attachment 729828 [details] [diff] [review]
patch
Attachment #729616 - Attachment is obsolete: true
Attachment #729828 - Flags: review?(dholbert)
(Assignee)

Comment 8

6 years ago
(In reply to Daniel Holbert [:dholbert] from comment #3)
> So, it looks like this code is copypasted in sequence 3 times now. Can we
> switch it to use a helper function? :)
> 
> (I suggested this back in bug 838256, when this code was copypasted twice,
> and you said "This is going to be changed to meet some B2G requirements in a
> later patch".  I imagine this patch here is that later patch?

Indeed. Done.

(In reply to Daniel Holbert [:dholbert] from comment #4)
> I think we should refactor this into a helper-function, too, though it
> doesn't necessarily have to happen in this bug.
> 
>  We now have 3 chunks of code that all basically do:
>  (a) Find the child's centered position in the "skinny" axis.
>  (b) Find the position/size in the "wide" axis.

Yes, I had a go at that, but it got a bit messy because they're doing subtly different things. I'll file a follow-up.
(Assignee)

Comment 9

6 years ago
(In reply to Jonathan Watt [:jwatt] from comment #8)
> Yes, I had a go at that, but it got a bit messy because they're doing subtly
> different things. I'll file a follow-up.

Filed bug 855101.
Comment on attachment 729828 [details] [diff] [review]
patch

>+++ b/layout/forms/nsRangeFrame.h
>   /**
>-   * The div used to show the track.
>+   * The div used to show the track, which is used to (optionally) style the
>+   * specific chunk of track leading up to the thumb's current position.
>    * @see nsRangeFrame::CreateAnonymousContent
>    */
>   nsCOMPtr<nsIContent> mTrackDiv;
> 
>   /**
>+   * The div used to show the progress pseudo-element.
>+   * @see nsRangeFrame::CreateAnonymousContent
>+   */
>+  nsCOMPtr<nsIContent> mProgressDiv;
>+
>+  /**

Looks like you edited the wrong comment there. (the track one, instead of the progress one)

r=me with that fixed.
Attachment #729828 - Flags: review?(dholbert) → review+
(Assignee)

Comment 11

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e912de49c12b

(In reply to Daniel Holbert [:dholbert] from comment #10)
> Looks like you edited the wrong comment there. (the track one, instead of
> the progress one)

Thanks for catching that, and thanks for the review, Daniel! :)
Sure!

Also: It slipped my mind before, but now I'm realizing that this should've included some tests, too...
Flags: in-testsuite?
Also, this ended up failing some assertions & needed a backout. :(

Backout:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/cee46001a526

The assertions were in debug crashtests & mochitest-5 (so far). e.g.:
https://tbpl.mozilla.org/php/getParsedLog.php?id=21131473&full=1&branch=mozilla-inbound

At least in the crashtest run, it looks like this:
{
16:59:20     INFO -  REFTEST TEST-START | file:///builds/slave/talos-slave/test/build/tests/reftest/tests/content/html/content/crashtests/838256-1.html | 232 / 2323 (9%)
16:59:20     INFO -  ###!!! ASSERTION: Please remove this from the document properly: '!IsInDoc()', file ../../../../content/base/src/FragmentOrElement.cpp, line 630
16:59:34     INFO -  mozilla::dom::HTMLDivElement::~HTMLDivElement() [obj-firefox/dist/include/mozilla/mozalloc.h:225]
16:59:34     INFO -  nsNodeUtils::LastRelease(nsINode*) [content/base/src/nsNodeUtils.cpp:260]
}
https://tbpl.mozilla.org/php/getParsedLog.php?id=21131473&full=1&branch=mozilla-inbound#error0
(The header at the top of the log attributes it to the wrong test -- the test after the bug has happened.  838256-1.html is the test that actually triggers it)

I'm 90% sure we just need a nsContentUtils::DestroyAnonymousContent() call for the progress pseudo-element's frame, but without a Try run here (and with the current several-pages-long mozilla-inbound backlog), I'm not confident enough that this will be 100% green to just push a bustage fix and call it good, hence the backout.
(philor says this failed assertions in other testsuites as well -- here's a link to its TBPL cycle, for reference: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=e912de49c12b )

jwatt: before you re-land, could you add a reftest for this?  We just need something to test that the range-progress actually renders, when it's styled, really.

e.g. testcase could be an <input type="range"> elem with a nonzero thumb position & w/ explicit style for ::-moz-range-progress, and the reference could be the same <input> element, but instead of having a specially-styled progress pseudo-elem, just stick an abspos div with similar styling over the <input>.
Two other cases worth testing:
* Testcase: <input type="range" value="0"> w/ styled progress-pseudo-element
  Reference: same, but w/out the styling
(The testcase's range-progress frame should have 0 width, so it shouldn't affect the rendering)

* Testcase: <input type="range" value="100"> w/ styled progress-pseudo-element
  Reference: same, but w/ the styling applied instead to the track-pseudo-element
(The range-progress frame should stretch all the way across, so it should cover the full track, and it should be equivalent to just restyling the track.)
Blocks: 855149
(Assignee)

Comment 16

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/805aff291ec2

I'm reluctant to write too many tests for this until the styling has solidified a bit more, but I've added the three tests that you requested. Note that the added complexity in the tests is because things are not quite as simple as you imply in comment 14 and 15. For example, slapping an abspos div on top wouldn't work because it needs to come between the track and the thumb. Also the ::-moz-range-progress is not completely hidden at 0% and does not completely hide the track at 100%, because at 0% it still extends to the middle of the thumb, and at 100% it doesn't extend to the right content edge of the range element for the same reason. Anyway, hopefully the tests I've written handle that in a way that isn't too likely to break in the face of future default style changes.
Gotcha -- thanks for the explanation on testability & for writing those tests!  Looks like it stuck this time -- yay. :)

(I'm not concerned with needing many tests -- I just wanted to make sure we have some baseline test-coverage for this, to verify that we don't have or introduce a bug that makes the range-progress frame refuse to paint at all, or paint 10px too high, or something like that.  Since it's hidden by default, we otherwise would have no way of detecting rendering issues from this bug's code w/ our existing tests.)
Flags: in-testsuite? → in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/805aff291ec2
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.