Bug 65542 - Need support for dirname attribute
Summary: Need support for dirname attribute
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 76766 (view as bug list)
Depends on: 72003
Blocks: 50910
  Show dependency treegraph
 
Reported: 2011-08-02 08:14 PDT by Aharon (Vladimir) Lanin
Modified: 2012-01-21 01:59 PST (History)
21 users (show)

See Also:


Attachments
test case (386 bytes, text/html)
2011-08-02 08:14 PDT, Aharon (Vladimir) Lanin
no flags Details
Proposed patch (9.38 KB, patch)
2011-10-17 03:32 PDT, Rakesh
no flags Details | Formatted Diff | Diff
Updated patch (11.11 KB, patch)
2011-10-17 22:54 PDT, Rakesh
no flags Details | Formatted Diff | Diff
Updated patch (14.95 KB, patch)
2011-11-03 04:10 PDT, Rakesh
no flags Details | Formatted Diff | Diff
Updated patch (15.06 KB, patch)
2011-11-04 01:07 PDT, Rakesh
no flags Details | Formatted Diff | Diff
Updated patch (17.81 KB, patch)
2011-11-07 23:27 PST, Rakesh
no flags Details | Formatted Diff | Diff
Compilation fix for mac (17.78 KB, patch)
2011-11-10 04:26 PST, Rakesh
no flags Details | Formatted Diff | Diff
Compilation fix for mac (17.81 KB, patch)
2011-11-14 01:31 PST, Rakesh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aharon (Vladimir) Lanin 2011-08-02 08:14:20 PDT
Created attachment 102654 [details]
test case

HTML5 added a new attribute for <input> and <textarea>, dirname (http://dev.w3.org/html5/spec/Overview.html#the-dirname-attribute):

"A form control dirname attribute on a form control element enables the submission of the directionality of the element, and gives the name of the field that contains this value during form submission. If such an attribute is specified, its value must not be the empty string."

Please note that the directionality is always either "ltr" or "rtl". It is never "auto" or "" or null.

Example:

<form action="addcomment.cgi" method=post>
 <p><label>Comment: <input type=text name="comment" dirname="comment.dir" required></label></p>
 <p><button name="mode" type=submit value="add">Post Comment</button></p>
</form>
When the user submits the form, the user agent includes three fields, one called "comment", one called "comment.dir", and one called "mode"; so if the user types "Hello", the submission body might be something like:

comment=Hello&comment.dir=ltr&mode=add
If the user manually switches to a right-to-left writing direction and enters "مرحبًا", the submission body might be something like:

comment=%D9%85%D8%B1%D8%AD%D8%A8%D9%8B%D8%A7&comment.dir=rtl&mode=add
Comment 1 Dongwoo Joshua Im 2011-09-21 20:56:46 PDT
This feature is still not supported. Is there anyone who have plan to implement this?
Comment 2 Rakesh 2011-10-17 03:32:10 PDT
Created attachment 111237 [details]
Proposed patch
Comment 3 Kent Tamura 2011-10-17 03:51:13 PDT
Comment on attachment 111237 [details]
Proposed patch

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

> Source/WebCore/ChangeLog:24
> +        * html/HTMLAttributeNames.in:
> +        * html/HTMLInputElement.cpp:
> +        (WebCore::HTMLInputElement::parseMappedAttribute):
> +        (WebCore::HTMLInputElement::appendFormData):
> +        * html/HTMLInputElement.idl:
> +        * html/HTMLTextAreaElement.cpp:
> +        (WebCore::HTMLTextAreaElement::appendFormData):
> +        * html/HTMLTextAreaElement.idl:
> +        * html/HTMLTextFormControlElement.cpp:
> +        (WebCore::HTMLTextFormControlElement::parseMappedAttribute):
> +        (WebCore::enclosingTextFormControl):
> +        (WebCore::HTMLTextFormControlElement::appendFormData):
> +        * html/HTMLTextFormControlElement.h:

Please write what you changed for each of files/functions.

> Source/WebCore/html/HTMLInputElement.cpp:825
> +    } else if (attr->name() == dirnameAttr) {
> +        HTMLTextFormControlElement::parseMappedAttribute(attr);

This should be removed.
We call HTMLTextFormControlElement::parseMappedAttribute(attr) at the bottom of the function.

> Source/WebCore/html/HTMLInputElement.cpp:941
> +    if (result && fastHasAttribute(dirnameAttr) && (isTextField() || isSearchField()))
> +        result = HTMLTextFormControlElement::appendFormData(encoding, multipart);
> +    return result;

* isSearchField() is redundant because isTextField() is always true if isSearchField() is true.
* This code should be in TextFieldInputType::appendFormData().
  Having dirname processing in HTMLTextFormControlElement::appendFormData() is very strange.
  So, I think we had better add a public helper function to HTMLTextFormControlElement, and it is called from TextFieldInputType::appendFormData() and HTMLTextAreaElemet::appendFormData().

> Source/WebCore/html/HTMLTextFormControlElement.cpp:434
> +    else if (attr->name() == dirnameAttr)
> +        m_dirName = attr->value();

m_dirName is not needed.

> Source/WebCore/html/HTMLTextFormControlElement.cpp:585
> +    else if (hasAttribute(dirAttr))
> +        dir = getAttribute(dirAttr).string();

Should use fastHasAttribute() and fastGetAttribute().

> Source/WebCore/html/HTMLTextFormControlElement.cpp:589
> +    list.appendData(m_dirName.string(), dir);

We can use fastGetAttribute(dirnameAttr)

> LayoutTests/fast/forms/form-dirname-attribute.html:18
> +var input = document.createElement('input');
> +input.setAttribute('dirName', "Hello");
> +shouldBeEqualToString('input.dirName', "Hello");
> +
> +var textArea = document.createElement('textarea');
> +textArea.setAttribute('dirName', "Hello");
> +shouldBeEqualToString('textArea.dirName', "Hello");

Need tests of form submission with dirname attribute.
Comment 4 Rakesh 2011-10-17 22:54:58 PDT
Created attachment 111386 [details]
Updated patch
Comment 5 Rakesh 2011-10-17 22:57:17 PDT
(In reply to comment #3)
> (From update of attachment 111237 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=111237&action=review
> 
Thanks for your inputs, made necessary changes.
Comment 6 Kent Tamura 2011-10-20 18:39:58 PDT
Comment on attachment 111386 [details]
Updated patch

r- because of test coverage.


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

> Source/WebCore/html/HTMLTextFormControlElement.cpp:584
> +    TextDirection textDirection = directionalityIfhasDirAutoAttribute(isAuto);
> +    if (isAuto)
> +        dir = String(textDirection == RTL ? "rtl" : "ltr");
> +    else if (hasAttribute(dirAttr))
> +        dir = getAttribute(dirAttr).string();
> +    else
> +        dir = String("ltr");

I don't know this is a right code to detect direction of the value.
Does any WebKit RTL expert have comment on this?

> Source/WebCore/html/HTMLTextFormControlElement.h:82
> +    String dirNameAttributeFormData() const;

I think the function name is not reasonable.  How about formDataDirection(), or directionForFormData()?

> LayoutTests/fast/forms/submit-form-with-dirname-attribute.html:16
> +<form action="#action" method="GET" name="f">
> +    <p><label>Comment: <input type=text name="comment" dirname="comment.dir" required></label></p>
> +    <p><button name="mode" type=submit value="add">Post Comment</button></p>
> +</form>

The test coverage is too low.  We need to test
* <input> and <textarea>
* The target element has no dir attribute, dir=rtl, dir=ltr, dir=invalid.
* The target element has no dir attribute but an ancestor element has dir= attribute.
* The target element has RTL value, and LTR value with/without dir= attribtue.
Comment 7 Rakesh 2011-10-20 22:57:44 PDT
(In reply to comment #6)
> (From update of attachment 111386 [details])
> r- because of test coverage.
> 
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=111386&action=review
> 
> > Source/WebCore/html/HTMLTextFormControlElement.cpp:584
> > +    TextDirection textDirection = directionalityIfhasDirAutoAttribute(isAuto);
> > +    if (isAuto)
> > +        dir = String(textDirection == RTL ? "rtl" : "ltr");
> > +    else if (hasAttribute(dirAttr))
> > +        dir = getAttribute(dirAttr).string();
> > +    else
> > +        dir = String("ltr");
> 
> I don't know this is a right code to detect direction of the value.
> Does any WebKit RTL expert have comment on this?
> 
Yes, RTL expert can comment on this.

> > Source/WebCore/html/HTMLTextFormControlElement.h:82
> > +    String dirNameAttributeFormData() const;
> 
> I think the function name is not reasonable.  How about formDataDirection(), or directionForFormData()?

directionForFormData() looks better to me.

> The test coverage is too low.  We need to test
> * <input> and <textarea>
> * The target element has no dir attribute, dir=rtl, dir=ltr, dir=invalid.
> * The target element has no dir attribute but an ancestor element has dir= attribute.
> * The target element has RTL value, and LTR value with/without dir= attribtue.

Ok, will cover this tests too.

Thanks for taking time to review.
Comment 8 Eric Seidel (no email) 2011-10-31 14:17:41 PDT
Comment on attachment 111386 [details]
Updated patch

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

> Source/WebCore/html/HTMLTextFormControlElement.cpp:580
> +        dir = String(textDirection == RTL ? "rtl" : "ltr");

Why are you doing these explicit string conversions?
Comment 9 Eric Seidel (no email) 2011-10-31 14:21:26 PDT
Comment on attachment 111386 [details]
Updated patch

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

> Source/WebCore/html/HTMLTextAreaElement.cpp:177
> +    if (fastHasAttribute(dirnameAttr))

is fastHasAttribute correct to use?  What's the difference between fast and normal hasAttribute?

> Source/WebCore/html/HTMLTextAreaElement.cpp:178
> +        encoding.appendData(fastGetAttribute(dirnameAttr).string(), dirNameAttributeFormData());

Why do we need the explicit .string() call?  AtomicStings and Strings convert implicitly jsut fine.

> Source/WebCore/html/HTMLTextFormControlElement.cpp:575
> +    String dir;

No need for a local, just return once you know the value.

> Source/WebCore/html/HTMLTextFormControlElement.cpp:578
> +    TextDirection textDirection = directionalityIfhasDirAutoAttribute(isAuto);

Ifhas?  Bad caps?

> LayoutTests/fast/forms/form-dirname-attribute-expected.txt:4
> +PASS input.dirName is "Hello"
> +PASS textArea.dirName is "Hello"

We're suppose to ignore values other than rtl/ltr, correct?  Do we do that at submission time or at parse time?
Comment 10 Jeremy Moskovich 2011-11-01 03:17:15 PDT
Comment on attachment 111386 [details]
Updated patch

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

disclaimer: I'm not  a WK reviewer

>>> Source/WebCore/html/HTMLTextFormControlElement.cpp:584
>>> +        dir = String("ltr");
>> 
>> I don't know this is a right code to detect direction of the value.
>> Does any WebKit RTL expert have comment on this?
> 
> Yes, RTL expert can comment on this.

Seems really wrong that you're resolving the style yourself here, then again that might be the right way of doing it.
Would  computedStyle()->direction() work?

>> LayoutTests/fast/forms/submit-form-with-dirname-attribute.html:16
>> +</form>
> 
> The test coverage is too low.  We need to test
> * <input> and <textarea>
> * The target element has no dir attribute, dir=rtl, dir=ltr, dir=invalid.
> * The target element has no dir attribute but an ancestor element has dir= attribute.
> * The target element has RTL value, and LTR value with/without dir= attribtue.

Seconded, esp. the "ancestor element with dir= attribute" case.
Comment 11 Yael 2011-11-01 06:33:49 PDT
(In reply to comment #10)
> (From update of attachment 111386 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=111386&action=review
> 
> disclaimer: I'm not  a WK reviewer
> 
> >>> Source/WebCore/html/HTMLTextFormControlElement.cpp:584
> >>> +        dir = String("ltr");
> >> 
> >> I don't know this is a right code to detect direction of the value.
> >> Does any WebKit RTL expert have comment on this?
> > 
> > Yes, RTL expert can comment on this.
> 
> Seems really wrong that you're resolving the style yourself here, then again that might be the right way of doing it.
> Would  computedStyle()->direction() work?
> 
My understanding of the spec is that we need to consider only "dir" attribute and not css style "direction". Perhaps Aharon can confirm that?
Comment 12 Aharon (Vladimir) Lanin 2011-11-01 10:19:16 PDT
(In reply to comment #11)
> (In reply to comment #10)
> > (From update of attachment 111386 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=111386&action=review
> > 
> > disclaimer: I'm not  a WK reviewer
> > 
> > >>> Source/WebCore/html/HTMLTextFormControlElement.cpp:584
> > >>> +        dir = String("ltr");
> > >> 
> > >> I don't know this is a right code to detect direction of the value.
> > >> Does any WebKit RTL expert have comment on this?
> > > 
> > > Yes, RTL expert can comment on this.
> > 
> > Seems really wrong that you're resolving the style yourself here, then again that might be the right way of doing it.
> > Would  computedStyle()->direction() work?
> > 
> My understanding of the spec is that we need to consider only "dir" attribute and not css style "direction". Perhaps Aharon can confirm that?

The HTML spec says that the value has to have "the directionality" of the element, i.e. http://dev.w3.org/html5/spec/Overview.html#the-directionality. This, indeed, is not a function of CSS, but only of the element's (or an ancestor's) dir attribute - and the element's value, if the dir value is auto. (The dirname control's value is always either "ltr" or "rtl" - never "auto".)

When I filed a bug on the HTML spec (http://www.w3.org/Bugs/Public/show_bug.cgi?id=10809) requesting this feature, I had asked for the computed direction style to be reported. Others had also expected this to be the outcome. However, when the HTML spec editor finally agreed to put in the feature (or a version of it that he liked), he formulated it to use the directionality, not the computed direction style. There may have been weighty reasons for that; I did not ask. It does seem a bit backwards to have an HTML feature be dependent on CSS.
Comment 13 Rakesh 2011-11-02 00:19:32 PDT
(In reply to comment #9)
> (From update of attachment 111386 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=111386&action=review
> 
> > Source/WebCore/html/HTMLTextAreaElement.cpp:177
> > +    if (fastHasAttribute(dirnameAttr))
> 
> is fastHasAttribute correct to use?  What's the difference between fast and normal hasAttribute?

As per comment in Element.h, 
// Call this to get the value of an attribute that is known not to be the style
// attribute or one of the SVG animatable attributes.
as dirname is not an style attribute, used fastHasAttribute.

> 
> > Source/WebCore/html/HTMLTextAreaElement.cpp:178
> > +        encoding.appendData(fastGetAttribute(dirnameAttr).string(), dirNameAttributeFormData());
> 
> Why do we need the explicit .string() call?  AtomicStings and Strings convert implicitly jsut fine.
> 

Agreed.

> > Source/WebCore/html/HTMLTextFormControlElement.cpp:575
> > +    String dir;
> 
> No need for a local, just return once you know the value.
> 

Agreed.

> > Source/WebCore/html/HTMLTextFormControlElement.cpp:578
> > +    TextDirection textDirection = directionalityIfhasDirAutoAttribute(isAuto);
> 
> Ifhas?  Bad caps?
> 

directionalityIfhasDirAutoAttribute is an existing api, it would be better if that change happens through another patch.  

> > LayoutTests/fast/forms/form-dirname-attribute-expected.txt:4
> > +PASS input.dirName is "Hello"
> > +PASS textArea.dirName is "Hello"
> 
> We're suppose to ignore values other than rtl/ltr, correct?  Do we do that at submission time or at parse time?

I think that code can be added at HTMLTextFormControlElement::dirNameAttributeFormData(), please let me know if this is not the right place to do.

Thanks for taking time to review.
Comment 14 Rakesh 2011-11-02 00:23:21 PDT
(In reply to comment #10)
> (From update of attachment 111386 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=111386&action=review

> > Yes, RTL expert can comment on this.
> 
> Seems really wrong that you're resolving the style yourself here, then again that might be the right way of doing it.
> Would  computedStyle()->direction() work?
> 

Yes, that would have taken care of even a wrong dir value other that rtl/ltr/auto case also but as suggested in the comment#11 and 12, dir attribute is needed as per spec.
Comment 15 Rakesh 2011-11-02 02:38:38 PDT
Please suggest if following code for finding directionality is correct?

if (fastHasAttribute(dirAttr)) {
    AtomicString dirAttributeValue = fastGetAttribute(dirAttr);
    if (equalIgnoringCase(dirAttributeValue, "rtl") || equalIgnoringCase(dirAttributeValue, "ltr"))
        return dirAttributeValue;             
  
    if (equalIgnoringCase(dirAttributeValue, "auto")) {
        bool isAuto;
        TextDirection textDirection = directionalityIfhasDirAutoAttribute(isAuto);
        return textDirection == RTL ? "rtl" : "ltr";             
    }                                   
}                      
    
return "ltr";

I think 'return "ltr";' should be 'return directionality() == RTL ? "rtl" : "ltr";'
but directionality() is private in HTMLElement.
Comment 16 Yael 2011-11-02 17:37:10 PDT
(In reply to comment #15)
> Please suggest if following code for finding directionality is correct?
> 
> if (fastHasAttribute(dirAttr)) {
>     AtomicString dirAttributeValue = fastGetAttribute(dirAttr);
>     if (equalIgnoringCase(dirAttributeValue, "rtl") || equalIgnoringCase(dirAttributeValue, "ltr"))
>         return dirAttributeValue;             
> 
>     if (equalIgnoringCase(dirAttributeValue, "auto")) {
>         bool isAuto;
>         TextDirection textDirection = directionalityIfhasDirAutoAttribute(isAuto);
>         return textDirection == RTL ? "rtl" : "ltr";             
>     }                                   
> }                      
> 
> return "ltr";
> 
> I think 'return "ltr";' should be 'return directionality() == RTL ? "rtl" : "ltr";'
> but directionality() is private in HTMLElement.

If an element does not have a dir attribute, the default should be "ltr" . 
return "ltr" is the right thing to do here. Directionality should be checked only when there is a dir attribute and its value is "auto".
Comment 17 Aharon (Vladimir) Lanin 2011-11-02 17:57:20 PDT
> If an element does not have a dir attribute, the default should be "ltr" . 
> return "ltr" is the right thing to do here. Directionality should be checked
> only when there is a dir attribute and its value is "auto".

Just making sure that there is no confusion. If an element does not have a dir attribute, one has to look for the closest ancestor that does. If no ancestor has it either, then it's "ltr". If the element (or the closest ancestor) has "auto", one has to check that element's directionality.

Thus:

<html><input dirname=foo></html> -> ltr (No ancestor has dir).
<html><input dirname=foo dir=rtl></html> -> rtl
<html dir=rtl><input dirname=foo></html> -> rtl (Closest ancestor has dir=rtl)
<html><div dir=auto><input dirname=foo value="שלום"> hello</div></html> -> ltr (Closest ancestor has dir=auto, and its first strong content is LTR ("hello"), so it winds up being LTR, and the input inherits that. The input's value is not included in the ancestor text content for the purposes of the ancestor dir=auto estimation, per usual dir=auto rules.)
Comment 18 Rakesh 2011-11-03 04:10:37 PDT
Created attachment 113458 [details]
Updated patch

Added more test converage, changes for finding direction as per commnet#16 and 17 and review comments incorporated
Comment 19 Rakesh 2011-11-03 04:12:10 PDT
(In reply to comment #17)
> > If an element does not have a dir attribute, the default should be "ltr" . 
> > return "ltr" is the right thing to do here. Directionality should be checked
> > only when there is a dir attribute and its value is "auto".
> 
> Just making sure that there is no confusion. If an element does not have a dir attribute, one has to look for the closest ancestor that does. If no ancestor has it either, then it's "ltr". If the element (or the closest ancestor) has "auto", one has to check that element's directionality.
> 
> Thus:
> 
> <html><input dirname=foo></html> -> ltr (No ancestor has dir).
> <html><input dirname=foo dir=rtl></html> -> rtl
> <html dir=rtl><input dirname=foo></html> -> rtl (Closest ancestor has dir=rtl)
> <html><div dir=auto><input dirname=foo value="שלום"> hello</div></html> -> ltr (Closest ancestor has dir=auto, and its first strong content is LTR ("hello"), so it winds up being LTR, and the input inherits that. The input's value is not included in the ancestor text content for the purposes of the ancestor dir=auto estimation, per usual dir=auto rules.)

Thanks Yael and Aharon for the detailed inputs.
Comment 20 Darin Adler 2011-11-03 10:27:47 PDT
Comment on attachment 113458 [details]
Updated patch

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

> Source/WebCore/html/HTMLTextAreaElement.cpp:178
> +    if (fastHasAttribute(dirnameAttr))
> +        encoding.appendData(fastGetAttribute(dirnameAttr), directionForFormData());

This is less efficient than it could be. The right way to write it is:

    const AtomicString& dirname = fastGetAttribute(dirnameAttr);
    if (!dirname.isNull())
        encoding.appendData(dirname, directionForFormData());

> Source/WebCore/html/HTMLTextFormControlElement.cpp:576
> +    const Element* element = this;
> +    while (element) {

This should be written as a for loop, not a while loop.

> Source/WebCore/html/HTMLTextFormControlElement.cpp:578
> +        if (element->fastHasAttribute(dirAttr)) {
> +            AtomicString dirAttributeValue(element->fastGetAttribute(dirAttr));

Not as efficient as it could be. Should be written like this:

    const AtomicString& dirAttributeValue = element->fastGetAttribute(dirAttr);
    if (dirAttributeValue.isNull())
        continue;
    ...

Note that we use “early continue” style instead of nesting the entire loop in an if statement.

> Source/WebCore/html/HTMLTextFormControlElement.cpp:584
> +                TextDirection textDirection = static_cast<const HTMLElement*>(element)->directionalityIfhasDirAutoAttribute(isAuto);

This cast to an HTMLElement can be a bad cast. Webpages can put an HTML element inside a non-HTML element, such as an SVG element. If we want to assume it’s an HTMLElement we need to actually check isHTMLElement. A bad cast can cause crashes, in some cases crashes that are exploitable security vulnerabilities.

> Source/WebCore/html/TextFieldInputType.cpp:375
> +    if (element()->fastHasAttribute(dirnameAttr))
> +        list.appendData(element()->fastGetAttribute(dirnameAttr), element()->directionForFormData());

Same point about a more efficient idiom as I mentioned above.
Comment 21 Rakesh 2011-11-04 00:32:08 PDT
(In reply to comment #20)
> (From update of attachment 113458 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=113458&action=review
> 

Thanks for the inputs, will upload a patch with suggested changes.

> > Source/WebCore/html/HTMLTextFormControlElement.cpp:584
> > +                TextDirection textDirection = static_cast<const HTMLElement*>(element)->directionalityIfhasDirAutoAttribute(isAuto);
> 
> This cast to an HTMLElement can be a bad cast. Webpages can put an HTML element inside a non-HTML element, such as an SVG element. If we want to assume it’s an HTMLElement we need to actually check isHTMLElement. A bad cast can cause crashes, in some cases crashes that are exploitable security vulnerabilities.

Yes, having a isHTMLElement check can avoid crashes. For me to have a better understanding, can non HTML elements have dir attribute? Spec specifies as "If the element's dir attribute is in the auto state", so should 'directionalityIfhasDirAutoAttribute' be an Element's function?
Comment 22 Rakesh 2011-11-04 01:07:40 PDT
Created attachment 113632 [details]
Updated patch
Comment 23 Darin Adler 2011-11-04 09:49:51 PDT
Comment on attachment 113632 [details]
Updated patch

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

review+ because I think we could land just like this

but I didn’t set commit-queue+ because I think you may want to make some improvements based on the comments below

> Source/WebCore/html/HTMLTextFormControlElement.cpp:576
> +        const AtomicString& dirAttributeValue = element->fastGetAttribute(dirAttr);

I think the non-HTMLElement coding here is not quite right. Is it right to do anything for a "dir" attribute on an element that is not an HTML element? I think we probably should just have:

    if (!element->isHTMLElement())
        continue;

at the start of the function, before we do anything with the dir attribute. It would be good to have some test cases for this edge case.

> Source/WebCore/html/HTMLTextFormControlElement.cpp:587
> +        if (equalIgnoringCase(dirAttributeValue, "auto") && element->isHTMLElement()) {
> +            bool isAuto;
> +            TextDirection textDirection = static_cast<const HTMLElement*>(element)->directionalityIfhasDirAutoAttribute(isAuto);
> +            return textDirection == RTL ? "rtl" : "ltr";
> +        }

It’s good that the bad cast is now gone. But I’m not sure this does the right thing when "auto" is specified on an element and the element is not an HTML element. It seems strange to look at the value of a dir attribute, respect it if it’s "rtl" or "ltr", but then continue looking at ancestors if it is “auto”. If this attribute is really specific to HTML elements, then I think we should filter out non-HTML elements earlier as I suggested above.

In fact, we could even write a parentHTMLElement function (not a member function) and use that and not deal with non-HTMLElement objects at all here.

> Source/WebCore/html/TextFieldInputType.h:39
>  class SpinButtonElement;
> +class FormDataList; 

We keep lists like this alphabetical. This is not alphabetical.

> Source/WebCore/html/TextFieldInputType.h:60
> +    virtual bool appendFormData(FormDataList&, bool multipart) const;

This should be protected or private, not public.
Comment 24 Rakesh 2011-11-07 23:27:52 PST
Created attachment 113999 [details]
Updated patch

Changes as per Comment #23
Comment 25 Eric Seidel (no email) 2011-11-09 17:33:18 PST
Comment on attachment 113999 [details]
Updated patch

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

Seems reasonable.

> Source/WebCore/html/HTMLTextAreaElement.cpp:178
> +    if (!dirnameAttrValue.isNull())

Does AtomicString have a bool operator?  Can this just be !dirnameAttrValue ?
Comment 26 WebKit Review Bot 2011-11-09 18:56:57 PST
Comment on attachment 113999 [details]
Updated patch

Rejecting attachment 113999 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
n/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.
Updating chromium port dependencies using gclient...

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/10394190
Comment 27 Rakesh 2011-11-09 21:35:02 PST
(In reply to comment #25)
> (From update of attachment 113999 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=113999&action=review
> 
> Seems reasonable.
> 

Thanks for taking time to review.

> > Source/WebCore/html/HTMLTextAreaElement.cpp:178
> > +    if (!dirnameAttrValue.isNull())
> 
> Does AtomicString have a bool operator?  Can this just be !dirnameAttrValue ?

No, AtomicString does not have a bool operator.
Comment 28 WebKit Review Bot 2011-11-09 22:50:53 PST
Comment on attachment 113999 [details]
Updated patch

Clearing flags on attachment: 113999

Committed r99816: <http://trac.webkit.org/changeset/99816>
Comment 29 WebKit Review Bot 2011-11-09 22:51:06 PST
All reviewed patches have been landed.  Closing bug.
Comment 30 Rakesh 2011-11-10 04:26:44 PST
Created attachment 114473 [details]
Compilation fix for mac
Comment 31 Ryosuke Niwa 2011-11-11 17:07:36 PST
Reopen because the patch was rolled out.
Comment 32 Eric Seidel (no email) 2011-11-13 15:03:01 PST
Comment on attachment 114473 [details]
Compilation fix for mac

Please upload a new one which properly applies on the EWSes.
Comment 33 Rakesh 2011-11-14 01:31:15 PST
Created attachment 114899 [details]
Compilation fix for mac
Comment 34 Eric Seidel (no email) 2011-11-15 13:35:19 PST
Comment on attachment 114899 [details]
Compilation fix for mac

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

What changed from teh previous patch?

> Source/WebCore/ChangeLog:1
> +2011-11-14  Rakesh KN  <rakesh.kn@motorola.com>

Normally we put our full names on this line (I'm assuming your family name is not KN, but maybe I'm wrong?)
Comment 35 Rakesh 2011-11-15 22:28:30 PST
(In reply to comment #34)
> (From update of attachment 114899 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=114899&action=review
> 
> What changed from teh previous patch?
> 
The function parentHTMLElement in Source/WebCore/html/HTMLTextFormControlElement.cpp is now static.

> > Source/WebCore/ChangeLog:1
> > +2011-11-14  Rakesh KN  <rakesh.kn@motorola.com>
> 
> Normally we put our full names on this line (I'm assuming your family name is not KN, but maybe I'm wrong?)

We generally use initials as full name is 4 words long, also I already committed couple of patches with this name.
Comment 36 Eric Seidel (no email) 2011-11-21 13:50:34 PST
Comment on attachment 114899 [details]
Compilation fix for mac

OK.
Comment 37 WebKit Review Bot 2011-11-21 15:13:15 PST
Comment on attachment 114899 [details]
Compilation fix for mac

Clearing flags on attachment: 114899

Committed r100965: <http://trac.webkit.org/changeset/100965>
Comment 38 WebKit Review Bot 2011-11-21 15:13:23 PST
All reviewed patches have been landed.  Closing bug.
Comment 39 Eric Seidel (no email) 2012-01-21 01:59:40 PST
*** Bug 76766 has been marked as a duplicate of this bug. ***