918940 - Implement setRangeText(DOMString) for HTMLTextAreaElement

Status: RESOLVED FIXED

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

Description

[Andrew Quartey [:drexler]]

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");

Comment 1

[Andrew Quartey [:drexler]]

Attachment: 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.

Comment 2

[(no longer active)]

(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.

Comment 3

[(no longer active)]

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?

Comment 4

[Andrew Quartey [:drexler]]

(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.

Comment 5

[Andrew Quartey [:drexler]]

Attachment: Part A: Make HTMLInputElement::SetSelectionRange fire select event asynchronously.

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.

Comment 6

[Andrew Quartey [:drexler]]

Attachment: Part B: Implement HTMLTextArea.setRangeText

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

Comment 7

[(no longer active)]

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!

Comment 8

[(no longer active)]

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.

Comment 9

[Andrew Quartey [:drexler]]

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

Comment 10

[Andrew Quartey [:drexler]]

Thanks for the review ehsan! :)

Comment 11

[Ed Morley [:emorley]]

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

Comment 12

[Andrew Quartey [:drexler]]

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

Comment 13

[Andrew Quartey [:drexler]]

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

Comment 14

[Phil Ringnalda (:philor)]

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