client icon indicating copy to clipboard operation
client copied to clipboard

[#6475] Whenever an iframe changes location Hypothesis needs to be injected again

Open jwgmeligmeyling opened this issue 1 year ago • 1 comments

When enabling annotations on an iframe with the enable-annotation attribute, it appears the client is only injected once. This leads to issues when the frame navigates to another page. I haven't checked whether the issue exists for all types of navigation (for example if it matters if the parent or the frame itself is causing the navigation). In this particular case the frame was initialised with about:blank.

I have found that reattaching the frame on the load event on the frame solves the issue. I am open for better suggestions fixing this.

Fixes #6475

jwgmeligmeyling avatar Jul 27 '24 19:07 jwgmeligmeyling

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

github-actions[bot] avatar Aug 27 '24 10:08 github-actions[bot]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 99.50%. Comparing base (f4e5e98) to head (9e3cc94). Report is 515 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6476      +/-   ##
==========================================
+ Coverage   99.43%   99.50%   +0.07%     
==========================================
  Files         271      275       +4     
  Lines       10238    11978    +1740     
  Branches     2425     3023     +598     
==========================================
+ Hits        10180    11919    +1739     
- Misses         58       59       +1     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar Sep 08 '24 10:09 codecov[bot]

@robertknight no interest in this fix?

jwgmeligmeyling avatar Mar 05 '25 22:03 jwgmeligmeyling

@robertknight no interest in this fix?

Sorry, we have been preoccupied by other things. When testing these changes manually in https://github.com/hypothesis/client/pull/6902 I found it was possible to get into a state where the client would be injected multiple times into the same iframe, causing the browser to get progressively slower after each navigation as more and more instances of the client try to load into the injected frame. From a quick glance it looks like this is happening because this PR will attach an additional load handler to the frame on every _addFrame call, and that handler is never removed.

robertknight avatar Mar 17 '25 11:03 robertknight

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

github-actions[bot] avatar Apr 17 '25 10:04 github-actions[bot]