Overview: Add selectors for markup-based directionality. Details: :ltr would match any element whose directionality is left-to-right according to the rules of the markup language that the element is in. :rtl would match any element whose directionality is right-to-left. The selectors are not affected by the value of the CSS 'direction' property, only the directionality declared in the markup language. If the markup language does not define a directionality switch, then neither selector matches. Note: HTML says the 'dir' attribute inherits, so an element whose nearest ancestor-or-self with a 'dir' attribute has dir="ltr" has ltr directionality according to HTML. Similarly for dir="rtl". Original proposal: http://lists.w3.org/Archives/Public/www-style/2008Mar/0193.html

(In reply to comment #0) > The selectors are not affected by the value of the CSS 'direction' > property, only the directionality declared in the markup language. Because of this, I wonder if those selectors will have much usefulness. Since css styles can overwrite and change the 'dir' attribute value, the selectors will do no good. The correct approach is to make the thing based on window.getComputedStyle() (or something similar) to get the real direction of the element.

(In reply to comment #1) > (In reply to comment #0) > > The selectors are not affected by the value of the CSS 'direction' > > property, only the directionality declared in the markup language. > > Because of this, I wonder if those selectors will have much usefulness. Since > css styles can overwrite and change the 'dir' attribute value, the selectors > will do no good. The correct approach is to make the thing based on > window.getComputedStyle() (or something similar) to get the real direction of > the element. These selectors are intended for a different purpose, they're not meant so answer the question "what's the computed direction of this element?".

If they are not, those selectors can't solve issues like those in bug 373733 (which is in the dependency tree of this bug). Notice that in the testcase provided in bug 373733, "direction: rtl;" was coincidently used.

(In reply to comment #3) > If they are not, those selectors can't solve issues like those in bug 373733 > (which is in the dependency tree of this bug). Notice that in the testcase > provided in bug 373733, "direction: rtl;" was coincidently used. Well, then that dependency is set incorrectly. fantasai can correct me if I'm wrong here, but as comment 0 says, these selectors are going to be used to select on markup-based directionality.

From bug 672757 comment 0: Selectors Level 4 (http://dev.w3.org/csswg/selectors4/#dir-pseudo) specifies the :dir() pseudo-class (:dir(ltr) and :dir(rtl)), which allows the author to write selectors that represent an element based on its HTML5 directionality (see http://dev.w3.org/html5/spec/Overview.html#the-directionality). This is a very useful capability for bidi support. Note: this capability was originally proposed as :ltr and :rtl. This was eventually changed to the :dir(ltr|rtl) syntax.

So the simplest way to implement this may be to: 1) Add a new type of pseudo-class that can match on an event state bit being NOT set. We've wanted to do this anyway for -moz-readonly and -moz-readwrite. 2) Add an event state bit for "is rtl". (We could add both "rtl" and "ltr" bits and then we don't need step 1 above, but it seems like step 1 is worth doing anyway.) We don't need the event state thing to just handle simple matching (which can walk up the tree), but dynamic changes are a pain.

(In reply to comment #7) > 1) Add a new type of pseudo-class that can match on an event state bit being > NOT > set. We've wanted to do this anyway for -moz-readonly and -moz-readwrite. IINM, this is useful in order for us not to need two event state bits for the stuff that are mutually exclusive, right? Would you mind describing what's needed for this? I'm not entirely familiar with the new event state bits infrastructure... Once we have that, I think implementing this should be rather easy, by handling the dir attribute in nsGenericHTMLElement::IntrinsicState, and UpdateState-ing when binding/unbinding/AfterSetAttr, right?

> this is useful in order for us not to need two event state bits for the stuff > that are mutually exclusive, right? Yes. > Would you mind describing what's needed for this? Not at all. Add a new macro to nsCSSPseudoClassList.h; call it CSS_STATE_NOT_SET_PSEUDO_CLASS. Copy/paste all the stuff that file does with defining/undefining the CSS_STATE_PSEUDO_CLASS macro, basically. Then in nsCSSRuleProcessor.cpp, modify sPseudoClassStates to record whether the pseudo-class should match when the states are set or when the states are not set, or just set up a parallel array with that information (may be simpler). Then change this line: 1941 if (!contentState.HasAtLeastOneOfStates(statesToCheck)) { to check that boolean and modify the test accordingly. That should be it. > Once we have that, I think implementing this should be rather easy As you describe, yes.

David, is this something that you're interested to work on? Comment 8 and 9 should give you a rough idea on what needs to happen here.

(In reply to comment #9) > > Would you mind describing what's needed for this? > > Not at all. Add a new macro to nsCSSPseudoClassList.h; call it > CSS_STATE_NOT_SET_PSEUDO_CLASS. Copy/paste all the stuff that file does > with defining/undefining the CSS_STATE_PSEUDO_CLASS macro, basically. Then > in nsCSSRuleProcessor.cpp, modify sPseudoClassStates to record whether the > pseudo-class should match when the states are set or when the states are not > set, or just set up a parallel array with that information (may be simpler). > Then change this line: > > 1941 if (!contentState.HasAtLeastOneOfStates(statesToCheck)) { > > to check that boolean and modify the test accordingly. That should be it. FWIW, this part is bug 617629.

(In reply to comment #8) > Once we have that, I think implementing this should be rather easy, by > handling the dir attribute in nsGenericHTMLElement::IntrinsicState, and > UpdateState-ing when binding/unbinding/AfterSetAttr, right? Just a note that dir=auto (bug 548206) will complicate matters slightly here.

(In reply to comment #12) > (In reply to comment #8) > > Once we have that, I think implementing this should be rather easy, by > > handling the dir attribute in nsGenericHTMLElement::IntrinsicState, and > > UpdateState-ing when binding/unbinding/AfterSetAttr, right? > > Just a note that dir=auto (bug 548206) will complicate matters slightly here. Just in case that it is relevant, a reminder that HTML5 defines the concept of element directionality (http://dev.w3.org/html5/spec/Overview.html#the-directionality), which is always ltr or rtl (even when dir=auto). It is thdirectionality value that is supposed to get reported by dirname (http://dev.w3.org/html5/spec/Overview.html#submitting-element-directionality), and :dir() should be feeding off it too.

> Just a note that dir=auto (bug 548206) will complicate matters slightly here. Only insofar as IntrinsicState() will need to do a bit more work to determine the state if @dir is set to "auto". It should otherwise fit into the proposed model just fine.

(In reply to Ehsan Akhgari [:ehsan] from comment #8) > (In reply to comment #7) > > 1) Add a new type of pseudo-class that can match on an event state bit being > > NOT > > set. We've wanted to do this anyway for -moz-readonly and -moz-readwrite. > > IINM, this is useful in order for us not to need two event state bits for > the stuff that are mutually exclusive, right? Hang on, are rtl and ltr really mutually exclusive in the general case? See comment #0: > If the markup language does not define a directionality switch, then > neither selector matches.

(In reply to Simon Montagu from comment #15) > (In reply to Ehsan Akhgari [:ehsan] from comment #8) > > (In reply to comment #7) > > > 1) Add a new type of pseudo-class that can match on an event state bit being > > > NOT > > > set. We've wanted to do this anyway for -moz-readonly and -moz-readwrite. > > > > IINM, this is useful in order for us not to need two event state bits for > > the stuff that are mutually exclusive, right? > > Hang on, are rtl and ltr really mutually exclusive in the general case? See > comment #0: > > > If the markup language does not define a directionality switch, then > > neither selector matches. Right, I stand corrected!

This needs polishing when I understand the event states fu better, but it's enough to get bug 548206 to work

Comment on attachment 625030 [details] [diff] [review] w-i-p patch This adds a word to the size of nsIContent. Couldn't you just use two bit flags instead?

Also, why the unconditional UpdateState in AfterSetAttr? And should -moz-dir(foopy) parse?

I believe this is now ready for review, except for one problem: there is a contradiction that I don't know how to resolve between the state-driven pseudo classes and the pseudo class with string argument (Boris, is this what your comment "And should -moz-dir(foopy) parse?" is about?). I've ended up implementing the original :ltr and :rtl syntax instead. If there is a way to do that internally while supporting the :dir(ltr/rtl) syntax as well, that might be as neat as anything else.

> (Boris, is this what your comment "And should -moz-dir(foopy) parse?" is about? No, that comment was literally about whether "-moz-dir(foopy)" (that exact string) should be treated as a parse error in the selector or not. The problem you're talking about is different: the fact that :dir(ltr) can't be automagically mapped to an event state; you have to do the work yourself manually. Or the event state stuff can be rejiggered a bit to allow producing an event state pseudo ID out of a more complicated selector. Either way, the web-exposed syntax should probably not depend on our implementation details too much.

Comment on attachment 637611 [details] [diff] [review] Patch v.0 If we want to do :rtl and :ltr, the style system parts of this look fine. I didn't really look at the rest; do you want me to?

(In reply to Boris Zbarsky (:bz) from comment #22) > The problem you're talking about is different: the fact that :dir(ltr) can't > be automagically mapped to an event state; you have to do the work yourself > manually. Or the event state stuff can be rejiggered a bit to allow > producing an event state pseudo ID out of a more complicated selector. I have no clue how to do either alternative myself. That's really the feedback that I'm asking for here. Another thing I'd like to improve if possible is to do UpdateState() inside SetDirectionality(), instead of calling both each time (especially in preparation for bug 548206). In general, knowing "what class to put things in and how to access them afterwards without thousands of casts and/or QIs" is a big challenge for me with my minimal experience in content/

> That's really the feedback that I'm asking for here. Ah. So for the "make it automagic" option I'd have to think about it, but for the other you would just write manual matching logic for the :dir() pseudoclass (which would not be an event state pseudo) which would look at the event state. You would need to change the places that consider event state pseudos for dynamic updates in nsCSSRuleProcessor also consider :dir(). I can probably just write the patch for that (though not this coming week) if we're reasonably settled on the syntax.

OK, so with that help I managed to write the patch myself, but looking back over it afterwards I don't see the advantage of making the pseudo not be an event state pseudo. Couldn't it just be like this second version? This patch passes tryserver.

For the record, I want to answer here my own question in bug 713621 comment 2: > http://www.whatwg.org/specs/web-apps/current-work/multipage/rendering.html#bidirectional-text ... has > :dir(ltr) { direction: ltr; } > :dir(rtl) { direction: rtl; } > rather than our > [dir="ltr"] { direction: ltr; } > [dir="rtl"] { direction: rtl; } > > Is that going to cause any change in behaviour? Will it be worth changing it > once we have support for :dir?) I experimented with making this change and it caused regressions. Example: <div style="direction: rtl">foo! <div>bar!!</div> </div> The inner div inherits "direction: rtl" from the outer div, but this gets overridden by ":dir(ltr) { direction: ltr; }" (assuming no ancestor has a dir attribute, so the HTML directionality has the default value of ltr). I know that HTML and CSS recommend not using CSS direction, but there's plenty of legacy content that does use it which we don't want to regress.

> I experimented with making this change and it caused regressions. Uh, yes. We should get the spec fixed here, I think.

> Couldn't it just be like this second version? Right now, event state pseudos with multiple states are defined as matching if any of the states match. On the other hand, I do agree that having the state stuff here simplifies change handling. What I think we should do is to introduce a CSS_STATE_DEPENDENT_PSEUDO_CLASS macro, and use it for this state. Then in nsCSSPseudoClassList, we want to define CSS_STATE_PSEUDO_CLASS to CSS_STATE_DEPENDENT_PSEUDO_CLASS if it's not already defined. And we want to define CSS_STATE_DEPENDENT_PSEUDO_CLASS to CSS_PSEUDO_CLASS if it's not already defined. Then in nsCSSRuleProcessor, set up two arrays: one for use in ComputeSelectorStateDependence (probably called sPseudoClassStateDependences) and the other for use in matching. :dir() would have an entry in the former, but not in the latter. As far as the matching logic itself goes, two comments: 1) You won't need the statesToCheck bit in the if condition, which is in any case redundant with the mType check. 2) If this passes tests, that means the tests are insufficient: the dynamic handling here is broken, because the code doesn't examine aNodeMatchContext.mStateMask so will presumably return false from HasStateDependentStyle on changes to direction state. You probably also want to either MOZ_ASSERT that the dirString is one of the two expected values and that elementIsRTL and elementIsLTR are not both set, or explicitly write the logic so it doesn't depend on those assumptions, like so: if ((dirString.EqualsLiteral("rtl") && !elementIsRTL) || (dirString.EqualsLiteral("ltr") && !elementIsLTR)) { return false; }

Like this?

N.B: the new nsDirectionalityUtils would be overkill for just this, but I want it as a separate file for bug 548206

(In reply to Boris Zbarsky (:bz) from comment #29) > 2) If this passes tests, that means the tests are insufficient: the dynamic > handling > here is broken, because the code doesn't examine > aNodeMatchContext.mStateMask so will > presumably return false from HasStateDependentStyle on changes to > direction state. This part worries me: there are plenty of tests for dynamic handling in the tests patch here and even more so in bug 548206, so I'm not sure what I'm missing.

Comment on attachment 640091 [details] [diff] [review] Patch v.2, content part Review of attachment 640091 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/public/Makefile.in @@ +20,5 @@ > nsIContentIterator.h \ > nsContentErrors.h \ > nsContentPolicyUtils.h \ > nsContentUtils.h \ > +nsDirectionalityUtils.h \ mozilla/DirectionalityUtils.h or mozilla/dom/DirectionalityUtils.h, please. ::: content/base/public/nsDirectionalityUtils.h @@ +16,5 @@ > + * Helper to set directionality and notify > + */ > +void DoSetDirectionality(nsINode* aNode, > + nsDirectionality aDir, > + bool aNotify = true); Move this into the namespace, please. @@ +25,5 @@ > + > +/** > + * Set the directionality of a node as defined in > + * http://dev.w3.org/html5/spec/single-page.html#the-directionality, not > + * including elements with auto direction. Link to the whatwg spec, please. @@ +27,5 @@ > + * Set the directionality of a node as defined in > + * http://dev.w3.org/html5/spec/single-page.html#the-directionality, not > + * including elements with auto direction. > + */ > +nsDirectionality SetDirectionalityFromDirAttribute(nsIContent* aNode, aNode should probably be a mozilla::dom::Element @@ +39,5 @@ > + * For performance reasons we walk down the descendant tree in the rare case > + * of setting the dir attribute, rather than walking up the ancestor tree in > + * the much more common case of getting the element's directionality. > + */ > +void WalkDescendantsSetDirectionality(nsINode* aNode, aNode can be an Element too, I think. ::: content/base/public/nsINode.h @@ +55,5 @@ > class Element; > } // namespace dom > } // namespace mozilla > > +enum nsDirectionality { Namespaced, I think. ::: content/base/src/nsDirectionalityUtils.cpp @@ +8,5 @@ > +#include "nsIDOMNodeFilter.h" > +#include "nsTreeWalker.h" > +#include "nsIDOMHTMLDocument.h" > + > +void DoSetDirectionality(nsINode* aNode, Should be an Element* too. @@ +25,5 @@ > + public: > + NS_DECL_ISUPPORTS > + > + NS_IMETHOD AcceptNode(nsIDOMNode* aNode, PRInt16* aFilter) { > + nsCOMPtr<nsIContent> content = do_QueryInterface(aNode); QI to Element, maybe? @@ +26,5 @@ > + NS_DECL_ISUPPORTS > + > + NS_IMETHOD AcceptNode(nsIDOMNode* aNode, PRInt16* aFilter) { > + nsCOMPtr<nsIContent> content = do_QueryInterface(aNode); > + if (content->GetAttrCount() > 0 && content will never be null? Also, don't bother with the GetAttrCount() count; it's a virtual call you don't need. @@ +27,5 @@ > + > + NS_IMETHOD AcceptNode(nsIDOMNode* aNode, PRInt16* aFilter) { > + nsCOMPtr<nsIContent> content = do_QueryInterface(aNode); > + if (content->GetAttrCount() > 0 && > + content->HasAttr(kNameSpaceID_None, nsGkAtoms::dir)) { Do you need to check if it is an HTML element? @@ +49,5 @@ > +{ > + nsAutoString dirAttr; > + nsDirectionality dir = eDir_LTR; > + > + if (aNode->GetAttrCount() > 0 && Same @@ +51,5 @@ > + nsDirectionality dir = eDir_LTR; > + > + if (aNode->GetAttrCount() > 0 && > + aNode->GetAttr(kNameSpaceID_None, nsGkAtoms::dir, dirAttr) && > + !dirAttr.IsEmpty()) { Are you sure about falling back to the parent if the value is empty, but not if it is any other invalid value? @@ +67,5 @@ > + // If there is no parent element and we are an HTML document, the > + // directionality is the same as the document direction. > + nsCOMPtr<nsIDOMHTMLDocument> html_doc = do_QueryInterface(aDocument); > + if (html_doc) { > + html_doc->GetDir(dirAttr); htmlDoc; though an API on nsIDocument would be nicer. @@ +87,5 @@ > + nsTreeWalker walker(aNode, nsIDOMNodeFilter::SHOW_ELEMENT, filter); > + nsCOMPtr<nsIDOMNode> node; > + > + while (NS_SUCCEEDED(walker.NextNode(getter_AddRefs(node))) && node) { > + nsCOMPtr<nsIContent> childNode = do_QueryInterface(node); A followup that would allow nsTreeWalker to use nsINodes would be nice... Either that or just using for (nsIContent* childNode = aNode; childNode; childNode = childNode->GetNextNode(aNode)) { if (cur->HasAttr(kNameSpaceID_None, nsGkAtoms::dir)) { DoSetDirectionality(childNode, aDir, aNotify); } } (I suspect that's equivalent.) ::: content/html/content/src/nsGenericHTMLElement.cpp @@ +1912,5 @@ > SyncEditorsOnSubtree(this); > } > + else if (aName == nsGkAtoms::dir) { > + nsDirectionality dir = SetDirectionalityFromDirAttribute(this, > + GetParent(), Intentionally not GetNodeParent()? @@ +1913,5 @@ > } > + else if (aName == nsGkAtoms::dir) { > + nsDirectionality dir = SetDirectionalityFromDirAttribute(this, > + GetParent(), > + GetCurrentDoc(), Or OwnerDoc()?

> This part worries me: there are plenty of tests for dynamic handling None of them are using the right selectors. ;) Try something like this: :not(:-moz-dir(rtl)) + div { color: blue } and then toggling the previous sibling of the div from ltr to rtl. I believe with the patch I looked at the color won't change.

(In reply to :Ms2ger from comment #33) > > + if (content->GetAttrCount() > 0 && > > + content->HasAttr(kNameSpaceID_None, nsGkAtoms::dir)) { > > Do you need to check if it is an HTML element? I don't think so: in any case there is going to be a followup to make the selectors work on XUL elements also. > A followup that would allow nsTreeWalker to use nsINodes would be nice... Good idea > Either that or just using > > for (nsIContent* childNode = aNode; > childNode; > childNode = childNode->GetNextNode(aNode)) { > if (cur->HasAttr(kNameSpaceID_None, nsGkAtoms::dir)) { > DoSetDirectionality(childNode, aDir, aNotify); > } > } > > (I suspect that's equivalent.) I don't think so: the point of using nsIDOMNodeFilter::FILTER_REJECT rather than FILTER_SKIP is that it skips all descendants as well as the node itself. > Intentionally not GetNodeParent()? No, I didn't know about GetNodeParent, but wouldn't GetElementParent be better? I think I addressed all your other comments.

> it skips all descendants as well as the node itself See nsINode::GetNextNonChildNode, if it's relevant.

OK, but why is is better to roll my own tree walker? Again I'm looking forward to bug 548206 where I have a couple of other treewalker filters, and I don't want to duplicate code.

Sure, that's fine. I haven't looked at the treewalking part at all yet. Have you had a chance to try comment 34?

Comment on attachment 641022 [details] [diff] [review] Patch v.3, content part Review of attachment 641022 [details] [diff] [review]: ----------------------------------------------------------------- Two nits; leaving the actual review to someone smart :) ::: content/base/public/DirectionalityUtils.h @@ +16,5 @@ > +class Element; > +} // namespace dom > +} // namespace mozilla > + > +typedef mozilla::dom::Element Element; Please remove this line. ::: content/base/src/DirectionalityUtils.cpp @@ +74,5 @@ > +GetDirectionality(const nsIContent* aElement) > +{ > + if (aElement->HasFlag(NODE_HAS_DIRECTION_RTL)) { > + return eDir_RTL; > + } else if (aElement->HasFlag(NODE_HAS_DIRECTION_LTR)) { No else after return, please.

(In reply to Boris Zbarsky (:bz) from comment #38) > Have you had a chance to try comment 34? Unfortunately it doesn't work with the current version either :(

Right, because the current version is still not setting aDependence as needed...

OK, one more try. If this is no good either (and it seems a bit clunky to me) I'm going to take you up on your offer to write the patch yourself. In any case I'm going to be offline for 2 or 3 days. I've also tried to update some of the comments to changes in the code in the hope that it'll be a bit easier for the next guy to figure out what's going on in SelectorMatches().

Comment on attachment 641022 [details] [diff] [review] Patch v.3, content part >Patch for dir(ltr/rtl) CSS selector, part 1. bug 562169 >* * * >Patch for dir(ltr/rtl) CSS selector, part 2. bug 562169 Please fix up the commit message? >+++ b/content/base/public/DirectionalityUtils.h >+void SetDirectionality(Element* aElement, Directionality aDir); >+void DoSetDirectionality(Element* aElement, Just add the aNotify arg to SetDirectionality, please. There's no reason to have this split. >+bool HasValidDir(const Element* aElement); This seems to only be used internally in the C++ file. Why not move it there and make it static? >+Directionality SetDirectionalityFromDirAttribute(Element* aElement, Probably worth documenting that the return value is what the directionality got set to on aElement. >+ nsIContent* aParent, >+ nsIDocument* aDocument, I don't think we need those two arguments. We can get those off the element in the function as needed. >+void WalkDescendantsSetDirectionality(Element* aElement, Maybe SetDirectionalityOnDescendants? >diff --git a/content/base/public/nsIDocument.h b/content/base/public/nsIDocument.h >+ virtual mozilla::directionality::Directionality GetDocumentDirectionality() = 0; This is going to get called a lot. When a subtree is appended, this will be called once per node in the subtree in the common case, right? Given that, I would prefer it if this were non-virtual. Ideally inline on nsIDocument. We'd need to store the directionality state directly on nsIDocument, but that's OK. It would also mean we don't have to call GetDir() or QI for this function. Also, we should either fix our document.dir behavior to follow the spec (in a separate bug from this one) or raise a spec issue about the attribute. >+++ b/content/base/src/DirectionalityUtils.cpp >+HasValidDir(const Element* aElement) This, again, will be called a lot. It might be better to cache this info in a boolean flag on the element. >+Directionality >+GetDirectionality(const nsIContent* aElement) Is there a reason to not have this take const Element*? Really, it feels like this should be an inline method on Element, with Element.h including our utils header. >+ if (aElement->HasFlag(NODE_HAS_DIRECTION_RTL)) { >+ return eDir_RTL; >+ } else if (aElement->HasFlag(NODE_HAS_DIRECTION_LTR)) { No else after return, please. So more like this: if (aElement->HasFlag(NODE_HAS_DIRECTION_RTL)) { return eDir_RTL; } if (aElement->HasFlag(NODE_HAS_DIRECTION_LTR)) { etc. >+SetDirectionalityFromDirAttribute(Element* aElement, nsIContent* aParent, Except this also sets from parent and document. Maybe call this RecomputeDirectionality? >+ nsAutoString dirAttr; This is unused in the common case, but we'll still pay the cost of the constructor and destructor. Perhaps move this into the HasValidDir() block? >+ if (HasValidDir(aElement)) { >+ // If this node has an explicit dir attribute with a valid value, the >+ // directionality of the node follows the dir attribute >+ aElement->GetAttr(kNameSpaceID_None, nsGkAtoms::dir, dirAttr); So this only needs to happen in the SetAttr case, right? In the BindToTree case, we would already have the right thing and could just return if HasValidDir(), I think. I wonder whether it makes any sense to pass in a boolean indicating whether our state already matches our attr. >+ // Otherwise, the directionality is the same as the parent element (but >+ // don't propagate the parent directionality if it isn't set yet). Please add a link to https://www.w3.org/Bugs/Public/show_bug.cgi?id=18339 here. >+ // If there is no parent element and we are an HTML document, the >+ // directionality is the same as the document direction. You're doing this for all documents, no? Just nix the "and we are an HTML document" bit from the comment. >+WalkDescendantsSetDirectionality(Element* aElement, Directionality aDir, I'd prefer we did something more like this: for (nsIContent* child = aElement->GetFirstChild(); child; ) { if (!child->IsElement()) { child = child->GetNextNode(); continue; } Element* element = child->AsElment(); if (HasValidDir(element)) { child = child->GetNextNonChildNode(); continue; } DoSetDirectionality(element, aDir, aNotify); child = child->GetNextNode(); } >+++ b/content/base/src/nsDocument.cpp >+nsGenericHTMLElement::IntrinsicState() const >+ } else { // at least for HTML, directionality is exclusively LTR or RTL Might be worth an assert to that effect. > @@ -1715,16 +1733,18 @@ nsGenericHTMLElement::BindToTree(nsIDocu I don't think this is right. In particular, consider a case in which an element X has a single child Y and X is appended as a child to another element Z. X and Y have no "dir" attributes, but Z has dir="rtl". We'll call BindToTree on X, which will recursively call it on Y. Y will set itself to the directionality of X, and then _after_ that X will set itself to the directionality of Z. The upshot is that Y will end up ltr when it should be rtl. If we don't have a test for this, we should add one. What needs to happen here is that SetDirectionalityFromDirAttribute as it's currently written needs to run after updating the parent pointer but before recursively binding the kids. So in particular, it probably needs to happen from nsGenericElement::BindToTree, but only for HTML elements. > nsGenericHTMLElement::UnbindFromTree(bool aDeep, bool aNullParent) This needs to also call SetDirectionalityFromDirAttribute, no? Otherwise an element whose directionality was set based on its parent will have the wrong directionality after it's been removed from the document... Again, that needs to happen between setting parent and recursively calling down to the kids, so needs to happen in nsGenericElement::UnbindFromTree. And again, we need tests here; once the second patch lands you can use querySelector() to detect rtl/ltr state on nodes that are not in the tree.

Comment on attachment 641022 [details] [diff] [review] Patch v.3, content part Also, the nsGenericHTMLElement constructor should set NODE_HAS_DIRECTION_LTR, I believe, since that's the directionality of an HTML element with no attributes and no parent. Though maybe it should actually set the directionality based on its owner document? And also also... if the document's directionality changes, do we call SetDir() on the document? If not, then we need to propagate such changes to elements.

Comment on attachment 641608 [details] [diff] [review] Patch v.3, style part >+++ b/layout/style/nsCSSPseudoClassList.h >+ * Only one >+ * of the bits needs to match for the pseudo-class to match. This is true for CSS_STATE_PSEUDO_CLASS, but not for CSS_STATE_DEPENDENT_PSEUDO_CLASS, right? In particular, the matching for CSS_STATE_DEPENDENT_PSEUDO_CLASS depends in some custom way that can't be derived from this file on the bits. Might be good to explicitly say that. >+++ b/layout/style/nsCSSRuleProcessor.cpp >+ if (aDependence) { I'd prefer that this were restricted to the ePseudoClass_dir case for now, to avoid the performance hit in the other cases that don't care about it here. >+ // XXX can we return right away since the return value is going to be >+ // or-ed with *aDependence anyway? Yes, you can. >+ if (dirString.EqualsLiteral("rtl") && !elementIsRTL || >+ dirString.EqualsLiteral("ltr") && !elementIsLTR) { I'd prefer parens like so to make this clearer: if ((dirString.EqualsLiteral("rtl") && !elementIsRTL) || (dirString.EqualsLiteral("ltr") && !elementIsLTR)) { r=me with those nits.

Comment on attachment 641609 [details] [diff] [review] Updated tests r=me

carrying over r=bz

Comment on attachment 644456 [details] [diff] [review] interdiff between the last two version of content patch Let's call the flag NodeHasValidDirAttribute. SetDocumentDirectionality should be a protected function on nsDocument, since that's the only consumer. The nsIDocument constructor should probably set mDirectionality to a sane default. And whatever ends up setting the bidi options initially should also set mDirectionality, right? nsIDocument.h needs to include the header that declares the enum. Please add comments in the nsGenericElement code about the ordering constraint that made us put the code there (must run after we set parent and before we call into kids). r=me with those nits.

The patch that adds the event state flags should also change the nsGenericHTMLElement constructor to set the event state correctly (corresponding to LTR directionality).

Comment on attachment 644459 [details] [diff] [review] interdiff with the new tests I'd like one more test, which checks that if you do: document.createElement("div").mozMatchesSelector(":dir(ltr)") you get true. I think that you get false without the change to the generic element constructor to add the event state... r=me with that.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c24b7287c5b https://hg.mozilla.org/integration/mozilla-inbound/rev/e226e413dd27 https://hg.mozilla.org/integration/mozilla-inbound/rev/841cd6e1e585

Sorry, I backed out these changes because they caused significant regressions (around 6%-8%) in the Dromaeo DOM benchmark on all platforms. :( https://hg.mozilla.org/integration/mozilla-inbound/rev/5faf4c39e64c Ideally the regression should be fixed before these patches land again, unless the module owners thinks it better to land sooner and fix the regression in a follow-up bug.

Reopening, since this got backed out. Simon, the dom-modify Dromaeo tests got about 30% slower with this patch. You probably want to look into which subtests under there slowed down (presumably the appendChild ones?) and why. See http://dromaeo.com/?dom-mod

https://hg.mozilla.org/mozilla-central/rev/9c24b7287c5b https://hg.mozilla.org/mozilla-central/rev/841cd6e1e585

No, really reopened. Shouldn't have been marked fixed to start with when just landing on inbound...

https://hg.mozilla.org/mozilla-central/rev/e226e413dd27 Note that commit message refers to the wrong bug number.

I narrowed this down to the calls to UpdateState in SetDirectionality, then came across some other comments that point to that as a performance black hole, e.g. in nsGenericElement::UpdateEditableState and bug 672709 comment 1. This patch makes us avoid calling UpdateState when we aren't notifying, and pretty much removes the dromaeo regression in my tests. http://dromaeo.com/?id=176787,176789,176851 shows test runs without any of the patches from this bug, with the patches as checked in, and with this patch.

Comment on attachment 650637 [details] [diff] [review] Patch for performance regression Good catch, and thanks for posting that link with the performance data! I really need to figure out a way to make initial state setup less perf-sucky. :(

Can we document this performance sensitive-ness in the new code as well, to prevent others from regressing this by mistake in the future? Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/3dde6600c45f https://hg.mozilla.org/integration/mozilla-inbound/rev/407bce2f4e97 https://hg.mozilla.org/integration/mozilla-inbound/rev/51739311572b with added comments about performance per comment 64

https://hg.mozilla.org/mozilla-central/rev/3dde6600c45f https://hg.mozilla.org/mozilla-central/rev/407bce2f4e97 https://hg.mozilla.org/mozilla-central/rev/51739311572b

I'm in the progress of documenting this, but I have a problem to make it work with dir="auto" that we supports for <textarea>. I don't see a test with the auto value, is this supposed to work with this: <textarea dir="auto">עִבְרִית</textarea> (in case the chars get borked, the content was hebrew chars) and :dir(rtl) { background-color:red; } :dir(ltr) { background-color:lime; } I get a lime textarea instead of a red one (my html document default to ltr, I think) ?

I believe that that behaviour is correct for <textarea> and <pre>, where dir=auto maps to unicode-bidi: plaintext, although it doesn't seem to be defined in any spec. The alternative would be for the background color possibly to change per paragraph, which I think would be odd.

(In reply to comment #68) > I believe that that behaviour is correct for <textarea> and <pre>, where > dir=auto maps to unicode-bidi: plaintext, although it doesn't seem to be > defined in any spec. The alternative would be for the background color possibly > to change per paragraph, which I think would be odd. I agree, but we should make sure that the behavior here is defined in the spec...

(In reply to Simon Montagu from comment #68) > I believe that that behaviour is correct for <textarea> and <pre>, where > dir=auto maps to unicode-bidi: plaintext, although it doesn't seem to be > defined in any spec. The alternative would be for the background color > possibly to change per paragraph, which I think would be odd. That dir=auto maps to unicode-bidi:plaintext for <pre> and <textarea> is specified in http://www.w3.org/TR/html5/single-page.html#bidirectional-text ("textarea[dir=auto i], pre[dir=auto i] { unicode-bidi: plaintext; }"), which is normative. It is also hinted at by the note in http://www.w3.org/TR/html5/single-page.html#the-dir-attribute on the auto keyword that "For textarea and pre elements, the heuristic is applied on a per-paragraph level". However, that is not the only part of the HTML5 specification relevant here. The usual dir=auto algorithm (http://www.w3.org/TR/html5/single-page.html#the-dir-attribute) for determining the element's directionality, and thus its default direction style, is also relevant to <pre> and <textarea>. In fact, for <textarea>, that algorithm has a special provision necessary due to the dynamic nature of a textarea's value: If the element is a textarea element and the dir attribute is in the auto state If the element's value contains a character of bidirectional character type AL or R, and there is no character of bidirectional character type L anywhere before it in the element's value, then the directionality of the element is 'rtl'. Otherwise, the directionality of the element is 'ltr'. The direction value for dir=auto on <pre> and <textarea> is important (despite the default unicode-bidi:plaintext) because: - It affects the alignment of an empty first paragraph, and thus the position of the caret in an empty textarea. See http://dev.w3.org/csswg/css3-text/#bidi-linebox: An empty line box (i.e. one that contains no atomic inlines or characters other than the line-breaking character, if any), takes its inline base direction from the preceding line box (if any), or, if this is the first line box in the containing block, then from the ‘direction’ property of the containing block. - It may affect the side on which the vertical scrollbar is displayed, although that is not something governed by a spec. Cnversely, the default unicode-bidi:plaintext for dir=auto on <pre> and <textarea> is important (despite the direction value determined from the element's content) because it means that the directionality of each bidi paragraph is set independently.

So somewhere between comment 42 and comment 52 the patch changed from :-moz-dir() to :dir(). What led to that? I've gotten some pushback in the WG about it, though I'd prefer not to ship new prefixes...

(In reply to David Baron [:dbaron] from comment #71) > So somewhere between comment 42 and comment 52 the patch changed from > :-moz-dir() to :dir(). What led to that? Bug 775235 is what led to that. "CSS :dir(ltr/rtl) - need bug (:smontagu)" ... but since this wasn't yet checked in when that was filed I just changed it in the patch rather than filing a separate bug.

I'm not aware of this being in CR or hitting any of the cases described in https://groups.google.com/group/mozilla.dev.platform/msg/6107b1b7bd4fb799 , so I think this should probably be prefixed right now.

Then again, I also think we'd be unlikely to have any compatibility problems if there are changes between now and when the feature reaches CR, so I think we'd be willing to update to match any changes. So maybe it's fine as is; I'd just need to convince the WG of that.

Then again, I think the basic point here is that we're not expecting authors to use the feature yet, though we use it internally. So I think it should probably just be prefixed until the feature hits CR.

pushed to tryserver at https://tbpl.mozilla.org/?tree=Try&rev=3e7fb0aba3dc

Comment on attachment 665815 [details] [diff] [review] Prefix it r=dbaron. Thanks for fixing this.

Callek pointed out on IRC that our own css uses :dir all over. This fixes that.

Is there plans to uptake this prefixing patch in Fx 17 (aurora now) to prevent an unprefixed :dir to be released?

Comment on attachment 665815 [details] [diff] [review] Prefix it [Approval Request Comment] Bug caused by (feature/regressing bug #): this bug User impact if declined: we end up shipping a feature unprefixed that we didn't intend to ship unprefixed Testing completed (on m-c, etc.): on mozilla-central Risk to taking this patch (and alternatives if risky): low String or UUID changes made by this patch: none

https://hg.mozilla.org/releases/mozilla-aurora/rev/68b0c806731c https://hg.mozilla.org/releases/mozilla-aurora/rev/846561f033db

Doc updated: https://developer.mozilla.org/en-US/docs/Web/CSS/:dir https://developer.mozilla.org/en-US/Firefox/Releases/17

May I know what are the plans for un-prefixing this feature?

Ramya, bug 859301 tracks unprefixing.