Closed Bug 1270713 Opened 8 years ago Closed 8 years ago

Enable corresponding wpt tests for pseudo class :dir()

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: bmo, Assigned: bmo)

References

Details

Attachments

(1 file, 3 obsolete files)

A follow-up of bug 859301 to enable wpt :dir tests.

Need some tweaks on below tests and remove corresponding ini files in meta folder.
/html/semantics/selectors/pseudo-classes/dir.html
/html/semantics/selectors/pseudo-classes/dir01.html
Depends on: 859301
Assignee: nobody → aschen
Status: NEW → ASSIGNED
Attachment #8750198 - Attachment is obsolete: true
Comment on attachment 8750203 [details] [diff] [review]
enable pseudo class :dir wpt test cases

Changes detailed as below:
1. Removed dir.html.ini and dir01.html.ini in meta folder to enable :dir tests.
2. Renamed testSelector() method to testSelectorIdsMatch() and add new method called testSelectorElementsMatch() in utils.js. One for Ids as an input to match selector result, another for Elements as an input.
3. Update method name testSelectorIdsMatch() to all test cases.
4. Update dir.html & dir01.html to ensure testing correctness. (Need review)
 - <span id=span1>WERBEH</span> -> should be ltr even direction:rtl applied.
 - &#x202E;<span id=span4>WERBEH</span>&#x202C; -> should be ltr.
Attachment #8750203 - Flags: review?(cam)
I'm not sure what the procedure is for modifying web-platform-tests that we have imported.  (I know that new tests can just be added, and will get upstreamed when someone runs the script, but I don't know what happens when you make modifications to existing tests.)  James, can you just confirm for me that this is OK?  Or should we be filing a PR upstream for these bug fixes and improvements?
Flags: needinfo?(james)
Yep, it's fine. The upstreaming script basically converts all commits modifying the testing/web-platform/tests/ directory to patches, rewrites the paths to match upstream and then imports them into the upstream repo, so additions, changes, and deletions are all supported.
Flags: needinfo?(james)
Comment on attachment 8750203 [details] [diff] [review]
enable pseudo class :dir wpt test cases

Review of attachment 8750203 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/web-platform/tests/html/semantics/selectors/pseudo-classes/dir.html
@@ +16,5 @@
> +  <body id=body>
> +    <div id="log"></div>
> +    <bdo dir="rtl" id=bdo1>WERBEH</bdo>
> +    <bdo dir="ltr" id=bdo2>HEBREW</bdo>
> +    <bdi id=bdi1>HEBREW</bdi>

Can you add a subtest with <bdi> with no dir="" attribute (so is treated like dir="auto") and with Arabic/Hebrew text content?  That should make it match :dir(rtl).
Attachment #8750203 - Flags: review?(cam) → review+
Attachment #8750203 - Attachment is obsolete: true
Comment on attachment 8752459 [details] [diff] [review]
enable pseudo class :dir wpt test cases. r=heycam

Addressed review comment 7. Wait for try result now.
Attachment #8752459 - Attachment description: enable pseudo class :dir wpt test cases → enable pseudo class :dir wpt test cases. r=heycam
Attachment #8752459 - Flags: review+
(In reply to Cameron McCormack (:heycam) from comment #7)
> Comment on attachment 8750203 [details] [diff] [review]
> enable pseudo class :dir wpt test cases
> 
> Review of attachment 8750203 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> testing/web-platform/tests/html/semantics/selectors/pseudo-classes/dir.html
> @@ +16,5 @@
> > +  <body id=body>
> > +    <div id="log"></div>
> > +    <bdo dir="rtl" id=bdo1>WERBEH</bdo>
> > +    <bdo dir="ltr" id=bdo2>HEBREW</bdo>
> > +    <bdi id=bdi1>HEBREW</bdi>
> 
> Can you add a subtest with <bdi> with no dir="" attribute (so is treated
> like dir="auto") and with Arabic/Hebrew text content?  That should make it
> match :dir(rtl).

Patch updated. Thanks for your feedback and review.
(In reply to Astley Chen [:astley] (UTC+8) from comment #10)
> Comment on attachment 8752459 [details] [diff] [review]
> enable pseudo class :dir wpt test cases. r=heycam
> 
> Addressed review comment 7. Wait for try result now.

Try result looks good. Ready to go.
Keywords: checkin-needed
Waiting for inbound merge and rebase the patch.
Keywords: checkin-needed
Attachment #8752459 - Attachment is obsolete: true
Comment on attachment 8753641 [details] [diff] [review]
enable pseudo class :dir wpt test cases. r=heycam

rebased. Waiting for another try result.
Attachment #8753641 - Attachment description: enable pseudo class :dir wpt test cases → enable pseudo class :dir wpt test cases. r=heycam
Attachment #8753641 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/7f0e63c09d7d
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: