search

hypothesis / h

Annotate with anyone, anywhere.
https://hypothes.is/
BSD 2-Clause "Simplified" License
2.91k stars 425 forks source link

WSJ page prevents tab from showing and sidebar from opening #1624

Closed dwhly closed 9 years ago

dwhly commented 9 years ago

From the forum: https://groups.google.com/d/msg/hypothesis-forum/38WVr59gDNw/m8aVMMXNP1wJ

I have a similar problem with this page: http://blogs.wsj.com/accelerators/2014/10/10/weekend-read-the-imminent-decentralized-computing-revolution/

However, unlike the above page, you cannot access the button to activate the sidebar at all (no work-around). In fact, the cursor shows up as if you were re-sizing the width, but will not function at all.

Confirmed.

aron commented 9 years ago

Nasty issue here, the WSG site is patching the native Array.prototype.reduce method and so it's not behaving correctly in our code. This particular issue was accidentally fixed by #1622.

However the correct fix is to update our Chrome extension to ensure that our code only runs in the context of the content script and not the page itself, thus keeping us isolated from things like this.

csillag commented 9 years ago

However the correct fix is to update our Chrome extension to ensure that our code only runs in the context of the content script and not the page itself, thus keeping us isolated from things like this.

Well, some of our code must run in the page itself, since it's interacting with the DOM. Isn't that so? (Or can we access the DOM from the outside?)

Furthermore, when dealing with bookmarklet and embedded deployment methods, we don't even have access to those external scopes - do we?

aron commented 9 years ago

@csillag created a ticket https://github.com/hypothesis/h/issues/1629 for this issue.

Short answer, no in the Chrome Extension we shouldn't run in the page itself. Chrome provides a sandboxed context that has full access to the DOM.

We're still stuffed with bookmarklets, but that has always been a workaround. And embeds are controlled by the implementor, so ideally they won't be doing anything stupid and if they are they at least have the power to fix it.

csillag commented 9 years ago

@csillag created a ticket https://github.com/hypothesis/h/issues/1629 for this issue.

Short answer, no in the Chrome Extension we shouldn't run in the page itself. Chrome provides a sandboxed context that has full access to the DOM.

Oh, sweet. My only concern is that going this way leads us into a Chrome-only place; ie. the differences between the code versions for the different deployment options will multiply.

More diversity -> more possibility for errors.

But I understand the technical superiority of this approach. Does something similar exist for FF, too?

nickstenning commented 9 years ago

My only concern is that going this way leads us into a Chrome-only place; ie. the differences between the code versions for the different deployment options will multiply.

Correct, but we don't have any other options. There are currently no cross-browser options to isolate us from pages monkeypatching native objects in ways that break h.

Does something similar exist for FF, too?

Something similar, yes: https://developer.mozilla.org/en-US/Add-ons/SDK/Guides/Content_Scripts

csillag commented 9 years ago

Correct, but we don't have any other options. There are currently no cross-browser options to isolate us from pages monkeypatching native objects in ways that break h.

The question is, is this trade-off worth the price? (Ie. do pages monkeypatch native objects often enough that avoiding these problems warrants bringing in the multitude of errors that will creep into our code because we try to maintain several incompatible code-paths and contexts?)

I am not sure about that - but I am happy to explore this direction, because it sounds elegant. (At least the chrome part. Adding the necessary condition logic to myriad of places for the different FF behavior - not so much.)

nickstenning commented 9 years ago

do pages monkeypatch native objects often enough that avoiding these problems warrants bringing in the multitude of errors that will creep into our code because we try to maintain several incompatible code-paths and contexts

This is a false dichotomy.

Yes, pages frequently monkeypatch important native objects in ways that break things. That's why this bug was opened. Our extension being unusable on the Wall St Journal would be reason enough to do this. See also https://github.com/openannotation/annotator/issues/290.

No, we do not have to "maintain several incompatible code-paths and contexts" any more than we will by shipping multiple browser extensions at all. The question is how we bootstrap the Hypothes.is application, not how we code the innards thereof.

csillag commented 9 years ago

Our extension being unusable on the Wall St Journal would be reason enough to do this.

Yes, that's true.

The question is how we bootstrap the Hypothes.is application, not how we code the innards thereof.

I am expecting this to spill out from bootstrapping to other areas as well - but I hope my concerns are exaggerated.

tilgovi commented 9 years ago

@dwhly maybe you shouldn't remove the done label. Keeping the label means it shows up in the done swimlane with an archive button, which I think is useful for retrospective. It means when we're all together we can celebrate what we got done and smile while you and Nick click "Archive" on each one.

dwhly commented 9 years ago

Happy to leave these till the retrospective.