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

Speedometer 2.0 fails at backbone test #18442

Closed
jonathandturner opened this issue Sep 10, 2017 · 12 comments
Closed

Speedometer 2.0 fails at backbone test #18442

jonathandturner opened this issue Sep 10, 2017 · 12 comments

Comments

@jonathandturner
Copy link

@jonathandturner jonathandturner commented Sep 10, 2017

If you run the Speedometer 2.0 test on AWFY, with the latest Servo you can get through the 20 or so tests but then it will stop on the Backbone test at BackboneJS-TodoMVC(22 / 480):

screen shot 2017-09-11 at 8 47 02 am

Terminal:

DEBUG: -------------------------------
DEBUG: Ember  : 2.6.2
DEBUG: jQuery : 2.2.4
DEBUG: -------------------------------
ERROR:script::dom::bindings::error: Error at https://mozilla.github.io/arewefastyet-speedometer/2.0/resources/tests.js:246:17 checkboxes[i] is undefined

(will try to dig in a bit further, but just wanted to post this before I did)

@jonathandturner
Copy link
Author

@jonathandturner jonathandturner commented Sep 10, 2017

I'm having deja vu about this one. Is this related to needing to bump the mozjs/sm version?

@KiChjang
Copy link
Member

@KiChjang KiChjang commented Sep 10, 2017

Here's the surrounding context of the error:

        new BenchmarkTestStep('CompletingAllItems', function (newTodo, contentWindow, contentDocument) {
            var checkboxes = contentDocument.querySelectorAll('.toggle');
            for (var i = 0; i < numberOfItemsToAdd; i++)
                checkboxes[i].click();
        }),

I'm suspecting that querySelectorAll here was not able to select any elements, causing the indexed getter to return undefined.

@collares
Copy link
Contributor

@collares collares commented Sep 12, 2017

Can I take this bug? It seems like the test is creating the checkboxes by simulating typing and pressing Enter on an input element. Servo is not handling the Enter keypress correctly for some reason (the event does not reach jQuery, and so it also doesn't reach the TodoMVC JS code), and I would like to investigate a bit.

@jdm
Copy link
Member

@jdm jdm commented Sep 12, 2017

Ooh, please do! Glad to hear you have a theory.

@jdm jdm added the C-assigned label Sep 12, 2017
@KiChjang
Copy link
Member

@KiChjang KiChjang commented Sep 12, 2017

@collares
Copy link
Contributor

@collares collares commented Sep 12, 2017

So, https://github.com/servo/servo/blob/master/components/script/dom/document.rs#L1393 also seems relevant. It seems we only trigger keypress events when they correspond to a printable key, which (naively) means we would only get keyup and keydown events for pressing Enter, but never keypress. Does this seem plausible, or is it the kind of thing which would manifest itself in popular websites?

Relevant: https://bugzilla.mozilla.org/show_bug.cgi?id=968056. Apparently Servo follows the DOM 3 Events standard, and so does Blink. I will try to figure out why this works on Chrome.

@jdm
Copy link
Member

@jdm jdm commented Sep 12, 2017

Easy test:

<script>addEventListener('keypress', function(e) { console.log("press: " + e.key); })</script>

I only see output when pressing keys like 'a', 'f', '5'. Enter does not trigger it.

@jdm
Copy link
Member

@jdm jdm commented Sep 12, 2017

I believe the is_printable() check is attempting to address this text from the specification:

If supported by a user agent, this event MUST be dispatched when a key is pressed down, if and only if that key normally produces a character value.

(https://w3c.github.io/uievents/#event-type-keypress)
I suspect that enter is supposed to be treated as a \n character value in this case, so perhaps printable is the wrong concept to be using.

@collares
Copy link
Contributor

@collares collares commented Sep 13, 2017

Apologies, I made a mistake! There are two issues related to TodoMVC:

  1. When running the Backbone.js version of TodoMVC from [0], the Enter key does not work. This is because the glutin frontend sees that Enter is a control character [1] and handles it differently, making the is_printable() check mentioned by @jdm return false. This does not match Firefox or Chrome, so I will file a separate bug tomorrow to investigate if/how this should be fixed.

  2. The actual bug mentioned by @jonathandturner, in which the Backbone.js template is not evaluated properly. To be precise, escaped HTML gets inserted into the page instead of checkboxes, and this happens because a script tag's innerHTML attribute is getting escaped (which doesn't happen in Firefox). This happens at [2], and I verified that self.parent().html_name is None when serializing the Text node inside the "item-template" script tag for this sample:

<script type="text/template" id="item-template"><div></div></script>
<script type="text/javascript">console.log(document.getElementById("item-template").innerHTML</script>

I will investigate why this is the case tomorrow.

[0] http://swannodette.github.io/todomvc/architecture-examples/backbone/index.html
[1] https://github.com/servo/servo/blob/master/ports/glutin/window.rs#L373
[2] https://github.com/servo/html5ever/blob/04cfbf3463191ae8229e9f49429e2cb42dd486fd/html5ever/src/serialize/mod.rs#L205

@jdm
Copy link
Member

@jdm jdm commented Sep 13, 2017

Oh, that will be fixed by #17896. However, we need to release a new version of html5ever before that can merge (which is waiting on servo/html5ever#304).

@collares
Copy link
Contributor

@collares collares commented Sep 13, 2017

Good to know, thanks! So I guess the bug @jonathandturner found is already fixed :) I'll look at the other one (item 1) as soon as possible.

@jdm jdm removed the C-assigned label May 21, 2020
@jdm
Copy link
Member

@jdm jdm commented May 21, 2020

We can now progress beyond this test.

@jdm jdm closed this May 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.