Bug 68592 – wrap attribute of textarea element cannot be accessed by JavaScript

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Dongwoo Joshua Im (dwim)]

"wrap" attribute is newly added into the "textarea" element.
(http://www.w3.org/TR/html5/the-button-element.html#attr-textarea-wrap)

This attribute is successfully working on WebKit, 
but it cannot be accessed by JavaScript.
This attrubute can be accessed by JavaScript on other browsers, such as FireFox and Opera.


That's because this attribute is not inserted in HTMLTextAreaElement.idl file in WebKit.
(I don't know this is intentional or not.)


I think "wrap" attribute should be included in HTMLTextAreaElement.idl file, 
because that is specified in the spec. document.
(http://www.w3.org/TR/html5/the-button-element.html#the-textarea-element)

[Vineet Chaudhary (vineetc)]

Created attachment 108332 [details]
Patch

Added wrap attribute to HTMLTextAreaElement.idl

[Kent Tamura]

Comment on attachment 108332 [details]
Patch

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

> Source/WebCore/html/HTMLTextAreaElement.idl:40
> +        attribute [Reflect] DOMString wrap;

Please insert this line at the next to "row" attribute. We'd like to follow the IDL in the specification as possible.

http://www.whatwg.org/specs/web-apps/current-work/multipage/the-button-element.html#the-textarea-element

Also [Reflect] is wrong in this case.
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-button-element.html#attr-textarea-wrap
If the wrap HTML attribute is missing, we need to return "soft" for the wrap IDL attribute.

> LayoutTests/fast/forms/textarea-wrap-attribute.html:6
> +<textarea wrap="hard" cols="20"> This is a simple text-area1 </textarea> 
> +<textarea wrap="soft"> This is a simple text-area2 </textarea> 

The test coverage is too low.
- wrap HTML attribtue is missing
- wrap HTML attribute has an invalid value
- setting values to wrap IDL attribute
- ...

> LayoutTests/fast/forms/textarea-wrap-attribute.html:22
> +logResult("Text Area with hard wrap","hard", textArea.wrap);

You had better load fast/js/resources/js-test-pre.js, and write shouldBe('textArea.wrap', '"hard"');

[Vineet Chaudhary (vineetc)]

(In reply to comment #2)

Thanks Kent for the review comments.

> http://www.whatwg.org/specs/web-apps/current-work/multipage/the-button-element.html#the-textarea-element
> 
> Also [Reflect] is wrong in this case.
> http://www.whatwg.org/specs/web-apps/current-work/multipage/the-button-element.html#attr-textarea-wrap

I actually confused here because http://www.whatwg.org/specs/web-apps/current-work/multipage/the-button-element.html#dom-textarea-wrap
this links says "The cols, placeholder, required, rows, and wrap attributes must reflect the respective content attributes of the same name."
Could you please comfirm that sure we don't want wrapAttr to be [Reflect] in IDL.

Other review comment I will incorporate with next patch once above thing is clear.

[Kent Tamura]

(In reply to comment #3)
> (In reply to comment #2)
> 
> Thanks Kent for the review comments.
> 
> > http://www.whatwg.org/specs/web-apps/current-work/multipage/the-button-element.html#the-textarea-element
> > 
> > Also [Reflect] is wrong in this case.
> > http://www.whatwg.org/specs/web-apps/current-work/multipage/the-button-element.html#attr-textarea-wrap
> 
> I actually confused here because http://www.whatwg.org/specs/web-apps/current-work/multipage/the-button-element.html#dom-textarea-wrap
> this links says "The cols, placeholder, required, rows, and wrap attributes must reflect the respective content attributes of the same name."
> Could you please comfirm that sure we don't want wrapAttr to be [Reflect] in IDL.
> 
> Other review comment I will incorporate with next patch once above thing is clear.

Oh, I was wrong.
- "wrap" HTML attribute is an enumerated attribute.
- "wrap" IDL attribute reflects the HTML attribute, and not "limited to only known values".
So [Reflect] is correct.  I'm sorry for my confusion.

http://www.whatwg.org/specs/web-apps/current-work/multipage/urls.html#limited-to-only-known-values

[Vineet Chaudhary (vineetc)]

Thanks Kent for clearing my doubt.

But here is another problem.
If we say wrapAttr as [Reflect] in idl, I have test case  where I specify wrap attr but no keyvalue like

<textarea wrap=> This is a simple text-area2 </textarea>

So in script if I do something like

var textArea = document.getElementsByTagName('textarea')[0];
alert(textArea.wrap);

As per spec ( section 4.10.13 The textarea element) it should return me SoftWrap but it returns nothing.

But as wrapAttr is enumerated attribute so as per this (2.8.1 Reflecting content attributes in IDL attributes) http://www.whatwg.org/specs/web-apps/current-work/multipage/urls.html#limited-to-only-known-values
this spec it should return "empty string if the attribute is in a state that has no associated keyword value" which is true in this case as there is no way to return default value.

Both these are contradicting with each other.

I think wrap attribute shouldn't be [Reflect] attribute and should have get/set methods for JS.

Kent/Ian Hickson 

Could you please guide me if I am wrong.

[Ian 'Hixie' Hickson]

Per spec, HTMLTextAreaElement.wrap reflects the wrap="" attribute. It is not "limited to only known values", and it is a DOMString attribute, which means it just returns the content attribute's value directly.

If the markup is <textarea wrap=> then the value of the "wrap" content attribute is the empty string.

i.e. Kent in comment 4 is correct.

HTH.

[Vineet Chaudhary (vineetc)]

Created attachment 108634 [details]
Updated Patch

Please find the attached patch as per the review comments.

[Kent Tamura]

Comment on attachment 108634 [details]
Updated Patch

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

r- because of test style.

> Source/WebCore/html/HTMLTextAreaElement.idl:4
> + * Portions Copyright (C) 2011 Motorola Mobility, Inc.  All rights reserved.

You don't need to add "Portions".

> LayoutTests/fast/forms/textarea-wrap-attribute.html:10
> +<script src="script-tests/textarea-wrap-attribute.js"></script>

Do not add a separated .js file. This way is obsolete.
Please put the content of textarea-wrap-attribute.js into here, and remove textarea-wrap-attribute.js.

> LayoutTests/fast/forms/script-tests/textarea-wrap-attribute.js:10
> +//If wrap attribute present.

This comment (and the followings) is useful information. Please print it to the test result.  You can print it by debug('Check if wrap attribute present').

[Vineet Chaudhary (vineetc)]

Created attachment 108811 [details]
Patch

> > Source/WebCore/html/HTMLTextAreaElement.idl:4
> > + * Portions Copyright (C) 2011 Motorola Mobility, Inc.  All rights reserved.
> You don't need to add "Portions".
Removed portions.

 
> > LayoutTests/fast/forms/textarea-wrap-attribute.html:10
> Do not add a separated .js file. This way is obsolete.
removed textarea-wrap-attribute.js, added the code to textarea-wrap-attribute.html.


> > LayoutTests/fast/forms/script-tests/textarea-wrap-attribute.js:10
> > +//If wrap attribute present.
> This comment (and the followings) is useful information. Please print it to the test result.  You can print it by debug('Check if wrap attribute present').

Added the debug statement to get more information on the test case.

[Kent Tamura]

Comment on attachment 108811 [details]
Patch

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

> LayoutTests/fast/forms/textarea-wrap-attribute.html:17
> +//If wrap attribute is not specified it sould be empty String.

The comment is redundant.  Please remove it.

> LayoutTests/fast/forms/textarea-wrap-attribute.html:21
> +//Check if it sets warpAttr as hard

ditto.

> LayoutTests/fast/forms/textarea-wrap-attribute.html:22
> +debug('<br>Check if it sets warpAttr value hard, should return hard.');

We prefer using no HTML tags for debug().  Please write:

debug('');
debug('Check if ...

> LayoutTests/fast/forms/textarea-wrap-attribute.html:27
> +//Check if it sets warpAttr as soft
> +debug('<br>Check if it sets warpAttr value as soft, should return soft.');

ditto.

> LayoutTests/fast/forms/textarea-wrap-attribute.html:32
> +//If attribute present but no keyVal.
> +debug('<br>Check if warpAttr present but no keyVal specified, should return empty String.');

ditto.

> LayoutTests/fast/forms/textarea-wrap-attribute.html:37
> +//If invalid attribute is set.
> +debug('<br>Check if it sets warpAttr invaild value, should return foo.');

ditto.

[Vineet Chaudhary (vineetc)]

Created attachment 108814 [details]
Updated Patch

Updated patch as per review comments.

[Kent Tamura]

Comment on attachment 108814 [details]
Updated Patch

ok.

[WebKit Review Bot]

Comment on attachment 108814 [details]
Updated Patch

Clearing flags on attachment: 108814

Committed r96096: <http://trac.webkit.org/changeset/96096>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.