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

Event handlers compiled from inline attributes aren't seeing properties of their current target as variables #25517

Closed
pshaughn opened this issue Jan 14, 2020 · 10 comments

Comments

@pshaughn
Copy link
Member

@pshaughn pshaughn commented Jan 14, 2020

After #25488, the first subtest in html/webappapis/scripting/events/compile-event-handler-lexical-scopes.html does what it's supposed to do far enough to find an actual problem. A more minimal case to focus on the one problem is:

    <div id="log"></div>
    <input value="4" onclick="document.getElementById('log').innerText+=value">
    <script>
      document.querySelector("input").click()
    </script>

In Firefox, this puts 4 in the div. In Servo, this logs nothing and reports an exception that value is not defined.

I need to look at specifications more to figure out what mechanism should be exposing the properties in this way and what part of it Servo is missing; prior to this test, I was unaware this behavior existed at all.

@jdm
Copy link
Member

@jdm jdm commented Jan 14, 2020

It's not about exposing properties; it's that the this value when dispatching the event should be the HTMLInputElement, so a bareword lookup of value should be equivalent to element['value'].

@jdm jdm added this to To do in web-platform-test failures via automation Jan 14, 2020
@pshaughn
Copy link
Member Author

@pshaughn pshaughn commented Jan 14, 2020

That's not what this does in a normal function call. For example:

<script>
      x={a:5, b: function() { console.log(this); console.log(this.a); console.log(a); } }
      x.b();
</script>

In firefox the above code logs the object, then logs 5 once, then reports an exception that a is not defined.

@pshaughn
Copy link
Member Author

@pshaughn pshaughn commented Jan 14, 2020

Also, the this is being defined (both on my branch for #25488 and in master); if I change my first snippet to innertext+=this, I get [object HTMLInputElement] to appear.

@pshaughn pshaughn changed the title Events aren't seeing properties of their current target as variables Event handlers compiled from inline attributes aren't seeing properties of their current target as variables Jan 14, 2020
@pshaughn
Copy link
Member Author

@pshaughn pshaughn commented Jan 14, 2020

This is a special behavior of inline handlers defined in on* attributes, and it happens when they are compiled, as specified under "Scope" in step 3.9 of https://html.spec.whatwg.org/#internal-raw-uncompiled-handler . The same function body assigned into onclick using a Javascript function() block instead of a string does not see value in Firefox.

@pshaughn
Copy link
Member Author

@pshaughn pshaughn commented Jan 14, 2020

// TODO step 1.10.1-3 (document, form owner, element in scope chain)
let scopechain = AutoObjectVectorWrapper::new(*cx);

This is the missing piece; step numbers have changed, but looking for a scope in the document, form owner, and element is what 3.9 of https://html.spec.whatwg.org/#internal-raw-uncompiled-handler is saying to do.
Someone with more Spidermonkey knowledge than I have might want to jump in here.

@pshaughn pshaughn removed their assignment Jan 14, 2020
@pshaughn
Copy link
Member Author

@pshaughn pshaughn commented Jan 15, 2020

It looks like we need to turn the document, the form owner if it exists, and the target if it's an element into *mut JSObject, and pass them into scopechain.append. Getting those JSObject pointers is already implemented for LayoutDom, but as this is not happening during layout, I don't know what safety considerations exist here, and there is very obviously aliasing involved since we are also continuing to use the target.

@jdm
Copy link
Member

@jdm jdm commented Jan 15, 2020

We can get the JSObject pointer for any DOM object by calling dom_object.reflector().get_jsobject().get() on it. It should be straightforward to get the scope chain correct if we only need to append them to that vector.

@pshaughn
Copy link
Member Author

@pshaughn pshaughn commented Jan 15, 2020

Ok, I'll try it!

@pshaughn
Copy link
Member Author

@pshaughn pshaughn commented Jan 15, 2020

https://github.com/pshaughn/servo/tree/compiledeventscope fixes this, but won't pass any of the existing tests until #25488 lands because the existing tests also involve capture/bubble behavior. Pushing a new test just for this would be redundant since the larger tests are already covering it thoroughly. Is this the kind of thing that the _mozilla tests are for?

@jdm
Copy link
Member

@jdm jdm commented Jan 15, 2020

Yep! Those don't get upstreamed by default, so they cover browser-specific behaviour or non-standard extensions.

bors-servo added a commit that referenced this issue Jan 15, 2020
Put target element, form owner, and element document on scope chain of compiled events

Event listeners that are created from a function object just get whatever closure the function object had, but event listeners created from a string need a special closure that acts like they were defined inside a series of `with` statements. This now happens. The existing WPT test for it, html/webappapis/scripting/events/compile-event-handler-lexical-scopes.html, also relies on other behavior we don't have, so I added an easier version of the test that doesn't involve bubbling or capturing and doesn't check any IDL properties we don't have. This new test will eventually be redundant when we have everything else the upstream test expects.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #25517

<!-- Either: -->
- [X] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
web-platform-test failures automation moved this from To do to Done Jan 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked pull requests

Successfully merging a pull request may close this issue.

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