849524 - Avoid reflowing when <input type=range>'s thumb position needs to be updated due to its value changing

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jonathan Watt [:jwatt]]

Right now <input type=range> is reflowed with NS_FRAME_IS_DIRTY when its value changes. Since it's an inline-block, this means than any adjacent inlines are also reflowed and repainted, which means quite a large area of the screen may end up repainting. We don't actually need to reflow though, since the position of the thumb does not affect the size or position of any other elements. So all we should do is set the position of the thumb and repaint its old and new locations.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 723080 [details] [diff] [review]
patch

>+++ b/content/html/content/src/nsHTMLInputElement.cpp
> nsHTMLInputElement::SetValueOfRangeForUserEvent(double aValue)
> {
[...]
>-  nsIFrame* frame = GetPrimaryFrame();
>+  nsRangeFrame* frame = do_QueryFrame(GetPrimaryFrame());
>   if (frame) {

This is causes unnecessary / redundant work in the (possibly uncommon but worth considering) case that our frame's subtree is already marked as dirty, because then, the pending reflow will update the thumb position for us.

To account for this: maybe keep this original line:
 nsIFrame* frame = GetPrimaryFrame();
...and add a check for "if (!NS_SUBTREE_DIRTY(frame))" before we bother to call do_QueryFrame on it etc.?

>diff --git a/layout/forms/nsRangeFrame.cpp b/layout/forms/nsRangeFrame.cpp
>+void
>+nsRangeFrame::UpdateThumbPositionForValueChange()
>+{
>+  nsIFrame* thumbFrame = mThumbDiv->GetPrimaryFrame();
>+  if (thumbFrame) { // diplay:none?
[...]
>+    nsSize thumbSize = thumbFrame->GetSize();

Probably worth asserting that thumbFrame has already received a reflow before we call GetSize() on it -- i.e. we should check that thumbFrame->GetStateBits() & NS_FRAME_FIRST_REFLOW is 0. (We can assert this, if you take my suggestions RE checking for dirtyness in this method's callers.)

>+    nsPoint newPosition(borderAndPadding.left, borderAndPadding.top);
>+    if (IsHorizontal()) {
>+      newPosition += nsPoint(NSToCoordRound(fraction * contentRect.width) -
>+                               thumbSize.width/2,
>+                             (contentRect.height - thumbSize.height)/2);
>+    } else {
>+      newPosition += nsPoint((contentRect.width - thumbSize.width)/2,
>+                             NSToCoordRound(fraction * contentRect.height) -
>+                               thumbSize.height/2);
>+    }
>+    thumbFrame->SetPosition(newPosition);

This duplicates a lot of work that we do during the "Position the thumb" part of reflow. I'd like to share code better, if possible... Maybe we could factor out a function like:
  nsPoint ComputeThumbPosition(nsSize aRangeFrameContentRectSize, nsSize aThumbFrameBorderBoxSize)
which both reflow & UpdateThumbPositionForValueChange could call (with their differing sources of positioning information)?

(I'm probably wrong about the exact args, but it'd be something along those lines.)

>+    SchedulePaint();

Is SchedulePaint() really the right thing to call here? I imagine we want to selectively repaint just the old-thumb-area & new-thumb-area, and SchedulePaint() appears to do some document-wide stuff... Maybe it's smart enough to figure out exactly what needs repainting though.  It may very well be right, I'm just unfamiliar with it. (pinged mattwoodrow for clarification but haven't heard back yet)

> NS_IMETHODIMP
> nsRangeFrame::AttributeChanged(int32_t  aNameSpaceID,
>                                nsIAtom* aAttribute,
>                                int32_t  aModType)
> {
>   NS_ASSERTION(mTrackDiv, "The track div must exist!");
>   NS_ASSERTION(mThumbDiv, "The thumb div must exist!");
> 
>   if (aNameSpaceID == kNameSpaceID_None &&
>       (aAttribute == nsGkAtoms::value ||
>        aAttribute == nsGkAtoms::min ||
>        aAttribute == nsGkAtoms::max ||
>        aAttribute == nsGkAtoms::step)) {
[...]
>+    UpdateThumbPositionForValueChange();

Like I said above for SetValueOfRangeForUserEvent, I think we should check if NS_SUBTREE_DIRTY(this) before we call UpdateThumbPositionForValueChange.

>+  /**
>+   * Helper to reposition the thumb and invalidate 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();

s/invalidate/schedule a paint/?  (right now, we're technically not calling Invalidate())

Also, needs a closing-paren at the end.

Comment 3

[Daniel Holbert [:dholbert]]

Never mind about the SchedulePaint question -- mattwoodrow confirmed (in #gfx) that your usage there is correct.
> <mattwoodrow> dholbert: That's correct. Just requesting a repaint 
> (via SchedulePaint()) is enough. DLBI will detect the the new display items are in a
> different location to what they were previously, and invalidate accordingly.

Comment 4

[Jonathan Watt [:jwatt]]

(In reply to Daniel Holbert [:dholbert] from comment #2)
> This is causes unnecessary / redundant work in the (possibly uncommon but
> worth considering) case that our frame's subtree is already marked as dirty,
> because then, the pending reflow will update the thumb position for us.
> 
> To account for this: maybe keep this original line:
>  nsIFrame* frame = GetPrimaryFrame();
> ...and add a check for "if (!NS_SUBTREE_DIRTY(frame))" before we bother to
> call do_QueryFrame on it etc.?

I put the dirty-bits check in UpdateThumbPositionForValueChange as discussed on IRC.

> Probably worth asserting that thumbFrame has already received a reflow
> before we call GetSize() on it -- i.e. we should check that
> thumbFrame->GetStateBits() & NS_FRAME_FIRST_REFLOW is 0. (We can assert
> this, if you take my suggestions RE checking for dirtyness in this method's
> callers.)

With the dirty-bits check in UpdateThumbPositionForValueChange, there's no point in an assert.

> This duplicates a lot of work that we do during the "Position the thumb"
> part of reflow. I'd like to share code better, if possible...

Me to, but I'd rather do that in bug 842179 after I know what the reflow code will look like.

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 724191 [details] [diff] [review]
patch

(In reply to Jonathan Watt [:jwatt] from comment #4)
> > This duplicates a lot of work that we do during the "Position the thumb"
> > part of reflow. I'd like to share code better, if possible...
> 
> Me to, but I'd rather do that in bug 842179 after I know what the reflow
> code will look like.

OK -- then maybe add an "todo"  comment, saying "this logic can & should be shared with Reflow", to decrease the likelihood that we'll forget about this. :)

># HG changeset patch
># Parent 9cad545caedc1c1fced3c585bf8e6d8b776c38e0
># User Jonathan Watt <[e-mail]>
>Bug 849524 - Avoid reflowing when <input type=range>'s thumb position needs to be updated due to its value changing. r=dholbert.

no need for period at the end of commit message

>+nsRangeFrame::UpdateThumbPositionForValueChange()
>+{
>+  if (NS_SUBTREE_DIRTY(this)) {
>+    return; // we're going to be updated when we reflow
>+  }
>+  nsIFrame* thumbFrame = mThumbDiv->GetPrimaryFrame();
>+  if (thumbFrame) { // diplay:none?

nit: we might as well make this "if (thumbFrame)" check an early-return, too, for consistency w/ this function's just-added early-return, and to save on indentation for the rest of this function.

r=me w/ the above.

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #6)
> >Bug 849524 - Avoid reflowing when <input type=range>'s thumb position needs to be updated due to its value changing. r=dholbert.
> 
> no need for period at the end of commit message

(after "r=dholbert." I mean :))

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 724191 [details] [diff] [review]
patch

Sorry, my cursor must've slipped. Meant to hit r+, not r-. :)

Comment 9

[Jonathan Watt [:jwatt]]

Thanks. :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/34f280f8fea5

Comment 10

[Ed Morley [:emorley]]

Backed out for crashes:
https://tbpl.mozilla.org/php/getParsedLog.php?id=20602358&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=20601899&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=20602373&tree=Mozilla-Inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/3c3f560a2a5c

Comment 11

[Jonathan Watt [:jwatt]]

The crash was actually a failing MOZ_ASSERT due to the way that nsHTMLInputElement handles type changes. I added a check for that case and repushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/85a2fba45e5b

What happens is that when changing the type of the <input>, nsHTMLInputElement::HandleTypeChange calls SetAttr(..., nsGkAtoms::value, ...). It does this after changing its type, but before the nsRangeFrame has been destroyed and replaced by a new frame of the appropriate type (ugh!), so under the SetAttr call we end up failing the type check.

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/85a2fba45e5b

Comment 13

[Jonathan Watt [:jwatt]]

For my own reference, the "value" attribute setting happens with the following stack:

  nsRangeFrame::GetValueAsFractionOfRange
  nsRangeFrame::UpdateThumbPositionForValueChange
  nsRangeFrame::AttributeChanged
  nsCSSFrameConstructor::AttributeChanged
  PresShell::AttributeChanged
  nsNodeUtils::AttributeChanged
  mozilla::dom::Element::SetAttrAndNotify
  mozilla::dom::Element::SetAttr
  nsGenericHTMLElement::SetAttr
  nsGenericHTMLElement::SetAttr             <- Setting 'value' here
  nsHTMLInputElement::HandleTypeChange
  nsHTMLInputElement::ParseAttribute
  mozilla::dom::Element::SetAttr
  nsGenericHTMLElement::SetAttr
  nsGenericHTMLElement::SetAttr
  nsGenericHTMLElement::SetAttrHelper
  nsHTMLInputElement::SetType