Closed Bug 859301 Opened 8 years ago Closed 4 years ago

Unprefix css4 :dir

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: rexyrexy2, Assigned: bmo)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(4 files, 10 obsolete files)

20.29 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
10.80 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
5.14 KB, patch
bmo
: review+
Details | Diff | Splinter Review
17.45 KB, patch
bmo
: review+
Details | Diff | Splinter Review
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
Summary: Css2 selectors not fully supported → Unprefix css4 :dir
Component: Untriaged → Style System (CSS)
Product: Firefox → Core
Blocks: unprefix
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86_64 → All
Blocks: selectors-4
Version: 23 Branch → Trunk
Looks like this wasn't communicated:
  RESOLVED: unprefix :dir and :lang
  https://lists.w3.org/Archives/Public/www-style/2015Jan/0005.html
Duplicate of this bug: 1247576
Depends on: 562169
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...
Attached patch Part 1 - unprefix :dir (obsolete) — Splinter Review
MozReview-Commit-ID: 2oZTasI1Pny
Assignee: nobody → aschen
Status: NEW → ASSIGNED
MozReview-Commit-ID: JwvmgJU3W8J
Attached patch Part 3 - unprefix test cases (obsolete) — Splinter Review
MozReview-Commit-ID: DOAy5jWM3h6
MozReview-Commit-ID: LPZzaIXxHrY
Attachment #8742857 - Attachment is obsolete: true
Attached patch Part 1 - unprefix :dir (obsolete) — Splinter Review
MozReview-Commit-ID: 4OhSiP6Jel1
Attachment #8742856 - Attachment is obsolete: true
MozReview-Commit-ID: CRyp3zQxayh
Attachment #8743140 - Attachment is obsolete: true
MozReview-Commit-ID: 6KvILd31r9Y
Attachment #8742858 - Attachment is obsolete: true
MozReview-Commit-ID: BL6OInUgLtM
Attached patch Part 5 - add more wpt test cases (obsolete) — Splinter Review
MozReview-Commit-ID: E6wBtYszptM
MozReview-Commit-ID: r3nEa9I3jw
Comment on attachment 8748490 [details] [diff] [review]
Part 1 - unprefix :dir

unprefix :-moz-dir to :dir.
Attachment #8748490 - Flags: review?(dbaron)
Comment on attachment 8748491 [details] [diff] [review]
Part 2 - unprefix internal css files. r=dbaron

unprefix internal css files.
Attachment #8748491 - Flags: review?(dbaron)
Comment on attachment 8748492 [details] [diff] [review]
Part 3 - unprefix test cases. r=dbaron

unprefix relevant reftest test cases.
Attachment #8748492 - Flags: review?(dbaron)
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
Attachment #8748493 - Flags: review?(bzbarsky)
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.
Attachment #8748494 - Flags: review?(bzbarsky)
Comment on attachment 8748495 [details] [diff] [review]
Part 6 - update wpt manifest after adding test cases

Update wpt manifest.
Attachment #8748495 - Flags: review?(bzbarsky)
Comment on attachment 8748493 [details] [diff] [review]
Part 4 - update wpt manifest before adding test cases

Over to James.
Attachment #8748493 - Flags: review?(bzbarsky) → review?(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.
Attachment #8748494 - Flags: review?(bzbarsky) → review?(dbaron)
Attachment #8748495 - Flags: review?(bzbarsky) → review?(dbaron)
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
Attachment #8748490 - Flags: review?(dbaron) → review+
Attachment #8748491 - Flags: review?(dbaron) → review+
Attachment #8748492 - Flags: review?(dbaron) → review+
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.
Attachment #8748494 - Flags: review?(dbaron) → review-
Comment on attachment 8748495 [details] [diff] [review]
Part 6 - update wpt manifest after adding test cases

needs revision per comments on patch 5
Attachment #8748495 - Flags: review?(dbaron)
Blocks: 1270406
Comment on attachment 8748493 [details] [diff] [review]
Part 4 - update wpt manifest before adding test cases

test cases moved from wpt to w3c-css.
Attachment #8748493 - Attachment is obsolete: true
Attachment #8748493 - Flags: review?(james)
Comment on attachment 8748494 [details] [diff] [review]
Part 5 - add more wpt test cases

moved test cases to w3c-css test suite.
Attachment #8748494 - Attachment is obsolete: true
Attachment #8748494 - Flags: review-
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.
Attachment #8748495 - Attachment is obsolete: true
MozReview-Commit-ID: EyKKRrhARI0
Attachment #8748490 - Attachment is obsolete: true
Attached patch Part 4 - add w3c-css test cases (obsolete) — Splinter Review
MozReview-Commit-ID: LTZTyAxqeU0
MozReview-Commit-ID: BD4uCfziQdA
Attachment #8748491 - Attachment description: Part 2 - unprefix internal css files → Part 2 - unprefix internal css files. r=dbaron
Attachment #8748492 - Attachment description: Part 3 - unprefix test cases → Part 3 - unprefix test cases. r=dbaron
Comment on attachment 8749119 [details] [diff] [review]
Part 1 - unprefix :dir. r=dbaron

address review feedback in comment 27.
Attachment #8749119 - Attachment description: Part 1 - unprefix :dir → Part 1 - unprefix :dir. r=dbaron
Attachment #8749119 - Flags: review+
Comment on attachment 8749120 [details] [diff] [review]
Part 4 - add w3c-css test cases

address review feedback in comment 28.
Attachment #8749120 - Flags: review?(dbaron)
(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?
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
Attachment #8749120 - Flags: review?(dbaron) → review+
Comment on attachment 8749121 [details] [diff] [review]
Part 5 - update wpt meta for :dir test cases

shift this follow up to bug 1270713.
Attachment #8749121 - Attachment is obsolete: true
MozReview-Commit-ID: ETP2Welagwc
Attachment #8749120 - Attachment is obsolete: true
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.
Attachment #8749525 - Attachment description: Part 4 - add w3c-css test cases → Part 4 - add w3c-css test cases. r=dbaron
Attachment #8749525 - Flags: 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.
Keywords: checkin-needed
Blocks: 1270713
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.)
Flags: needinfo?(aschen)
(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
Flags: needinfo?(aschen)
(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.
Flags: needinfo?(aschen)
Blocks: 1278020
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.
Flags: needinfo?(aschen)
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.
You need to log in before you can comment on or make changes to this bug.