Bug 68773

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.

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

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.

(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.

Created attachment 132928 [details] Patch

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

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

Created attachment 132937 [details] Fixed GTK & Qt builds

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

Apparently this failure only manifests in release builds :(

Created attachment 133123 [details] Fixed a bug

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.

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

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

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 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 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 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 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. :)

(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 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 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.

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