client icon indicating copy to clipboard operation
client copied to clipboard

Error with iframe `allow` attribute when annotating pages on archive.org

Open robertknight opened this issue 1 year ago • 2 comments

Steps to reproduce:

Try to activate browser extension on https://web.archive.org/web/20210725180830/https://www.northcentralcollege.edu/shimer-great-books-blog/2018/09/05/fresh-eyes-great-books-students-learn-see-world-through-new-lens

Expected:

This error in the terminal:

Uncaught (in promise) TypeError: Cannot assign to read only property 'allow' of object '#<HTMLIFrameElement>'
    at Wombat.rewriteNodeFuncArgs (wombat.js?v=txqj7nKC:21:35153)
    at HTMLDivElement.appendChild (wombat.js?v=txqj7nKC:21:93112)
    at new kr (sidebar.tsx:236:28)
    at index.ts:109:21

Wombat is part of pywb, the same system that we use in Via.

robertknight avatar Feb 15 '24 14:02 robertknight

Hypothesis sets a non-writable property on the sidebar iframe specifically to avoid pywb messing with it. See https://github.com/hypothesis/client/blob/1e02301eeca096d14648a33980688a8a315f6cd8/src/annotator/sidebar.tsx#L93. We could avoid the error here by making the allow property writable, but making the setter a no-op, which is how various native DOM properties work if you assign invalid values to them.

robertknight avatar Feb 15 '24 14:02 robertknight

I created a PR upstream to avoid the need for any workaround, after agreeing with the maintainer that it made sense, but it has been stale since https://github.com/webrecorder/wombat/pull/134

We could avoid the error here by making the allow property writable, but making the setter a no-op

I think this is very reasonable.

acelaya avatar Feb 15 '24 14:02 acelaya

The PR in wombat has been merged and released. I suppose we'll get the fix when pywb is updated.

acelaya avatar Feb 22 '24 11:02 acelaya

I suppose we'll get the fix when pywb is updated.

I'm not sure how long that will take. We should probably put an interim workaround in our own code, such as giving the sidebar iframe a no-op allow setter.

robertknight avatar Feb 22 '24 11:02 robertknight

I suppose we'll get the fix when pywb is updated.

I'm not sure how long that will take. We should probably put an interim workaround in our own code, such as giving the sidebar iframe a no-op allow setter.

Yep, agree.

acelaya avatar Feb 22 '24 13:02 acelaya

I created https://github.com/hypothesis/client/pull/6227 to address this with a noop setter as suggested.

acelaya avatar Feb 22 '24 14:02 acelaya