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

Implement Range and Selection APIs #2719

Merged
merged 2 commits into from Dec 7, 2019
Merged

Conversation

@pmdartus
Copy link
Member

pmdartus commented Nov 21, 2019

Changes

This PR implements the Range and the Selection APIs. Sorry for this large PR, however the Range WPT tests are highly coupled with the Selection WPT tests and because of this wanted to make those 2 land at the same time.

Fix #317, fix #937

Copy link
Member

domenic left a comment

Wow, good stuff. Only one concern with the Range constructor.

I admit not reviewing several of the very-long files in great detail, but scrolling through them they all seem very nicely structured, with lots of spec references, just in the awesome manner I've come to appreciate from your work. So I'm happy to merge this soon.

@@ -401,6 +414,27 @@ function Window(options) {
writable: true
});

// https://dom.spec.whatwg.org/#dom-range-range
function Range() {

This comment has been minimized.

Copy link
@domenic

domenic Nov 28, 2019

Member

Why implement this in this manner instead of using webidl2js?

This comment has been minimized.

Copy link
@pmdartus

pmdartus Nov 29, 2019

Author Member

This is not needed anymore, it was used to associate the Range with the Document. This is not needed anymore with the webidl2js constructor reform. The Document can now be extracted out of the newly introduced globalObject.

lib/jsdom/living/nodes/Document-impl.js Show resolved Hide resolved
test/web-platform-tests/to-run.yaml Show resolved Hide resolved
@pmdartus pmdartus force-pushed the pmdartus:pmdartus/range branch from 2993143 to 2d4e35c Nov 29, 2019
@domenic

This comment has been minimized.

Copy link
Member

domenic commented Nov 29, 2019

I re-ran the CI three times and it gives segfaults on the latest Node stable once we start hitting the dom/ranges tests. https://travis-ci.org/jsdom/jsdom/jobs/618664359 . So probably we shouldn't merge until we either find a workaround or Node.js fixes their upstream bug.

I haven't tried reproducing locally yet; I hope it isn't hard to do so :(.

@pmdartus

This comment has been minimized.

Copy link
Member Author

pmdartus commented Nov 30, 2019

I was able to reproduce the issue locally against node 13.2.0.

@pmdartus

This comment has been minimized.

Copy link
Member Author

pmdartus commented Dec 7, 2019

The segfault was fixed with node 13.3.0. The CI is now passing and I also doubled checked locally just in case.

@domenic domenic merged commit 30bedcf into jsdom:master Dec 7, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rharriso

This comment has been minimized.

Copy link

rharriso commented Dec 14, 2019

I really appreciate this work. I'm refactoring a highlighting feature, so this will be extremely valuable to me.

@rpearce

This comment has been minimized.

Copy link

rpearce commented Feb 23, 2020

Thank you for this! Should it have also implemented Range.getClientRects()?

If range itself is fully supported, I believe this will open the door to automated color contrast accessibility testing via axe-core: https://github.com/dequelabs/axe-core/blob/104834fdc1fd09f5bb002ae48709f8939c0c7864/lib/rules/color-contrast-matches.js#L151.

@TimothyGu

This comment has been minimized.

Copy link
Member

TimothyGu commented Feb 24, 2020

There's probably no way for us to implement getClientRects, since we don't have a layout engine. See https://github.com/jsdom/jsdom#unimplemented-parts-of-the-web-platform.

@pmdartus pmdartus deleted the pmdartus:pmdartus/range branch Feb 24, 2020
@rpearce

This comment has been minimized.

Copy link

rpearce commented Feb 24, 2020

@TimothyGu Thank you for the information!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can’t perform that action at this time.