Andrea Marchesini [:baku] Reporter
	Description • 11 years ago

http://www.whatwg.org/specs/web-apps/current-work/#the-input-element

void setRangeText(DOMString replacement);
void setRangeText(DOMString replacement, unsigned long start, unsigned long end, optional SelectionMode selectionMode);

	Andrew Quartey [:drexler] Assignee
	Comment 1 • 11 years ago

Current work minus tests.

	Andrew Quartey [:drexler] Assignee
	Comment 2 • 11 years ago

Complete implementation with tests. Try: https://tbpl.mozilla.org/?tree=Try&rev=5d761ddc1db1

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 3 • 11 years ago

Comment on attachment 798361 [details] [diff] [review]
Impl v1

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

I'm afraid I won't manage to do this review well enough before I leave, so punting to Mounir. Sorry.

::: content/html/content/src/HTMLInputElement.cpp
@@ +4181,5 @@
> +  using namespace mozilla::dom::SelectionModeValues;
> +
> +  Optional<SelectionMode> selectMode;
> +  selectMode.Construct(SelectionMode::Preserve);
> +  SetRangeText(aReplacement, GetSelectionStart(aRv), GetSelectionEnd(aRv),

Same about checking aRv

@@ +4208,5 @@
> +  GetValueInternal(value);
> +  uint32_t inputValueLength = value.Length();
> +
> +  if (aStart > inputValueLength)
> +    aStart = inputValueLength;

{}

@@ +4211,5 @@
> +  if (aStart > inputValueLength)
> +    aStart = inputValueLength;
> +
> +  if (aEnd > inputValueLength)
> +    aEnd = inputValueLength;

{}

@@ +4213,5 @@
> +
> +  if (aEnd > inputValueLength)
> +    aEnd = inputValueLength;
> +
> +  uint32_t selectionStart = GetSelectionStart(aRv);

You need to check if aRv.Failed(), and return if so.

@@ +4224,5 @@
> +
> +  uint32_t newEnd = aStart + aReplacement.Length();
> +  uint32_t delta =  aReplacement.Length() - (aEnd - aStart);
> +
> +  using namespace mozilla::dom::SelectionModeValues;

Do you need this?

@@ +4226,5 @@
> +  uint32_t delta =  aReplacement.Length() - (aEnd - aStart);
> +
> +  using namespace mozilla::dom::SelectionModeValues;
> +
> +  // assume default case SelectionMode::End

Default is preserve, no? Better to handle that in IDL, though: I filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=23193

@@ +4267,5 @@
> +  nsAutoString dir(NS_LITERAL_STRING("none"));
> +  Optional<nsAString> direction;
> +  direction = &dir;
> +  SetSelectionRange(selectionStart, selectionEnd, direction, aRv);
> +}

Is this missing

> Queue a task to fire a simple event that bubbles named select
> at the element, using the user interaction task source as the
> task source.

?

	Mounir Lamouri (:mounir)
	Comment 4 • 11 years ago

Comment on attachment 798361 [details] [diff] [review]
Impl v1

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

I guess Ehsan could have a look into that.

	Andrew Quartey [:drexler] Assignee
	Comment 5 • 11 years ago

(In reply to :Ms2ger (away 11-21 September) from comment #3)
> Comment on attachment 798361 [details] [diff] [review]
> Impl v1

Thanks for the feedback. 
 

> > +  using namespace mozilla::dom::SelectionModeValues;
> 
> Do you need this?
Necessary for SelectionMode::Select and other enumerations.
 
> 
> @@ +4267,5 @@
> > +  nsAutoString dir(NS_LITERAL_STRING("none"));
> > +  Optional<nsAString> direction;
> > +  direction = &dir;
> > +  SetSelectionRange(selectionStart, selectionEnd, direction, aRv);
> > +}
> 
> Is this missing
> 
> > Queue a task to fire a simple event that bubbles named select
> > at the element, using the user interaction task source as the
> > task source.
> 
> ?

Yes. I intentionally left out firing the event here after reading the specification for SetSelectionRange which required the User Agent to fire the same event type. Otherwise, for SetRangeText we'd be firing duplicate events.

	Andrew Quartey [:drexler] Assignee
	Comment 6 • 11 years ago

> 
> Yes. I intentionally left out firing the event here after reading the
> specification for SetSelectionRange which required the User Agent to fire
> the same event type. Otherwise, for SetRangeText we'd be firing duplicate
> events.

Presumed wrongly since I cant locate where this event is fired by SetSelectionRange. I'll add it with other changes pending Ehsan's review.

	(no longer active)
	Comment 7 • 11 years ago

Comment on attachment 798361 [details] [diff] [review]
Impl v1

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

Thanks, this patch is mostly good.  Minusing because it doesn't fire the select event, and also that I believe the delta computation is incorrect.

::: content/html/content/src/HTMLInputElement.cpp
@@ +4172,5 @@
> +HTMLInputElement::SetRangeText(const nsAString& aReplacement, ErrorResult& aRv)
> +{
> +  if (mType != NS_FORM_INPUT_TEXT && mType != NS_FORM_INPUT_SEARCH &&
> +      mType != NS_FORM_INPUT_URL && mType != NS_FORM_INPUT_TEL &&
> +      mType != NS_FORM_INPUT_PASSWORD) {

Can you please refactor this condition into a helper method, such as AcceptsSetRangeText or something?

@@ +4177,5 @@
> +    aRv.Throw(NS_ERROR_DOM_NOT_SUPPORTED_ERR);
> +    return;
> +  }
> +
> +  using namespace mozilla::dom::SelectionModeValues;

Hmm, why do you do this?  You can simply use mozilla::dom::SelectionMode::Preserve, etc. right?

@@ +4180,5 @@
> +
> +  using namespace mozilla::dom::SelectionModeValues;
> +
> +  Optional<SelectionMode> selectMode;
> +  selectMode.Construct(SelectionMode::Preserve);

You can just pass Preserve to the ctor and construct the object in one go.

@@ +4181,5 @@
> +  using namespace mozilla::dom::SelectionModeValues;
> +
> +  Optional<SelectionMode> selectMode;
> +  selectMode.Construct(SelectionMode::Preserve);
> +  SetRangeText(aReplacement, GetSelectionStart(aRv), GetSelectionEnd(aRv),

What Ms2ger said!

In addition, calling GetSelectionStart and GetSelectionEnd like this is a bit wasteful.  I think instead you should call GetSelectionRange() and if it fails, query it form the nsTextEditorState object.  Please see the GetSelectionStart/End implementation for how this is supposed to work.

@@ +4214,5 @@
> +  if (aEnd > inputValueLength)
> +    aEnd = inputValueLength;
> +
> +  uint32_t selectionStart = GetSelectionStart(aRv);
> +  uint32_t selectionEnd = GetSelectionEnd(aRv);

Again, see my comment about not doing repeated work above.

@@ +4218,5 @@
> +  uint32_t selectionEnd = GetSelectionEnd(aRv);
> +
> +  if (aStart < aEnd) {
> +    value.Replace(aStart, aEnd - aStart, aReplacement);
> +    SetValueInternal(value, false, true);

You want to pass false to aSetValueChanged here, since we don't want this to fire an input event.

@@ +4222,5 @@
> +    SetValueInternal(value, false, true);
> +  }
> +
> +  uint32_t newEnd = aStart + aReplacement.Length();
> +  uint32_t delta =  aReplacement.Length() - (aEnd - aStart);

If |aReplacement.Length() - (aEnd - aStart) < 0| then delta would have a very large value.  You should use a signed type here.

@@ +4224,5 @@
> +
> +  uint32_t newEnd = aStart + aReplacement.Length();
> +  uint32_t delta =  aReplacement.Length() - (aEnd - aStart);
> +
> +  using namespace mozilla::dom::SelectionModeValues;

Ditto.

@@ +4226,5 @@
> +  uint32_t delta =  aReplacement.Length() - (aEnd - aStart);
> +
> +  using namespace mozilla::dom::SelectionModeValues;
> +
> +  // assume default case SelectionMode::End

Yeah this should happen in the IDL.  Then, you wouldn't need to set selectionStart/selectionEnd before the switch statement.

@@ +4259,5 @@
> +      break;
> +      // Inclusion of case SelectionMode::Preserve is to stop compilation
> +      // failures on non-windows platforms. 
> +      case SelectionMode::Preserve:
> +      {}

Nit:

case SelectionMode::Preserve:
  // Silence the compiler warnings
  break;

@@ +4260,5 @@
> +      // Inclusion of case SelectionMode::Preserve is to stop compilation
> +      // failures on non-windows platforms. 
> +      case SelectionMode::Preserve:
> +      {}
> +    }//end switch

Please remove this comment.

@@ +4265,5 @@
> +  }
> +
> +  nsAutoString dir(NS_LITERAL_STRING("none"));
> +  Optional<nsAString> direction;
> +  direction = &dir;

You don't need to initialize direction here, SetSelectionRange will do the right thing if direction.WasPassed() == false.

@@ +4267,5 @@
> +  nsAutoString dir(NS_LITERAL_STRING("none"));
> +  Optional<nsAString> direction;
> +  direction = &dir;
> +  SetSelectionRange(selectionStart, selectionEnd, direction, aRv);
> +}

Yes.  In fact, it seems like we don't do this for setSelectionRange either, which violates the spec in a similar way.  Can you please do this at the end of SetSelectionRange()?

::: content/html/content/test/forms/test_set_range_text.html
@@ +37,5 @@
> +<script class="testbody" type="text/javascript">
> +
> +  /** Test for Bug 850364 **/
> +
> +  var SupportedTypes = ["text", "search", "url", "telephone", "password"];

"tel"

@@ +70,5 @@
> +      input.focus();
> +      try {
> +        input.setRangeText("abc");
> +      } catch (ex) {
> +        opThrows = (ex.name == "NotSupportedError" && ex.code == DOMException.NOT_SUPPORTED_ERR);

We should not throw _any_ kind of exception here, so you can just set opThrows to true here.

@@ +160,5 @@
> +      input.setSelectionRange(4, 9);
> +      input.setRangeText("QRST", 2, 6);
> +      is(input.value, "01QRST6789", msg + ".value == \"01QRST6789\"");
> +      is(input.selectionStart, 2, msg + ".selectionStart == 2, with \"default\"");
> +      is(input.selectionEnd, 9, msg + ".selectionEnd == 9, with \"default\"");

Please also add a test to make sure the select event fires, but the input event doesn't.

::: dom/webidl/HTMLInputElement.webidl
@@ +15,5 @@
> +enum SelectionMode {
> +  "select",
> +  "start",
> +  "end",
> +  "preserve"

Nit: please include a trailing comma so that this is exactly the same as what appears in the spec.

	Andrew Quartey [:drexler] Assignee
	Comment 8 • 11 years ago

Fixed nits and amended tests to check for input and select events. 
Due to the reasons i have outlined in Bug 916320, this patch doesn't have the default value specified in the webidl, so it still handles the default case - SelectionMode::Preserve - outside the switch. 

> I believe the delta computation is incorrect.

How so?  From the specs:
9. Let new length be the length of the value of the first argument.
1. Let old length be end minus start.
2. Let delta be new length minus old length.

	Andrew Quartey [:drexler] Assignee
	Comment 9 • 11 years ago

Jumped the gun a bit. Do disregard the previous comment about bug 916320.

	Andrew Quartey [:drexler] Assignee
	Comment 10 • 11 years ago

qref'd extra bits with previous patch.

	(no longer active)
	Comment 11 • 11 years ago

Comment on attachment 804836 [details] [diff] [review]
Impl

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

r=me with the below addressed.

::: content/html/content/src/HTMLInputElement.cpp
@@ +4579,5 @@
> +      selectionStart = state->GetSelectionProperties().mStart;
> +      selectionEnd = state->GetSelectionProperties().mEnd;
> +      aRv = NS_OK;
> +    }
> +  }

This work will be needless if you call the other overload.  I suggest you add two optional arguments to this function called aSelectionStart and aSelectionEnd, set them both to -1 by default, pass those values from the other overload, and do this work here only if they're both -1.  This way, calling this overload directly from content script does this work once, and calling the other overload will reuse the same values computed there.

@@ +4626,5 @@
> +  SetSelectionRange(selectionStart, selectionEnd, direction, aRv);
> +  nsContentUtils::DispatchTrustedEvent(OwnerDoc(),
> +                                       static_cast<nsIDOMHTMLInputElement*>(this),
> +                                       NS_LITERAL_STRING("select"), true,
> +                                       false);

Please fire this event at the end of SetSelectionRange, not here.

::: content/html/content/test/forms/test_set_range_text.html
@@ +56,5 @@
> +      elem.focus();
> +      try {
> +        elem.setRangeText("abc");
> +      } catch (ex) {
> +       opThrows = (ex.name == "NotSupportedError" && ex.code == DOMException.NOT_SUPPORTED_ERR);

You forgot to address my comment on this in the previous patch.

	Andrew Quartey [:drexler] Assignee
	Comment 12 • 11 years ago

Fixed nits and pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/ce65d48e182e

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 13 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/ce65d48e182e