| 01:19 | <Krinkle> | JakeA: Found an odd not-so-funny IE script loading bug. https://phabricator.wikimedia.org/T178943 |
| 01:19 | <Krinkle> | TL;DR: onreadystate fires before script has actually executed, in IE 10, if and only if a certain strange <link rel=stylesheet> is on the page. |
| 01:19 | <Krinkle> | (after the <script async src>) |
| 01:20 | <Krinkle> | I was hoping your depressing article https://www.html5rocks.com/en/tutorials/speed/script-loading/ would cover this, but unfortunately not (as far as I can see) |
| 01:20 | <Krinkle> | Only IE10+ is required. |
| 01:20 | <Krinkle> | I think I'll get rid of the onreadystate as starting point, to make sure it really is that one that is the issue and not script.onload |
| 05:32 | <annevk> | Edge will have URL’s toJSON soonish |
| 07:53 | <smaug____> | hayato: commented on the composedPath issue. Do you still see there to be some performance issue? |
| 07:53 | <smaug____> | since I don't |
| 08:01 | <smaug____> | annevk: does my suggestion there for composedPath sound somewhat reasonable? |
| 08:03 | <hayato> | smaug____: I commented in the issue. |
| 08:03 | <smaug____> | thanks |
| 08:33 | <annevk> | smaug____: I suspect I don't have time to look into it soonish unless it's high priority |
| 08:33 | <smaug____> | annevk: well, I need to implement something |
| 08:33 | <annevk> | smaug____: ah okay, I'll have a look |
| 08:33 | <smaug____> | great thanks |
| 08:35 | <rniwa> | smaug____, annevk: i guess the change is to just pre-compute the composed path upfront? |
| 08:35 | <smaug____> | annevk: btw, it is hard to know what all you spec authors are busy with. Like you and Domenic are dealing with so many different spec issues |
| 08:35 | <rniwa> | smaug____, annevk: that seems fine with your proposed optimization. |
| 08:36 | <annevk> | smaug____: I'm mostly busy with a baby at the moment 😊 |
| 08:36 | <smaug____> | rniwa: I wouldn't call it precomputation. Just annotate the tuples so that the composed path can be calculated later |
| 08:36 | <smaug____> | annevk: oh sure. I thought you'd be mostly offline, but since you aren't ... ;) |
| 08:38 | <rniwa> | smaug____: so the challenge here is that we'd still have to compute which tree is visible to which tree |
| 08:38 | <rniwa> | smaug____: that could be expensive |
| 08:39 | <annevk> | rniwa: yeah, I'm not entirely sure how smaug____'s model works |
| 08:39 | <rniwa> | annevk: you'd remember which tree is visible to which tree upfront |
| 08:39 | <annevk> | If we stored a reference to the root of the item in event's path for instance I don't think we'd have enough information |
| 08:39 | <rniwa> | annevk: and then store that information along with event path |
| 08:40 | <rniwa> | annevk: right, because a shadow host can be moved from one tree to another |
| 08:40 | <rniwa> | annevk: so you'd have to remember the entire tree relationship :( |
| 08:41 | <rniwa> | now that i think about it, we probably don't want to make this change. |
| 08:41 | <rniwa> | it'd make event dispatching a lot more expensive... |
| 08:41 | <annevk> | Yeah, it seems you'd basically have to do that ancestor check during dispatch instead |
| 08:41 | <smaug____> | I'm strongly against the current model |
| 08:41 | <rniwa> | smaug____: okay but we've already shipped with the spec behavior |
| 08:41 | <smaug____> | since one can't rely on composedPath to return the path |
| 08:42 | <rniwa> | smaug____: of re-computing which node is visible to which node |
| 08:42 | <rniwa> | smaug____: we don't have the blink bug, for example |
| 08:43 | <annevk> | I wonder how expensive it would be though to calculate during dispatch |
| 08:43 | <annevk> | Since we have to do some traversal |
| 08:43 | <rniwa> | annevk: to do this, we'd have to remember the current/old "parent" tree for each shadow root. |
| 08:44 | <rniwa> | annevk: i've specifically designed webkit's event dispatching code not having to do those kinds of expensive computations or storing of information |
| 08:44 | <rniwa> | so I'd be very much against changing the status quo |
| 08:48 | <rniwa> | it's really annoying that event dispatching code keeps getting more complex & expensive |
| 08:48 | <rniwa> | at some point, we ought to start care about perf of these things... |
| 08:48 | <smaug____> | event path already remembers the old parent |
| 08:48 | <smaug____> | event path knows the dispatch time DOM structure |
| 08:49 | <rniwa> | smaug____: old parent node, sure, not old parent node's shadow tree's parent tree |
| 08:49 | <smaug____> | I do care about performance a lot |
| 08:50 | <smaug____> | it knows the path through shadow trees |
| 08:50 | <smaug____> | what am I missing here |
| 08:50 | <rniwa> | smaug____: so the problem is that let's say a path contains n1 > n2 |
| 08:50 | <rniwa> | where n1 and n2 are nodes |
| 08:50 | <rniwa> | if they belong to two different trees, say, t1, and t2 |
| 08:51 | <rniwa> | the relationship between t1 and t2 may have changed. |
| 08:51 | <smaug____> | sure, and we don't care about that change |
| 08:51 | <rniwa> | t1 and t2 could belong to two completely disjoint shadow trees |
| 08:51 | <smaug____> | right. I don't want to keep that in mind at all |
| 08:51 | <annevk> | smaug____: if we don't isn't there a possibility that we end up revealing something that should be hidden? |
| 08:51 | <rniwa> | smaug____: well, you should. otherwise, how do we decide whether n1 is visible to n2 or not? |
| 08:52 | <annevk> | smaug____: if I move a node into a closed tree during dispatch for instance |
| 08:52 | <smaug____> | Oh, you're worried about moving something from an open shadow dom to a closed one? |
| 08:52 | <rniwa> | smaug____: yeah, that can totally happen |
| 08:52 | <smaug____> | I'm not worried about that all |
| 08:52 | <smaug____> | that is a case which is totally fine. The node was already in path |
| 08:53 | <smaug____> | which exposed it to the outside world |
| 08:53 | <rniwa> | smaug____: you can move n1 into n2 or vice versa as well |
| 08:53 | <smaug____> | so hiding it then suddenly doesn't really matter |
| 08:53 | <rniwa> | smaug____: well, it totally matters depending on when the change was made |
| 08:54 | <rniwa> | smaug____: the very DOM mutation that just happened may have been done to hide those nodes |
| 08:54 | <smaug____> | composedPath should be able to answer to the question which all event targets will handle the event |
| 08:54 | <rniwa> | smaug____: except it shouldn't disclose nodes in the closed shadow trees |
| 08:55 | <smaug____> | right. But that modification happens after event path is created |
| 08:55 | <rniwa> | smaug____: i'd be opposed to any API change that results in a node inside a closed shadow tree to be exposed to the outside world |
| 08:55 | <rniwa> | smaug____: that would break the very important encapsulation invariant |
| 08:55 | <smaug____> | if one can move non-closed node to be inside closed shadow root, there is already some leak happening |
| 08:56 | <smaug____> | it is similar to having random node in non-shadow dom, keeping js ref to it, and then just move it to closed shadow dom |
| 08:56 | <smaug____> | you definitely give access to the internals of the closed shadow dom there |
| 08:57 | <smaug____> | so I'm missing to see the issue here |
| 08:57 | <rniwa> | smaug____: sure, it's not really a security threat model we're talking about here |
| 08:57 | <rniwa> | smaug____: the whole point of event retargeting and composedPath() is there to hide nodes in the shadow tree at the time of the event dispatching |
| 08:58 | <smaug____> | exactly. at the time of event dispatching |
| 08:58 | <smaug____> | not later |
| 08:58 | <rniwa> | smaug____: when I say dispatch I mean when we call event listener on each node |
| 08:59 | <rniwa> | smaug____: as in https://dom.spec.whatwg.org/#concept-event-listener-invoke |
| 08:59 | <smaug____> | and when I say event dispatching, I mean the time when event is actually dispatched. |
| 09:00 | <smaug____> | and if one moves open nodes to be suddenly inside closed trees, they are already having issues with encapsulation |
| 09:00 | <annevk> | smaug____: I don't understand why a leak is acceptable in this small edge case |
| 09:00 | <smaug____> | small? |
| 09:00 | <smaug____> | having a composedPath() which lies is totally nuts |
| 09:01 | <annevk> | smaug____: having it violate encapsulation seems equally wrong |
| 09:01 | <smaug____> | I see it so that one has already exposed the node |
| 09:01 | <smaug____> | since it wasn't in a closed tree |
| 09:02 | <annevk> | Okay, I guess I can buy that |
| 09:02 | <annevk> | I am somewhat sympathetic to your point of view, I guess the question is figuring out a model that satisfies rniwa |
| 09:05 | <rniwa> | annevk, smaug____: i don't think the visibility of a node should be computed upfront. |
| 09:05 | <smaug____> | why? |
| 09:05 | <rniwa> | annevk, smaug____: it should be computed each time composedPath() is called |
| 09:05 | <rniwa> | smaug____: to keep the encapsulation invariant valid |
| 09:05 | <smaug____> | I'd prefer a fast composedPath |
| 09:06 | <rniwa> | smaug____: I like fast event dispatching. |
| 09:06 | <rniwa> | smaug____: since most people won't be using composedPath |
| 09:06 | <smaug____> | my model doesn't slow down event dispatching |
| 09:06 | <rniwa> | smaug____: i don't want to increase the cost of dispatching an event |
| 09:06 | <rniwa> | smaug____: it absolutely would |
| 09:06 | <smaug____> | no |
| 09:06 | <smaug____> | well, sure, some flags |
| 09:06 | <rniwa> | smaug____: storing the extra tree scope information & its parent tree scope |
| 09:06 | <rniwa> | smaug____: is quit expensive |
| 09:07 | <smaug____> | you don't need that all |
| 09:07 | <smaug____> | you play with the path only |
| 09:07 | <rniwa> | smaug____: we would totally need that in our implementation |
| 09:07 | <smaug____> | I don't see why the encapsulation need to work that way |
| 09:07 | <rniwa> | smaug____: Reverse computing the tree path computation from the event path would be insanely complicated |
| 09:07 | <annevk> | smaug____: I think the problem is that rniwa doesn't agree with you on composedPath() revealing previously non-closed nodes |
| 09:07 | <smaug____> | yup |
| 09:08 | <smaug____> | and I don't understand why |
| 09:08 | <smaug____> | any listener in the path may have taken references to such nodes already and revealing the nodes anyhow |
| 09:08 | <rniwa> | smaug____: here's a question. if you think moving a node from one tree to another is not really a serious concern |
| 09:08 | <rniwa> | smaug____: then why do you care about this behavior at all? |
| 09:08 | <rniwa> | smaug____: no. that's not at all true |
| 09:09 | <smaug____> | I care about API sanity |
| 09:09 | <rniwa> | smaug____: you can move a node from one tree to another before any other node would have received the event |
| 09:09 | <smaug____> | right now we have API which doesn't return the value one would expect |
| 09:09 | <annevk> | rniwa: what smaug____ cares about is that composedPath() reflects the true event path |
| 09:09 | <rniwa> | smaug____: it totally does. |
| 09:09 | <rniwa> | annevk: and it does that |
| 09:09 | <smaug____> | it does not |
| 09:09 | <rniwa> | smaug____, annevk: event path itself is computed upfront |
| 09:10 | <rniwa> | smaug____, annevk: the only thing that's dynamically computed is whether a given node is visible to the current event target or not |
| 09:11 | <smaug____> | which is weird |
| 09:11 | <rniwa> | smaug____, annevk: i don't see why that affects anything about event path |
| 09:11 | <rniwa> | smaug____: it's not weird at all |
| 09:11 | <smaug____> | and doesn't let us optimize composePath calls |
| 09:11 | <rniwa> | smaug____: everywhere else in the DOM API where we support shaodw tree |
| 09:11 | <rniwa> | smaug____: we hide nodes whenever the node is closed-shadow-hidden from another node |
| 09:12 | <rniwa> | smaug____: you can totally optimize composedPath. |
| 09:12 | <rniwa> | smaug____: the only thing you need is detect a case where a shadow tree gets moved from one tree to another and becomes visible |
| 09:12 | <rniwa> | smaug____: since such a DOM mutation is very rare, you can keep using the cached composed path in most cases |
| 09:12 | <annevk> | smaug____: do we expect frequent composedPath() calls? (It seems there's some opportunity to optimize in any case though.) |
| 09:13 | <rniwa> | smaug____: just like you'd do for live node lists like getElementsByTagName. |
| 09:13 | <smaug____> | rniwa: tracking whether some particular tree is moved around isn't exactly fast |
| 09:13 | <smaug____> | live node lists are horrible for the performance |
| 09:13 | <rniwa> | smaug____: sure it is. you'd have to already update the value of isConnected for every node. |
| 09:14 | <smaug____> | yes. and that code is very hot |
| 09:14 | <rniwa> | smaug____: so your engine already needs to be aware when a node gets removed from the tree |
| 09:14 | <smaug____> | adding more stuff there makes it slower |
| 09:14 | <rniwa> | smaug____: well, adding stuff to event dispatch also makes things slower |
| 09:15 | <rniwa> | smaug____: event dispatching is also quite hot. |
| 09:15 | <rniwa> | smaug____: storing extra 2-3 objects in our event path code would end up introducing more dependent loads & writes |
| 09:16 | <smaug____> | and I wouldn't want to sure more objects there |
| 09:16 | <smaug____> | but we disagree with encapsulation here |
| 09:16 | <rniwa> | smaug____: it seems so |
| 09:16 | <rniwa> | smaug____: i really don't understand why you think it's okay to leak nodes in this case |
| 09:17 | <smaug____> | this isn't much different to leaking closed shadow via event.target or so |
| 09:17 | <smaug____> | first event.target is in open tree, one takes a ref to event, then target is moved to a closed tree |
| 09:17 | <rniwa> | smaug____: that's why event.target gets re-targeted not to do that |
| 09:17 | <smaug____> | you leak closed shadow |
| 09:18 | <rniwa> | smaug____: if you get to have access to it first, yes |
| 09:18 | <rniwa> | smaug____: but you can't always do that |
| 09:19 | <smaug____> | adding capturing listener to window is quite effective |
| 09:19 | <rniwa> | smaug____: unless the target is already in another closed shadow tree |
| 09:20 | <smaug____> | well, in my example target was in open tree |
| 09:20 | <rniwa> | smaug____: you can be in open tree inside a closed shadow tree |
| 09:21 | <smaug____> | oh sure. composedPath wouldn't leak that case, since it was initially hidden too from light dom |
| 09:22 | <rniwa> | smaug____: it would leak to the open trees of that closed shadow tree |
| 09:22 | <annevk> | smaug____: I think to make useful progress you'd have to make a more detailed proposal that rniwa can then critique |
| 09:23 | <smaug____> | but you leak that already if you move anything from open to closed. |
| 09:24 | <rniwa> | smaug____: that's not really true. |
| 09:24 | <rniwa> | smaug____ |
| 09:24 | <rniwa> | smaug____: if you're talking about the security level of leak, then yes |
| 09:24 | <rniwa> | smaug____: but you're talking about accidental leaks, then no |
| 09:25 | <smaug____> | easily. If you just happen to keep the reference to the originally open node |
| 09:25 | <rniwa> | smaug____: since shadow tree in its current form isn't a security feature, i don't think making an observation that something is technically possible isn't even useful |
| 09:25 | <smaug____> | and then start to play with it when it has been moved to closed tree |
| 09:26 | <rniwa> | smaug____: well, it's totally okay for a node to be in an open tree for a brief moment of time |
| 09:26 | <smaug____> | rniwa: right. So then I don't get your point here :) |
| 09:26 | <rniwa> | smaug____: also, JS keeping a reference to a node isn't really a serious concern to me since this is not really a security model |
| 09:26 | <rniwa> | smaug____: so let's say you have some component which moves child nodes of a shadow host into its own tree |
| 09:26 | <rniwa> | smaug____: and it does this lazily |
| 09:27 | <rniwa> | smaug____: and that lazily movement may happen when it detects that an event got dispatched on its host element |
| 09:27 | <rniwa> | smaug____: with the event target being one of the child nodes of the host |
| 09:27 | <rniwa> | smaug____: in that case, if this component moves the nodes into its closed shadow tree |
| 09:28 | <rniwa> | smaug____: then the outside observer who is listening to this event shouldn't be able to see these nodes as target's |
| 09:28 | <smaug____> | except if the outside observer is capturing listener, and would handle the event before the listener in the host |
| 09:29 | <rniwa> | smaug____: again, this is not a security model |
| 09:29 | <smaug____> | why would the composedPath be different for capturing vs. bubble listener |
| 09:29 | <smaug____> | sure |
| 09:29 | <smaug____> | I'm talking about API consistency |
| 09:29 | <rniwa> | smaug____: well, the component might be moving nodes at the capturing phase |
| 09:30 | <rniwa> | smaug____: the API is consistent to the extent nodes inside a closed shadow tree is always hidden |
| 09:30 | <rniwa> | smaug____: I agree that the list of nodes inside a composed event path shouldn't change |
| 09:30 | <rniwa> | smaug____: in fact, composedTree as currently spec'ed would always return the same event path if there are no closed shadow trees |
| 09:31 | <rniwa> | smaug____: however, there is a very important invariant for closed shadow trees, which is that no node inside a closed shadow tree is exposed to the outside DOM tree. |
| 09:32 | <smaug____> | but the node has been exposed to the outside world already... this is going circles or how does one say it in English |
| 09:32 | <rniwa> | smaug____: going in circles |