Bug 68773 – BDI element should have dir=auto by default

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

[Aharon (Vladimir) Lanin]

Unless bdi has an explicitly assigned dir=ltr or dir=rtl, it should behave as if it had dir=auto. See http://dev.w3.org/html5/spec/Overview.html#the-bdi-element, where it says:

Note: The dir global attribute defaults to auto on this element (it never inherits from the parent element like with other elements).

BTW, please note related bug https://bugs.webkit.org/show_bug.cgi?id=63903 (dir=auto should imply unicode-bidi:isolate by default)

[Aharon (Vladimir) Lanin]

Created attachment 118988 [details]
test case (ref file coming as separate attachment)

This is one of the tests being submitted to [e-mail] for inclusion into the W3C's HTML5 test suite.

[Aharon (Vladimir) Lanin]

Created attachment 118989 [details]
ref file for the test case above

[Ryosuke Niwa]

It seems like it'll be much easier to do this if dir=auto had a CSS equivalent. I've filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=16456 to that end.

[Aharon (Vladimir) Lanin]

(In reply to comment #3)
> It seems like it'll be much easier to do this if dir=auto had a CSS equivalent. I've filed https://www.w3.org/Bugs/Public/show_bug.cgi?id=16456 to that end.

I do not think that a CSS equivalent to dir=auto is possible. When Fantasai gets around to responding on that bug, that is what she will say. I do not want this issue to be blocked waiting for that, because it just isn't going to happen.

[Ryosuke Niwa]

Created attachment 132928 [details]
Patch

[Ryosuke Niwa]

Created attachment 132930 [details]
Fixed the file ordering in xcodeproj

[Gustavo Noronha (kov)]

Comment on attachment 132930 [details]
Fixed the file ordering in xcodeproj

Attachment 132930 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12090028

[Ryosuke Niwa]

Created attachment 132937 [details]
Fixed GTK & Qt builds

[WebKit Review Bot]

Comment on attachment 132937 [details]
Fixed GTK & Qt builds

Attachment 132937 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12086328

New failing tests:
fast/text/international/bdi-dir-default-to-auto.html

[Ryosuke Niwa]

Apparently this failure only manifests in release builds :(

[Ryosuke Niwa]

Created attachment 133123 [details]
Fixed a bug

[Ryosuke Niwa]

Comment on attachment 133123 [details]
Fixed a bug

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

> Source/WebCore/html/HTMLBDIElement.h:40
> +        // FIXME: Rename setSelfOrAncestorHasDirAutoAttribute to reflect the fact bdi also uses this flag.
> +        setSelfOrAncestorHasDirAutoAttribute(true);

I'm more than happy to make the change if anyone can come up with a better name for this function.

[Ryosuke Niwa]

Apparently HTMLElementFactory.cpp is generated from HTMLTagNames.in so I had to modify HTMLTagNames.in instead of HTMLElementFactory.cpp.

[Ryosuke Niwa]

Created attachment 133130 [details]
Removed the extra change log entry

[Ryosuke Niwa]

Comment on attachment 133130 [details]
Removed the extra change log entry

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

> Source/WebCore/html/HTMLElement.cpp:934
> -            if (node->isElementNode() && toElement(node)->hasAttribute(dirAttr))
> +            if (node->isElementNode() && elementAffectsDirectionality(toHTMLElement(node)))

This should be checking for isHTMLElement instead; will fix before landing it if this patch gets r+ed.

[Ryosuke Niwa]

Comment on attachment 133130 [details]
Removed the extra change log entry

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

>> Source/WebCore/html/HTMLElement.cpp:934
>> +            if (node->isElementNode() && elementAffectsDirectionality(toHTMLElement(node)))
> 
> This should be checking for isHTMLElement instead; will fix before landing it if this patch gets r+ed.

Or rather we should be casting to toElement instead.

[Daniel Bates]

Comment on attachment 133130 [details]
Removed the extra change log entry

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

This patch looks straight forward to me. I had some minor comments.

> Source/WebCore/css/CSSStyleSelector.cpp:1370
> +    if ((element->isHTMLElement() && toHTMLElement(element)->hasDirectionAuto()) || (m_element->isHTMLElement() && toHTMLElement(m_element)->hasDirectionAuto()))

The disjuncts in this disjunction are the same up to the object they operate on. You may want to consider extracting a disjunct into an inline function, say elementHasDirectionAuto() or doesElementHaveDirectionAuto?

inline bool elementHasDirectionAuto(Element* element)
{
    return element->isHTMLElement() && toHTMLElement(element)->hasDirectionAuto();
}

Then you can write this line as:

if (elementHasDirectionAuto(element) || elementHasDirectionAuto(m_element)) {
    ...
}

> Source/WebCore/html/HTMLBDIElement.h:41
> +        fprintf(stderr, "HTMLBDIElement:%s\n", selfOrAncestorHasDirAutoAttribute() ? "true" : "false");

Did you mean to have this fprintf() in the patch?

> Source/WebCore/html/HTMLBDIElement.h:45
> +} // namespace

This should read:

// namespace WebCore

> Source/WebCore/html/HTMLElement.cpp:945
> +    while (oldMarkedNode && oldMarkedNode->isHTMLElement() && elementAffectsDirectionality(toHTMLElement(oldMarkedNode)))

As we talked about on IRC, you may want to consider having elementAffectsDirectionality() take a const Node* instead of a const Element*. And moving the HTMLElement cast (it's sufficient to call toElement()) into elementAffectsDirectionality() such that we can write this line as:

while (oldMarkedNode && oldMarkedNode->isHTMLElement() && elementAffectsDirectionality(oldMarkedNode))

[Ryosuke Niwa]

Comment on attachment 133130 [details]
Removed the extra change log entry

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

Thanks for the review!

>> Source/WebCore/css/CSSStyleSelector.cpp:1370
>> +    if ((element->isHTMLElement() && toHTMLElement(element)->hasDirectionAuto()) || (m_element->isHTMLElement() && toHTMLElement(m_element)->hasDirectionAuto()))
> 
> The disjuncts in this disjunction are the same up to the object they operate on. You may want to consider extracting a disjunct into an inline function, say elementHasDirectionAuto() or doesElementHaveDirectionAuto?
> 
> inline bool elementHasDirectionAuto(Element* element)
> {
>     return element->isHTMLElement() && toHTMLElement(element)->hasDirectionAuto();
> }
> 
> Then you can write this line as:
> 
> if (elementHasDirectionAuto(element) || elementHasDirectionAuto(m_element)) {
>     ...
> }

Added elementHasDirectionAuto.

>> Source/WebCore/html/HTMLBDIElement.h:41
>> +        fprintf(stderr, "HTMLBDIElement:%s\n", selfOrAncestorHasDirAutoAttribute() ? "true" : "false");
> 
> Did you mean to have this fprintf() in the patch?

Oops, definitely not. Removed.

>> Source/WebCore/html/HTMLBDIElement.h:45
>> +} // namespace
> 
> This should read:
> 
> // namespace WebCore

Fixed.

>> Source/WebCore/html/HTMLElement.cpp:945
>> +    while (oldMarkedNode && oldMarkedNode->isHTMLElement() && elementAffectsDirectionality(toHTMLElement(oldMarkedNode)))
> 
> As we talked about on IRC, you may want to consider having elementAffectsDirectionality() take a const Node* instead of a const Element*. And moving the HTMLElement cast (it's sufficient to call toElement()) into elementAffectsDirectionality() such that we can write this line as:
> 
> while (oldMarkedNode && oldMarkedNode->isHTMLElement() && elementAffectsDirectionality(oldMarkedNode))

Done.

[Eric Seidel (no email)]

Comment on attachment 133130 [details]
Removed the extra change log entry

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

> Source/WebCore/html/HTMLTagNames.in:18
> +bdi interfaceName=HTMLBDIElement, JSInterfaceName=HTMLElement

I'm surprised you need any interfaceName, I guess HTMLBdiElement would be default.... I think make_names attempts to understand some of these capitalizations, probably with a hard-coded list.  I guess we should move more of the hard-coded list items to being in these .in files instead... or maybe we already have. :)

[Ryosuke Niwa]

(In reply to comment #19)
> (From update of attachment 133130 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133130&action=review
> 
> > Source/WebCore/html/HTMLTagNames.in:18
> > +bdi interfaceName=HTMLBDIElement, JSInterfaceName=HTMLElement
> 
> I'm surprised you need any interfaceName, I guess HTMLBdiElement would be default.... I think make_names attempts to understand some of these capitalizations, probably with a hard-coded list.  I guess we should move more of the hard-coded list items to being in these .in files instead... or maybe we already have. :)

I think we've already moved to .in files. e.g. hr interfaceName=HTMLHRElement is there.

[WebKit Review Bot]

Comment on attachment 133130 [details]
Removed the extra change log entry

Attachment 133130 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12090660

New failing tests:
fast/css/default-bidi-css-rules.html

[Ryosuke Niwa]

Comment on attachment 133130 [details]
Removed the extra change log entry

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

> Source/WebCore/html/HTMLElement.cpp:834
> +    return hasTagName(bdiTag) || equalIgnoringCase(fastGetAttribute(dirAttr), "auto");

Oops, I need to check that the dir attribute isn't set to ltr or rtl when the element is bdi :( Will fix before landing the patch.

[Ryosuke Niwa]

Committed r111632: <http://trac.webkit.org/changeset/111632>