[Webkit-unassigned] [Bug 65542] Need support for dirname attribute

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=65542


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #113632|review?                     |review+
               Flag|                            |




--- Comment #23 from Darin Adler <darin at apple.com>  2011-11-04 09:49:51 PST ---
(From update of attachment 113632)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.