Replace StaticRange with a dictionary or figure out if normal Ranges could be used

[smaug----]

If StaticRange approach (but just using dictionaries) is taken, dom mutations during event dispatch need to be observed so that getTargetRanges() can return something valid (empty array is ok, but StaticRanges pointing to invalid data isn't).

[chong-z]

The resolution from TPAC is: "(StaticRange) interface should be turned into dictionary".
See https://www.w3.org/2016/09/22-webapps-minutes.html#resolution04

Currently I cannot find any spec other than the original one proposed by @garykac
garykac/staticrange#2 (comment)

I was assuming the resolution would be something like below. For the convenience of development do we want to

1. Add it to InputEvent spec (as it's the only usage) and maybe move later, or
2. Somewhere else (with a link in InputEvent spec)?

dictionary StaticRange {
    Node startContainer;
    unsigned long startOffset;
    Node endContainer;
    unsigned long endOffset;
};

[johanneswilm]

I thought @garykac would update his proposal and move it to be a w3c editor draft. Is that correct? This would be my preferred solution.
But if this is not the case, I would also be ok with moving it to the input events spec.

[smaug----]

Just put it to the spec which needs it. If other specs will need something similar, they can add their own dictionaries or link to the InputEvent spec.

(I'd still like to understand the issue we're trying to solve with StaticRange. It mostly makes coding harder since it can so easily become invalid. And some of the performance issues should have been fixed by using a method and not attribute to get the range.)

[garykac]

It was also suggested (after TPAC) that it could be added to DOM where Ranges is defined: https://dom.spec.whatwg.org/#ranges

Now that it's a Dictionary (rather than an Interface), a new spec does seem like overkill. Johannes, why don't you add just add the definition to the Input spec for now. We can move it later if needed.

[ojanvafai]

Re: the problem being solved...

The performance issue is that web pages holding on to Ranges slows down subsequent DOM operations until the author stops holding on to them. This is unexpected for authors and makes composability hard, e.g. one library can't rely on the performance of appendChild being reasonable because some other editing library on the page might make appendChild calls slower by holding on to a lot of Ranges.

DOM operations are critical to a pages performance in many important scenarios. It's not OK to expose more APIs that make appendChild slower IMO.

[smaug----]

@ojanvafai you're still worried about web pages holding to range objects with the current InputEvent design? I agree if a range object is an attribute of an event, then copying the event easily causes one to keep Range objects alive too long.

I'd like to see some real world testcases showing Ranges actually cause issues (hopefully not implementation specific).

I'm a bit worried about over-engineering here. And I feel I'm missing some background information which did lead to use of StaticRange.

[ojanvafai]

http://jsbin.com/qotihebeqe/1/edit?html,output

It's a microbenchmark, but it's certainly within the ballpark of what a web site might do (200 appendChild+remove calls while holding on to 100 ranges). I did two calls of doTest each time and used the second one to avoid noise from JS engine optimizations.

Firefox 49: 0.6ms --> 1.1ms
Chrome 53: 0.2ms --> 0.5ms
Firefox 49 on Nexus 5x: 1.5ms --> 2.7ms
Chrome 53 on Nexus 5x: 1.4ms --> 6.4ms

The 5x numbers had a ton of variance, but the basic trend was the same.

Even if it's rare that authors do this, the platform shouldn't have performance footguns like this in something as low-level as appendChild.

[smaug----]

yeah, that is not a real world case. Has range objects shown up in real world cases. I assume they have since why would people otherwise start looking at StaticRange approach. So, I'm curious to know what is that real world case.

StaticRange is rather error prone concept, since any DOM mutation may invalidate the object, and whoever is keeping a reference to the StaticRange has no way to know whether it is still valid
(unless also keeping reference to a Range or use MutationObserver).

[smaug----]

But I'm not really objecting adding StaticRange dictionary. I just don't quite understand what kind of programming model should be used with it. Since it is guaranteed to be valid only a short while, the methods returning such objects should hint about that.

[johanneswilm]

I've added the dictionary definition without any of the explanatory text of Gary's spec. But maybe we should include some of that as well? Would that be helpful for implementers?

[ojanvafai]

@smaug---- exactly! The goal is to have people using these things only for the lifetime of the beforeInput/input events. I think naming it differently would be reasonable, but I don't know what to call it. Any suggestions?

@esprehn mentioned a real-world example. It's not on web content per se. We had an issue where we were using Ranges internally for some text insertion and if it took too long for a GC, your dom operations slowed to a crawl. crbug.com/613658 if you're curious. If Chrome internal implementation did this, it's not hard to imagine web authors doing so. But, unlike Chrome, web authors don't have an equivalent to StaticRange to hold use.

Also, @esprehn is concerned about switching this to be a Dictionary since it doesn't match what we do elsewhere on the platform. Hard to see why this should be special. It's hard to see from the notes why we decided this.

[garykac]

IIRC, the Dictionary/Interface discussion came about because concerns were raised about:

• the runtime cost of building a StaticRange object for getTargetRanges and
• the code/maintenance cost of adding support in the UA for an Interface used only in one place.

My personal view is that Range is an Interface so it would be nice to mirror that for consistency, but I didn't feel strongly enough about it to block moving forward with this.

[garykac]

@johanneswilm I think that explanation should be somewhere, but it would be distracting to include it in the Editing spec. If we don't end up with a StaticRange spec, we should at least have an explainer where that information lives.

[annevk]

@ojanvafai but with multiple event listeners they're not even valid for the lifetime of those events.

[annevk]

(And dictionary here makes sense, I'm pretty sure there's precedent elsewhere too, but it's hard to search for. There's no reason to have a class here since you're just returning some bits of information grouped together.)

[annevk]

That is, dictionary makes sense if @smaug----'s concerns are somehow invalid, but it's not entirely clear to me they are.

[johanneswilm]

Given that it has now been changed to a dictionary, the original bug report has been solved. If there are more and other things to be changed about this, let's open a new bug for that.

[annevk]

OP as-is is not addressed @johanneswilm and it's pretty clear there's still an ongoing discussion.

[johanneswilm]

Yes, but the discussion starts being about other things. And new people entering this have to wade through a lot of stuff that has been changed already. The main issue people had here is gone now. Now some may have issues with how it is now after the change. That's a new discussion.

[annevk]

The issue raised by OP

If StaticRange approach (but just using dictionaries) is taken, dom mutations during event dispatch need to be observed so that getTargetRanges() can return something valid

Is not addressed and is still being discussed. This issue was not about using dictionaries to begin with. Discussion about those is off-topic.

[johanneswilm]

@annevk I will not start a closing-opening issue war with you here, so I won't close it again. I can only repeat my argument that this other issue should better be discussed in a new issue as the situation is now different than what it was at the beginning of this thread and new people entering the discussion have to go through an unnecessarily large part of the history of the spec.

If StaticRange approach (but just using dictionaries) is taken, dom mutations during event dispatch need to be observed so that getTargetRanges() can return something valid

Yes, this seems to be a secondary issue. The issue mentioned in the title of this issue has been resolved.

[johanneswilm]

@annevk Can we at least change the issue title?

[johanneswilm]

If StaticRange approach (but just using dictionaries) is taken, dom mutations during event dispatch need to be observed so that getTargetRanges() can return something valid

Is the main concern here having multiple event listeners and getTargetRanges() being able to return something even though the JS has changed the DOM so that the initial values are invalid?

From a JS perspective, it seems to me it is enough for getTargetRanges() to return a useful response to the first event listener before the JS code has made any DOM changes itself. Beyond that, it doesn't really matter what it returns, or whether it differs from implementation to implementation.

[smaug----]

@esprehn mentioned a real-world example. It's not on web content per se. We had an issue where we were using Ranges internally for some text insertion and if it took too long for a GC, your dom operations slowed to a crawl. crbug.com/613658 if you're curious. If Chrome internal implementation did this, it's not hard to imagine web authors doing so. But, unlike Chrome, web authors don't have an equivalent to StaticRange to hold use.

As you say crbug.com/613658 is very much blink internal bug. About oilpan scheduling or so and not following the spec with selection.getRangeAt()
https://bugs.chromium.org/p/chromium/issues/detail?id=613658#c84
https://bugs.chromium.org/p/chromium/issues/detail?id=613658#c87
So, I'm still missing to see some real world case, not implementation bug ;)

(This reminds me that we could make Range objects' wrapper allocated from nursery heap and get the wrapper and C++ object collected really fast in Gecko, when possible.)

[esprehn]

There's a couple things:

Dictionaries in return values are bad:

I don't want us returning dictionaries in general from platform APIs. They're for pattern matching input, not for returning structured data. There's a bunch of reasons, see a summary here:
immersive-web/webxr#107 (comment)

So this either needs to be live Range objects, or StaticRange which matches StaticNodeList like the return value from querySelector(All).

Range is a performance footgun:

Having a Range instance makes DOM mutation slower until there's GC. I don't think it's okay for the web platform to require special casing various objects in the garbage collector to get reasonable performance. Eden generation semantics are not enough. If you create a Range object and keep it around while running other code, we're going to tenure it, and then your DOM is slow until we do a bigger GC.

That bug manifested as Blink-internal, but it's a general web platform bug, and you can easily end up in the same situation in your own code using Range (literally all we were doing is allocating Ranges and it was making the DOM slow). It's not a bug for the browser to create a Range object internally to implement an algorithm, since it's not observable. If the browser was written with a garbage collector (in JS, Oilpan, etc.), now you have an issue where calling some method that allocates Ranges makes everything slow for an indeterminate amount of time.

Performance footguns hurt developer confidence in the platform and we should avoid introducing more of these.

StaticRange is generally useful outside this spec:

StaticRange is independently useful since you can create one and then do operations on it without causing a performance penalty for all DOM mutations. GC has nothing to do with this. This isn't any different than StaticNodeList, which offers forEach()/iterators and other methods. StaticRange should similarly offer methods like Range, ex. expand() and so on.

We'll likely want to use StaticRange for other API surface too, since we're pretty against shipping any new API that allocates Ranges for the above mentioned footgun reasons.

Authors already cope with non-liveness in multi-actor situations:

Authors have done fine with querySelectorAll which can become stale when two actors are participating. This is a similar situation, and having the data become stale between event listeners if you change the DOM is already possible in a variety of situations, for example calling .focus() from inside your focus event handler means other event handlers will get a stale focus target.

[johanneswilm]

In general, I'm ok with all of this. One little note:

This isn't any different than StaticNodeList, which offers forEach()/iterators and other methods.

As a JavaScript developer, I still haven't quite gotten what the point is of obtaining a NodeList rather than an array. I just leared that I need to replace all

document.querySelectorAll("div");

with

[].slice.call(document.querySelectorAll("div"));

So that I get a regular array. I don't remember why I started doing that. I think otherwise I had no forEach, but now it's apparently there (Edit: in Chrome, not Firefox).

My point is this: I don't care much on whether or not it's a dictionary or another type of object you return. But if it's an array of these you return, let's either have it be a real array or if it's not a real array, let it have all the same methods and attributes as an array would have, because otherwise that just leads to confusion and for JS developers to translate them all into real arrays by default even when it isn't needed.

[esprehn]

My point is this: I don't care much on whether or not it's a dictionary or another type of object you
return. But if it's an array of these you return, let's either have it be a real array [...]

Totally! I think we've stopped adding "almost" array things. This is sequence<StaticRange> in idl which becomes Array<StaticRange> in JS. 😄

(I'd support making querySelectorAll return an Array too, but I think there were some libraries/pages that broke when we tried that in the past. We could try it again though!)

[rniwa]

Okay, it looks like @smaug---- suggested using dictionary instead of a separate StaticRange interface during TPAC on the grounds that it makes it clear it doesn't get updated, and everyone reached a consensus then. And now @esprehn is arguing for change this back to an interface?

This is all too confusing. We don't really have a strong opinion about this so could people who have opinion about this go back & discuss amongst your colleagues and come back with each organization's position?

It's unclear to me whether @smaug----'s position represents the broader perspective of Mozilla, and it's also unclear if @esprehn's position represents the boarder perspective of Google since yo1 (presumably from Google) agreed with the resolution at TPAC. Both @garykac and @travisleithead seem to be okay with dictionary at TPAC, and I'm not sure if their opinions have changed since then.

[rniwa]

One observation: as things stand, none of the IDL files that exist in WebKit ever return a dictionary. As far as I can tell, they're all arguments to some function.

[chong-z]

It appears that we cannot get into a general agreement on dictionary vs. interface, so I think it's safer to follow the style of other IDL files and return an interface. We could always switch if the dictionary approach was proven beneficial.

Besides, Webkit already has the interface-based implementation, and it would be risky for Blink to use dictionary and create interop issue.

// Webkit StaticRange.idl
[
    EnabledAtRuntime=InputEvents,
    ImplementationLacksVTable,
] interface StaticRange {
    readonly attribute unsigned long startOffset;
    readonly attribute unsigned long endOffset;
    readonly attribute Node startContainer;
    readonly attribute Node endContainer;
    readonly attribute boolean collapsed;
};

[johanneswilm]

I don't think it makes much of a difference for JS and agree that @rniwa and @choniong have a point. So unless someone feels very strongly about this, I think we should go with interface. Chong, have you discussed this with the rest of the Chromium team ane is this your common position?

[chong-z]

Yes there is a discussion but no there isn't a general agreement. However since esprehn@ has a strong preference on interface and others don't have intent to join the debate here, I would say it's relatively safe to match Webkit.
(BTW I'm not exactly sure who is yo1 but according to minutes he didn't have a strong preference on the dictionary-based approach)

[garykac]

Olli (smaug@) was a proponent of the dictionary, so I hope that he can chime in here. My preference is for an Interface, primarily for consistency reasons.

…

On Fri, Feb 17, 2017 at 1:33 PM, Chong Zhang ***@***.***> wrote: Yes there is a discussion but no there isn't a general agreement. However since esprehn@ has a strong preference on interface and others don't have intent to join the debate here, I would say it's relatively safe to match Webkit. (BTW I'm not exactly sure who is yo1 but according to minutes he didn't have a strong preference on the dictionary-based approach) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#38 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABaVShjPWqJ-PGq87mwRu7ViOMQQCMvHks5rdhIsgaJpZM4KD3Td> .

[chong-z]

To keep things moving I propose we change StaticRange to an interface to follow existing IDL style.

The debate about dictionary vs. interface should be moved to https://w3ctag.github.io/design-principles and shouldn't affect InputEvent until a general agreement was reached.

--
For a concrete IDL proposal @whsieh @rniwa are you OK with @garykac's proposal garykac/staticrange#2 (comment) or are you strongly against the constructor and toRange() method?
If so Blink will need to change implementation for best interop.

[johanneswilm]

I believe we have a resolution on this, which likely means we need to overturn that resolution first. This shouldn't be too complicated though.

[johanneswilm]

@chaals: Not sure if this also applies to taskforces, but it seems like if this had been a resolution of a committee, the chair (you) should note that the issue has been reopened [1]. Among the last commenters there does not seem much disagreement, but the suggestion goes against the resolutio n that came out if tge F2F. Should we do anything more before we can change it? Maybe notify/ask the email list?

[1] https://www.w3.org/2014/Process-20140801/#WGChairReopen

[smaug----]

There isn't really good reasoning to use interface here, but I can live with it.

[garykac]

OK. I'll start working on the StaticRange spec again to move it forward in the W3C. I had set it aside while we were using a dictionary.

[johanneswilm]

@smaug---- Thanks. With that it seems like we are all onboard.

[travisleithead]

I disagree with @esprehn's assertion that returning dictionaries is an anti-pattern. In the linked webvr discussion, instanceof tests can break down across Realms, and Use Counters tracking (an implementation detail) should not be a blocking design concern. The point about not being able to extend a dictionary in the future with a method (e.g., we wouldn't be able to add a isValid() method to our StaticRange dictionary) is something to keep in mind. Given that this dictionary is intended to package node/offset pairs (pure data) I don't see this as a concern. The data package approach is certainly a valid one in WebRTC stats, which returns a dictionary. I'm still in favor of a dictionary return type for our case here.

To @smaug----'s original question in #38 (comment), the behavior still needs to be clarified. I believe the case with getTargetRanges() is that after the DOM is mutated, the platform doesn't know what the target range is/was anymore. So a subsequent call to obtain a new target range would be... what? an empty array? This question doesn't go away when using a static interface. (And it gets worse when using a live Range, since the range will change with the DOM mutation and give incorrect information relative to the original target ranges...)

[smaug----]

I'd say live range still gives more reasonable result, exactly because it is changed to reflect the mutations. It effectively coalesces several mutations.

[johanneswilm]

Would it help to discuss this at the meeting on 2017-03-14? It seems like the views in the question whether live or static ranges are betetr are the same as at the F2F, so maybe there isn't much point in discussing that again?

I'm still in favor of a dictionary return type for our case here.

@travisleithead I understand your concerns. But how strong are they? Would you want for us to discuss this some more, or are you OK with the interface based on the argument that Webkit apparently is about to ship this anyway?

[gked]

@travisleithead I understand your concerns. But how strong are they? Would you want for us to discuss this some more, or are you OK with the interface based on the argument that Webkit apparently is about to ship this anyway?

@johanneswilm: Making a spec to follow someone else's implementation just because it is being shipped does not seem optimal. I like your idea to discuss this at 3/14 meeting.

[ojanvafai]

I suspect we'll want to add methods to this interface eventually. It's pretty hard to commit to never doing so in the future.

For example, I think we probably should add some of the methods from Range, e.g. comparePoint, getClientRects, etc. Seems problematic to force you to convert your StaticRange to a Range just to get that information. We went with this very minimal thing to start with just so we could get agreement on a v1 for beforeInput to finally ship. At least, my mind was on finding the most minimal thing to reduce any likelihood of controversity. Ironically, it might have been easier to suggest the less minimal thing since then it wouldn't have looked dictionary-like. :)

It's hard for me to see what we gain with dictionaries that is worth committing to never adding methods to this object.

[travisleithead]

If the group may want to add methods in the future, then interface is the right future-proof approach. Ironically @ojanvafai's hypothetical proposed future added methods look like they would be designed to mirror regular Range... which brings me full circle to question why a static range is desired... :-|

The group should probably discuss [again] at the F2F why static ranges are desired. If we don't need to have a new primitive thing (especially an interface) in the platform, I'd like to avoid it. Might be nice to run tests through an experimental implementation that offers both options to see which one is used/needed in practice.

[johanneswilm]

Might be nice to run tests through an experimental implementation that offers both options to see which one is used/needed in practice.

My guess is that as long as the beforeinput event is cancelable, it doesn't really matter to the JS developer, because the JS just reads the values once before manipulating the DOM itself. If the beforeinput event is not cancelable, and there are no target ranges on the input event, then live ranges may be more useful in that one can see where the action took place. But then again -- the non-cancelable beforeinput events may not be used at all and in that case it doesn't matter.

If the group may want to add methods in the future, then interface is the right future-proof approach.

Ok, sounds like we are all ok with the interface approach for now.

[johanneswilm]

Resolution from the last voice call:

"We use interface for static range" https://lists.w3.org/Archives/Public/public-editing-tf/2017Mar/0004.html

[ojanvafai]

Incidentally, regarding real world performance issues. We saw a case of hangs on Facebook that turns out to involve 30k Ranges being held on to. It appears to be fixed with facebook/react#9992. So, we have a least one real world case.