Bug 140925 – HTMLElement.dir should only return known values

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Chris Dumez]

HTMLElement.dir should only return known values according to the HTML specification:
https://html.spec.whatwg.org/multipage/dom.html#dom-document-dir

Chrome and Firefox 28 match the specification. IE11's behavior is slightly different:
- When setting an unknown 'dir' value, an exception will be thrown. (stricter than the spec)
- Setting a known 'dir' value in a non-canonical case (e.g. "RTL"), the value will be directly converted to its canonical form (e.g. "rtl") before updating the DOM tree. (close to the spec but changing the case upon setting).

[Chris Dumez]

Created attachment 245425 [details]
Patch

[Darin Adler]

Comment on attachment 245425 [details]
Patch

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

I made some comments but I still set commit-queue+. We can refine with a follow-up patch, I suppose.

> Source/WebCore/html/HTMLElement.cpp:466
> +// Returns the conforming 'dir' value associated with the state the attribute is in (in its canonical case), if any,
> +// or the empty string if the attribute is in a state that has no associated keyword value or if the attribute is
> +// not in a defined state (e.g. the attribute is missing and there is no missing value default).
> +// http://www.whatwg.org/specs/web-apps/current-work/multipage/common-dom-interfaces.html#limited-to-only-known-values

This comment is too long. There are patterns like this throughout the DOM and I don’t think every one needs such a long comment.

> Source/WebCore/html/HTMLElement.cpp:467
> +static inline const AtomicString& toValidDirValue(const AtomicString& value)

I don’t think this needs to be a separate function. I would just put this logic inside the dir function.

> Source/WebCore/html/HTMLElement.cpp:471
> +    static NeverDestroyed<AtomicString> ltrValue("ltr", AtomicString::ConstructFromLiteral);
> +    static NeverDestroyed<AtomicString> rtlValue("rtl", AtomicString::ConstructFromLiteral);
> +    static NeverDestroyed<AtomicString> autoValue("auto", AtomicString::ConstructFromLiteral);

Why not define these inside the return statements instead of in a block at the top of the function? That would actually be slightly more efficient. We could also even consider adding these strings to the “commonly used atomic strings” list in AtomicString.h.

[WebKit Commit Bot]

Comment on attachment 245425 [details]
Patch

Clearing flags on attachment: 245425

Committed r179181: <http://trac.webkit.org/changeset/179181>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.