Comments (48)
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.
from input-events.
There isn't really good reasoning to use interface here, but I can live with it.
from input-events.
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.
from input-events.
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
- Add it to
InputEvent
spec (as it's the only usage) and maybe move later, or - Somewhere else (with a link in
InputEvent
spec)?
dictionary StaticRange {
Node startContainer;
unsigned long startOffset;
Node endContainer;
unsigned long endOffset;
};
from input-events.
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.
from input-events.
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.)
from input-events.
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.
from input-events.
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.
from input-events.
@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.
from input-events.
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.
from input-events.
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).
from input-events.
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.
from input-events.
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?
from input-events.
@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.
from input-events.
IIRC, the Dictionary/Interface discussion came about because concerns were raised about:
- the runtime cost of building a
StaticRange
object forgetTargetRanges
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.
from input-events.
@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.
from input-events.
@ojanvafai but with multiple event listeners they're not even valid for the lifetime of those events.
from input-events.
(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.)
from input-events.
That is, dictionary makes sense if @smaug----'s concerns are somehow invalid, but it's not entirely clear to me they are.
from input-events.
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.
from input-events.
OP as-is is not addressed @johanneswilm and it's pretty clear there's still an ongoing discussion.
from input-events.
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.
from input-events.
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.
from input-events.
@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.
from input-events.
@annevk Can we at least change the issue title?
from input-events.
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.
from input-events.
@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.)
from input-events.
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.
from input-events.
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.
from input-events.
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!)
from input-events.
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.
from input-events.
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;
};
from input-events.
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?
from input-events.
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)
from input-events.
from input-events.
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.
from input-events.
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.
from input-events.
@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
from input-events.
@smaug---- Thanks. With that it seems like we are all onboard.
from input-events.
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...)
from input-events.
I'd say live range still gives more reasonable result, exactly because it is changed to reflect the mutations. It effectively coalesces several mutations.
from input-events.
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?
from input-events.
@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.
from input-events.
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.
from input-events.
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.
from input-events.
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.
from input-events.
Resolution from the last voice call:
"We use interface for static range" https://lists.w3.org/Archives/Public/public-editing-tf/2017Mar/0004.html
from input-events.
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.
from input-events.
Related Issues (20)
- InputType to insert image from software keyboard HOT 9
- Encourage browsers to include files in `beforeinput` DataTransfer event HOT 2
- Request: a way to get and react to target ranges for all changes HOT 6
- Add spec-prod GitHub Actions HOT 4
- Bug on Android Web. Input Value duplicates when typing too fast between input fields.
- Use new `[=xref=]` syntax for DND terms HOT 1
- Move to Jitsi for meetings? HOT 1
- inputType not specified for the input type="number" 'step up/down' event
- Gather informations on content replaced by insertReplacementText or trigger a deleteByReplacement event?
- Should the last beforeinput to occur before compositionend be cancellable? HOT 11
- Order and cancelation behavior of events in composition disagrees with UI Events
- InputEvent fired twice when inserted emoji HOT 5
- What is the intended/recommended approach to hijack composition events? HOT 2
- inputType on safari mobile is undefined HOT 2
- Proposal: better encapsulation of composition events HOT 1
- Adjustments for EditContext HOT 5
- Definitions for 'Update the DOM'/'Update the DOM element' HOT 2
- getTargetRanges of `beforeinput` differ between browsers (should not happen in EditContext) HOT 33
- allow you to specify targetRanges and dataTransfer in InputEventInit HOT 1
- Update intention for insertReplacementText to include writing suggestions HOT 2
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from input-events.