Bug 91907 – Implement setRangeText() on text controls

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Pablo Flouret]

In short, setRangeText "Replaces a range of text with the new text."

Details: http://www.whatwg.org/specs/web-apps/current-work/multipage/association-of-controls-and-forms.html#dom-textarea/input-setrangetext
whatwg thread: http://lists.w3.org/Archives/Public/public-whatwg-archive/2012Apr/0277.html

[Pablo Flouret]

Created attachment 153615 [details]
Patch

[Pablo Flouret]

I ran into some small issues in the spec while implementing this (https://www.w3.org/Bugs/Public/show_bug.cgi?id=18337). They look straightforward to fix, but if it's deemed important, this patch can probably wait for the spec to be fixed.

[Pablo Flouret]

Also, i noticed setSelectionRange (and consequently setRangeText) never fires the "select" event  via HTMLTextFormControlElement::selectionChanged() since the selection is not marked as UserTriggered when set on the frame. Is this intentional?

[Kent Tamura]

You should announce this feature on webkit-dev.

[Kent Tamura]

Comment on attachment 153615 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153615&action=review

> LayoutTests/ChangeLog:9
> +        * fast/forms/text-control-setrangetext.html: Added.
> +

need to add text-control-setrangetext-expected.txt.

> LayoutTests/fast/forms/text-control-setrangetext.html:11
> +<input type=text>
> +<input type=text dir="rtl">

We need to test with all of input types.

[Pablo Flouret]

I'll send an email to webkit-dev, and add the expectation i missed to the patch.

Any ideas on what to do with the gtk issue? I guess the gobject generator doesn't like IDL overloads?

[Kent Tamura]

(In reply to comment #6)
> Any ideas on what to do with the gtk issue? I guess the gobject generator doesn't like IDL overloads?

I don't know much about gobject binding. If you can't resolve the issue, you can disable gobject binding by wrapping functions with #if !defined(LANGUAGE_GOBJECT) || !LANGUAGE_GOBJECT in *.idl.

[Pablo Flouret]

Created attachment 154797 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 154797 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154797&action=review

> Source/WebCore/html/HTMLInputElement.idl:86
> +#if !defined(LANGUAGE_GOBJECT) || !LANGUAGE_GOBJECT
> +        void setRangeText(in DOMString replacement) raises(DOMException);
> +#endif

Why do we have a different interface for Gobject?

> Source/WebCore/html/HTMLTextAreaElement.idl:58
> +#if !defined(LANGUAGE_GOBJECT) || !LANGUAGE_GOBJECT
> +        void setRangeText(in DOMString replacement) raises(DOMException);
> +#endif

Ditto.

[Pablo Flouret]

Comment on attachment 154797 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154797&action=review

New patch adds the expectation file, tests all input types, and fixes a few issues that popped up.

> Source/WebCore/html/HTMLTextFormControlElement.cpp:270
> +    // FIXME: What should happen to the value (as in value()) if there's no renderer?
> +    if (!renderer())
> +        return;

The form control's value should be set even if there's no renderer, right? I'm not sure what would be the correct way of setting it.

[Pablo Flouret]

(In reply to comment #9)
> (From update of attachment 154797 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154797&action=review
> 
> > Source/WebCore/html/HTMLInputElement.idl:86
> > +#if !defined(LANGUAGE_GOBJECT) || !LANGUAGE_GOBJECT
> > +        void setRangeText(in DOMString replacement) raises(DOMException);
> > +#endif
> 
> Why do we have a different interface for Gobject?

GTK build failed on the last patch, seemingly due to the idl overload of the method. I'm not sure what's the proper solution, and i can't build for gtk to play around. Do you have any suggestions?

[Kent Tamura]

(In reply to comment #10)
> (From update of attachment 154797 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154797&action=review
> 
> New patch adds the expectation file, tests all input types, and fixes a few issues that popped up.
> 
> > Source/WebCore/html/HTMLTextFormControlElement.cpp:270
> > +    // FIXME: What should happen to the value (as in value()) if there's no renderer?
> > +    if (!renderer())
> > +        return;
> 
> The form control's value should be set even if there's no renderer, right? I'm not sure what would be the correct way of setting it.

I think so. Why do you check renderer()? Did you have any trouble if you removed the !renderer() check?

[Kent Tamura]

Comment on attachment 154797 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154797&action=review

> Source/WebCore/html/HTMLTextFormControlElement.cpp:245
> +    if (!isTextFormControl())
> +        return;

According to the standard, we should throw InvalidStateError.
Also, HTMLInputElement::isTextFormControl() implementation is wrong. We shouldn't support setRangeText() only for text, search, url, tel, and password.
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-input-element.html#input-type-attr-summary


http://www.whatwg.org/specs/web-apps/current-work/multipage/association-of-controls-and-forms.html#textFieldSelection
> When these methods and attributes are used with input elements while they don't apply, they must throw an InvalidStateError exception.

[Kent Tamura]

(In reply to comment #13)
> Also, HTMLInputElement::isTextFormControl() implementation is wrong. We shouldn't support setRangeText() only for text, search, url, tel, and password.

Oops, shouldn't -> should

[Pablo Flouret]

(In reply to comment #12)
> (In reply to comment #10)
>
> > The form control's value should be set even if there's no renderer, right? I'm not sure what would be the correct way of setting it.
> 
> I think so. Why do you check renderer()? Did you have any trouble if you removed the !renderer() check?

subtreeHasChanged() asserts for a renderer. Eventually the value gets updated via setValueFromRenderer(), that's why i was wondering how to properly update the value if there isn't a renderer.

(In reply to comment #13)
> (From update of attachment 154797 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154797&action=review
> 
> > Source/WebCore/html/HTMLTextFormControlElement.cpp:245
> > +    if (!isTextFormControl())
> > +        return;
> 
> According to the standard, we should throw InvalidStateError.
> Also, HTMLInputElement::isTextFormControl() implementation is wrong. We shouldn't support setRangeText() only for text, search, url, tel, and password.
> http://www.whatwg.org/specs/web-apps/current-work/multipage/the-input-element.html#input-type-attr-summary
> 
> 
> http://www.whatwg.org/specs/web-apps/current-work/multipage/association-of-controls-and-forms.html#textFieldSelection
> > When these methods and attributes are used with input elements while they don't apply, they must throw an InvalidStateError exception.

Ah, hadn't seen those, will fix.

[Pablo Flouret]

Created attachment 155677 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 155677 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155677&action=review

> Source/WebCore/html/HTMLInputElement.idl:86
> +#if !defined(LANGUAGE_GOBJECT) || !LANGUAGE_GOBJECT
> +        void setRangeText(in DOMString replacement) raises(DOMException);
> +#endif

I don't think this is right.

[Ryosuke Niwa]

Comment on attachment 155677 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155677&action=review

> Source/WebCore/html/HTMLInputElement.idl:86
> +#if !defined(LANGUAGE_GOBJECT) || !LANGUAGE_GOBJECT
> +        void setRangeText(in DOMString replacement) raises(DOMException);
> +#endif

I don't think this is right.

[Ryosuke Niwa]

Martin, there's some weird G-object binding here. Could you take a look?

[Pablo Flouret]

Any ideas on a proper fix for overloads on the gobject bindings?

[Martin Robinson]

Zan or Xan more experience with the bindings, but I think what's happening is that the code generator doesn't know how to deal with method overrides in the IDL. What do the other code generators do in this case?

[Pablo Flouret]

(In reply to comment #21)
> Zan or Xan more experience with the bindings, but I think what's happening is that the code generator doesn't know how to deal with method overrides in the IDL. What do the other code generators do in this case?

Yeah, doesn't look like the GObject generator deals with overloads at all. For what is worth, and from a cursory glance, the JSC generator generates a function with a different name for each overload, and one with the proper name that dispatches to the correct one.

Looking around the idl files, CanvasRenderingContext2D declares a whole bunch of drawImage overloads without any #ifdeffery, i wonder what's going on there.

In other places it seems to be handled with custom bindings or other hackery (e.g. XMLHttpRequest.send, window.set{Timeout,Interval}, HTMLSelectElement.add, etc).

[Zan Dobersek]

(In reply to comment #21)
> Zan or Xan more experience with the bindings, but I think what's happening is that the code generator doesn't know how to deal with method overrides in the IDL. What do the other code generators do in this case?

That's exactly the problem, overloading functions support in GObject DOM bindings is non-existent.

I'd recommend skipping the function:
http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm#L213

[Pablo Flouret]

Created attachment 168308 [details]
Patch

[Kent Tamura]

Comment on attachment 168308 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168308&action=review

> Source/WebCore/html/HTMLInputElement.cpp:1882
> +    if (!m_inputType->isTextType() || m_inputType->isEmailField()) {

Please do not add type checking code in HTMLInputElement.  We should add InputType::supportsSelectionAPI().

> Source/WebCore/html/HTMLTextAreaElement.h:65
> +    virtual void setRangeText(const String& replacement, ExceptionCode& ec) OVERRIDE { HTMLTextFormControlElement::setRangeText(replacement, ec); }
> +    virtual void setRangeText(const String& replacement, unsigned start, unsigned end, const String& selectionMode, ExceptionCode& ec) OVERRIDE { HTMLTextFormControlElement::setRangeText(replacement, start, end, selectionMode, ec); }
> +

They look unnecessary.

> Source/WebCore/html/HTMLTextFormControlElement.cpp:252
> +    start = min(start, textLength);
> +    end = min(end, textLength);

They should be std::min.
http://www.webkit.org/coding/coding-style.html#using-in-cpp

> LayoutTests/fast/forms/text-control-setrangetext.html:207
> +runTestsShouldFail("input", { type: "submit" });

We need to test other types; date, datetime, datetime-local, month, time, and week.
Their support status depend on platforms. So we should prepare test files for each of types and put them into the type directories.
e.g.
fast/forms/resource/common-setrangetext.js
fast/forms/text/text-setrangetext.html
fast/forms/search/search-setrangetext.html
fast/forms/input-button/input-button-setrangetext.html
fast/forms/file/file-setrangetext.html
fast/forms/date/date-setrangetext.html
....

[Pablo Flouret]

Created attachment 169309 [details]
Patch

[Kent Tamura]

Comment on attachment 169309 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169309&action=review

> LayoutTests/fast/forms/color/color-setrangetext-expected.txt:11
> +element.value = '0123456789XYZ'
> +PASS element.setRangeText('ABC', 0, 0) threw exception Error: INVALID_STATE_ERR: DOM Exception 11.
> +PASS element.value is "0123456789XYZ"

Because input[type=color] sanitizes the value, element.value should be "#000000"

> LayoutTests/fast/forms/datalist/datalist-setrangetext.html:11
> +runTestsShouldFail("input", { type: "datalist" });

datalist is not an input type.  You don't need add a test.

> LayoutTests/fast/forms/date/date-setrangetext-expected.txt:11
> +PASS element.value is "0123456789XYZ"

element.value should be "" because of value sanitization.

> LayoutTests/fast/forms/datetime/datetime-setrangetext-expected.txt:11
> +PASS element.value is "0123456789XYZ"

element.value should be "" because of value sanitization.

> LayoutTests/fast/forms/datetimelocal/datetimelocal-setrangetext-expected.txt:11
> +PASS element.value is "0123456789XYZ"

element.value should be "" because of value sanitization.

> LayoutTests/fast/forms/month/month-setrangetext-expected.txt:11
> +PASS element.value is "0123456789XYZ"

element.value should be "" because of value sanitization.

> LayoutTests/fast/forms/time/time-setrangetext-expected.txt:11
> +PASS element.value is "0123456789XYZ"

element.value should be "" because of value sanitization.

> LayoutTests/fast/forms/week/week-setrangetext-expected.txt:11
> +PASS element.value is "0123456789XYZ"

element.value should be "" because of value sanitization.

[Pablo Flouret]

Created attachment 169341 [details]
Patch

[WebKit Review Bot]

Comment on attachment 169341 [details]
Patch

Attachment 169341 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14384878

New failing tests:
fast/forms/time/time-setrangetext.html
fast/forms/datetimelocal/datetimelocal-setrangetext.html

[Kent Tamura]

Comment on attachment 169341 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169341&action=review

> LayoutTests/fast/forms/datetimelocal/datetimelocal-setrangetext.html:11
> +runTestsShouldFail("input", { type: "datetimelocal" });

should be
    type: "datetime-local"

> LayoutTests/fast/forms/time/time-setrangetext.html:10
> +description("Test setRangeText() method rs not available in time inputs.");

rs -> is

[Pablo Flouret]

Created attachment 169433 [details]
Patch

Thanks, sorry about that.

[Kent Tamura]

Comment on attachment 169433 [details]
Patch

ok.
Thank you!

[WebKit Review Bot]

Comment on attachment 169433 [details]
Patch

Rejecting attachment 169433 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
commit-queue/Source/WebKit/chromium/third_party/v8-i18n --revision 153 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
47>At revision 153.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/14459262

[Kent Tamura]

Comment on attachment 169433 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169433&action=review

> LayoutTests/ChangeLog:8
> +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).

Need to update/remove this line.

[Pablo Flouret]

Created attachment 169653 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 169653 [details]
Patch for landing

Clearing flags on attachment: 169653

Committed r131969: <http://trac.webkit.org/changeset/131969>