Clearing 'dir' attribute does not correctly set page directionality

• Component: DOM
• Status: RESOLVED CONFIGURATION CHANGED

[Prashant Hiremath]

Merge https://chromium.googlesource.com/chromium/blink/+/2d592d1c998bec9438e421e1ce1ee6caba05a884

The dir attribute by default resolves to ltr. After its set to rtl, clearing it should cause it to once again resolve to its default,
ltr. This patch makes this change from the current behavior where the rtl persists the clear.

[Prashant Hiremath]

Created attachment 219738 [details]
Patch

proposed patch

[Build Bot]

Comment on attachment 219738 [details]
Patch

Attachment 219738 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/49338234

New failing tests:
fast/dom/HTMLElement/set-and-clear-dir-attribute.html

[Build Bot]

Created attachment 219743 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Ryosuke Niwa]

Comment on attachment 219738 [details]
Patch

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

> Source/WebCore/html/HTMLElement.cpp:209
> +            AtomicString dirValue = isValidDirAttribute(value) ? value : "ltr";

This is incorrect. When dir="" attribute doesn't parse correctly, it needs to be treated as if it doesn't exist for the purpose of determining the directionality of text.
I've specifically added a test case for this in fast/dom/HTMLElement/set-and-clear-dir-attribute.html

i.e. the blink change is wrong.

[Prashant Hiremath]

(In reply to comment #4)
> (From update of attachment 219738 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=219738&action=review
> 
> > Source/WebCore/html/HTMLElement.cpp:209
> > +            AtomicString dirValue = isValidDirAttribute(value) ? value : "ltr";
> 
> This is incorrect. When dir="" attribute doesn't parse correctly, it needs to be treated as if it doesn't exist for the purpose of determining the directionality of text.
> I've specifically added a test case for this in fast/dom/HTMLElement/set-and-clear-dir-attribute.html
> 
> i.e. the blink change is wrong.

Ok, got it :) . will close the bug.

[Ryosuke Niwa]

I don't think we should close the bug given that the bug still exists in WebKit.

It's just that the fix is incorrect.

[Prashant Hiremath]

(In reply to comment #6)
> I don't think we should close the bug given that the bug still exists in WebKit.
> 
> It's just that the fix is incorrect.

Thanks.
I'll try to look into this issue then :).

[Ahmad Saleem]

This patch was modifying - equalIgnoringCase and it was removed in below:

https://github.com/WebKit/WebKit/commit/507aa34586fdcf0b9a5ea433c2402f295a717da8

-> [e-mail] - I think you landed 'dir' related changes just recently, have this been addressed or is this something needed? Thanks!

[Ryosuke Niwa]

Yeah, I'm sure I fixed this with my latest patches.