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

Introduce a StaticRange constructor #590

Closed
annevk opened this issue Mar 12, 2018 · 14 comments
Closed

Introduce a StaticRange constructor #590

annevk opened this issue Mar 12, 2018 · 14 comments
Labels
addition/proposal New features or enhancements topic: ranges

Comments

@annevk
Copy link
Member

annevk commented Mar 12, 2018

Somehow StaticRange got added to browsers without the TAG noting the lack of a constructor. #589 adds StaticRange to the DOM Standard.

The proposal by @garykac in https://w3c.github.io/staticrange/index.html#interface-staticrange seems reasonable. You require a dictionary to be passed specifying all the bits.

One question is whether those bits should be validated as w3c/staticrange#13 suggests or whether we should allow any inputs and just pass them through. After all, you need to validate a StaticRange object whenever you use it, so why do it here as well? (Do we have any APIs yet that take StaticRange objects?)

It's not entirely clear to me what the tradeoffs are here.

cc @rniwa @johanneswilm @bzbarsky @dstorey

@annevk annevk added needs implementer interest Moving the issue forward requires implementers to express interest needs tests Moving the issue forward requires someone to write tests labels Mar 12, 2018
@annevk
Copy link
Member Author

annevk commented Mar 12, 2018

(As part of adding a constructor we should also expand the introduction of these objects a bit and mention the relative high cost of Range objects.)

@bzbarsky
Copy link

I dunno that there's any point to doing validation here. Anything working with StaticRange needs to handle invalid ones somehow anyway.

Apart from that, the constructor generally makes sense to me, assuming we're doing StaticRange at all. Are we? Has anyone other than WebKit added it? Where is WebKit using it?

@johanneswilm
Copy link

Chromium has added it as well. It's used with Input Events.

@rniwa
Copy link
Collaborator

rniwa commented Mar 14, 2018

WebKit uses it in input events.

@annevk
Copy link
Member Author

annevk commented Mar 17, 2018

Right, https://w3c.github.io/input-events/index.html has a method that can return them (not defined in detail though).

It seems like we should restrict the boundary node from being a doctype and possibly from being an attribute as well, depending on #607.

@sanketj
Copy link
Member

sanketj commented Aug 5, 2019

Hi all! I work on the Microsoft Edge team and we were interested in using static ranges as part of our Highlight API proposal. In case you’re unfamiliar with the highlight API, it provides a way for web developers to style the text inside arbitrary ranges within the DOM, which can be useful for editing applications that are trying to implement their own selection, find-on-page, spellchecking, etc. If you would like more details, please refer to this explainer: https://github.com/MicrosoftEdge/MSEdgeExplainers/blob/master/highlight/explainer.md. We believe the highlight API is a great use case for StaticRange; however, we would need StaticRanges to be constructible, so that authors can create these range objects and add them to one or more HighlightRangeGroups (see examples in the explainer).

From reading the responses here as well as w3c/staticrange#3, it sounds like there is general agreement about giving StaticRange a constructor and we’re just trying to iron out whether we need input validation. Is that correct? If so, I was wondering if there has been any further discussion on this topic (possibly elsewhere) or if we could pick this up again? I’m also happy to help drive this forward if needed.

@annevk
Copy link
Member Author

annevk commented Aug 6, 2019

There has been no further discussion that I'm aware of. I think we should restrict the types of nodes you can specify, but otherwise I don't think there's a need for validation here.

Also, to be clear, happy for you to drive this. We'll need web-platform-tests, bugs against implementations, and a PR against the DOM Standard (dom.bs in this repository).

@sanketj
Copy link
Member

sanketj commented Aug 7, 2019

Thanks @annevk ! I’m happy to help with all of those things.

I think Range and StaticRange should have the same set of allowed/disallowed nodes. From looking at the spec for Range.setStart/setEnd (https://dom.spec.whatwg.org/#concept-range-bp-set), it seems that doctype is the only disallowed one right now. So I propose we throw an exception (InvalidNodeTypeError) only if a doctype is passed to the StaticRange constructor. In the future, if we want to disallow other node types like Attr, we can make an update to disallow them on both range types.

Let me know if that sounds good.

cc: @BoCupp-Microsoft, @gked

@annevk
Copy link
Member Author

annevk commented Aug 7, 2019

Unless there's a good case for allowing Attr I'd rather be conservative as it's much easier to remove a restriction than to add one. In particular if there'll be APIs that take StaticRange but not Range this might be beneficial as they do not have to care about Attr at that point.

@sanketj
Copy link
Member

sanketj commented Aug 7, 2019

That's a good point actually - we wouldn't want an author to take a dependency on StaticRange working with Attr if we don't want to support it long term. I certainly don't see a use case for why we would need to support it.

So are we fine with disallowing DocumentType and Attr then? If so, I can get going with the spec PR, tests and implementation bugs.

@annevk
Copy link
Member Author

annevk commented Aug 8, 2019

Sounds great, though note that I'd expect at least some quibbling over the shape of the constructor as well, but who knows. 😊

@sanketj
Copy link
Member

sanketj commented Aug 22, 2019

I've created spec and WPT test PRs for this, and also filed implementation bugs. Please take a look and let me know what you think.

Spec PR: #778
WPT tests PR: web-platform-tests/wpt#18619

Implementation bugs:
Chromium: https://bugs.chromium.org/p/chromium/issues/detail?id=992606
Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1575980
Webkit: https://bugs.webkit.org/show_bug.cgi?id=201055

@rniwa
Copy link
Collaborator

rniwa commented Aug 23, 2019

I've uploaded a patch for WebKit on https://bugs.webkit.org/show_bug.cgi?id=201055. There is one minor bug in the WPT test but everything else looks good to me.

@annevk annevk removed needs implementer interest Moving the issue forward requires implementers to express interest needs tests Moving the issue forward requires someone to write tests labels Aug 23, 2019
@rniwa
Copy link
Collaborator

rniwa commented Aug 24, 2019

The WebKit implementation has landed in https://trac.webkit.org/changeset/249079.

@annevk annevk closed this as completed in a93f9f8 Aug 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements topic: ranges
Development

No branches or pull requests

5 participants