User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20130405 Firefox/23.0 Build ID: 20130405040205 Steps to reproduce: went to http://css4-selectors.com/browser-selector-test/ and tested the support for selectors. Actual results: it said 100% on css1, 100% on css3, 30% on css4, and 92% on css2. Expected results: said 100% on css1, css2, and css3.

That page is wrongly referencing :dir as a css2 psudo selector, but it was removed from the css2 spec cause no browser implemented it. It is currently part of the css4 draft[0], and is implemented in firefox with a prefix (-moz-dir)[1]. This could prob be a future bug for unprefixing this property though. [0] http://dev.w3.org/csswg/selectors4/#the-dir-pseudo [1] https://developer.mozilla.org/en-US/docs/CSS/:dir#Browser_compatibility

See bug 562169 comment 71 and following for the decision to prefix it until the feature reaches CR.

(In reply to rexyrexy2 from comment #0) > went to http://css4-selectors.com/browser-selector-test/ and tested the > support for selectors. > Expected results: > said 100% on css1, css2, and css3. Contacted them to fix their test so it should correctly give 100% for 1,2,3.

Looks like this wasn't communicated: RESOLVED: unprefix :dir and :lang https://lists.w3.org/Archives/Public/www-style/2015Jan/0005.html

We do need to make sure we match the spec, but the spec is simple enough that I think this is mostly a matter of testing edge-cases of dir attribute support than anything else. There are some web platform tests for this stuff in https://github.com/w3c/web-platform-tests/blob/master/html/semantics/selectors/pseudo-classes/dir.html and https://github.com/w3c/web-platform-tests/blob/master/html/semantics/selectors/pseudo-classes/dir01.html but they probably need significant beefing up to test dynamic changes, to test dir="auto" (and dynamic changes to it), etc. I wonder whether we can just convert our existing :dir tests to web platforms tests and/or web platform reftests...

MozReview-Commit-ID: 2oZTasI1Pny

MozReview-Commit-ID: JwvmgJU3W8J

MozReview-Commit-ID: DOAy5jWM3h6

MozReview-Commit-ID: LPZzaIXxHrY

MozReview-Commit-ID: 4OhSiP6Jel1

MozReview-Commit-ID: CRyp3zQxayh

MozReview-Commit-ID: 6KvILd31r9Y

MozReview-Commit-ID: BL6OInUgLtM

MozReview-Commit-ID: E6wBtYszptM

MozReview-Commit-ID: r3nEa9I3jw

Comment on attachment 8748490 [details] [diff] [review] Part 1 - unprefix :dir unprefix :-moz-dir to :dir.

Comment on attachment 8748491 [details] [diff] [review] Part 2 - unprefix internal css files. r=dbaron unprefix internal css files.

Comment on attachment 8748492 [details] [diff] [review] Part 3 - unprefix test cases. r=dbaron unprefix relevant reftest test cases.

Comment on attachment 8748493 [details] [diff] [review] Part 4 - update wpt manifest before adding test cases update wpt manifest before adding new test cases. details: https://goo.gl/OcCILS

Comment on attachment 8748494 [details] [diff] [review] Part 5 - add more wpt test cases Convert relevant test cases into wpt :dir selector test suite with a bit of fine tweaks.

Comment on attachment 8748495 [details] [diff] [review] Part 6 - update wpt manifest after adding test cases Update wpt manifest.

Comment on attachment 8748493 [details] [diff] [review] Part 4 - update wpt manifest before adding test cases Over to James.

Comment on attachment 8748494 [details] [diff] [review] Part 5 - add more wpt test cases Seems to me like David should review this if he's reviewing the main patches; he'd have to look at these anyway to check whether the coverage is right.

Comment on attachment 8748490 [details] [diff] [review] Part 1 - unprefix :dir It's better to leave :-moz-dir() for a short transition period rather than removing it at exactly the same time as adding :dir. Thus: >diff --git a/layout/style/nsCSSParser.cpp b/layout/style/nsCSSParser.cpp > if (aType == CSSPseudoClassType::mozLocaleDir || > aType == CSSPseudoClassType::dir) { add "|| aType == CSSPseudoClassType::mozDir" here. >diff --git a/layout/style/nsCSSPseudoClassList.h b/layout/style/nsCSSPseudoClassList.h >-CSS_STATE_DEPENDENT_PSEUDO_CLASS(dir, ":-moz-dir", 0, "", >+CSS_STATE_DEPENDENT_PSEUDO_CLASS(dir, ":dir", 0, "", > NS_EVENT_STATE_LTR | NS_EVENT_STATE_RTL) And add a: CSS_STATE_DEPENDENT_PSEUDO_CLASS(mozDir, ":-moz-dir", 0, "", NS_EVENT_STATE_LTR | NS_EVENT_STATE_RTL) here right below the dir entry. And then please file a followup bug on removing :-moz-dir later. r=dbaron with that

Comment on attachment 8748494 [details] [diff] [review] Part 5 - add more wpt test cases >diff --git a/testing/web-platform/tests/html/semantics/selectors/pseudo-classes/dir-style-01-ref.html b/testing/web-platform/tests/html/semantics/selectors/pseudo-classes/dir-style-01-ref.html These really seem like tests of a CSS feature much more than an HTML one. I think that either: * they should go in a css/selectors/ directory of some sort (which jgraham should approe), or * they should go in layout/reftests/w3c-css/submitted/selectors4/ (which you'd need to create, and add to its parent directory's reftest.list) >+ <meta name="assert" content="The color of texts should match the reference html."> This (repeated between the tests) should say what assertion of the spec is being tested, not how to use the test. >+ <script type="text/javascript"> >+ function switchDir() >+ { >+ divs = document.getElementsByTagName("div"); This function should probably start with something that flushes layout, e.g., reading divs[0].offsetWidth, so that you actually test dynamic change. Same for the other places with similar patterns.

Comment on attachment 8748495 [details] [diff] [review] Part 6 - update wpt manifest after adding test cases needs revision per comments on patch 5

Comment on attachment 8748493 [details] [diff] [review] Part 4 - update wpt manifest before adding test cases test cases moved from wpt to w3c-css.

Comment on attachment 8748494 [details] [diff] [review] Part 5 - add more wpt test cases moved test cases to w3c-css test suite.

Comment on attachment 8748495 [details] [diff] [review] Part 6 - update wpt manifest after adding test cases no need to update manifest now since we move test cases to w3c-css test suite.

MozReview-Commit-ID: EyKKRrhARI0

MozReview-Commit-ID: LTZTyAxqeU0

MozReview-Commit-ID: BD4uCfziQdA

Comment on attachment 8749119 [details] [diff] [review] Part 1 - unprefix :dir. r=dbaron address review feedback in comment 27.

Comment on attachment 8749120 [details] [diff] [review] Part 4 - add w3c-css test cases address review feedback in comment 28.

(In reply to David Baron [:dbaron] ⌚️UTC-7 (review requests must explain patch) from comment #27) > Comment on attachment 8748490 [details] [diff] [review] > Part 1 - unprefix :dir > > It's better to leave :-moz-dir() for a short transition period rather than > removing it at exactly the same time as adding :dir. > > Thus: > > >diff --git a/layout/style/nsCSSParser.cpp b/layout/style/nsCSSParser.cpp > > > if (aType == CSSPseudoClassType::mozLocaleDir || > > aType == CSSPseudoClassType::dir) { > > add "|| > aType == CSSPseudoClassType::mozDir" here. > > > >diff --git a/layout/style/nsCSSPseudoClassList.h b/layout/style/nsCSSPseudoClassList.h > >-CSS_STATE_DEPENDENT_PSEUDO_CLASS(dir, ":-moz-dir", 0, "", > >+CSS_STATE_DEPENDENT_PSEUDO_CLASS(dir, ":dir", 0, "", > > NS_EVENT_STATE_LTR | NS_EVENT_STATE_RTL) > > > And add a: > CSS_STATE_DEPENDENT_PSEUDO_CLASS(mozDir, ":-moz-dir", 0, "", > NS_EVENT_STATE_LTR | NS_EVENT_STATE_RTL) > here right below the dir entry. > > And then please file a followup bug on removing :-moz-dir later. > > > r=dbaron with that patch updated according to your suggestion. It makes sense to keep a short transition.

(In reply to David Baron [:dbaron] ⌚️UTC-7 (review requests must explain patch) from comment #28) > Part 5 - add more wpt test cases > > >diff --git a/testing/web-platform/tests/html/semantics/selectors/pseudo-classes/dir-style-01-ref.html b/testing/web-platform/tests/html/semantics/selectors/pseudo-classes/dir-style-01-ref.html > > These really seem like tests of a CSS feature much more than an HTML one. > > I think that either: > * they should go in a css/selectors/ directory of some sort (which jgraham > should approe), or > * they should go in layout/reftests/w3c-css/submitted/selectors4/ (which > you'd need to create, and add to its parent directory's reftest.list) it looks more like a CSS feature test so I've put them into w3c-css test suites as your suggested path. > > >+ <meta name="assert" content="The color of texts should match the reference html."> > > This (repeated between the tests) should say what assertion of the spec is > being tested, not how to use the test. > > >+ <script type="text/javascript"> > >+ function switchDir() > >+ { > >+ divs = document.getElementsByTagName("div"); > > This function should probably start with something that flushes layout, > e.g., reading divs[0].offsetWidth, so that you actually test dynamic change. > > Same for the other places with similar patterns. addressed in new patch part 4 and rename patch title to "add w3c-css test cases."

Do we ever actually upstream CSS tests in the submitted directory?

(In reply to James Graham [:jgraham] from comment #41) > Do we ever actually upstream CSS tests in the submitted directory? Yes. I do it manually every so often. See, e.g.,: https://hg.csswg.org/test/rev/c602a05dca72 https://hg.csswg.org/test/rev/148664ab44bb https://hg.csswg.org/test/rev/8e043c73c859 https://hg.csswg.org/test/rev/e34085afdf03 https://hg.csswg.org/test/rev/51621f94e740 https://hg.csswg.org/test/rev/e29b28569d52 https://hg.csswg.org/test/rev/14b927d052f9 https://hg.csswg.org/test/rev/830303254a87

Comment on attachment 8749120 [details] [diff] [review] Part 4 - add w3c-css test cases >+ <meta name="assert" content="Text color of selected elements via :dir() should be styled correctly."> This still isn't very useful. I think the assert should be one of two things. It should either be: (1) a quote from the spec that is the sentence you're testing, or (2) a sentence that is *different* for each test, and explains what the test is testing and how it's different from the other tests. I think (2) is probably more useful. (If I'd actually felt the need to review the tests closely, I'd have asked you for (2) before I reviewed them.) r=dbaron with that fixed

Comment on attachment 8749121 [details] [diff] [review] Part 5 - update wpt meta for :dir test cases shift this follow up to bug 1270713.

MozReview-Commit-ID: ETP2Welagwc

Comment on attachment 8749525 [details] [diff] [review] Part 4 - add w3c-css test cases. r=dbaron Addressed comment 43 and update test assert description carefully for different tests based on dbaron's review.

(In reply to David Baron [:dbaron] ⌚️UTC-7 (review requests must explain patch) from comment #43) > Comment on attachment 8749120 [details] [diff] [review] > Part 4 - add w3c-css test cases > > >+ <meta name="assert" content="Text color of selected elements via :dir() should be styled correctly."> > > This still isn't very useful. > > I think the assert should be one of two things. It should either be: > > (1) a quote from the spec that is the sentence you're testing, or > > (2) a sentence that is *different* for each test, and explains what > the test is testing and how it's different from the other tests. > > I think (2) is probably more useful. > > (If I'd actually felt the need to review the tests closely, I'd have > asked you for (2) before I reviewed them.) Thanks for your feedback, it makes sense and need to be followed to better elaborate each test. I think it looks better now.

Looks like it's good to go now.

https://hg.mozilla.org/integration/mozilla-inbound/rev/300cbe6f1935 https://hg.mozilla.org/integration/mozilla-inbound/rev/81aa39025cb5 https://hg.mozilla.org/integration/mozilla-inbound/rev/a67f5076431d https://hg.mozilla.org/integration/mozilla-inbound/rev/125010deb798

https://hg.mozilla.org/mozilla-central/rev/300cbe6f1935 https://hg.mozilla.org/mozilla-central/rev/81aa39025cb5 https://hg.mozilla.org/mozilla-central/rev/a67f5076431d https://hg.mozilla.org/mozilla-central/rev/125010deb798

Also, could you send an intent-to-ship email to dev-platform? (Something modeled on https://wiki.mozilla.org/WebAPI/ExposureGuidelines#Intent_to_Implement but with Implement replaced by Ship.)

Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/dir-css-pseudo-class-has-been-unprefixed/

(In reply to David Baron [:dbaron] ⌚️UTC-7 (review requests must explain patch) (busy May 9-13) from comment #51) > Also, could you send an intent-to-ship email to dev-platform? (Something > modeled on > https://wiki.mozilla.org/WebAPI/ExposureGuidelines#Intent_to_Implement but > with Implement replaced by Ship.) Thanks. Sent: https://groups.google.com/forum/#!topic/mozilla.dev.platform/F0_UbXAfB_4

Updated: https://developer.mozilla.org/en-US/docs/Web/CSS/:dir and https://developer.mozilla.org/en-US/Firefox/Releases/49#CSS

(In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) (vacation June 4-12) from comment #28) > * they should go in layout/reftests/w3c-css/submitted/selectors4/ (which > you'd need to create, and add to its parent directory's reftest.list) When re-reviewing, I missed that you didn't add to (or really, just uncomment a line in) the parent directory's reftest.list. Therefore these tests aren't being run.

David, I'm sorry for the miss. At that time I just found selectors4 was there and didn't notice that it's commented out. I've created a follow up bug 1278020 and waiting for review and try result now. Thanks for your intensive review.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f5df0a09686af630e023c17e965977d9aa1c38a Remove commented-out subdirectories that were listed purely hypothetically to avoid their confusing people in the future. No bug.