Bug 68773

Summary: BDI element should have dir=auto by default
Product: WebKit Reporter: Aharon (Vladimir) Lanin <aharon>
Component: DOMAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: aharon, amir.aharoni, dbates, dglazkov, eric, gns, leviw, macpherson, menard, mitz, playmobil, rniwa, webkit.review.bot, xan.lopez, xji, yael
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 50910, 50913    
Attachments:
Description Flags
test case (ref file coming as separate attachment)
none
ref file for the test case above
none
Patch
none
Fixed the file ordering in xcodeproj
none
Fixed GTK & Qt builds
none
Fixed a bug
none
Removed the extra change log entry dbates: review+, webkit.review.bot: commit-queue-

Description Aharon (Vladimir) Lanin 2011-09-25 01:26:47 PDT
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)
Comment 1 Aharon (Vladimir) Lanin 2011-12-13 02:54:54 PST
Created attachment 118988 [details]
test case (ref file coming as separate attachment)

This is one of the tests being submitted to public-html-testsuite@w3.org for inclusion into the W3C's HTML5 test suite.
Comment 2 Aharon (Vladimir) Lanin 2011-12-13 02:56:32 PST
Created attachment 118989 [details]
ref file for the test case above
Comment 3 Ryosuke Niwa 2012-03-20 13:32:35 PDT
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.
Comment 4 Aharon (Vladimir) Lanin 2012-03-20 16:11:31 PDT
(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.
Comment 5 Ryosuke Niwa 2012-03-20 16:59:54 PDT
Created attachment 132928 [details]
Patch
Comment 6 Ryosuke Niwa 2012-03-20 17:02:27 PDT
Created attachment 132930 [details]
Fixed the file ordering in xcodeproj
Comment 7 Gustavo Noronha (kov) 2012-03-20 17:08:41 PDT
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
Comment 8 Ryosuke Niwa 2012-03-20 17:12:06 PDT
Created attachment 132937 [details]
Fixed GTK & Qt builds
Comment 9 WebKit Review Bot 2012-03-21 04:38:24 PDT
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
Comment 10 Ryosuke Niwa 2012-03-21 12:36:43 PDT
Apparently this failure only manifests in release builds :(
Comment 11 Ryosuke Niwa 2012-03-21 15:16:51 PDT
Created attachment 133123 [details]
Fixed a bug
Comment 12 Ryosuke Niwa 2012-03-21 15:18:29 PDT
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.
Comment 13 Ryosuke Niwa 2012-03-21 15:23:55 PDT
Apparently HTMLElementFactory.cpp is generated from HTMLTagNames.in so I had to modify HTMLTagNames.in instead of HTMLElementFactory.cpp.
Comment 14 Ryosuke Niwa 2012-03-21 15:46:49 PDT
Created attachment 133130 [details]
Removed the extra change log entry
Comment 15 Ryosuke Niwa 2012-03-21 16:05:35 PDT
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.
Comment 16 Ryosuke Niwa 2012-03-21 16:06:10 PDT
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.
Comment 17 Daniel Bates 2012-03-21 16:31:10 PDT
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))
Comment 18 Ryosuke Niwa 2012-03-21 16:40:05 PDT
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.
Comment 19 Eric Seidel (no email) 2012-03-21 16:41:05 PDT
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. :)
Comment 20 Ryosuke Niwa 2012-03-21 16:55:28 PDT
(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.
Comment 21 WebKit Review Bot 2012-03-21 17:14:34 PDT
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
Comment 22 Ryosuke Niwa 2012-03-21 18:04:09 PDT
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.
Comment 23 Ryosuke Niwa 2012-03-21 18:08:31 PDT
Committed r111632: <http://trac.webkit.org/changeset/111632>