Skip to content
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

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

Closed
dwhly opened this issue Oct 31, 2014 · 10 comments
Closed

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

dwhly opened this issue Oct 31, 2014 · 10 comments

Comments

@dwhly
Copy link
Member

dwhly commented Oct 31, 2014

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
Copy link
Contributor

aron commented Nov 4, 2014

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.

@aron aron closed this as completed Nov 4, 2014
@csillag
Copy link
Contributor

csillag commented Nov 4, 2014

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
Copy link
Contributor

aron commented Nov 4, 2014

@csillag created a ticket #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
Copy link
Contributor

csillag commented Nov 4, 2014

@csillag created a ticket #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?

@aron aron mentioned this issue Nov 4, 2014
@nickstenning
Copy link
Contributor

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
Copy link
Contributor

csillag commented Nov 4, 2014

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
Copy link
Contributor

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 openannotation/annotator#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
Copy link
Contributor

csillag commented Nov 4, 2014

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.

@dwhly dwhly removed the 3 - Done label Nov 4, 2014
@tilgovi
Copy link
Contributor

tilgovi commented Nov 5, 2014

@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
Copy link
Member Author

dwhly commented Nov 5, 2014

Happy to leave these till the retrospective.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants