Status

()

defect
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: rexyrexy2, Assigned: bmo)

Tracking

(Blocks 2 bugs, {dev-doc-complete, site-compat})

Trunk
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(4 attachments, 10 obsolete attachments)

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
(Reporter)

Description

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

Comment 1

6 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
Summary: Css2 selectors not fully supported → Unprefix css4 :dir
Component: Untriaged → Style System (CSS)
Product: Firefox → Core

Updated

6 years ago
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.

Updated

6 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86_64 → All

Updated

4 years ago
Blocks: selectors-4
Version: 23 Branch → Trunk

Comment 4

4 years ago
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
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...
(Assignee)

Comment 8

3 years ago
Posted patch Part 1 - unprefix :dir (obsolete) — Splinter Review
MozReview-Commit-ID: 2oZTasI1Pny
(Assignee)

Updated

3 years ago
Assignee: nobody → aschen
Status: NEW → ASSIGNED
(Assignee)

Comment 9

3 years ago
MozReview-Commit-ID: JwvmgJU3W8J
(Assignee)

Comment 10

3 years ago
Posted patch Part 3 - unprefix test cases (obsolete) — Splinter Review
MozReview-Commit-ID: DOAy5jWM3h6
(Assignee)

Comment 11

3 years ago
MozReview-Commit-ID: LPZzaIXxHrY
(Assignee)

Updated

3 years ago
Attachment #8742857 - Attachment is obsolete: true
(Assignee)

Comment 13

3 years ago
Posted patch Part 1 - unprefix :dir (obsolete) — Splinter Review
MozReview-Commit-ID: 4OhSiP6Jel1
(Assignee)

Updated

3 years ago
Attachment #8742856 - Attachment is obsolete: true
(Assignee)

Comment 14

3 years ago
MozReview-Commit-ID: CRyp3zQxayh
(Assignee)

Updated

3 years ago
Attachment #8743140 - Attachment is obsolete: true
(Assignee)

Comment 15

3 years ago
MozReview-Commit-ID: 6KvILd31r9Y
(Assignee)

Updated

3 years ago
Attachment #8742858 - Attachment is obsolete: true
(Assignee)

Comment 16

3 years ago
MozReview-Commit-ID: BL6OInUgLtM
(Assignee)

Comment 17

3 years ago
MozReview-Commit-ID: E6wBtYszptM
(Assignee)

Comment 18

3 years ago
MozReview-Commit-ID: r3nEa9I3jw
(Assignee)

Comment 19

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

unprefix :-moz-dir to :dir.
Attachment #8748490 - Flags: review?(dbaron)
(Assignee)

Comment 20

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

unprefix internal css files.
Attachment #8748491 - Flags: review?(dbaron)
(Assignee)

Comment 21

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

unprefix relevant reftest test cases.
Attachment #8748492 - Flags: review?(dbaron)
(Assignee)

Comment 22

3 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
Attachment #8748493 - Flags: review?(bzbarsky)
(Assignee)

Comment 23

3 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.
Attachment #8748494 - Flags: review?(bzbarsky)
(Assignee)

Comment 24

3 years ago
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)
(Assignee)

Updated

3 years ago
Blocks: 1270406
(Assignee)

Comment 31

3 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.
Attachment #8748493 - Attachment is obsolete: true
Attachment #8748493 - Flags: review?(james)
(Assignee)

Comment 32

3 years ago
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-
(Assignee)

Comment 33

3 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.
Attachment #8748495 - Attachment is obsolete: true
(Assignee)

Comment 34

3 years ago
MozReview-Commit-ID: EyKKRrhARI0
(Assignee)

Updated

3 years ago
Attachment #8748490 - Attachment is obsolete: true
(Assignee)

Comment 35

3 years ago
MozReview-Commit-ID: LTZTyAxqeU0
(Assignee)

Comment 36

3 years ago
MozReview-Commit-ID: BD4uCfziQdA
(Assignee)

Updated

3 years ago
Attachment #8748491 - Attachment description: Part 2 - unprefix internal css files → Part 2 - unprefix internal css files. r=dbaron
(Assignee)

Updated

3 years ago
Attachment #8748492 - Attachment description: Part 3 - unprefix test cases → Part 3 - unprefix test cases. r=dbaron
(Assignee)

Comment 37

3 years ago
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+
(Assignee)

Comment 38

3 years ago
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)
(Assignee)

Comment 39

3 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.
(Assignee)

Comment 40

3 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."
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+
(Assignee)

Comment 44

3 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.
Attachment #8749121 - Attachment is obsolete: true
(Assignee)

Comment 45

3 years ago
MozReview-Commit-ID: ETP2Welagwc
(Assignee)

Updated

3 years ago
Attachment #8749120 - Attachment is obsolete: true
(Assignee)

Comment 46

3 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.
Attachment #8749525 - Attachment description: Part 4 - add w3c-css test cases → Part 4 - add w3c-css test cases. r=dbaron
Attachment #8749525 - Flags: review+
(Assignee)

Comment 47

3 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.
(Assignee)

Comment 48

3 years ago
Looks like it's good to go now.
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
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)
(Assignee)

Comment 53

3 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
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)
(Assignee)

Updated

3 years ago
Blocks: 1278020
(Assignee)

Comment 56

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