Bug 65542

This feature is still not supported. Is there anyone who have plan to implement this?

Created attachment 111237 [details] Proposed patch

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.

Created attachment 111386 [details] Updated patch

(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 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.

(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 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 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 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.

(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?

(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.

(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.

(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.

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.

(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".

> 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.)

Created attachment 113458 [details] Updated patch Added more test converage, changes for finding direction as per commnet#16 and 17 and review comments incorporated

(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 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.

(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?

Created attachment 113632 [details] Updated patch

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.

Created attachment 113999 [details] Updated patch Changes as per Comment #23

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 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

(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 on attachment 113999 [details] Updated patch Clearing flags on attachment: 113999 Committed r99816: <http://trac.webkit.org/changeset/99816>

All reviewed patches have been landed. Closing bug.

Created attachment 114473 [details] Compilation fix for mac

Reopen because the patch was rolled out.

Comment on attachment 114473 [details] Compilation fix for mac Please upload a new one which properly applies on the EWSes.

Created attachment 114899 [details] Compilation fix for mac

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?)

(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 on attachment 114899 [details] Compilation fix for mac OK.

Comment on attachment 114899 [details] Compilation fix for mac Clearing flags on attachment: 114899 Committed r100965: <http://trac.webkit.org/changeset/100965>

All reviewed patches have been landed. Closing bug.

*** Bug 76766 has been marked as a duplicate of this bug. ***