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.
Oops, this bug blocks the bug 60529, not the other way around.
Created attachment 99945 [details] work in progress
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 on attachment 99945 [details] work in progress Attachment 99945 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/8999066
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 on attachment 99945 [details] work in progress Attachment 99945 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9000041
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 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 on attachment 99945 [details] work in progress Attachment 99945 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/8991486
Created attachment 100899 [details] implements selectionDirection property
Created attachment 100903 [details] fixed tests
Comment on attachment 100903 [details] fixed tests Attachment 100903 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9099020
(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 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
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 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.
Created attachment 101191 [details] fixed the test
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 on attachment 101191 [details] fixed the test Attachment 101191 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9115400
(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 :(
Corresponding Mozilla bug: https://bugzilla.mozilla.org/show_bug.cgi?id=674558
FYI, thanks to Ehsan https://bugzilla.mozilla.org/show_bug.cgi?id=674558 is now fixed, and selectionDirection should be supported in FF8.
Can someone review my patch? Firefox 8 is going to have this feature implemented.
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 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.
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.
Created attachment 102428 [details] Fixed per comment
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.
(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.
Created attachment 102441 [details] Reverted Objective-C binding change
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 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.
Committed r92088: <http://trac.webkit.org/changeset/92088>