Bug 60403

Summary: [HTML5] Implement the selectionDirection property on input and textarea
Product: WebKit Reporter: Ojan Vafai <ojan>
Component: HTML EditingAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: adele, bugs, darin, dglazkov, ehsan, enrica, gns, kalman, koz, leviw, rniwa, tkent, tony, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 64118, 64133, 64134    
Bug Blocks: 60529    
Attachments:
Description Flags
work in progress
none
implements selectionDirection property
none
fixed tests
none
Archive of layout-test-results from ec2-cr-linux-02
none
fixed the test
none
Fixed per comment
none
Reverted Objective-C binding change darin: review+

Description Ojan Vafai 2011-05-06 14:24:33 PDT
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.
Comment 1 Ryosuke Niwa 2011-07-06 16:55:07 PDT
Oops, this bug blocks the bug 60529, not the other way around.
Comment 2 Ryosuke Niwa 2011-07-06 23:44:38 PDT
Created attachment 99945 [details]
work in progress
Comment 3 WebKit Review Bot 2011-07-07 00:16:27 PDT
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.
Comment 4 Gyuyoung Kim 2011-07-07 00:23:19 PDT
Comment on attachment 99945 [details]
work in progress

Attachment 99945 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/8999066
Comment 5 WebKit Review Bot 2011-07-07 00:28:01 PDT
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
Comment 6 Early Warning System Bot 2011-07-07 00:30:54 PDT
Comment on attachment 99945 [details]
work in progress

Attachment 99945 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9000041
Comment 7 Gustavo Noronha (kov) 2011-07-07 01:51:59 PDT
Comment on attachment 99945 [details]
work in progress

Attachment 99945 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8997200
Comment 8 WebKit Review Bot 2011-07-07 01:56:32 PDT
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
Comment 9 WebKit Review Bot 2011-07-07 01:57:04 PDT
Comment on attachment 99945 [details]
work in progress

Attachment 99945 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8991486
Comment 10 Ryosuke Niwa 2011-07-14 17:08:07 PDT
Created attachment 100899 [details]
implements selectionDirection property
Comment 11 Ryosuke Niwa 2011-07-14 17:39:01 PDT
Created attachment 100903 [details]
fixed tests
Comment 12 WebKit Review Bot 2011-07-14 18:44:50 PDT
Comment on attachment 100903 [details]
fixed tests

Attachment 100903 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9099020
Comment 13 Ryosuke Niwa 2011-07-14 18:47:59 PDT
(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.
Comment 14 WebKit Review Bot 2011-07-14 20:27:09 PDT
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
Comment 15 WebKit Review Bot 2011-07-14 20:27:15 PDT
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
Comment 16 Ryosuke Niwa 2011-07-14 21:57:52 PDT
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.
Comment 17 Ryosuke Niwa 2011-07-18 12:35:09 PDT
Created attachment 101191 [details]
fixed the test
Comment 18 WebKit Review Bot 2011-07-18 13:37:57 PDT
Comment on attachment 101191 [details]
fixed the test

Attachment 101191 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9110515
Comment 19 WebKit Review Bot 2011-07-18 13:59:01 PDT
Comment on attachment 101191 [details]
fixed the test

Attachment 101191 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9115400
Comment 20 Ryosuke Niwa 2011-07-18 15:10:28 PDT
(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 :(
Comment 21 Ryosuke Niwa 2011-07-27 10:33:14 PDT
Corresponding Mozilla bug: https://bugzilla.mozilla.org/show_bug.cgi?id=674558
Comment 22 Olli Pettay (:smaug) 2011-07-29 03:12:54 PDT
FYI, thanks to Ehsan https://bugzilla.mozilla.org/show_bug.cgi?id=674558
is now fixed, and selectionDirection should be supported in FF8.
Comment 23 Ryosuke Niwa 2011-07-29 11:15:44 PDT
Can someone review my patch?  Firefox 8 is going to have this feature implemented.
Comment 24 Darin Adler 2011-07-29 16:18:03 PDT
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.
Comment 25 Ryosuke Niwa 2011-07-29 16:45:30 PDT
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.
Comment 26 Ryosuke Niwa 2011-07-29 22:16:23 PDT
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.
Comment 27 Ryosuke Niwa 2011-07-29 23:55:18 PDT
Created attachment 102428 [details]
Fixed per comment
Comment 28 Darin Adler 2011-07-30 10:38:25 PDT
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.
Comment 29 Ryosuke Niwa 2011-07-30 15:20:40 PDT
(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.
Comment 30 Ryosuke Niwa 2011-07-30 15:23:51 PDT
Created attachment 102441 [details]
Reverted Objective-C binding change
Comment 31 Darin Adler 2011-07-30 20:18:49 PDT
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())?
Comment 32 Ryosuke Niwa 2011-07-30 20:27:41 PDT
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.
Comment 33 Ryosuke Niwa 2011-07-31 13:45:04 PDT
Committed r92088: <http://trac.webkit.org/changeset/92088>