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

gracefully handle NONE computedstyle #1401

Merged
merged 1 commit into from
Dec 17, 2024

Conversation

LawyZheng
Copy link
Collaborator

@LawyZheng LawyZheng commented Dec 17, 2024

Important

Replace window.getComputedStyle with getElementComputedStyle in domUtils.js to handle null returns gracefully, improving robustness.

  • Behavior:
    • Replace window.getComputedStyle with getElementComputedStyle in domUtils.js to handle null returns gracefully.
    • Add null checks for computed styles in isScrollableOverflow(), checkRequiredFromStyle(), getElementContext(), and other functions.
  • Functions:
    • Modify isInteractable() to use getElementComputedStyle() for cursor checks.
    • Update isWindowScrollable() to use getElementComputedStyle() for overflow checks.
    • Adjust expectHitTarget() to handle display: contents elements correctly.
  • Misc:
    • Add null checks in loops and conditions to prevent errors when computed styles are null.

This description was created by Ellipsis for 1513292. It will automatically update as commits are pushed.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Reviewed everything up to 1513292 in 16 seconds

More details
  • Looked at 176 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. skyvern/webeye/scraper/domUtils.js:106
  • Draft comment:
    Ensure consistent handling of null or undefined computed styles. Consider using optional chaining (?.) consistently across the codebase to access properties of computed styles, as seen in line 109 and elsewhere.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The PR introduces a new function getElementComputedStyle to handle cases where window.getComputedStyle might return null. This function is used throughout the code to ensure that the computed style is safely accessed. However, there are some places where the code checks for null or undefined computed styles, and these checks are not consistent. For instance, in some places, the code uses optional chaining (?.) to access properties of the computed style, while in others, it explicitly checks if the computed style is null or undefined. This inconsistency can lead to confusion and potential bugs if not handled properly.

Workflow ID: wflow_tp9iDvKcBnlVhxI5


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@LawyZheng LawyZheng merged commit ad0171f into main Dec 17, 2024
2 checks passed
@LawyZheng LawyZheng deleted the lawy/gracefully-handle-getcomputedstyle branch December 17, 2024 05:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant