-
Notifications
You must be signed in to change notification settings - Fork 128
Conversation
This adds a timeout after the UI is first displayed. The UI is displayed, and handlers are added, but for EVERYTHING to get setup and working takes slightly longer.
// This avoids a problem where the UI has been instantiated, and handlers | ||
// added, but not everything is fully setup yet; by waiting we give it | ||
// time to set everything up | ||
return setTimeoutPromise(500); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're here again, the right way to fix this, according to webdriver best practices, is to use an implicit or explicit wait, rather than bake a long delay into the tests. @davehunt any suggestions on how to ensure webdriver waits for iframed UI to be fully ready?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to fix this we'd have to do something a bit fancier. The elements appear (it does wait), but if you click them right away there are problems.
This display code is the issue:
screenshots/addon/webextension/selector/ui.js
Lines 414 to 418 in 2abb4de
display(installHandlerOnDocument, standardOverlayCallbacks) { | |
return iframeSelection.display(installHandlerOnDocument) | |
.then(() => iframePreSelection.display(installHandlerOnDocument, standardOverlayCallbacks)) | |
.then(() => iframePreview.display(installHandlerOnDocument, standardOverlayCallbacks)); | |
}, |
All of iframeSelection.display
, iframePreSelection.display
and iframePreview.display
have to complete before you click on elements. They all complete in a reasonable time, but not reasonable for a computer that's polling for the interface.
To fix this properly we'd have to keep that promise, and use it to guard all the actual functions in installHandlerOnDocument
and standardOverlayCallbacks
. It felt a bit too complicated, though I suppose it's only 15ish lines of code when I think about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Followup in #3653
eh, let's just land this and do the even better thing in a followup bug |
No description provided.