-
Notifications
You must be signed in to change notification settings - Fork 204
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
Make annotation of iframes opt-in #533
Conversation
Run `prettier` on `frame-util.js` to fix up some minor formatting inconsistencies.
The client's iframe support is not yet robust enough (see #530) to enable it automatically for all iframes on arbitrary web pages. To support the needs of EPUB viewers and others in the meantime while preventing problems on eg. pages with larger numbers of iframed ads, require the publisher to opt iframes into annotation by adding the "enable-annotation" attribute to them.
Rather than just requiring publishers to tag their iframes, could we not detect common ad sizes and ignore those frames? Most adverts are one of several sizes, most of which would be super unlikely to be a frame with content anyone wants to annotate. |
Fair question. We already apply some heuristics like that, but they weren't good enough and we don't have a lot of bandwidth to validate anything we do come up with thoroughly at present for compatibility, performance etc. This approach is dumb & conservative but predictable. |
Codecov Report
@@ Coverage Diff @@
## master #533 +/- ##
==========================================
+ Coverage 90.93% 90.93% +<.01%
==========================================
Files 136 136
Lines 5426 5428 +2
Branches 945 945
==========================================
+ Hits 4934 4936 +2
Misses 492 492
Continue to review full report at Codecov.
|
…otation tools More info: hypothesis/client#533
Thanks for testing this out @sheetaluk 🙂 |
…otation tools More info: hypothesis/client#533
…otation tools More info: hypothesis/client#533
It turns out that the client's iframe support is not yet robust enough (see #530) to enable it automatically for all same-origin iframes on arbitrary web pages.
To support the needs of EPUB viewers and others in the meantime while preventing problems on eg. pages with larger numbers of iframed ads, require the publisher to opt iframes into annotation by
adding the
enable-annotation
attribute to them.Note that this doesn't fix the root issues described in #530 (comment) , but it does alleviate the symptoms on real-world websites until those issues are all addressed. Even when they are resolved, we will probably still want to use tougher heuristics before automatically enabling annotation for iframes because of the performance cost.
CC @jccr