WebKit / Speedometer

An open source repository for the Speedometer benchmark
Other
592 stars 70 forks source link

This PR fixes the issue #365: jQuery test only ever marks the first todo item as completed #366

Closed rniwa closed 7 months ago

rniwa commented 7 months ago

The bug was caused by jQuery replacing the whole todo list whenever each todo item is toggled.

Fixed it by running querySelectorAll in each iteration.

rniwa commented 7 months ago

An alternative fix is modify the jquery todo implementation so that it won't nuke the whole todo list whenever one of them is toggled but that seemed like a bigger overhaul.

julienw commented 7 months ago

This produces a lot of GC due to the full rerenders... Maybe actually we should actually fix this for toggles? And maybe even for creates and destroys...

FTR I tried different approaches than the one with querySelectorAll here, but I got the same scores as a result. I tried using

querySelector(`li:nth-child(${i}) .toggle`)

and with getElementsByClassName. If you want to try, here are the links: with querySelectorAll with nth-child with getElementsByClassName

I'd like to hear other opinions.

camillobruni commented 7 months ago

I probably slightly prefer getElementsByClassName, but given the little difference in score I don't have a strong opinion.

julienw commented 7 months ago

What do folks think then? Should we favor https://github.com/WebKit/Speedometer/pull/367 using :nth-child?

bgrins commented 7 months ago

I think either this PR or #367 - we should close #368 since it doesn't use PageElement as-is

bgrins commented 7 months ago

367 is fine with me, if it works for @camillobruni / @flashdesignory

julienw commented 7 months ago

fixed by #367