feat: implement disableHover, improve safeArea computation and other assertView fixes#1205
feat: implement disableHover, improve safeArea computation and other assertView fixes#1205shadowusr wants to merge 6 commits intotestplane@9from
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc62fd05b3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| }); | ||
|
|
||
| exports.cleanupPointerEvents = function () { |
There was a problem hiding this comment.
Its possible to encounter rare issue here
If we dont receive response to disable pointer events request, we will make another one.
In this case, exports.cleanupPointerEvents will overwrite another not empty function, and then "cleanup" wont clean this style, so it would be better to store somewhere array of style elements remove and remove elements from that array inside "exports.cleanupPointerEvents"
however, i see, i implemented "disableFrameAnimationsUnsafe" the same way, and looks like nobody encountered any issues, so we can keep it, i guess
| return runWithoutHistory({}, async () => { | ||
| if (opts.disableAnimation) { | ||
| await this._cleanupPageAnimations(); | ||
| } | ||
| if (opts.disableHover && opts.disableHover !== "never") { | ||
| await this._cleanupPointerEvents(); | ||
| } | ||
| await this._restorePointerPosition(); | ||
| }); |
There was a problem hiding this comment.
Can we await Promise.all it?
Also we could implement generic "cleanup" client script with multiple flags "what to cleanup" so we would be able to do it in one "browser.execute" instead of 2 consecutive. The difference is noticeable when using remote grids
| allowViewportOverflow: false, | ||
| ignoreDiffPixelCount: 0, | ||
| waitForStaticToLoadTimeout: 5000, | ||
| disableHover: DisableHoverMode.WhenScrollingNeeded, |
There was a problem hiding this comment.
Potential issue here
I suppose "WhenScrollingNeeded" is needed in cases, when pointer event happens after scroll.
But we can encounter this issue like this:
- Viewport height is 600px
- I move cursor to 550px height element and make screenshot of it with "hover" position
- Later i make some change to an element and its now 650px, so "disableHover" evaluates to "true"
- "hover" effect is lost
- User has no idea "why it looks like element lost its hover"
However, i have no idea "what to do in this case"
| const isFullPage = utils.isFullPage(imageArea, page, this._screenshotMode); | ||
| const cropArea = _.clone(page.viewport); | ||
| const isFullPage = utils.isFullPage(imageArea, viewport, this._screenshotMode); | ||
| const cropArea = _.clone(viewport); |
There was a problem hiding this comment.
2 participants
What's done:
disableHoverthat behaves similarly todisableAnimationand is intended to be used with long screenshot where hovering over elements may introduce unwanted artifacts