rexyrexy2 Reporter
	Description • 11 years ago

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.

	Cork
	Comment 1 • 11 years ago

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

	Simon Montagu :smontagu
	Comment 2 • 11 years ago

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

	John Drinkwater (:beta)
	Comment 3 • 11 years ago

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

	fantasai
	Comment 4 • 9 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 8 years ago

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

	Astley Chen (inactive) Assignee
	Comment 7 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb75f72054cb

	Astley Chen (inactive) Assignee
	Comment 8 • 8 years ago

MozReview-Commit-ID: 2oZTasI1Pny

	Astley Chen (inactive) Assignee
	Comment 9 • 8 years ago

MozReview-Commit-ID: JwvmgJU3W8J

	Astley Chen (inactive) Assignee
	Comment 10 • 8 years ago

MozReview-Commit-ID: DOAy5jWM3h6

	Astley Chen (inactive) Assignee
	Comment 11 • 8 years ago

MozReview-Commit-ID: LPZzaIXxHrY

	Astley Chen (inactive) Assignee
	Comment 12 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=89355c8b53a9

	Astley Chen (inactive) Assignee
	Comment 13 • 8 years ago

MozReview-Commit-ID: 4OhSiP6Jel1

	Astley Chen (inactive) Assignee
	Comment 14 • 8 years ago

MozReview-Commit-ID: CRyp3zQxayh

	Astley Chen (inactive) Assignee
	Comment 15 • 8 years ago

MozReview-Commit-ID: 6KvILd31r9Y

	Astley Chen (inactive) Assignee
	Comment 16 • 8 years ago

MozReview-Commit-ID: BL6OInUgLtM

	Astley Chen (inactive) Assignee
	Comment 17 • 8 years ago

MozReview-Commit-ID: E6wBtYszptM

	Astley Chen (inactive) Assignee
	Comment 18 • 8 years ago

MozReview-Commit-ID: r3nEa9I3jw

	Astley Chen (inactive) Assignee
	Comment 19 • 8 years ago

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

unprefix :-moz-dir to :dir.

	Astley Chen (inactive) Assignee
	Comment 20 • 8 years ago

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

unprefix internal css files.

	Astley Chen (inactive) Assignee
	Comment 21 • 8 years ago

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

unprefix relevant reftest test cases.

	Astley Chen (inactive) Assignee
	Comment 22 • 8 years ago

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

	Astley Chen (inactive) Assignee
	Comment 23 • 8 years ago

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.

	Astley Chen (inactive) Assignee
	Comment 24 • 8 years ago

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

Update wpt manifest.

	Boris Zbarsky [:bzbarsky]
	Comment 25 • 8 years ago

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

Over to James.

	Boris Zbarsky [:bzbarsky]
	Comment 26 • 8 years ago

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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 27 • 8 years ago

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

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 28 • 8 years ago

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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 29 • 8 years ago

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

needs revision per comments on patch 5

	Astley Chen (inactive) Assignee
	Comment 30 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ec39cd09100

	Astley Chen (inactive) Assignee
	Comment 31 • 8 years ago

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

test cases moved from wpt to w3c-css.

	Astley Chen (inactive) Assignee
	Comment 32 • 8 years ago

Comment on attachment 8748494 [details] [diff] [review]
Part 5 - add more wpt test cases

moved test cases to w3c-css test suite.

	Astley Chen (inactive) Assignee
	Comment 33 • 8 years ago

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.

	Astley Chen (inactive) Assignee
	Comment 34 • 8 years ago

MozReview-Commit-ID: EyKKRrhARI0

	Astley Chen (inactive) Assignee
	Comment 35 • 8 years ago

MozReview-Commit-ID: LTZTyAxqeU0

	Astley Chen (inactive) Assignee
	Comment 36 • 8 years ago

MozReview-Commit-ID: BD4uCfziQdA

	Astley Chen (inactive) Assignee
	Comment 37 • 8 years ago

Comment on attachment 8749119 [details] [diff] [review]
Part 1 - unprefix :dir. r=dbaron

address review feedback in comment 27.

	Astley Chen (inactive) Assignee
	Comment 38 • 8 years ago

Comment on attachment 8749120 [details] [diff] [review]
Part 4 - add w3c-css test cases

address review feedback in comment 28.

	Astley Chen (inactive) Assignee
	Comment 39 • 8 years ago

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

	Astley Chen (inactive) Assignee
	Comment 40 • 8 years ago

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

	James Graham [:jgraham]
	Comment 41 • 8 years ago

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

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 42 • 8 years ago

(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

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 43 • 8 years ago

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

	Astley Chen (inactive) Assignee
	Comment 44 • 8 years ago

Comment on attachment 8749121 [details] [diff] [review]
Part 5 - update wpt meta for :dir test cases

shift this follow up to bug 1270713.

	Astley Chen (inactive) Assignee
	Comment 45 • 8 years ago

MozReview-Commit-ID: ETP2Welagwc

	Astley Chen (inactive) Assignee
	Comment 46 • 8 years ago

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.

	Astley Chen (inactive) Assignee
	Comment 47 • 8 years ago

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

	Astley Chen (inactive) Assignee
	Comment 48 • 8 years ago

Looks like it's good to go now.

	Pulsebot
	Comment 49 • 8 years ago

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

	Carsten Book [:Tomcat]
	Comment 50 • 8 years ago
bugherder

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

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 51 • 8 years ago

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

	Kohei Yoshino [:kohei]
	Comment 52 • 8 years ago

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

	Astley Chen (inactive) Assignee
	Comment 53 • 8 years ago

(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

	Jean-Yves Perrier [:teoli]
	Comment 54 • 8 years ago

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

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 55 • 8 years ago

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

	Astley Chen (inactive) Assignee
	Comment 56 • 8 years ago

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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 57 • 8 years ago

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.