Closed Bug 918940 Opened 6 years ago Closed 6 years ago

Implement setRangeText(DOMString) for HTMLTextAreaElement

Categories

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

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: drexler, Assigned: drexler)

Details

(Keywords: dev-doc-needed)

Attachments

(2 files, 1 obsolete file)

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");
Attached patch Impl (obsolete) — Splinter Review
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.
Attachment #808954 - Flags: feedback?(ehsan)
(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 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?
Attachment #808954 - Flags: feedback?(ehsan) → feedback+
(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.
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.
Attachment #808954 - Attachment is obsolete: true
Attachment #812420 - Flags: review?(ehsan)
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!
Attachment #812420 - Flags: review?(ehsan) → review+
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.
Attachment #812421 - Flags: review?(ehsan) → review+
Thanks for the review ehsan! :)
Hmmm, must be other patch i pushed along with this. Try push looks green: https://tbpl.mozilla.org/?tree=Try&rev=19ae3101ba46
https://hg.mozilla.org/mozilla-central/rev/dd3a2bdd7edb
https://hg.mozilla.org/mozilla-central/rev/1e32e9c242ad
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.