Disable animations for UI regression runs #5
Conversation
See T306229 This will fix the dancing tabs issue.
Co-authored-by: Nick Ray <[email protected]>
src/engine-scripts/puppet/jsReady.js
Outdated
| default: | ||
| return true; | ||
| } | ||
| return mw.loader.getState( moduleName ) === 'ready'; |
There was a problem hiding this comment.
Should this be a promise? e.g.
return mw.loader.using( moduleName )
As it stands, it doesn't look like its waiting for the module to finish loading. It looks like it's only checking its current state?
There was a problem hiding this comment.
No using would have side effects.
I think I'm getting confused by the evaluate method. I think a setInterval that resolves when this is true is perhaps what I need?
My understanding is evaluate has to resolve to true to avoid timeouts.
There was a problem hiding this comment.
The latest looks fine to me. I'm curious where you heard that it needs to resolve to true though? The puppeteer docs have examples that don't strictly return true e.g. https://pptr.dev/#?product=Puppeteer&version=v13.6.0&show=api-pageevaluatepagefunction-args
There was a problem hiding this comment.
When I was working on the scroll in the other patch, the scroll was synchronois and I was getting time outs if I didn't return something and didn't use an asynchronous function inside evaluate. I have found the documentation a little frustrating as it hasn't matched up with what I've been seeing
src/engine-scripts/puppet/jsReady.js
Outdated
| // wait for animation frame or two. | ||
| // Wait until the next frame before resolving. | ||
| await new Promise( resolve => { | ||
| requestAnimationFrame( () => { | ||
| requestAnimationFrame( () => { | ||
| resolve(); | ||
| } ); | ||
| } ); | ||
| } ); |
There was a problem hiding this comment.
This is currently throwing an error because it needs to go in a page.evaluate, but I'm also wondering if it's needed at this point? Can it be removed or have you had issues without it?
There was a problem hiding this comment.
Even with the removal of animations we still use requestAnimationFrame for the resize to get it off the critical path so I think this is needed to be 100% sure. Moving it into the evaluate is obviously needed, that's my bad.
I'm wrapping up for the day so feel free to amend while I am out. If not I'll pick this up Monday.
See T306229
This will fix the dancing tabs issue.