Commit 4179c78e authored by Eduardo Sanz García's avatar Eduardo Sanz García Committed by Eduardo

Remove unnecessary check and simplify code

- Observe changes in `enable-annotation` attribute. Currently, if an
  existing iframe adds the attribute it won't be detected.

- `sidebar` or `notebook` iframes are shadow DOMed so they are not
  returned by `querySelector...` or `getElement...` (from elements
  outside the shadow root). Therefore, it is not necessary to filter
  these iframes out.
parent 8dacfead
...@@ -29,6 +29,7 @@ export default class FrameObserver { ...@@ -29,6 +29,7 @@ export default class FrameObserver {
this._mutationObserver.observe(this._target, { this._mutationObserver.observe(this._target, {
childList: true, childList: true,
subtree: true, subtree: true,
attributeFilter: ['enable-annotation'],
}); });
} }
......
/** /**
* Return all `<iframe>` elements under `container` which are annotate-able. * Return all `<iframe>` elements under `container` which are annotate-able.
* *
* To enable annotation, an iframe must be opted-in by adding the
* `enable-annotation` attribute.
*
* Eventually we may want annotation to be enabled by default for iframes that
* pass certain tests. However we need to resolve a number of issues before we
* can do that. See https://github.com/hypothesis/client/issues/530
*
* @param {Element} container * @param {Element} container
* @return {HTMLIFrameElement[]} * @return {HTMLIFrameElement[]}
*/ */
export function findFrames(container) { export function findFrames(container) {
const frames = Array.from(container.getElementsByTagName('iframe')); return Array.from(container.querySelectorAll('iframe[enable-annotation]'));
return frames.filter(shouldEnableAnnotation);
} }
// Check if the iframe has already been injected // Check if the iframe has already been injected
...@@ -40,29 +46,6 @@ export function isAccessible(iframe) { ...@@ -40,29 +46,6 @@ export function isAccessible(iframe) {
} }
} }
/**
* Return `true` if an iframe should be made annotate-able.
*
* To enable annotation, an iframe must be opted-in by adding the
* "enable-annotation" attribute and must be visible.
*
* @param {HTMLIFrameElement} iframe the frame being checked
* @returns {boolean} result of our validity checks
*/
function shouldEnableAnnotation(iframe) {
// Ignore the Hypothesis sidebar.
const isNotClientFrame = !iframe.classList.contains('h-sidebar-iframe');
// Require iframes to opt into annotation support.
//
// Eventually we may want annotation to be enabled by default for iframes that
// pass certain tests. However we need to resolve a number of issues before we
// can do that. See https://github.com/hypothesis/client/issues/530
const enabled = iframe.hasAttribute('enable-annotation');
return isNotClientFrame && enabled;
}
export function isDocumentReady(iframe, callback) { export function isDocumentReady(iframe, callback) {
if (iframe.contentDocument.readyState === 'loading') { if (iframe.contentDocument.readyState === 'loading') {
iframe.contentDocument.addEventListener('DOMContentLoaded', function () { iframe.contentDocument.addEventListener('DOMContentLoaded', function () {
......
import * as frameUtil from '../frame-util'; import * as frameUtil from '../frame-util';
describe('annotator.util.frame-util', function () { describe('annotator/util/frame-util', function () {
describe('findFrames', function () { describe('findFrames', function () {
let container; let container;
...@@ -52,17 +52,5 @@ describe('annotator.util.frame-util', function () { ...@@ -52,17 +52,5 @@ describe('annotator.util.frame-util', function () {
const foundFrames = frameUtil.findFrames(container); const foundFrames = frameUtil.findFrames(container);
assert.lengthOf(foundFrames, 0); assert.lengthOf(foundFrames, 0);
}); });
it('should not return the Hypothesis sidebar', function () {
_addFrameToContainer({ className: 'h-sidebar-iframe other-class-too' });
const foundFrames = frameUtil.findFrames(container);
assert.lengthOf(
foundFrames,
0,
'frames with hypothesis className should not be found'
);
});
}); });
}); });
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment