Bug 50952 – Inputs of type "text" and "search" should support interoperable "set direction" functionality

• Component: DOM
• Status: RESOLVED FIXED

[Xiaomei Ji]

Following is from the spec:

4.10.7.1.2 Text state and Search state

When an input element's type attribute is in the Text state or the Search state, the rules in this section apply.

The input element represents a one line plain text edit control for the element's value.

The difference between the Text state and the Search state is primarily stylistic: on platforms where search fields are distinguished from regular text fields, the Search state might result in an appearance consistent with the platform's search fields rather than appearing like a regular text field.

If the element is mutable, its value should be editable by the user. User agents must not allow users to insert U+000A LINE FEED (LF) or U+000D CARRIAGE RETURN (CR) characters into the element's value.

If the element is mutable, the user agent should allow the user to change the writing direction of the element, setting it either to a left-to-right writing direction or a right-to-left writing direction. If the user does so, the user agent must then run the following steps:

Set the element's dir attribute to "ltr" if the user selected a left-to-right writing direction, and "rtl" if the user selected a right-to-left writing direction.

Queue a task to first fire a simple event that bubbles named input at the input element, and to then broadcast forminput events from the input element's form owner, if any.

The value attribute, if specified, must have a value that contains no U+000A LINE FEED (LF) or U+000D CARRIAGE RETURN (CR) characters.

The value sanitization algorithm is as follows: Strip line breaks from the value.

The following common input element content attributes, IDL attributes, and methods apply to the element: autocomplete, dirname, list, maxlength, pattern, placeholder, readonly, required, and size content attributes; list, selectedOption, selectionStart, selectionEnd, and value IDL attributes; select() and setSelectionRange() methods.
The value IDL attribute is in mode value.
The input and change events apply.
The following content attributes must not be specified and do not apply to the element: accept, alt, checked, formaction, formenctype, formmethod, formnovalidate, formtarget, height, max, min, multiple, src, step, and width.
The following IDL attributes and methods do not apply to the element: checked, files, valueAsDate, and valueAsNumber IDL attributes; stepDown() and stepUp() methods.

[Aharon (Vladimir) Lanin]

(In reply to comment #0)
Please note that the equivalent requirements have been added to textarea elements, i.e.:

<quote>

4.10.13 The textarea element

[...]

If the element is mutable, the user agent should allow the user to change the writing direction of the element, setting it either to a left-to-right writing direction or a right-to-left writing direction. If the user does so, the user agent must then run the following steps:

Set the element's dir attribute to "ltr" if the user selected a left-to-right writing direction, and "rtl" if the user selected a right-to-left writing direction.

Queue a task to first fire a simple event that bubbles named input at the textarea element, and to then broadcast forminput events from the textarea element's form owner, if any.

</quote>

Implementation status:


Chrome: Direction is set using keyboard shortcuts - CTRL + LEFT SHIFT for LTR and CTRL + RIGHT SHIFT for RTL. They set the value of the element's dir attribute as required. They trigger the keyup event, at which time the dir value is already changed, which is good. However, they do not trigger the input event - which is what needs to be changed to comply with the spec.

Safari: Right-click on the <input> or <textarea> provides a "Set paragraph direction" submenu. Using "Set paragraph direction" sets the value of the element's dir attribute as required. However, it does not trigger the input event - which is what needs to be changed to comply with the spec.

[Hironori Bono]

Greetings,

This is just for clarification.
The forminput event has been deleted from the HTML5 spec this month <http://www.w3.org/Bugs/Public/show_bug.cgi?id=11129>. Therefore, this issue is that we need to send an input event of HTML5 from this function when WebKit changes the text direction, right?
For what it's worth, both Chrome and Safari call Editor::setBaseWritingDirection() to change the text direction. So, it seems we just add code that calls HTMLElement::dispatchEvent() there.

Regards,

Hironori Bono

> Chrome: Direction is set using keyboard shortcuts - CTRL + LEFT SHIFT for LTR and CTRL + RIGHT SHIFT for RTL. They set the value of the element's dir attribute as required. They trigger the keyup event, at which time the dir value is already changed, which is good. However, they do not trigger the input event - which is what needs to be changed to comply with the spec.
> 
> Safari: Right-click on the <input> or <textarea> provides a "Set paragraph direction" submenu. Using "Set paragraph direction" sets the value of the element's dir attribute as required. However, it does not trigger the input event - which is what needs to be changed to comply with the spec.

[Aharon (Vladimir) Lanin]

(In reply to comment #2)
> Greetings,
> 
> This is just for clarification.
> The forminput event has been deleted from the HTML5 spec this month <http://www.w3.org/Bugs/Public/show_bug.cgi?id=11129>. Therefore, this issue is that we need to send an input event of HTML5 from this function when WebKit changes the text direction, right?

That is correct. The HTML5 spec before that change called for sending both input and forminput. After the change, it just calls for sending input. I am quoting from http://dev.w3.org/html5/spec/Overview.html#text-state-and-search-state:

"If the element is mutable, the user agent should allow the user to change the writing direction of the element, setting it either to a left-to-right writing direction or a right-to-left writing direction. If the user does so, the user agent must then run the following steps:

"Set the element's dir attribute to "ltr" if the user selected a left-to-right writing direction, and "rtl" if the user selected a right-to-left writing direction.

"Queue a task to fire a simple event that bubbles named input at the input element."

[Hironori Bono]

Created attachment 87313 [details]
A quick fix v1

Greetings Aharon,

(In reply to comment #3)
> That is correct. The HTML5 spec before that change called for sending both input and forminput. After the change, it just calls for sending input. I am quoting from http://dev.w3.org/html5/spec/Overview.html#text-state-and-search-state:
> 
> "If the element is mutable, the user agent should allow the user to change the writing direction of the element, setting it either to a left-to-right writing direction or a right-to-left writing direction. If the user does so, the user agent must then run the following steps:
> "Set the element's dir attribute to "ltr" if the user selected a left-to-right writing direction, and "rtl" if the user selected a right-to-left writing direction.
> "Queue a task to fire a simple event that bubbles named input at the input element."

Thank you for your clarification. I have written a change that sends an input event after changing the "dir" attribute. Unfortunately, the layout test in this change works only on Safari and Chrome because it requires a new JavaScript function. (Should I change 'Skipped' files?)

Regards,

Hironori Bono

[Eric Seidel (no email)]

Dimitri likes events. :)

[Eric Seidel (no email)]

Comment on attachment 87313 [details]
A quick fix v1

Lovely patch.  Seems reasonable to me.

[Eric Seidel (no email)]

Comment on attachment 87313 [details]
A quick fix v1

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

> Source/WebCore/editing/Editor.cpp:1554
> +        focusedNode->dispatchInputEvents();

The only danger here is that callers might not expect that javascript gets executed here.  JS can do all sorts of bad bad things to our state. :)

[Eric Seidel (no email)]

Letting the security folks see we have another JS hook.  I doubt there is anything to worry about, but htey might be curious.

[Build Bot]

Attachment 87313 [details] did not build on win:
Build output: http://queues.webkit.org/results/8282339

[Hironori Bono]

Greetings,

(In reply to comment #9)
> Attachment 87313 [details] did not build on win:
> Build output: http://queues.webkit.org/results/8282339

Ouch. I forgot testing this change with Win. I temporarily add an empty function since I do not have development environment for Win Safari. (I will implement it when I get it.)

Regards,

Hironori Bono

[Hironori Bono]

Created attachment 87346 [details]
A quick fix v2

Greetings,

I would like to send a review request of another patch to test it does not cause build breaks on Win. 

Regards,

Hironori Bono

Comment on attachment 87313 [details]
A quick fix v1

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

> Source/WebKit/mac/WebView/WebViewPrivate.h:594
> +    @result Returns the default minimum timer interval.

This comment is wrong.

> Source/WebKit/mac/WebView/WebViewPrivate.h:596
> +- (void)_setTextDirection:(int)direction;

I think this SPI is unnecessary, and therefore should not be added. It’s especially bad that it takes an int parameter whose meaning is not clear at all.

WebView has -makeBaseWritingDirectionLeftToRight: and -makeBaseWritingDirectionRightToLeft: and a degenerate implementation of -makeBaseWritingDirectionNatural:. Can DumpRenderTree use those?

Comment on attachment 87346 [details]
A quick fix v2

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

> Source/WebKit/mac/WebView/WebViewPrivate.h:596
> +    @method setTextDirection:
> +    @discussion
> +    @result Returns the default minimum timer interval.
> +*/
> +- (void)_setTextDirection:(int)direction;

Please see my comments about this SPI in the review of the previous patch.

[Hironori Bono]

Greetings,

Thank you for your comments.

(In reply to comment #13)
> Please see my comments about this SPI in the review of the previous patch.

Sorry. I have uploaded the second patch before reading your comments. I'm now applying them.

Regards,

Hironori Bono

[Abhishek Arya]

(In reply to comment #8)
> Letting the security folks see we have another JS hook.  I doubt there is anything to worry about, but htey might be curious.

We should probably have a testcase here showing that we donot crash when we blow/delete ourselves out while inside the input event. We have had several of those problems and it is better to check it now than discover later.

[Hironori Bono]

Greetings,

Thank you for your comments.

(In reply to comment #12)

> WebView has -makeBaseWritingDirectionLeftToRight: and -makeBaseWritingDirectionRightToLeft: and a degenerate implementation of -makeBaseWritingDirectionNatural:. Can DumpRenderTree use those?

I would like to clarify this comment a little.
If I understand WebView correctly, these methods seem to be internal ones. That is, we need to add them to WebViewPrivate.h (or WebView.h) to use them directly from the outside of WebKit, including DumpRenderTree. (I have added a new SPI [setTextDirection:direction] just because it may be better not to expose existing internal functions. Nevertheless, I do not stick to my opinion at all.) If it is OK for you, I will update my change to expose them.

Regards,

Hironori Bono

(In reply to comment #16)
> Greetings,
> 
> Thank you for your comments.
> 
> (In reply to comment #12)
> 
> > WebView has -makeBaseWritingDirectionLeftToRight: and -makeBaseWritingDirectionRightToLeft: and a degenerate implementation of -makeBaseWritingDirectionNatural:. Can DumpRenderTree use those?
> 
> I would like to clarify this comment a little.
> If I understand WebView correctly, these methods seem to be internal ones.

In Mac OS X 10.6 and later, these are NSResponder methods, and WebView is an NSResponder.

> That is, we need to add them to WebViewPrivate.h (or WebView.h) to use them directly from the outside of WebKit, including DumpRenderTree.

I would just add them in a category in DumpRenderTree when building on Leopard or Tiger.

[Gustavo Noronha (kov)]

Attachment 87313 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8284406

[Gustavo Noronha (kov)]

Attachment 87346 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8280392

[Hironori Bono]

Created attachment 87536 [details]
A quick fix v3

Greetings,

(In reply to comment #17)
> In Mac OS X 10.6 and later, these are NSResponder methods, and WebView is an NSResponder.
> I would just add them in a category in DumpRenderTree when building on Leopard or Tiger.

Ah, thank you for correcting me. I notice I read an old document that did not include these tasks. I have updated my change to use them and also added my new test to 'Skipped' files.

Regards,

Hironori Bono

[Eric Seidel (no email)]

Comment on attachment 87536 [details]
A quick fix v3

Looks reasonable to me.  mitz might have comment yet, but this looks very clean.

[Alexey Proskuryakov]

Comment on attachment 87536 [details]
A quick fix v3

I think that this needs another iteration.

+        // Send an input event to this element as written in "4.10.7.1.2 Text state and Search state".
+        // (See <http://dev.w3.org/html5/spec/Overview.html#text-state-and-search-state>.)

I don't think that we should have such comments in code. Pretty much every line of code in WebCore could have a link to HTML5, so it's pointless.

What is important is that regression tests cover such things, and that those tests make it clear why they require such behavior. This test doesn't explain anything, provoking the person who breaks it to just "update the results".

+        focusedNode->dispatchInputEvents();

What guarantees that focusedNode still exists after style update?

+        This adds a new layout test to verify that we can reveive input events

Typo: receive.

+    // Call [NSResponder makeBaseWritingDirection*:] since WebView implements
+    // them and it calls Editor::setBaseWritingDirection().

Another comment that should be removed in my opinion. Of course we call a function that is implemented - why would we call an unimplemented one?

+    if (JSStringIsEqualToUTF8CString(directionName, "ltr"))
+        [[mainFrame webView] makeBaseWritingDirectionLeftToRight:0];
+    else if (JSStringIsEqualToUTF8CString(directionName, "rtl"))
+        [[mainFrame webView] makeBaseWritingDirectionRightToLeft:0];

I suggest adding a check for invalid values.

--- LayoutTests/fast/html/script-tests/set-text-direction.js	(revision 0)

Please don't split tests into .html and .js parts.

+// This also removes the element to verify WebKit works without crashes.

Perhaps we should force garbage collection to make the test stronger?

[Hironori Bono]

Created attachment 87711 [details]
A quick fix v4

Thank you for your review and comments.

(In reply to comment #22)
> I don't think that we should have such comments in code. Pretty much every line of code in WebCore could have a link to HTML5, so it's pointless.
> What is important is that regression tests cover such things, and that those tests make it clear why they require such behavior. This test doesn't explain anything, provoking the person who breaks it to just "update the results".

Ah, right. Even though my layout test describes "WebKit sends an input event", this test does not check if it actually sends the event. I have added a flag to verify it. (Please feel free to blame me if I'm wrong.)
 
> +        focusedNode->dispatchInputEvents();
> 
> What guarantees that focusedNode still exists after style update?

In my honest opinion, I'm not totally sure what the style update does, i.e. I cannot guarantee it at all. As far as I quickly read the code, Element::setAttribute() does not seem to delete the node. So, I have moved this call after the Element::setAttribute(). (This analysis may be wrong, though.)
 
> +        This adds a new layout test to verify that we can reveive input events
>
> Typo: receive.

Thank you for noticing it.
 
> +    // Call [NSResponder makeBaseWritingDirection*:] since WebView implements
> +    // them and it calls Editor::setBaseWritingDirection().
> 
> Another comment that should be removed in my opinion. Of course we call a function that is implemented - why would we call an unimplemented one?

OK. I have removed it.
 
> +    if (JSStringIsEqualToUTF8CString(directionName, "ltr"))
> +        [[mainFrame webView] makeBaseWritingDirectionLeftToRight:0];
> +    else if (JSStringIsEqualToUTF8CString(directionName, "rtl"))
> +        [[mainFrame webView] makeBaseWritingDirectionRightToLeft:0];
> 
> I suggest adding a check for invalid values.

Thank you. I have added a ASSERT_NOT_REACHED() call here.

> --- LayoutTests/fast/html/script-tests/set-text-direction.js    (revision 0)
> 
> Please don't split tests into .html and .js parts.

Sorry. It seems I misunderstood the script tests. (I thought a script test must consists of an .html file and a .js one.)

> +// This also removes the element to verify WebKit works without crashes.
> 
> Perhaps we should force garbage collection to make the test stronger?

It makes sense. I have copied the code that force GC from 'fast/dom/gc-1.html'.

Finally, my WebKit knowledge is rusty and I may misunderstand it. So, please feel free to blame me if I'm wrong.

Regards,

Hironori Bono

[Alexey Proskuryakov]

> In my honest opinion, I'm not totally sure what the style update does, i.e. I cannot guarantee it at all. As far as I quickly read the code, Element::setAttribute() does not seem to delete the node. So, I have moved this call after the Element::setAttribute(). (This analysis may be wrong, though.)

Seems fine to me, but I'm also not sure about how style update should be used in editing code.

> It makes sense. I have copied the code that force GC from 'fast/dom/gc-1.html'.

There is no need to force gc with your own code - js-test-pre.js provides a nice gc() function.

[Eric Seidel (no email)]

Comment on attachment 87711 [details]
A quick fix v4

Still LGTM.

[Hironori Bono]

Created attachment 88186 [details]
A quick fix v5

Greetings Alexey and Eric,

Thank you for your reviews and sorry for my slow response. I have updated my layout test to apply the comment from Alexey. It would be definitely helpful to give me your feedback.

(In reply to comment #24)
> There is no need to force gc with your own code - js-test-pre.js provides a nice gc() function.

Thank you for noticing it. It is really nice and I would love to use this function.

(In reply to comment #25)
> Still LGTM.

Thank you again for your positive review. Even though the updated one is almost as same as the previous one, would it be possible to review the updated one?

Regards,

Hironori Bono

[Eric Seidel (no email)]

Comment on attachment 87313 [details]
A quick fix v1

Cleared Eric Seidel's review+ from obsolete attachment 87313 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Eric Seidel (no email)]

Comment on attachment 88186 [details]
A quick fix v5

LGTM.

[Hironori Bono]

Comment on attachment 88186 [details]
A quick fix v5

Greetings,

Sorry for my slow response. Unfortunately, I'm not a WebKit committer and cannot land this change. Is it possible to land this change on my behalf?

Regards,

Hironori Bono

[WebKit Commit Bot]

Comment on attachment 88186 [details]
A quick fix v5

Rejecting attachment 88186 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'apply-..." exit_code: 2

Last 500 characters of output:
orm/mac-tiger/Skipped	(working copy)
--------------------------
No file to patch.  Skipping patch.
1 out of 1 hunk ignored
patching file LayoutTests/platform/qt/Skipped
Hunk #1 FAILED at 3236.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/qt/Skipped.rej
patching file LayoutTests/platform/win/Skipped
Hunk #1 succeeded at 989 (offset 13 lines).

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Eric Seidel', u'--force']" exit_code: 1

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

[Xiaomei Ji]

Hi Hironori,

I do not know why it is rejected in commit-queue.
But could you upload a new version (merged with the latest webkit) and give it a try again?

[Hironori Bono]

Greetings Xiaomei,

(In reply to comment #31)
> Hi Hironori,
> 
> I do not know why it is rejected in commit-queue.
> But could you upload a new version (merged with the latest webkit) and give it a try again?

It took long time since I have uploaded this change and it seems Skipped files have been changed since I uploaded the change. I will ask Tokyo committers to land this change manually.

Regards,

Hironori Bono

[Eric Seidel (no email)]

Sorry hbono.  Seems you slipped through the cracks.  You were nominated (and passed) as a committer over a month ago, but somehow I think the paperwork was never sent your way.

[Hajime Morrita]

Committed r87770: <http://trac.webkit.org/changeset/87770>

[Aharon (Vladimir) Lanin]

Created attachment 102483 [details]
test case

As far as I can see, the bug is fixed in neither Chrome 12.0 nor Safari 5.1, neither for input nor for textarea. To reproduce:

1. Open the attached setdir.html
2. Click on the input and type a. You will see a period appear above the input indicating that an input event has been fired.
3. In Chrome on Windows, press Ctrl-Right Shift; in Safari, right-click on the input and choose RTL paragraph direction.
Expected behavior: "rtl." should appear next to the period placed above the input earlier. This would indicate that an input event has been fired.
Actual behavior: nothing happens.
4. Click on the input and type b. You will see "rtl." appear next to the period placed above the input earlier.
5. Repeat 1-4 using the textarea instead.

In other words, the user setting an element's direction sets its dir attribute, but does not fire the input event, which is what this bug is all about.

[Hironori Bono]

Greetings Aharon,

Thank you for your update.

(In reply to comment #35)
> As far as I can see, the bug is fixed in neither Chrome 12.0 nor Safari 5.1, neither for input nor for textarea. To reproduce:

When I test your test case with Chrome 14.0.835.2, it sends input events as you expected. (I think Chrome 12 does not merge my WebKit r87770.) Would it be possible to install a dev channel or a canary build as written in <http://www.chromium.org/getting-involved/dev-channel> and try it?

Regards,

Hironori Bono

[Aharon (Vladimir) Lanin]

(In reply to comment #36)
> Greetings Aharon,
> 
> Thank you for your update.
> 
> (In reply to comment #35)
> > As far as I can see, the bug is fixed in neither Chrome 12.0 nor Safari 5.1, neither for input nor for textarea. To reproduce:
> 
> When I test your test case with Chrome 14.0.835.2, it sends input events as you expected. (I think Chrome 12 does not merge my WebKit r87770.) Would it be possible to install a dev channel or a canary build as written in <http://www.chromium.org/getting-involved/dev-channel> and try it?
> 
> Regards,
> 
> Hironori Bono

Oops, you are absolutely correct. The canary build works as advertised.