Spinning this out from bug 344618.

As discussed with dbaron and dholbert, requesting review from dholbert. For now this patch hardcodes nsRangeFrame::IsHorizontal to return true until I can get agreement from the various invested parties on how we should allow authors to get a vertical range. (The logic to support vertical range is in place and just needs the ifdef'ed out code in nsRangeFrame::IsHorizontal to be enabled.) dholbert: you can ignore the XXX commented parts. They are just there to remind me of various things, and are fixed later.

Partial review: > double >+nsHTMLInputElement::GetValueAsFractionOfRange() >+{ >+ // Should only be used for <input type='range'> for the moment. >+ MOZ_ASSERT(mType == NS_FORM_INPUT_RANGE); >+ >+ double value = GetValueAsDouble(); >+ double minimum = GetMinimum(); >+ double maximum = GetMaximum(); It's probably worth asserting that DoesMinMaxApply() is true before you call GetMinimum()/GetMaximum(), per those methods' header documentation. (Given that we're only expecting to hit this code for <input type='range'>, it seems like we should be safe to assert about DoesMinMaxApply, instead of explicitly checking it.) >+ MOZ_ASSERT(MOZ_DOUBLE_IS_FINITE(value) && >+ MOZ_DOUBLE_IS_FINITE(minimum) && >+ MOZ_DOUBLE_IS_FINITE(maximum), >+ "type=range should have a default maximum/minimum"); Can we really assert that these values are finite? It seems like it could be possible for web content to run afoul of this assertion, by specifying a sufficiently-huge minimum or maximum value, such that ConvertStringToNumber produces the double-value for positive or negative infinity. >+ if (maximum <= minimum) { >+ MOZ_ASSERT(value == minimum); Add a message for why we expect this to hold. (Maybe "Unsanitized value", like below, or something like that, since it looks like this is checking whether we've properly clamped our value.) >diff --git a/content/html/content/src/nsHTMLInputElement.h b/content/html/content/src/nsHTMLInputElement.h >--- a/content/html/content/src/nsHTMLInputElement.h >+++ b/content/html/content/src/nsHTMLInputElement.h >@@ -279,16 +279,54 @@ public: [...] >+ double GetMinimum() const; [...] >+ double GetMaximum() const; [...] >+ double GetValueAsDouble() const; It looks like you're making these public, but I don't think you need to (at least, not for this bug)... Their only new caller in this patch is GetValueAsFractionOfRange(), which is in the same class, so it's already got access to these. So, I'd assume they can stay protected. >+ /** >+ * Returns the value as a fraction of the difference between the input's >+ * minimum and its maximum (i.e. returns 0.0 when the value is the same as >+ * the minimum, and returns 1.0 when the value is the same as the maximum). >+ * >+ * NOTE: Only usable with input types that have a default minimum and >+ * maximum. >+ */ >+ double GetValueAsFractionOfRange(); The NOTE at the end seems overly narrow -- I don't see why having a "default minimum & maximum" would really be necessary for this function. In theory, I could call this on some <input> as long it's got min & max attributes set, right? It doesn't necessarily need to have *default* min/max. Might want to replace the NOTE with something like: * NOTE: Only usable for input type='range', for now. (since you do assert that in the impl currently, and I think that's really what you're getting at w/ the "default minimum and maximum" thing) Or, if you want to be more generic, you could say something like: * NOTE: This method assumes that GetMinimum(), GetMaximum(), and * GetValueAsDouble() will all return finite values. (modulo your feedback on whether we can safely assume these methods return finite values, per my comment above about the assert.) >+++ b/layout/forms/nsRangeFrame.cpp [...] >+#include "nsContentCreatorFunctions.h" >+#include "nsContentUtils.h" >+#include "nsFormControlFrame.h" >+#include "nsContentList.h" >+#include "nsFontMetrics.h" >+#include "mozilla/dom/Element.h" >+#include "nsContentList.h" nit: you've got nsContentList.h listed twice there. Delete that latter one (and maybe shift the earlier one up to hang out with the other nsContentXYZ.h includes) >+NS_IMPL_FRAMEARENA_HELPERS(nsRangeFrame) >+ >+nsRangeFrame::nsRangeFrame(nsStyleContext* aContext) >+ : nsContainerFrame(aContext) >+ , mTrackDiv(nullptr) >+ , mThumbDiv(nullptr) Nit: technically you don't need to explicitly initialize mTrackDiv and mThumbDiv to nullptr -- the nsCOMPtr constructor does that for you. This is fine either way, though. >+void >+nsRangeFrame::AppendAnonymousContentTo(nsBaseContentList& aElements, >+ uint32_t aFilter) >+{ >+ aElements.MaybeAppendElement(mTrackDiv); >+ aElements.MaybeAppendElement(mThumbDiv); >+} >+ >+NS_QUERYFRAME_HEAD(nsRangeFrame) >+ NS_QUERYFRAME_ENTRY(nsRangeFrame) >+ NS_QUERYFRAME_ENTRY(nsIAnonymousContentCreator) >+NS_QUERYFRAME_TAIL_INHERITING(nsContainerFrame) This NS_QUERYFRAME chunk probably wants to be at the top of the file with the other boilerplate (near NS_IMPL_FRAMEARENA_HELPERS), I imagine. Seems out of place in between two random function-definitions right now. >+void >+nsRangeFrame::ReflowAnonymousContent(nsPresContext* aPresContext, >+ nsHTMLReflowMetrics& aDesiredSize, >+ const nsHTMLReflowState& aReflowState, >+ nsReflowStatus& aStatus) >+{ [...] >+ nsIFrame* trackFrame = mTrackDiv->GetPrimaryFrame(); >+ NS_ASSERTION(trackFrame, "The range frame should have a child with a frame!"); >+ >+ nsIFrame* thumbFrame = mThumbDiv->GetPrimaryFrame(); >+ NS_ASSERTION(thumbFrame, "The range frame should have a child with a frame!"); These assertion-messages seem a little off -- i.e. they don't quite describe the thing we're asserting. e.g. Suppose we pass the first but fail the second -- we'll print an assertion-failure-message about how we "should have a child with a frame" -- but we *do* have a child with a frame -- the trackFrame. We're just missing a thumbFrame. >+bool >+nsRangeFrame::IsHorizontal(const nsSize *aFrameSizeOverride) const >+{ >+ return true; // until we decide how we want to allow vertical range >+ >+#if 0 >+ // This implements https://bugzilla.mozilla.org/show_bug.cgi?id=833742#c4 Will this "#if 0" chunk land like this, or is it going to go away (or perhaps be enabled) before this lands? (I seem to recall hearing a prohibition against explicitly-dead-code at some point... not 100% sure though.) >+++ b/layout/forms/nsRangeFrame.h [...] >+#include "nsRepeatService.h" What is this ^^ header here for? Pretty sure this can be removed (or moved to the .cpp file, if it's actually needed there). >+#include "nsDOMTouchEvent.h" >+#include "nsIDOMEventListener.h" These, too -- I don't immediately see anything in the header file that needs these. >+class nsRangeFrame; >+ >+class nsRangeFrame : public nsContainerFrame, >+ public nsIAnonymousContentCreator Drop the unnecessary forward-declaration of nsRangeFrame there. >+public: [...] >+ nsRangeFrame(nsStyleContext* aContext); >+ virtual ~nsRangeFrame(); I believe best-practice is to make the constructor & destructor protected, and then declare NS_NewRangeFrame() method as a friend. This enforces the invariant that all nsRangeFrames are allocated via the NS_New method (using the frame arena), and *not* via "new nsRangeFrame()", and that they're freed via DestroyFrom, and *not* via an arbitrary "delete" call. (See e.g. nsBlockFrame for an example of this in action.) >+ NS_IMETHOD Reflow(nsPresContext* aCX, >+ nsHTMLReflowMetrics& aDesiredSize, >+ const nsHTMLReflowState& aReflowState, >+ nsReflowStatus& aStatus); s/aCX/aPresContext/ for consistency w/ other frame classes and with the param-name you use in the impl of this method >+ /** >+ * The div used to show the thumb. >+ * @see nsRangeFrame::CreateAnonymousContent >+ */ >+ nsCOMPtr<nsIContent> mThumbDiv; >+}; >+ >+#endif >+ ^^^ Remove bonus extra line @ end of file. >+++ b/layout/style/forms.css >+input[type="range"] { >+ display: inline-block ! important; Is there any reason you've got a space after the "!" there? Local style (and popular-vote by MXR search) favors "!important", with no space after the "!". (This happens multiple times in this file.) >+/** >+ * If content authers want to have a vertical range, they will also need to s/authers/authors/ >+ * set the width/height of this pseudo element. s/pseudo element/pseudo-element/ (reference: https://developer.mozilla.org/en-US/docs/CSS/Pseudo-elements ) >+ border: 2px solid grey; >+ width: 100%; >+ height: 0px; >+} >+ >+/** >+ * Layout handles positioning of this pseudo element specially (so that content As above, s/pseudo element/pseudo-element/ >+::-moz-range-thumb { >+ /* Block styles that would change the type of frame we construct. */ >+ display: inline-block ! important; "Block" is a bit of an overloaded term here. :) (I initially read this w/ the layout meaning of "Block", and the comment made no sense.) Maybe "s/Block styles/Prevent styles/"?

Also: (In reply to Jonathan Watt [:jwatt] from comment #1) > dholbert: you can ignore the XXX commented parts. They are just there to > remind me of various things, and are fixed later. By "fixed later" do you mean that you'll fix them in this patch before you land it, or they're fixed in a patch on another bug? I'd prefer the former (particularly for the unnecessary SetInitialChildList impl (and its decl), and the XXXkill commented out line of code -- I don't think there's any reason we should land those only to remove them in a later patch. :)

(In reply to Daniel Holbert [:dholbert] from comment #2) > It's probably worth asserting that DoesMinMaxApply() is true before you call > GetMinimum()/GetMaximum(), per those methods' header documentation. (Given > that we're only expecting to hit this code for <input type='range'>, it > seems like we should be safe to assert about DoesMinMaxApply, instead of > explicitly checking it.) I've added that check, although it does seem a little redundant given the check for the input type. > Can we really assert that these values are finite? It seems like it could > be possible for web content to run afoul of this assertion, by specifying a > sufficiently-huge minimum or maximum value, such that ConvertStringToNumber > produces the double-value for positive or negative infinity. Good point. I'll fix GetMinimum and GetMaximum to make sure they return the default values in the case that the result isn't finite. > It looks like you're making these public, but I don't think you need to (at > least, not for this bug)... Moved to a later patch. > These assertion-messages seem a little off -- i.e. they don't quite describe > the thing we're asserting. To allow for these frames being given |display:none| I've removed the assertions and changed the code to deal with them being null. It's not currently necessary given the !important rules in the UA style sheet, but it costs us virtually nothing and is safer for future changes. > Will this "#if 0" chunk land like this, or is it going to go away (or > perhaps be enabled) before this lands? (I seem to recall hearing a > prohibition against explicitly-dead-code at some point... not 100% sure > though.) My intention was to land it as-is. I've moved it to a later patch in my patch queue though. > Is there any reason you've got a space after the "!" there? Local style > (and popular-vote by MXR search) favors "!important", with no space after > the "!". > (This happens multiple times in this file.) I've no strong preference, and the file has both. I'll change it to remove the spaces.

Jonathan, I see this is changing some content/ code, make sure to ask a DOM peer to review those changes.

(presumably this now depends on whatever bug ended up w/ the content code that was split out per comment 6, yes? Not a huge deal, but it'd be handy to have that dependency codified so that bug-interactions are clearer.) Posting two notes on that /content code, which I'd written as a part of an in-progress review comment. Posting these separately from the review-comment now, since that code's no longer in the patch: (In reply to Jonathan Watt [:jwatt] from comment #4) > (In reply to Daniel Holbert [:dholbert] from comment #2) > > It's probably worth asserting that DoesMinMaxApply() is true before you call > > GetMinimum()/GetMaximum() > > I've added that check, although it does seem a little redundant given the > check for the input type. I think the check is worthwhile, even though it appears redundant -- note that the input-type check is eventually going to go away (per its "for the moment" comment), whereas the DoesMinMaxApply() check should presumably stay around (either as an "if" or as an assertion). It'd be worth adding actual text to the assertion, too, so that we've got a useful error message if it ever fails. (e.g. "Can't get fraction of an unbounded range"). > Good point. I'll fix GetMinimum and GetMaximum to make sure they return the > default values in the case that the result isn't finite. Cool -- though, I'm not sure that's always the right behavior, for other GetMinimum/Maximum callers... It *is* probably what we want for GetValueAsFractionOfRange(), but I could imagine situations where a caller would want GetMaximum() to actually return +infinity if someone specified a sufficiently-large maximum value. So this edge-case behavior might be better left to the clients (like GetValueAsFractionOfRange). I guess that's a call for a DOM peer to make, anyway. :) If we keep your updated behavior, though, it'd probably be worth updating the GetMinimum/Maximum documentation to indicate that it's guaranteed to return a finite value (when DoesMinMaxApply returns true), so that callers know they don't have to worry about checking for that.

(Oh, sorry -- looks like that code wasn't split out after all -- I just skipped past it when glancing at the updated patch)

(In reply to Daniel Holbert [:dholbert] from comment #8) > note > that the input-type check is eventually going to go away (per its "for the > moment" comment) Actually rather than the check going away, the "for the moment" is supposed to indicate that other types _may_ be added in future. Anyway, it really doesn't matter. I'm quite happy to have both checks. , whereas the DoesMinMaxApply() check should presumably stay > around (either as an "if" or as an assertion). > > It'd be worth adding actual text to the assertion, too, so that we've got a > useful error message if it ever fails. (e.g. "Can't get fraction of an > unbounded range"). > > > Good point. I'll fix GetMinimum and GetMaximum to make sure they return the > > default values in the case that the result isn't finite. > > Cool -- though, I'm not sure that's always the right behavior, for other > GetMinimum/Maximum callers... It is when type=range, since range should always have finite default minimum/maximum. We also never want to return +/-Infinity values, since the spec requires finite values: http://www.whatwg.org/specs/web-apps/current-work/multipage/common-input-element-attributes.html#concept-input-min or else we use NaN to indicate there is no minimum/maximum. > I guess that's a call for a DOM peer to make, anyway. :) Yup. :) A DOM peer will be reviewing the DOM changes. > If we keep your > updated behavior, though, it'd probably be worth updating the > GetMinimum/Maximum documentation to indicate that it's guaranteed to return > a finite value (when DoesMinMaxApply returns true), so that callers know > they don't have to worry about checking for that. Note that it's _not_ guaranteed to return a finite value, since for the non-range types defaultMinimum/Maximum is set to MOZ_DOUBLE_NaN. The change only guarantees that we won't return +/-Infinity values. (By saying "if we don't parse out a finite value, return the default value, which may be NaN to indicate that there is no default".)

(In reply to Jonathan Watt [:jwatt] from comment #10) > Note that it's _not_ guaranteed to return a finite value, since for the > non-range types defaultMinimum/Maximum is set to MOZ_DOUBLE_NaN. (Yup, sorry -- instead of "when DoesMinMaxApply returns true", I meant to say "when we're not returning NaN". I thought DoesMinMaxApply might imply that, but it looks like it doesn't.) > (By saying "if we > don't parse out a finite value, return the default value, which may be NaN > to indicate that there is no default".) That text is very clear -- is that already in the documentation for these functions (perhaps in another patch)? If so, awesome; if not, it'd be great if you could add something to that effect.

Regarding the whole "maybe ConvertStringToNumber can return +/-Infinity" issue, see bug 836314 comment 13. DOM peer (Mounir) r-'ed on the basis that ConvertStringToNumber should always return a finite value. I'll add an assertion for that in the patch over there.

(In reply to Daniel Holbert [:dholbert] from comment #11) > That text is very clear -- is that already in the documentation for these > functions (perhaps in another patch)? If so, awesome; if not, it'd be great > if you could add something to that effect. The documentation in m-c says: /** * Returns the input's "minimum" (as defined by the HTML5 spec) as a double. * Note this takes account of any default minimum that the type may have. * Returns NaN if the min attribute isn't a valid floating point number and * the input's type does not have a default minimum. * * NOTE: Only call this if you know DoesMinMaxApply() returns true. */ That's what I think the comment should say. If you want some other text, can you suggest the exact change you want?

(In reply to Jonathan Watt [:jwatt] from comment #12) > Regarding the whole "maybe ConvertStringToNumber can return +/-Infinity" > issue, see bug 836314 comment 13. DOM peer (Mounir) r-'ed on the basis that > ConvertStringToNumber should always return a finite value. I'll add an > assertion for that in the patch over there. Actually this will be covered by bug 840720.

(In reply to Jonathan Watt [:jwatt] from comment #13) > That's what I think the comment should say. If you want some other text, can > you suggest the exact change you want? My only suggested-tweak to that documentation you quoted there would be to add "Otherwise, guaranteed to return a finite value" to the end, so that it's clear that its callers (like GetValueAsFractionOfRange) don't have to worry about checking for infinity. (That doc-tweak should probably happen in bug 840720, though, since that's what will make that guarantee valid.)

Okay, sounds good. Added to the patch over in bug 840720.

Some more partial-review (sorry for doing this in chunks; posting this now so you can start looking at these before you head to bed, if you like): >+// XXX delete this >+NS_IMETHODIMP >+nsRangeFrame::SetInitialChildList(ChildListID aListID, >+ nsFrameList& aChildList) >+{ >+ nsresult rv = nsContainerFrame::SetInitialChildList(aListID, aChildList); >+ return rv; >+} This still needs deleting. >+nsRangeFrame::DestroyFrom(nsIFrame* aDestructRoot) >+{ >+ NS_ASSERTION(!GetPrevContinuation(), >+ "nsRangeFrame should not have continuations; if it does we " >+ "need to call RegUnregAccessKey only for the first."); This should probably assert !GetNextContinuation(), too, to fully-check what the assertion says you're checking. [...] >+ // Create the track div: >+ nodeInfo = doc->NodeInfoManager()->GetNodeInfo(nsGkAtoms::div, nullptr, >+ kNameSpaceID_XHTML, [a bunch of lines omitted here] >+ // Create the thumb div: >+ nodeInfo = doc->NodeInfoManager()->GetNodeInfo(nsGkAtoms::div, nullptr, >+ kNameSpaceID_XHTML, [a bunch of lines omitted here] Looks like the "create the track div" and "create the thumb div" chunks here are almost identical. Could we split that duplicated logic out into a helper-function, which we just can just invoke twice here?

[continuing review] >+NS_IMETHODIMP >+nsRangeFrame::Reflow(nsPresContext* aPresContext, >+ nsHTMLReflowMetrics& aDesiredSize, >+ const nsHTMLReflowState& aReflowState, >+ nsReflowStatus& aStatus) >+{ >+ DO_GLOBAL_REFLOW_COUNT("nsRangeFrame"); >+ DISPLAY_REFLOW(aPresContext, this, aReflowState, aDesiredSize, aStatus); >+ >+ NS_ASSERTION(mThumbDiv, "Thumb div must exist!"); Probably worth asserting about the track div, too, right? >+ NS_ASSERTION(!GetPrevContinuation(), >+ "nsRangeFrame should not have continuations; if it does we " >+ "need to call RegUnregAccessKey only for the first."); Per above, might as well add "&& !GetNextContinuation()" here. >+void >+nsRangeFrame::ReflowAnonymousContent(nsPresContext* aPresContext, >+ nsHTMLReflowMetrics& aDesiredSize, >+ const nsHTMLReflowState& aReflowState, >+ nsReflowStatus& aStatus) >+{ [...] >+ // The width/height of our content box, which is the available width/height >+ // for our anonymous content: >+ nscoord availableWidth = aReflowState.ComputedWidth(); >+ nscoord availableHeight = aReflowState.ComputedHeight(); [...] >+ nscoord trackY = availableHeight / 2; What if ComputedHeight is NS_AUTOHEIGHT? We don't want to be doing arithmetic with that. (and it's generally possible to have that value in aReflowState.ComputedHeight() -- definitely w/ "display:block; height:auto", possibly w/ other situations too) Pretty sure we need to either check for that here, or assert that it's impossible w/ an explanation of why. >+ // These need to be relative to this frame (our border box) by the time we >+ // call ReflowChild: This comment could use clarification. At least: - s/this frame/the nsRangeFrame/ (if that's what you mean), since there are multiple frames in play here. - Indicate what point we're actually trying to compute here w/ trackX/trackY. (It looks like it's the upper-left corner of the track frame, such that it ends up being centered in the nsRangeFrame, right?) >+ // Account for the track's border and padding: >+ trackX -= trackReflowState.mComputedBorderPadding.left + >+ trackReflowState.ComputedWidth() / 2; >+ trackY -= trackReflowState.mComputedBorderPadding.top + >+ trackReflowState.ComputedHeight() / 2; What about the track's margin? Do we need to account for that? Or is its margin just ignored? Worth clarifying that here. >+ // Make relative to our border box instead of our content box: >+ trackX += aReflowState.mComputedBorderPadding.left; >+ trackY += aReflowState.mComputedBorderPadding.top; >+ >+ nsHTMLReflowMetrics trackDesiredSize; >+ ReflowChild(trackFrame, aPresContext, trackDesiredSize, trackReflowState, >+ trackX, trackY, 0, aStatus); >+ FinishReflowChild(trackFrame, aPresContext, &trackReflowState, >+ trackDesiredSize, trackX, trackY, 0); Shouldn't we be checking the return values of these methods, and aStatus?

Reftest coverage for <input> is a bit basic, but here's some reftests to match what we currently do for other input types.

>+ if (thumbFrame) { // display:none? >+ >+ // Position the thumb: >+ // The idea here is that we want to position the thumb so that the center >+ // of the thumb is on an imaginary line drawn from the middle of one edge >+ // of the content box to the middle of the opposite edge of the content box Somewhere in here, you should do... s/content box/the <input> element's content box/ (or perhaps "the range frame's content box") ...to be clear which content-box you're talking about. >+ double valueAsFraction = >+ static_cast<nsHTMLInputElement*>(mContent)->GetValueAsFractionOfRange(); Maybe assert that mContent->Tag() == nsGkAtoms::input, as a sanity-check before the static_cast? >+ MOZ_ASSERT(valueAsFraction >= 0 && valueAsFraction <= 1); Nit: s/0/0.0/ s/1/1.0/ (to be sure the compiler treats these as double values) >+ nsHTMLReflowState thumbReflowState(aPresContext, aReflowState, thumbFrame, >+ nsSize(aReflowState.ComputedWidth(), >+ NS_UNCONSTRAINEDSIZE)); >+ >+ // These need to be relative to this frame (our border box) by the time we >+ // call ReflowChild: >+ nscoord thumbX, thumbY; >+ >+ if (isHorizontal) { >+ thumbX = nscoord(double(availableWidth) * valueAsFraction); This probably wants to be: NSToCoordRound(availableWidth * valueAsFraction); >+ thumbY = availableHeight - >+ nscoord(double(availableHeight) * valueAsFraction); Why the "availableHeight -" here? (we don't use "availableWidth - " above) This also should use NSToCoordRound, as above. >+ ReflowChild(thumbFrame, aPresContext, thumbDesiredSize, thumbReflowState, >+ thumbX, thumbY, 0, aStatus); >+ FinishReflowChild(thumbFrame, aPresContext, &thumbReflowState, >+ thumbDesiredSize, thumbX, thumbY, 0); >+ } As noted in my prev. comment for the track frame, we should be checking return-values and asserting that NS_FRAME_IS_COMPLETE(aStatus) is for the thumb frame, here, too. >+NS_IMETHODIMP >+nsRangeFrame::AttributeChanged(int32_t aNameSpaceID, >+ nsIAtom* aAttribute, >+ int32_t aModType) >+{ [...] >+ nsIFrame* trackFrame = mTrackDiv->GetPrimaryFrame(); >+ NS_ASSERTION(trackFrame, "The range frame should have a child with a frame!"); >+ PresContext()->PresShell()->FrameNeedsReflow(trackFrame, nsIPresShell::eResize, >+ NS_FRAME_IS_DIRTY); >+ >+ nsIFrame* barFrame = mThumbDiv->GetPrimaryFrame(); >+ NS_ASSERTION(barFrame, "The range frame should have a child with a frame!"); >+ PresContext()->PresShell()->FrameNeedsReflow(barFrame, nsIPresShell::eResize, >+ NS_FRAME_IS_DIRTY); >+ These "should have a child with a frame" assertion messages are still weird, as noted in one of my comments above. Also: what if one of those child elements is styled as "display:none"? Won't it not have a frame? Seems like that might invalidate these assertions. >+nsSize >+nsRangeFrame::ComputeAutoSize(nsRenderingContext *aRenderingContext, >+ nsSize aCBSize, nscoord aAvailableWidth, >+ nsSize aMargin, nsSize aBorder, >+ nsSize aPadding, bool aShrinkWrap) [...] >+ if (isHorizontal) { >+ autoSize.width = 10 * oneEm; >+ autoSize.height = IsThemed() ? 0 : oneEm; >+ } else { >+ autoSize.width = IsThemed() ? 0 : oneEm; >+ autoSize.height = 10 * oneEm; >+ } 10em and 1em are a bit magic here -- is that specified anywhere? It's also duplicated across this file in a few places (e.g. at the end of GetPrefWidth, at least)... Might be nice to abstract that away into a helper-method, a #define, or a const variable, rather than having magic-number-10 all over the place. >+bool >+nsRangeFrame::IsHorizontal(const nsSize *aFrameSizeOverride) const >+{ >+ return true; // until we decide how we want to allow vertical range >+} Maybe mention a bug # here? (is there one covering this?) >diff --git a/layout/forms/nsRangeFrame.h b/layout/forms/nsRangeFrame.h >new file mode 100644 >--- /dev/null >+++ b/layout/forms/nsRangeFrame.h >@@ -0,0 +1,116 @@ >+ // nsIFrame overrides >+ NS_IMETHODIMP SetInitialChildList(ChildListID aListID, >+ nsFrameList& aChildList) MOZ_OVERRIDE; [assuming this is going away, per XXX comment in the .cpp file] >+ // XXX huh? We have child frames when -moz-appearance:none >+ virtual bool IsLeaf() const MOZ_OVERRIDE { return true; } Why that XXX comment? We do have child frames, but they're created via nsIAnonymousContentCreator, and the nsIFrame documentation for this method says that's fine. >+ NS_IMETHOD AttributeChanged(int32_t aNameSpaceID, >+ nsIAtom* aAttribute, >+ int32_t aModType); use MOZ_OVERRIDE >+ virtual nscoord GetMinWidth(nsRenderingContext *aRenderingContext); >+ virtual nscoord GetPrefWidth(nsRenderingContext *aRenderingContext); >+ >+ virtual nsIAtom* GetType() const; >+ >+ virtual bool IsFrameOfType(uint32_t aFlags) const >+ { >+ return nsContainerFrame::IsFrameOfType(aFlags & >+ ~(nsIFrame::eReplaced | nsIFrame::eReplacedContainsBlock)); >+ } use MOZ_OVERRIDE on all 4 of these >+ >+ /** >+ * Returns true if the slider's thumb moves horizontally, or else false if it >+ * moves vertically. >+ * >+ * aOverrideFrameSize If specified, this will be used instead of the size of >+ * the frame's rect (i.e. the frame's computed width/height) Replace "computed width/height" with "border-box size" (since that's what the frame's rect actually represents). >+ * rect would have otherwise been examined. This should only be specified >+ * during reflow when the frame's [new] computed width/height has not yet >+ * been stored in its mRect. Here as well. Also: We apparently *always* pass aOverrideFrameSize right now -- there aren't any calls to IsHorizontal() that take advantage of the optional-ness of the parameter. (maybe those will come later when we actually implement IsHorizontal() though) >+protected: >+ // Helper function which reflow the anonymous div frame. s/reflow/reflows/ s/frame/frames/ >+input[type="range"] { >+ display: inline-block !important; >+ cursor: default; >+ width: 130px; >+ height: 15px; >+ background: none; >+ border: none; >+ margin: 2px; >+} Do these values come from anywhere, out of curiosity? (I wonder if this needs ui-review, at least before being preffed on?) >+/** >+ * Layout handles positioning of this pseudo-element specially (so that content >+ * authors can concentrate on styling the thumb without worrying about the >+ * logic to position it). Specifically the 'margin', 'top' and 'left' >+ * properties are ignored. >+ */ >+::-moz-range-thumb { >+ /* Prevent styling that would change the type of frame we construct. */ Both of these comments (RE the ignored properties and the frame-type enforcement) probably want to be copied up to the ::-moz-range-track {} chunk above, because they apply there as well.

I've addressed your review comments, and in some cases answered your questions by adding comments directly to the code. For the other review comments here are my replies: (In reply to Daniel Holbert [:dholbert] from comment #17) > Looks like the "create the track div" and "create the thumb div" chunks here > are almost identical. Could we split that duplicated logic out into a > helper-function, which we just can just invoke twice here? This is going to be changed to meet some B2G requirements in a later patch, so I'd rather leave as is. (In reply to Daniel Holbert [:dholbert] from comment #18) > What if ComputedHeight is NS_AUTOHEIGHT? We don't want to be doing > arithmetic with that. Addressed by falling back to zero as discussed on IRC. (In reply to Daniel Holbert [:dholbert] from comment #20) > Also: We apparently *always* pass aOverrideFrameSize right now -- there > aren't any calls to IsHorizontal() that take advantage of the optional-ness > of the parameter. (maybe those will come later when we actually implement > IsHorizontal() though) Yeah, there's code in subsequent patches that makes use of that. > Do these values come from anywhere, out of curiosity? (I wonder if this > needs ui-review, at least before being preffed on?) Not really. They just seemed like sane starting values that can easily be tweaked later.

Comment on attachment 713204 [details] [diff] [review] patch with some reftests >+++ b/layout/reftests/forms/input/range/input-range-moz-appearance-none-1.html >@@ -0,0 +1,6 @@ >+<!DOCTYPE html> >+<html> >+ <body> >+ <input type="range" style="-moz-appearence:none;"> A few global comments on the reftests: a) s/rence/rance/ in "-moz-appearence" (in various tests in this patch) b) Don't we need to turn on dom.experimental_forms in the reftest manifest for these tests to do anything useful? c) The testcase / reference case naming seems a bit odd -- e.g. there's a generically-nanmed (un-numbered) reference case "input-range-moz-appearance-none-ref.html", but it only looks like one of the three "input-range-moz-appearance-none-[NUMBER].html" test files, which makes it hard to figure out what tests are supposed to look like which references without consulting the manifest. Why not just use the usual > == foo-1.html foo-1-ref.html naming scheme (with deviations from it where necessary due to multiple files sharing the same reference, if that happens)? d) On the subject of test-naming -- maybe s/moz-appearance-none/unthemed/ in the filenames, to make them less gigantic? (Seems like that's a relatively unimportant part of the test to be calling out in the filename, for the amount of characters it takes up. :))

Comment on attachment 713231 [details] [diff] [review] patch - layout with comments addressed >+NS_IMETHODIMP >+nsRangeFrame::Reflow(nsPresContext* aPresContext, >+ nsHTMLReflowMetrics& aDesiredSize, >+ const nsHTMLReflowState& aReflowState, >+ nsReflowStatus& aStatus) [...] >+ aDesiredSize.height = aReflowState.ComputedHeight() + >+ aReflowState.mComputedBorderPadding.TopBottom(); If ComputedHeight() is NS_AUTOHEIGHT here, this will end up being crazy-huge. Need to check for that here & use 0 instead of ComputedHeight(). (and then still add the borderpadding) We definitely need to include a reftest w/ <input type="range" style="height:auto"> to exercise this behavior & be sure it works, too... >+ nsresult rv = >+ ReflowAnonymousContent(aPresContext, aDesiredSize, aReflowState, aStatus); >+ NS_ENSURE_SUCCESS(rv, rv); Pretty sure that we don't need to pass |aDesiredSize| to ReflowAnonymousContent() - that param is completely unused there. Also, looks like you shouldn't be passing |aStatus| here anymore, either. (since ReflowAnonymousContent() doesn't take that as an arg anymore, in the current patch.) >+ nsIFrame* trackFrame = mTrackDiv->GetPrimaryFrame(); >+ ConsiderChildOverflow(aDesiredSize.mOverflowAreas, trackFrame); >+ >+ nsIFrame* barFrame = mThumbDiv->GetPrimaryFrame(); >+ ConsiderChildOverflow(aDesiredSize.mOverflowAreas, barFrame); This will crash if these pseudo-elements are 'display:none', right? Pretty sure this needs a null-check, and probably at least one reftest (or crashtest) w/ display:none on these pseudo-elements, to be sure that we don't crash here or elsewhere when that happens. >+nsresult >+nsRangeFrame::ReflowAnonymousContent(nsPresContext* aPresContext, >+ nsHTMLReflowMetrics& aDesiredSize, >+ const nsHTMLReflowState& aReflowState) >+{ >+ nsresult rv = NS_OK; >+ >+ if (ShouldUseNativeStyle()) { >+ return rv; // No need to reflow since we're not using these frames >+ } [...] >+ return rv; >+} I don't think there's any use in declaring |rv| all the way at the top, and then having 'return rv' statements that we already know are going to just be 'return NS_OK' statements (because we've got NS_ENSURE_SUCCESS calls at all the lines where we assign rv to something else). I'd just replace both of the existing 'return rv' statements (at the beginning & end of this method) with "return NS_OK", and declare 'nsresult rv' when you actually assign it to something unknown -- at the ReflowChild invocations. >+ nsReflowStatus frameStatus = NS_FRAME_COMPLETE; Don't assign this to anything -- just declare it w/out a value. It's an outparam for ReflowChild(). If ReflowChild() succeeds, this will have a valid value; if it fails, the NS_ENSURE_SUCCESS will make us return before we inspect this value, so it's fine for it to be uninitialized. (This happens twice -- for track frame & thumb frame) >+ nsHTMLReflowMetrics trackDesiredSize; >+ rv = ReflowChild(trackFrame, aPresContext, trackDesiredSize, >+ trackReflowState, trackX, trackY, 0, frameStatus); >+ NS_ENSURE_SUCCESS(rv, rv); >+ MOZ_ASSERT(NS_FRAME_IS_FULLY_COMPLETE(frameStatus), "how did this happen?"); s/how did this happen?/We gave our child unconstrained height, so it should be complete/ ...or something to that effect, with an informative error message. (This happens twice -- for track frame & thumb frame) >+ // Position the thumb: >+ // The idea here is that we want to position the thumb so that the center >+ // of the thumb is on an imaginary line drawn from the middle of one edge >+ // of the range frame's content box to the middle of the opposite edge of >+ // its content box (the opposite edges being the left/right edge if the >+ // range is horizontal, or else the top/bottom edges if the range is >+ // vertical). How far along this line the center of the thumb is placed >+ // depends on the value of the range.) Drop the terminal close-paren. >+NS_IMETHODIMP >+nsRangeFrame::AttributeChanged(int32_t aNameSpaceID, >+ nsIAtom* aAttribute, >+ int32_t aModType) [...] >+ >+ nsIFrame* barFrame = mThumbDiv->GetPrimaryFrame(); >+ if (thumbFrame) { // diplay:none? >+ PresContext()->PresShell()->FrameNeedsReflow(barFrame, nsIPresShell::eResize, >+ NS_FRAME_IS_DIRTY); >+ } >+ >+ InvalidateFrame(); I don't think this InvalidateFrame() call belongs here... If we end up changing the thumb-position, I'd expect that would trigger a reflow, and (if appropriate) change our display list, and DLBI would invalidate as-appropriate. >+nsSize >+nsRangeFrame::ComputeAutoSize(nsRenderingContext *aRenderingContext, >+ nsSize aCBSize, nscoord aAvailableWidth, >+ nsSize aMargin, nsSize aBorder, >+ nsSize aPadding, bool aShrinkWrap) >+{ >+ nscoord oneEm = nscoord(float(GetStyleFont()->mFont.size) * >+ nsLayoutUtils::FontSizeInflationFor(this)); // 1em Drop the float() cast, and replace the nscoord() wrapper with NSToCoordRound(). >+ // frameSizeOverride values just gets us to fall back to being horizontal: >+ nsSize frameSizeOverride(10,1); >+ bool isHorizontal = IsHorizontal(&frameSizeOverride); I realized that I don't actually understand what benefit we get here (and in GetPrefWidth()) with this frameSizeOverride(10,1)-passed-to-IsHorizontal thing. Hypothetically, even if we had a non-trivial IsHorizontal() implementation, the above-quoted lines are still equivalent to just "bool isHorizontal = true", right? So what's the use of calling IsHorizontal() with a bogus frameSizeOverride here? It seems like we should either call IsHorizontal() directly (without passing a hardcoded bogus value), or we shouldn't call it at all. >+ if (!isHorizontal && IsThemed()) { >+ // nsFrame::ComputeSize calls GetMinimumWidgetSize to prevent us from being >+ // given too small a size when we're natively themed. We return zero and >+ // depend on that correction to get our "natuaral" width when we're a >+ // vertical slider. >+ return nscoord(0); 'return 0' is fine -- no need for "nscoord(0)" >+ nscoord prefWidth = nscoord(float(GetStyleFont()->mFont.size) * >+ nsLayoutUtils::FontSizeInflationFor(this)); // 1em >+ As above, replace nscoord cast with NSToCoordRound. >+++ b/layout/forms/nsRangeFrame.h [...] >+ NS_IMETHOD Reflow(nsPresContext* aPresContext, >+ nsHTMLReflowMetrics& aDesiredSize, >+ const nsHTMLReflowState& aReflowState, >+ nsReflowStatus& aStatus); Use MOZ_OVERRIDE >+#ifdef DEBUG >+ NS_IMETHOD GetFrameName(nsAString& aResult) const { >+ return MakeFrameName(NS_LITERAL_STRING("Range"), aResult); >+ } >+#endif Use MOZ_OVERRIDE >+++ b/layout/style/forms.css >@@ -659,17 +659,17 @@ progress { >+input[type="range"] { >+ display: inline-block !important; >+ cursor: default; >+ width: 130px; >+ height: 15px; >+ background: none; >+ border: none; >+ margin: 2px; >+} Can you file a followup bug on getting UI input on the forms.css chunks? (I haven't tested it, but it looks reasonable; still, I think it probalby needs feedback from UX folks.) Assuming this is preffed-off (right?), that shouldn't block this landing, but it does block it being enabled. I've never actually gone through a UI review, so I don't know what it involves, but I'm sure jaws or fyan or dolske could tell you whether it's needed & what to expect. r=me with the above addressed, though I'd probably like to see what you end up doing w.r.t. the "frameSizeOverride(10,1)" thing.

(In reply to Daniel Holbert [:dholbert] from comment #23) > >+nsSize > >+nsRangeFrame::ComputeAutoSize(nsRenderingContext *aRenderingContext, > >+ nsSize aCBSize, nscoord aAvailableWidth, > >+ nsSize aMargin, nsSize aBorder, > >+ nsSize aPadding, bool aShrinkWrap) > >+{ > >+ nscoord oneEm = nscoord(float(GetStyleFont()->mFont.size) * > >+ nsLayoutUtils::FontSizeInflationFor(this)); // 1em Also: I'm unfamiliar with font-inflation, so I'm not clear on whether this is an appropriate usage. It looks like nsProgressFrame::ComputeAutoSize does this a bit differently, by calling GetFontMetricsForFrame: https://mxr.mozilla.org/mozilla-central/source/layout/forms/nsProgressFrame.cpp#239 Why aren't we doing something like that here?

Comment on attachment 713231 [details] [diff] [review] patch - layout with comments addressed Review of attachment 713231 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/html/content/src/nsHTMLInputElement.h @@ +292,5 @@ > + * NOTE: Only use this method with <input type=range> for now (it assumes > + * that GetMinimum and GetMaximum will return finite values, and that > + * GetValueAsDouble will return an in-range value). > + */ > + double GetValueAsFractionOfRange(); I would prefer this to live in nsRangeFrame. If we can keep layout logic in layout, that's better and I also prefer to reduce the number of type's specific methods in HTMLInputElement. I'm fine with having GetMinimum, GetMaximum and GetValueAsDouble to be public in HTMLInputElement. ::: layout/style/forms.css @@ +715,5 @@ > +input[type="range"] { > + display: inline-block !important; > + cursor: default; > + width: 130px; > + height: 15px; Why dimensions in px? I'm not much of a CSS person but I like how <meter> and <progress> have their size depending on the text size. @@ +718,5 @@ > + width: 130px; > + height: 15px; > + background: none; > + border: none; > + margin: 2px; Why a margin? @@ +737,5 @@ > + float: none !important; > + position: static !important; > + border: 2px solid grey; > + width: 100%; > + height: 0px; Seems less hacky to me to have a background and a height instead of a border with no height.

(In reply to Daniel Holbert [:dholbert] from comment #23) > We definitely need to include a reftest w/ <input type="range" > style="height:auto"> to exercise this behavior & be sure it works, too... Filed bug 842028. > Pretty sure that we don't need to pass |aDesiredSize| to > ReflowAnonymousContent() - that param is completely unused there. It is used - to initialize frameSizeOverride, to pass IsHorizontal the correct fallback ratio. > at least one reftest (or > crashtest) w/ display:none on these pseudo-elements, to be sure that we > don't crash here or elsewhere when that happens. Added a crashtest. > Don't assign this to anything -- just declare it w/out a value. It's an > outparam for ReflowChild(). If ReflowChild() succeeds, this will have a > valid value; if it fails, the NS_ENSURE_SUCCESS will make us return before > we inspect this value, so it's fine for it to be uninitialized. > > (This happens twice -- for track frame & thumb frame) Hmm, okay. That's what nsContainerFrame does, so I assumed that that was probably right. > >+ // frameSizeOverride values just gets us to fall back to being horizontal: > >+ nsSize frameSizeOverride(10,1); > >+ bool isHorizontal = IsHorizontal(&frameSizeOverride); > > I realized that I don't actually understand what benefit we get here (and in > GetPrefWidth()) with this frameSizeOverride(10,1)-passed-to-IsHorizontal > thing. I've tacked on "(the actual values are irrelevant, as long as width > height)" to that comment to clarify. > Hypothetically, even if we had a non-trivial IsHorizontal() implementation, > the above-quoted lines are still equivalent to just "bool isHorizontal = > true", right? No. These are just fallback values and only used if, say, -moz-orient hasn't been set. > Can you file a followup bug on getting UI input on the forms.css chunks? (I > haven't tested it, but it looks reasonable; still, I think it probalby needs > feedback from UX folks.) Assuming this is preffed-off (right?), Right, it's pref'ed off. Filed bug 842021 for the UI discussions. (In reply to Daniel Holbert [:dholbert] from comment #24) > Also: I'm unfamiliar with font-inflation, so I'm not clear on whether this > is an appropriate usage. > > It looks like nsProgressFrame::ComputeAutoSize does this a bit differently, > by calling GetFontMetricsForFrame: > https://mxr.mozilla.org/mozilla-central/source/layout/forms/nsProgressFrame. > cpp#239 > > Why aren't we doing something like that here? Yeah, I discussed this part with dbaron. What nsProgressFrame::ComputeAutoSize is doing is currently probably not what it should be doing. What I'm doing is correct.

(In reply to Mounir Lamouri (:mounir) from comment #25) > I would prefer this to live in nsRangeFrame. If we can keep layout logic in > layout, that's better and I also prefer to reduce the number of type's > specific methods in HTMLInputElement. > > I'm fine with having GetMinimum, GetMaximum and GetValueAsDouble to be > public in HTMLInputElement. Done. > Why dimensions in px? I'm not much of a CSS person but I like how <meter> > and <progress> have their size depending on the text size. Changed to em. > Why a margin? Just to improve the initial look. I fully expect the forms.css styling to change after people start flipping the pref and arguing over the look. We can tweak this stuff later in bug 842021. > Seems less hacky to me to have a background and a height instead of a border > with no height. Done.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0f9734e836ec https://hg.mozilla.org/integration/mozilla-inbound/rev/7db39954cc10 https://hg.mozilla.org/integration/mozilla-inbound/rev/0f9734e836ec The code patch is split up into two changesets to try and give Wes some credit for the initial work that he did.

(In reply to Jonathan Watt [:jwatt] from comment #28) > https://hg.mozilla.org/integration/mozilla-inbound/rev/0f9734e836ec > https://hg.mozilla.org/integration/mozilla-inbound/rev/7db39954cc10 > https://hg.mozilla.org/integration/mozilla-inbound/rev/0f9734e836ec First cset was re-pasted at the end there -- I think you meant for the third cset to be: https://hg.mozilla.org/integration/mozilla-inbound/rev/5e660787834c (for the tests)

(nit: the reftests w/ scripts should probably use MozReftestInvalidate instead of onload, since their point is to render as non-range, and then dynamically change to a range frame -- and onload doesn't guarantee that we'll do the first part. Maybe file a followup on that? (or just push a followup as part of this bug))

Thanks for the correct link. Oops - failing to use MozReftestInvalidate is more than a nit. Thanks for picking up on that. I'll fix that with a follow-up in an hour or so.

(In reply to Jonathan Watt [:jwatt] from comment #26) > > Don't assign this to anything -- just declare it w/out a value. It's an > > outparam for ReflowChild(). If ReflowChild() succeeds, this will have a > > valid value; if it fails, the NS_ENSURE_SUCCESS will make us return before > > we inspect this value, so it's fine for it to be uninitialized. > > > > (This happens twice -- for track frame & thumb frame) > > Hmm, okay. That's what nsContainerFrame does, so I assumed that that was > probably right. I filed bug 842080 on cleaning this up there & elsewhere, FWIW.

> (In reply to Daniel Holbert [:dholbert] from comment #24) > > Also: I'm unfamiliar with font-inflation, so I'm not clear on whether this > > is an appropriate usage. > > > > It looks like nsProgressFrame::ComputeAutoSize does this a bit differently, > > by calling GetFontMetricsForFrame: > > https://mxr.mozilla.org/mozilla-central/source/layout/forms/nsProgressFrame.cpp#239 > > > > Why aren't we doing something like that here? > > Yeah, I discussed this part with dbaron. What > nsProgressFrame::ComputeAutoSize is doing is currently probably not what it > should be doing. What I'm doing is correct. In that case, could you file a bug on fixing this in nsProgressFrame?

https://hg.mozilla.org/mozilla-central/rev/0f9734e836ec https://hg.mozilla.org/mozilla-central/rev/7db39954cc10 https://hg.mozilla.org/mozilla-central/rev/5e660787834c

For the MozReftestInvalidate thing I pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/87d5ffb1da72 (In reply to Daniel Holbert [:dholbert] from comment #33) > In that case, could you file a bug on fixing this in nsProgressFrame? I filed bug 842128.