Last Comment Bug 562169 - Implement the :dir(rtl/ltr) selector to select on HTML directionality
: Implement the :dir(rtl/ltr) selector to select on HTML directionality
Status: RESOLVED FIXED
[DocArea=CSS]
: dev-doc-complete
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: -- enhancement with 1 vote (vote)
: mozilla17
Assigned To: Simon Montagu :smontagu
:
: Jet Villegas (:jet)
Mentors:
: 672757 (view as bug list)
Depends on: 741293
Blocks: 373733 Instantbird DirAuto 784279 784313 859301 1247576
  Show dependency treegraph
 
Reported: 2010-04-27 14:14 PDT by fantasai
Modified: 2016-02-11 06:15 PST (History)
21 users (show)
smontagu: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Tests (8.10 KB, patch)
2012-05-18 00:56 PDT, Simon Montagu :smontagu
no flags Details | Diff | Splinter Review
w-i-p patch (15.82 KB, patch)
2012-05-18 00:59 PDT, Simon Montagu :smontagu
no flags Details | Diff | Splinter Review
Patch v.0 (15.61 KB, patch)
2012-06-28 11:25 PDT, Simon Montagu :smontagu
bzbarsky: feedback+
Details | Diff | Splinter Review
Patch v.1a (20.52 KB, patch)
2012-07-02 01:58 PDT, Simon Montagu :smontagu
bzbarsky: feedback+
Details | Diff | Splinter Review
Patch v.2, style part (12.31 KB, patch)
2012-07-08 12:08 PDT, Simon Montagu :smontagu
no flags Details | Diff | Splinter Review
Patch v.2, content part (16.81 KB, patch)
2012-07-08 12:10 PDT, Simon Montagu :smontagu
no flags Details | Diff | Splinter Review
Patch v.3, content part (19.13 KB, patch)
2012-07-11 05:46 PDT, Simon Montagu :smontagu
bzbarsky: review-
Details | Diff | Splinter Review
Patch v.3, style part (16.44 KB, patch)
2012-07-12 14:46 PDT, Simon Montagu :smontagu
bzbarsky: review+
Details | Diff | Splinter Review
Updated tests (10.78 KB, patch)
2012-07-12 14:48 PDT, Simon Montagu :smontagu
bzbarsky: review+
Details | Diff | Splinter Review
Patch v.4, content part (22.41 KB, patch)
2012-07-20 13:49 PDT, Simon Montagu :smontagu
no flags Details | Diff | Splinter Review
interdiff between the last two version of content patch (17.52 KB, patch)
2012-07-20 13:50 PDT, Simon Montagu :smontagu
bzbarsky: review+
Details | Diff | Splinter Review
Tests, with some new ones (14.10 KB, patch)
2012-07-20 13:51 PDT, Simon Montagu :smontagu
no flags Details | Diff | Splinter Review
interdiff with the new tests (6.53 KB, patch)
2012-07-20 13:51 PDT, Simon Montagu :smontagu
bzbarsky: review+
Details | Diff | Splinter Review
Patch v.4, style part (with review nits picked) (15.62 KB, patch)
2012-07-20 13:53 PDT, Simon Montagu :smontagu
smontagu: review+
Details | Diff | Splinter Review
Patch for performance regression (6.88 KB, patch)
2012-08-09 12:04 PDT, Simon Montagu :smontagu
bzbarsky: review+
Details | Diff | Splinter Review
Prefix it (13.26 KB, patch)
2012-09-28 00:34 PDT, Simon Montagu :smontagu
dbaron: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Use the prefix in internal css files (13.39 KB, patch)
2012-09-29 14:20 PDT, Simon Montagu :smontagu
dao+bmo: review+
dbaron: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description fantasai 2010-04-27 14:14:52 PDT
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
Comment 1 Anas Husseini [:linostar] 2011-05-03 01:57:46 PDT
(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.
Comment 2 :Ehsan Akhgari 2011-05-03 10:55:52 PDT
(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?".
Comment 3 Anas Husseini [:linostar] 2011-05-04 02:32:00 PDT
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.
Comment 4 :Ehsan Akhgari 2011-05-05 07:29:16 PDT
(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.
Comment 5 :Ehsan Akhgari 2011-07-20 16:34:29 PDT
*** Bug 672757 has been marked as a duplicate of this bug. ***
Comment 6 :Ehsan Akhgari 2011-07-20 16:36:37 PDT
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.
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2011-07-20 19:08:01 PDT
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.
Comment 8 :Ehsan Akhgari 2011-07-21 08:49:35 PDT
(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?
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2011-07-21 09:04:36 PDT
> 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.
Comment 10 :Ehsan Akhgari 2011-07-21 12:45:39 PDT
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.
Comment 11 :Ehsan Akhgari 2011-07-25 10:46:13 PDT
(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.
Comment 12 Simon Montagu :smontagu 2011-07-27 02:19:18 PDT
(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.
Comment 13 Aharon (Vladimir) Lanin 2011-07-27 02:30:04 PDT
(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.
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2011-07-27 08:30:53 PDT
> 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.
Comment 15 Simon Montagu :smontagu 2011-09-01 02:29:29 PDT
(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.
Comment 16 :Ehsan Akhgari 2011-09-06 13:40:43 PDT
(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!
Comment 17 Simon Montagu :smontagu 2012-05-18 00:56:45 PDT
Created attachment 625029 [details] [diff] [review]
Tests
Comment 18 Simon Montagu :smontagu 2012-05-18 00:59:21 PDT
Created attachment 625030 [details] [diff] [review]
w-i-p patch

This needs polishing when I understand the event states fu better, but it's enough to get bug 548206 to work
Comment 19 :Ehsan Akhgari 2012-05-18 12:21:34 PDT
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?
Comment 20 Boris Zbarsky [:bz] (still a bit busy) 2012-05-18 12:59:08 PDT
Also, why the unconditional UpdateState in AfterSetAttr?

And should -moz-dir(foopy) parse?
Comment 21 Simon Montagu :smontagu 2012-06-28 11:25:59 PDT
Created attachment 637611 [details] [diff] [review]
Patch v.0

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.
Comment 22 Boris Zbarsky [:bz] (still a bit busy) 2012-06-28 12:24:03 PDT
> (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 23 Boris Zbarsky [:bz] (still a bit busy) 2012-06-28 12:25:38 PDT
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?
Comment 24 Simon Montagu :smontagu 2012-06-28 12:51:12 PDT
(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/
Comment 25 Boris Zbarsky [:bz] (still a bit busy) 2012-06-28 13:01:49 PDT
> 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.
Comment 26 Simon Montagu :smontagu 2012-07-02 01:58:17 PDT
Created attachment 638281 [details] [diff] [review]
Patch v.1a

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.
Comment 27 Simon Montagu :smontagu 2012-07-03 06:20:42 PDT
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.
Comment 28 Boris Zbarsky [:bz] (still a bit busy) 2012-07-06 05:55:14 PDT
> I experimented with making this change and it caused regressions.

Uh, yes.  We should get the spec fixed here, I think.
Comment 29 Boris Zbarsky [:bz] (still a bit busy) 2012-07-06 06:22:07 PDT
> 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;
  }
Comment 30 Simon Montagu :smontagu 2012-07-08 12:08:20 PDT
Created attachment 640090 [details] [diff] [review]
Patch v.2, style part

Like this?
Comment 31 Simon Montagu :smontagu 2012-07-08 12:10:02 PDT
Created attachment 640091 [details] [diff] [review]
Patch v.2, content part

N.B: the new nsDirectionalityUtils would be overkill for just this, but I want it as a separate file for bug 548206
Comment 32 Simon Montagu :smontagu 2012-07-08 12:11:57 PDT
(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 33 :Ms2ger (⌚ UTC+1/+2) 2012-07-08 12:29:19 PDT
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()?
Comment 34 Boris Zbarsky [:bz] (still a bit busy) 2012-07-08 21:41:27 PDT
> 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.
Comment 35 Simon Montagu :smontagu 2012-07-11 05:46:20 PDT
Created attachment 641022 [details] [diff] [review]
Patch v.3, content part

(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.
Comment 36 Boris Zbarsky [:bz] (still a bit busy) 2012-07-11 09:29:19 PDT
> it skips all descendants as well as the node itself

See nsINode::GetNextNonChildNode, if it's relevant.
Comment 37 Simon Montagu :smontagu 2012-07-11 09:53:49 PDT
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.
Comment 38 Boris Zbarsky [:bz] (still a bit busy) 2012-07-11 09:55:09 PDT
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 39 :Ms2ger (⌚ UTC+1/+2) 2012-07-11 10:01:46 PDT
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.
Comment 40 Simon Montagu :smontagu 2012-07-11 22:01:31 PDT
(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 :(
Comment 41 Boris Zbarsky [:bz] (still a bit busy) 2012-07-12 05:10:24 PDT
Right, because the current version is still not setting aDependence as needed...
Comment 42 Simon Montagu :smontagu 2012-07-12 14:46:44 PDT
Created attachment 641608 [details] [diff] [review]
Patch v.3, style part

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 43 Simon Montagu :smontagu 2012-07-12 14:48:00 PDT
Created attachment 641609 [details] [diff] [review]
Updated tests
Comment 44 Boris Zbarsky [:bz] (still a bit busy) 2012-07-19 18:24:37 PDT
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 45 Boris Zbarsky [:bz] (still a bit busy) 2012-07-19 18:34:32 PDT
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 46 Boris Zbarsky [:bz] (still a bit busy) 2012-07-19 18:52:40 PDT
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 47 Boris Zbarsky [:bz] (still a bit busy) 2012-07-19 18:55:47 PDT
Comment on attachment 641609 [details] [diff] [review]
Updated tests

r=me
Comment 48 Simon Montagu :smontagu 2012-07-20 13:49:42 PDT
Created attachment 644454 [details] [diff] [review]
Patch v.4, content part
Comment 49 Simon Montagu :smontagu 2012-07-20 13:50:30 PDT
Created attachment 644456 [details] [diff] [review]
interdiff between the last two version of content patch
Comment 50 Simon Montagu :smontagu 2012-07-20 13:51:10 PDT
Created attachment 644457 [details] [diff] [review]
Tests, with some new ones
Comment 51 Simon Montagu :smontagu 2012-07-20 13:51:50 PDT
Created attachment 644459 [details] [diff] [review]
interdiff with the new tests
Comment 52 Simon Montagu :smontagu 2012-07-20 13:53:24 PDT
Created attachment 644461 [details] [diff] [review]
Patch v.4, style part (with review nits picked)

carrying over r=bz
Comment 53 Boris Zbarsky [:bz] (still a bit busy) 2012-07-20 15:01:24 PDT
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.
Comment 54 Boris Zbarsky [:bz] (still a bit busy) 2012-07-20 15:03:24 PDT
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 55 Boris Zbarsky [:bz] (still a bit busy) 2012-07-20 15:07:02 PDT
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.
Comment 57 Matt Brubeck (:mbrubeck) 2012-08-08 08:19:04 PDT
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.
Comment 58 Boris Zbarsky [:bz] (still a bit busy) 2012-08-08 08:57:02 PDT
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
Comment 60 Boris Zbarsky [:bz] (still a bit busy) 2012-08-08 09:39:16 PDT
No, really reopened.  Shouldn't have been marked fixed to start with when just landing on inbound...
Comment 61 Richard Newman [:rnewman] 2012-08-08 10:32:17 PDT
https://hg.mozilla.org/mozilla-central/rev/e226e413dd27

Note that commit message refers to the wrong bug number.
Comment 62 Simon Montagu :smontagu 2012-08-09 12:04:36 PDT
Created attachment 650637 [details] [diff] [review]
Patch for performance regression

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 63 Boris Zbarsky [:bz] (still a bit busy) 2012-08-09 12:11:25 PDT
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.  :(
Comment 64 :Ehsan Akhgari 2012-08-09 12:27:54 PDT
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!
Comment 67 Jean-Yves Perrier [:teoli] 2012-08-14 04:52:32 PDT
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)
?
Comment 68 Simon Montagu :smontagu 2012-08-14 06:37:46 PDT
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.
Comment 69 :Ehsan Akhgari 2012-08-14 07:39:35 PDT
(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...
Comment 70 Aharon (Vladimir) Lanin 2012-08-14 20:33:35 PDT
(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.
Comment 71 David Baron :dbaron: ⌚️UTC-10 2012-09-05 09:47:48 PDT
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...
Comment 72 Simon Montagu :smontagu 2012-09-05 10:11:39 PDT
(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.
Comment 73 David Baron :dbaron: ⌚️UTC-10 2012-09-17 12:56:20 PDT
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.
Comment 74 David Baron :dbaron: ⌚️UTC-10 2012-09-17 12:57:57 PDT
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.
Comment 75 David Baron :dbaron: ⌚️UTC-10 2012-09-17 13:03:51 PDT
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.
Comment 76 Simon Montagu :smontagu 2012-09-28 00:34:54 PDT
Created attachment 665815 [details] [diff] [review]
Prefix it

pushed to tryserver at https://tbpl.mozilla.org/?tree=Try&rev=3e7fb0aba3dc
Comment 77 David Baron :dbaron: ⌚️UTC-10 2012-09-29 11:24:14 PDT
Comment on attachment 665815 [details] [diff] [review]
Prefix it

r=dbaron.  Thanks for fixing this.
Comment 79 Simon Montagu :smontagu 2012-09-29 14:20:20 PDT
Created attachment 666250 [details] [diff] [review]
Use the prefix in internal css files

Callek pointed out on IRC that our own css uses :dir all over. This fixes that.
Comment 80 Ryan VanderMeulen [:RyanVM] 2012-09-29 21:09:36 PDT
https://hg.mozilla.org/mozilla-central/rev/72e03ee74f40
Comment 82 Ryan VanderMeulen [:RyanVM] 2012-09-30 18:03:00 PDT
https://hg.mozilla.org/mozilla-central/rev/71b82c3cd96c
Comment 83 Jean-Yves Perrier [:teoli] 2012-10-01 03:46:50 PDT
Is there plans to uptake this prefixing patch in Fx 17 (aurora now) to prevent an unprefixed :dir to be released?
Comment 84 David Baron :dbaron: ⌚️UTC-10 2012-10-01 07:30:50 PDT
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
Comment 87 Ramya Vadlamudi 2016-02-11 01:07:48 PST
May I know what are the plans for un-prefixing this feature?
Comment 88 Boris Zbarsky [:bz] (still a bit busy) 2016-02-11 06:15:38 PST
Ramya, bug 859301 tracks unprefixing.

Note You need to log in before you can comment on or make changes to this bug.