[Webkit-unassigned] [Bug 50916] Add support for dir=auto

[bugzilla-daemon at webkit.org]

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





--- Comment #17 from Jeremy Moskovich <playmobil at google.com>  2011-02-03 01:39:05 PST ---
(From update of attachment 80968)
View in context: https://bugs.webkit.org/attachment.cgi?id=80968&action=review

Disclaimer: I'm not a WebKit reviewer, feel free to ignore my comments.

My comments are mainly targeted at making things a little easier to understand for non-native language speakers

> Source/WebCore/html/HTMLElement.cpp:868
> +    ASSERT(equalIgnoringCase(fastGetAttribute(dirAttr), "auto"));

It's only legal to call this method on an element which has dir=auto set?
Could you add a comment on why that is?

> Source/WebCore/html/HTMLElement.cpp:895
> +    return direction;

It looks like you don't modify direction in the function body, can you just return LTR here?

> LayoutTests/fast/dom/HTMLElement/attr-dir-auto-change-child-node.html:19
> +description('Test that when an element has dir attribute with value "auto", if its first child changes, the element\'s directionality is re-evaluated.');

How about:
Test that directionality is re-evaluated when an element with dir=auto set, has it's first child modified.

The current description was a little hard for me to parse :) , same comment for other tests below.

> LayoutTests/fast/dom/HTMLElement/attr-dir-auto.html:17
> +<div id="ltr" dir="auto" class="testDiv">Test test test</div>

Perhaps give this an id of left-to-right or similar so someone looking at the test doesn't get confused with the dir attribute?

> LayoutTests/fast/dom/HTMLElement/attr-dir-auto.html:19
> +<div id="rtl1" dir="auto" class="testDiv">מקור השם עברית</div>

How about putting an "!" at the end of the string e.g. 
FOO!

For RTL the ! will be on the right side and for LTR it'll be on the left side.

This way you can phrase the pass/fail text above as:
"!" should be on the right side of the string.

This makes it easier for non-RTL speakers to tell that the test is passing when doing a visual inspection.

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