Bug 140925 - HTMLElement.dir should only return known values
Summary: HTMLElement.dir should only return known values
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL: https://src.chromium.org/viewvc/blink...
Keywords: BlinkMergeCandidate
Depends on:
Blocks:
 
Reported: 2015-01-26 22:13 PST by Chris Dumez
Modified: 2015-01-27 08:00 PST (History)
4 users (show)

See Also:


Attachments
Patch (9.90 KB, patch)
2015-01-26 22:42 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2015-01-26 22:13:07 PST
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).
Comment 1 Chris Dumez 2015-01-26 22:42:50 PST
Created attachment 245425 [details]
Patch
Comment 2 Darin Adler 2015-01-27 07:17:28 PST
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.
Comment 3 WebKit Commit Bot 2015-01-27 07:59:59 PST
Comment on attachment 245425 [details]
Patch

Clearing flags on attachment: 245425

Committed r179181: <http://trac.webkit.org/changeset/179181>
Comment 4 WebKit Commit Bot 2015-01-27 08:00:04 PST
All reviewed patches have been landed.  Closing bug.