Goggles doesn't add class attributes to <html> element

RESOLVED FIXED

Status

Webmaker
X-Ray Goggles
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: brett, Assigned: pomax)

Tracking

Details

(URL)

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
Happily we can now remix sites with odd markup like NYtimes - however, if you publish a work on NYtimes, you see odd CSS:

http://etherworks.makes.org/goggles/etherworkss-remix-of-the-new-york-times-breaking-news-world-news-amp-multimedia-2

Is there any simple solution that could be done here?  We discussed in IRC but wanted to file a bug as NYTimes is often the "go to page" for the goggles.
* This is a blocker for me publishing a Webmaker blost post announcement about the "new and improved" Goggles. So CCing myself here.
* Is this a blocker for letting people about our new and improved Goggles? I ask because:

a) I have a Webmaker blog post waiting on this. 
b) We talked about mentioning it in today's 2pm ET All Hands meeting
Flags: needinfo?(pomax)
From irc, I think we're cooked on this:

11:28 < Pomax> nytimes does some CORS on its own resources
11:29 < Pomax> so if the request isn't from nytimes.com, but makes.org, no dice
11:29 < Pomax> unless we start faking the entire network architecture through curl.js or something, I doubt we can get that to work
11:29 < humph> in which case
11:29 < humph> we aren't going to fix this
11:29 < Pomax> bottom line: nytimes REALLY doesn't want to be public data. just publically visible
11:29 < humph> s/aren't/can't/
11:29 < humph> OpenMatt: ^^
11:29 < humph> RESOLVED CRAPPY
11:30 < Pomax> I wonder if we can somehow steal JSON.stringify(document.styleSheets)
11:30 < cade> I think
11:30 < Pomax> then strip all script and links
11:30 < Pomax> though that would turn a website into a "picture of a website" much more than it does atm

I don't know how we resolve this, since they are basically not going to honour requests from us at makes.org for that extra content.  I suggest we resolve this as WONTFIX/CANTFIX, unless someone else has ideas?
(Assignee)

Comment 4

4 years ago
nytimes.com only has an issue with some vertical space, but otherwise is the same. cbc.ca I can't see any obvious differences, so I'd say show it off because being able to remix these sites is super cool. And then NYTimes.com has some nifty code that relies on requests for resources coming from their own server, which also demonstrates the limitations of a technology like goggles. Which is a lot better than goggles not even working on NYtimes.com

My vote is to show it off, and then as an actual issue resolvement, this is something we should probably not try to solve either. If a site has CORS that prevents their resources from loading except on their own site, then that's just a thing that happens on the internet. goggles currently can't, and probably never should, get around that kind of content security policy set up by the content owner.
Flags: needinfo?(pomax)

Updated

4 years ago
Summary: Investigate broken NYTimes.com published make → Goggles doesn't add class attributes to <html> element
(Assignee)

Comment 5

4 years ago
turns out that the page works just fine, IF goggles copies over the attributes on the <html> element, which it seems to be ignoring completely at the moment. So: it works, it just doesn't do an extra check on whether there is data on the html element.
(Assignee)

Comment 6

4 years ago
pretty sure our culprit ihttps://github.com/mozilla/goggles.webmaker.org/blob/master/lib/overrides/uproot.js#L91, which creates a new document with "clean" doctype and html, then injects the document.innerHTML

We can probably rewrite this to use document.querySelector("html").outerHTML instead of building "<html"> + document innerhtml + "</html>".
(Assignee)

Comment 7

4 years ago
Created attachment 8362622 [details] [review]
https://github.com/mozilla/goggles.webmaker.org/pull/91

uses outerHTML on document.querySelector("html"), which is a guaranteed fix on modern browsers and may or may not work on legacy versions of IE and/or Safari
Attachment #8362622 - Flags: review?(david.humphrey)
Comment on attachment 8362622 [details] [review]
https://github.com/mozilla/goggles.webmaker.org/pull/91

Awesome work tracking this down and fixing so fast.

^5 Pomax!
Attachment #8362622 - Flags: review?(david.humphrey) → review+

Comment 9

4 years ago
Commit pushed to master at https://github.com/mozilla/goggles.webmaker.org

https://github.com/mozilla/goggles.webmaker.org/commit/6fc68b42abd2ae50c7a4433692a3b1624e9b6c74
Merge pull request #91 from Pomax/bug961406

folds the <html> tag into the HTML we take off a page
(Assignee)

Updated

4 years ago
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
* Now that this is fixed, I'm going to publish that "What's new on Webmaker" blog post we prepped after Friday's demos
You need to log in before you can comment on or make changes to this bug.