Andrew Quartey [:drexler] Assignee
	Description • 11 years ago

http://www.whatwg.org/specs/web-apps/current-work/multipage/association-of-controls-and-forms.html#dom-textarea/input-setrangetext.

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

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

Similar to the implementation in bug 850364. In addition, i adjusted where the select event should be fired for HTMLInputElement::SetSelectionRange. This patch broke a few addon-sdk tests which expected the select event to be of type UIEvent for textareas. For example, see:http://mxr.mozilla.org/mozilla-central/source/addon-sdk/source/test/test-selection.js#710. There it's explicitly fired as a UIEvent for the test. However, the spec specifically states that the Event interface should be used when it comes to setRangeText. See step 13 of the spec.

	(no longer active)
	Comment 2 • 11 years ago

(In reply to Andrew Quartey [:drexler] from comment #1)
> Created attachment 808954 [details] [diff] [review]
> Impl
> 
> Similar to the implementation in bug 850364. In addition, i adjusted where
> the select event should be fired for HTMLInputElement::SetSelectionRange.
> This patch broke a few addon-sdk tests which expected the select event to be
> of type UIEvent for textareas. For example,
> see:http://mxr.mozilla.org/mozilla-central/source/addon-sdk/source/test/test-
> selection.js#710. There it's explicitly fired as a UIEvent for the test.
> However, the spec specifically states that the Event interface should be
> used when it comes to setRangeText. See step 13 of the spec.

That seems to be a bug in the jetpack code.

	(no longer active)
	Comment 3 • 11 years ago

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

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

This looks good overall, but I didn't review it thoroughly.  Please let me know when you're ready for review.  And thanks!

::: content/html/content/src/HTMLTextAreaElement.h
@@ +16,5 @@
>  #include "nsEventStates.h"
>  #include "nsStubMutationObserver.h"
>  #include "nsIConstraintValidation.h"
>  #include "mozilla/dom/HTMLFormElement.h"
> +#include "mozilla/dom/HTMLTextAreaElementBinding.h"

Isn't SelectionMode declared in HTMLInputElementBinding.h?

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

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #3)
> Comment on attachment 808954 [details] [diff] [review]
> Impl
> 
> Review of attachment 808954 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good overall, but I didn't review it thoroughly.  Please let me
> know when you're ready for review.  And thanks!
> 
> ::: content/html/content/src/HTMLTextAreaElement.h
> @@ +16,5 @@
> >  #include "nsEventStates.h"
> >  #include "nsStubMutationObserver.h"
> >  #include "nsIConstraintValidation.h"
> >  #include "mozilla/dom/HTMLFormElement.h"
> > +#include "mozilla/dom/HTMLTextAreaElementBinding.h"
> 
> Isn't SelectionMode declared in HTMLInputElementBinding.h?

Yes. Codegen includes HTMLInputElementBinding.h in HTMLTextAreaElementBinding.h.

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

From working with cpearce on an associated bug, according to him when the spec says:" Queue a task to fire a simple event...", it means that the event should be run asynchronously. i.e. the current JS context should finish executing before the code to dispatch the event runs. 

Therefore, i amended HTMLInputElement::SetSelectionRange to fire the select event asynchronously and adjusted the relevant tests.

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

Try Push (Part A + B): https://tbpl.mozilla.org/?tree=Try&rev=59f674eb4925

	(no longer active)
	Comment 7 • 11 years ago

Comment on attachment 812420 [details] [diff] [review]
Part A: Make HTMLInputElement::SetSelectionRange fire select event asynchronously.

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

Oh, right!

	(no longer active)
	Comment 8 • 11 years ago

Comment on attachment 812421 [details] [diff] [review]
Part B: Implement HTMLTextArea.setRangeText

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

::: content/html/content/src/HTMLTextAreaElement.h
@@ +16,5 @@
>  #include "nsEventStates.h"
>  #include "nsStubMutationObserver.h"
>  #include "nsIConstraintValidation.h"
>  #include "mozilla/dom/HTMLFormElement.h"
> +#include "mozilla/dom/HTMLTextAreaElementBinding.h"

Still, it should be enough to just include HTMLInputElementBinding.h here.

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

Nit fixed and pushed to inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/74c696729302
http://hg.mozilla.org/integration/mozilla-inbound/rev/9ee3b91945ce

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

Thanks for the review ehsan! :)

	Ed Morley [:emorley]
	Comment 11 • 11 years ago

Backed out for compilation failures:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=9ee3b91945ce

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e256c2e2d8bf
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/09eb2a6480ae

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

Hmmm, must be other patch i pushed along with this. Try push looks green: https://tbpl.mozilla.org/?tree=Try&rev=19ae3101ba46

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

Repushed: 
http://hg.mozilla.org/integration/mozilla-inbound/rev/dd3a2bdd7edb
http://hg.mozilla.org/integration/mozilla-inbound/rev/1e32e9c242ad

	Phil Ringnalda (:philor)
	Comment 14 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/dd3a2bdd7edb
https://hg.mozilla.org/mozilla-central/rev/1e32e9c242ad