Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bugs in the matches() test. #1577

Closed
wants to merge 1 commit into from

Conversation

@Ms2ger
Copy link
Contributor

Ms2ger commented Jan 22, 2015

No description provided.

@tobie tobie added dom wg-html labels Jan 22, 2015
@hoppipolla-critic-bot

This comment has been minimized.

Copy link

hoppipolla-critic-bot commented Jan 22, 2015

Critic review: https://critic.hoppipolla.co.uk/r/3818

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

@Ms2ger

This comment has been minimized.

Copy link
Contributor Author

Ms2ger commented Jan 28, 2015

@Ms2ger

This comment has been minimized.

Copy link
Contributor Author

Ms2ger commented Oct 20, 2015

I extracted the first commit into #2267.

@cvrebert

This comment has been minimized.

Copy link
Contributor

cvrebert commented Feb 20, 2017

@Ms2ger Could you rebase this? I've likewise encountered the scopedSelectors bug.

@wpt-pr-bot

This comment has been minimized.

Copy link
Collaborator

wpt-pr-bot commented Feb 20, 2017

These worked under the mistaken assumption that matches() takes a relative
selector.
@Ms2ger Ms2ger force-pushed the Ms2ger:matches branch from f7bbaff to c58c565 Feb 20, 2017
@Ms2ger

This comment has been minimized.

Copy link
Contributor Author

Ms2ger commented Feb 20, 2017

Remaining comments:

--------------------------------------------------------------------------------
358| * These selectors are intended to be used with the find() and findAll() methods.  Expected results
--------------------------------------------------------------------------------
Lachlan Hunt at 2015-01-28 23:27 +00:00:
  find() and findAll() were the names from the original Selectors API 2 draft.
  The DOM spec renamed these methods to query() and queryAll().

  I also couldn't find any actual test scripts for query() and queryAll().
  Those should be written one day.

--------------------------------------------------------------------------------
417| * expect.matches(selector, context)    // Only where refNodes is not specified
--------------------------------------------------------------------------------
Lachlan Hunt at 2015-01-28 23:07 +00:00:
  These documentation comments no longer accurately describe how the matches()
  tests are run because the spec no longer includes support for context/refNodes
  in matches.

  Since no matches() tests are run on the selectors below any more, it would be
  better to move this explanation for the TEST_MATCH type above the other tests
  that are run for it.


General issue:
  https://critic.hoppipolla.co.uk/showcomment?chain=10265
--------------------------------------------------------------------------------
Lachlan Hunt at 2015-01-28 23:07 +00:00:
  Element-matches.js still runs tests based on the assumption that matches()
  takes a second refNodes paramter.  You should probably also edit that script
  to remove that entirely.
@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Apr 16, 2018

@Ms2ger, can you rebase this if it's still relevant?

@wpt-pr-bot wpt-pr-bot requested review from ayg, jdm and zqzhang Apr 16, 2018
@Ms2ger

This comment has been minimized.

Copy link
Contributor Author

Ms2ger commented Apr 16, 2018

I have no clue if it's still relevant. What do the test results look like for current browsers?

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Apr 16, 2018

Looks pretty good: https://wpt.fyi/dom/nodes/Element-matches.html

By scrolling, I see only one test that is failing in all browsers: "In-document Element.matches: Universal selector, matching all descendants of the specified reference element (with refNode Element): * "

Copy link

fathi966 left a comment

what this

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Oct 3, 2019

@Ms2ger do you want to rebase this and get it landed, or abandon?

@Ms2ger

This comment has been minimized.

Copy link
Contributor Author

Ms2ger commented Oct 3, 2019

If I haven't done anything a week from now, abandon.

@foolip

This comment has been minimized.

Copy link
Contributor

foolip commented Oct 10, 2019

A week has passed, closing. I suspect we'd find these same issues again by looking for tests that fail in all browsers.

@foolip foolip closed this Oct 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.