Bug 60403 – [HTML5] Implement the selectionDirection property on input and textarea

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ojan Vafai]

This was recently added to the HTML spec: http://www.whatwg.org/specs/web-apps/current-work/#dom-textarea/input-selectiondirection. The API looks good to me and this should be straightforward to implement.

Since this is a new feature it will need an email to webkit-dev first.

[Ryosuke Niwa]

Oops, this bug blocks the bug 60529, not the other way around.

[Ryosuke Niwa]

Created attachment 99945 [details]
work in progress

[WebKit Review Bot]

Attachment 99945 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/WebCore.exp.in', u'Source/W..." exit_code: 1

Source/WebCore/editing/VisibleSelection.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gyuyoung Kim]

Comment on attachment 99945 [details]
work in progress

Attachment 99945 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/8999066

[WebKit Review Bot]

Comment on attachment 99945 [details]
work in progress

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

[Early Warning System Bot]

Comment on attachment 99945 [details]
work in progress

Attachment 99945 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9000041

[Gustavo Noronha (kov)]

Comment on attachment 99945 [details]
work in progress

Attachment 99945 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8997200

[WebKit Review Bot]

Comment on attachment 99945 [details]
work in progress

Attachment 99945 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9000058

[WebKit Review Bot]

Comment on attachment 99945 [details]
work in progress

Attachment 99945 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8991486

[Ryosuke Niwa]

Created attachment 100899 [details]
implements selectionDirection property

[Ryosuke Niwa]

Created attachment 100903 [details]
fixed tests

[WebKit Review Bot]

Comment on attachment 100903 [details]
fixed tests

Attachment 100903 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9099020

[Ryosuke Niwa]

(In reply to comment #12)
> (From update of attachment 100903 [details])
> Attachment 100903 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/9099020

It seems like the bot failed to apply the patch properly.

[WebKit Review Bot]

Comment on attachment 100903 [details]
fixed tests

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

New failing tests:
editing/deleting/delete-all-text-in-text-field-assertion.html

[WebKit Review Bot]

Created attachment 100920 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Ryosuke Niwa]

Comment on attachment 100903 [details]
fixed tests

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

> LayoutTests/editing/deleting/delete-all-text-in-text-field-assertion-expected.txt:3
> +EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification

It seems like this rebaseline is only required on Mac.

[Ryosuke Niwa]

Created attachment 101191 [details]
fixed the test

[WebKit Review Bot]

Comment on attachment 101191 [details]
fixed the test

Attachment 101191 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9110515

[WebKit Review Bot]

Comment on attachment 101191 [details]
fixed the test

Attachment 101191 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9115400

[Ryosuke Niwa]

(In reply to comment #19)
> (From update of attachment 101191 [details])
> Attachment 101191 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/9115400

For whatever reason mac ews doesn't like my patch even though it builds perfectly fine on my machine :(

[Ryosuke Niwa]

Corresponding Mozilla bug: https://bugzilla.mozilla.org/show_bug.cgi?id=674558

[Olli Pettay (:smaug)]

FYI, thanks to Ehsan https://bugzilla.mozilla.org/show_bug.cgi?id=674558
is now fixed, and selectionDirection should be supported in FF8.

[Ryosuke Niwa]

Can someone review my patch?  Firefox 8 is going to have this feature implemented.

[Darin Adler]

Comment on attachment 101191 [details]
fixed the test

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

Do you know why the Mac build failed?

review- because of the missing return statement in the JavaScript binding.

> Source/WebCore/bindings/js/JSHTMLInputElementCustom.cpp:76
> +    if (!input->canHaveSelection())
> +        return throwTypeError(exec);

I don’t see tests for this.

> Source/WebCore/bindings/js/JSHTMLInputElementCustom.cpp:85
> +    if (!input->canHaveSelection())
> +        throwTypeError(exec);

I don’t see tests for this.

There’s a missing return statement here, and we need a test that would have discovered it.

> Source/WebCore/bindings/v8/custom/V8HTMLInputElementCustom.cpp:101
> +    if (!imp->canHaveSelection())
> +        return throwError("Accessing selectionDirection on an input element that cannot have a selection.");

You are putting in a custom DOM message in the error object for V8, but not for JavaScriptCore. That’s not good. We want the DOM bindings to be the same.

> Source/WebCore/html/HTMLTextFormControlElement.h:36
> +enum TextFieldSelectionDirection {SelectionHasNoDirection, SelectionHasForwardDirection, SelectionHasBackwardDirection};

Missing spaces inside the braces.

> Source/WebCore/html/HTMLTextFormControlElement.h:58
> +    String selectionDirection() const;

This could return a const String& rather than a String. Might also be slightly better to return a const AtomicString&; the cost to atomize would be a one-time cost in the directionString function.

[Ryosuke Niwa]

Comment on attachment 101191 [details]
fixed the test

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

Thanks for the review!

>> Source/WebCore/bindings/js/JSHTMLInputElementCustom.cpp:85

> 
> I don’t see tests for this.
> 
> There’s a missing return statement here, and we need a test that would have discovered it.

Oops, will fix.  But I'm not sure now I'll test this because selectionDirection also throws an error when the input element can't have selection so I won't be able to observe whether selectionDirection has been modified or not.

>> Source/WebCore/bindings/v8/custom/V8HTMLInputElementCustom.cpp:101
>> +        return throwError("Accessing selectionDirection on an input element that cannot have a selection.");
> 
> You are putting in a custom DOM message in the error object for V8, but not for JavaScriptCore. That’s not good. We want the DOM bindings to be the same.

I simply followed the pattern in the file.  If we want to add a message on JSC, then we should do the same for selectionEnd, selectionStart, setSelectionEnd, and setSelectionEnd and I'd rather do it in a separate patch.

>> Source/WebCore/html/HTMLTextFormControlElement.h:36
>> +enum TextFieldSelectionDirection {SelectionHasNoDirection, SelectionHasForwardDirection, SelectionHasBackwardDirection};
> 
> Missing spaces inside the braces.

Oops, will fix.

>> Source/WebCore/html/HTMLTextFormControlElement.h:58
>> +    String selectionDirection() const;
> 
> This could return a const String& rather than a String. Might also be slightly better to return a const AtomicString&; the cost to atomize would be a one-time cost in the directionString function.

Okay, will try.

[Ryosuke Niwa]

Mac build failure was caused by the following error:

Public API change. There are missing public properties and/or methods from the "DOMHTMLInputElement" class.
- (void)setSelectionRange:(int)start end:(int)end;
Died at WebCore/bindings/scripts//CodeGeneratorObjC.pm line 323.
make: *** [DOMHTMLInputElement.h] Error 25

I've modified PublicDOMInterfaces.h to fix this.

[Ryosuke Niwa]

Created attachment 102428 [details]
Fixed per comment

[Darin Adler]

Comment on attachment 102428 [details]
Fixed per comment

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

> Source/WebCore/bindings/objc/PublicDOMInterfaces.h:632
> -- (void)setSelectionRange:(int)start end:(int)end AVAILABLE_WEBKIT_VERSION_3_0_AND_LATER;
> +- (void)setSelectionRange:(int)start end:(int)end direction:(NSString *)direction AVAILABLE_WEBKIT_VERSION_3_0_AND_LATER;

We can’t just change the Mac OS X public WebKit ObjC interface like this. Getting rid of the old method will break existing applications using WebKit that use it. They’ll get a method not found at runtime, and also won’t compile against newer versions of the WebKit framework.

The point of this file is to catch WebKit developers when we are about to create a binary incompatibility like this one.

To keep compatible, we’ll need setSelectionRange:end: to be implemented somehow. I’m not sure what the best way of accomplishing that is, but one way to do would be by adding optional-argument support to the Objective-C bindings generator.

[Ryosuke Niwa]

(In reply to comment #28)
> We can’t just change the Mac OS X public WebKit ObjC interface like this. Getting rid of the old method will break existing applications using WebKit that use it. They’ll get a method not found at runtime, and also won’t compile against newer versions of the WebKit framework.

Ah, ok.  That's what I suspected as well.

> To keep compatible, we’ll need setSelectionRange:end: to be implemented somehow. I’m not sure what the best way of accomplishing that is, but one way to do would be by adding optional-argument support to the Objective-C bindings generator.

I'll use if-def for now.

[Ryosuke Niwa]

Created attachment 102441 [details]
Reverted Objective-C binding change

[Darin Adler]

Comment on attachment 102441 [details]
Reverted Objective-C binding change

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

> Source/WebCore/html/HTMLTextFormControlElement.cpp:219
> +    DEFINE_STATIC_LOCAL(String, forward, ("forward"));
> +    DEFINE_STATIC_LOCAL(String, backward, ("backward"));
> +
> +    TextFieldSelectionDirection direction = SelectionHasNoDirection;
> +    if (directionString == forward)
> +        direction = SelectionHasForwardDirection;
> +    else if (directionString == backward)
> +        direction = SelectionHasBackwardDirection;

It occurs to me that comparing a String with another String is not significantly faster than comparing a String with a const char*, so we might not get any benefit from\ those static local strings at all.

> Source/WebCore/html/HTMLTextFormControlElement.cpp:347
> +TextFieldSelectionDirection HTMLTextFormControlElement::computeSelectionDirection() const
> +{
> +    Frame* frame = document()->frame();
> +    if (!frame)
> +        return SelectionHasNoDirection;

Should this function ASSERT(isTextFormControl())?

[Ryosuke Niwa]

Comment on attachment 102441 [details]
Reverted Objective-C binding change

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

>> Source/WebCore/html/HTMLTextFormControlElement.cpp:219
>> +        direction = SelectionHasBackwardDirection;
> 
> It occurs to me that comparing a String with another String is not significantly faster than comparing a String with a const char*, so we might not get any benefit from\ those static local strings at all.

Good point.  Will get rid of those static strings.

>> Source/WebCore/html/HTMLTextFormControlElement.cpp:347
>> +        return SelectionHasNoDirection;
> 
> Should this function ASSERT(isTextFormControl())?

Sure. This function is only called in HTMLTextFormControlElement::selectionChanged as far as I know so I can do that.  In fact, computeSelectionStart and computeSelectionEnd probably want the same assertion as well.

[Ryosuke Niwa]

Committed r92088: <http://trac.webkit.org/changeset/92088>