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.

Sign up for GitHub

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

Jump to bottom

Copy user styles into dynamic IFRAMEs #39

Closed
wants to merge 4 commits into from
Closed

Copy user styles into dynamic IFRAMEs #39

wants to merge 4 commits into from

Conversation

hideheader
Copy link
Contributor

One MutationObserver listens for IFRAME additions and copies the existing STYLE.stylish elements into the new frames. A second MutationObserver listens for STYLE.stylish additions and deletions and copies STYLEs or removes existing copies into existing frames.

Fixes #3 ; some issues remain.

@hideheader
Merge pull request #5 from JasonBarnabe/master
9996839 
Update from JasonBarnabe
@hideheader
MutationObservers for IFRAMEs, Stylish STYLEs
8465604 
Working copy, with test scaffolding in place. Putting it in its own file because it's still a tangle.
@hideheader
Copy styles to src-less IFRAMEs
1bcb72d 
Copy user styles into child `IFRAME`s from the containing document.
@hideheader
Remove the test observers
5bb26e4 
Remove the IFRAME onLoad() and contentDocument observers.
@JasonBarnabe
Copy link
Contributor

I don't understand the purpose of the STYLE.stylish observer. Surely it would be easier to change apply.js's removeStyle and applySections to handle iframes than it is to listen for those changes and apply them to iframes.

@hideheader
Copy link
Contributor Author

Easier for who?

Evidently you do understand its purpose.

@JasonBarnabe
Copy link
Contributor

Easier as in less code, easier to understand code, and probably quicker code. You'd be able to skip all the code you have to figure out what styles are changing because apply.js's functions already know.

@hideheader
Copy link
Contributor Author

"Probably quicker"? Not enough to matter. I added eight IFRAMEs [1] to http://ace.c9.io/#nav=embedding and toggled the associated test style [2] on and off five times. Chrome spent 194ms in observers.js; 127ms of that was spent in styleObserverReport, writing to console.log() [3]. That's 2.5ms/operation/frame with the debug info, 0.8ms without it.

I'm not gonna refactor the code just to make it more beautiful. Right now it's entirely separable from the rest of the codebase. I suggest that, barring any substantive issues, you leave it that way for at least one release, and add a preference so that you can disable the observers if need be after 1.3.0 is distributed.

if (!emergencyStop) observer.observe()

If they haven't broken in a year, you can knit the code into 1.4.0.

@JasonBarnabe
Copy link
Contributor

Is the purpose of the observer on the style class="stylish" to react to what Stylish does when someone is installing/deleting/enabling/disabling styles, to react to the iframe being rewritten after it's added, or both?

@hideheader
Copy link
Contributor Author

Yes, no, and no. The style observer watches Stylish add or remove STYLE elements in the top-level document, and adds or removes copies of those same STYLE elements in the IFRAMES that are already in the document. It's only a proxy for the existing content script, just as you said. The frame observer sees IFRAME elements being added to the top-level document, and copies the Stylish styles that are already in the top-level document into the new frames.

This style observer can't see DOM mutations in the frames. For that you need to start a second style observer on an element in the frame's contentDocument. If that document is rewritten or replaced, though, the observer won't be called. To see frame content get rewritten you need to watch load on the IFRAME element, or else watch onCommitted in the background script.

That gets messy. There can be 2-3 loads for each observer call - once when the IFRAME is inserted with a pro forma <html><head/><body/></html> document, again when the pro forma document is replace with (I think) about:blank, then again with the document that's finally in place when the MutationObserver is called. It can be sorted, but without a use-case it didn't seem worthwhile.

There's an example intra-IFRAME observer and and onLoad listener in my preceding commit. Enable them with localStorage["observer.observeFrameContent"] = "true" and localStorage["observer.observeFrameLoad"] = "true", respectively.

@tophf
Copy link
Contributor

tophf commented Mar 2, 2015

Since forEach is extremely slow, this might be a problem on potentially large arrays like mutations and addedNodes. Unless performance tests on lots of big/complex pages (realtime complex charts, games, etc.) prove otherwise, it seems obviously reasonable to use a plain for-loop. The same goes for filter (also very slow, even on Chrome 43) and map (only slightly slower).

@JasonBarnabe
Copy link
Contributor

I've committed a version of this in 908d769

The most significant difference is that I removed the observer for style add/remove and instead changed the logic in apply.js to recursively grab dynamic iframes. I also moved the iframe add/remove observer to apply.js to be able to reuse some of the functions there.

@hideheader
Copy link
Contributor Author

And is it faster?

@tophf
Copy link
Contributor

tophf commented Mar 4, 2015

At least it has a bug!

@hideheader
Copy link
Contributor Author

A fast bug?

@tophf
Copy link
Contributor

tophf commented Mar 4, 2015

:-)
Haven't tested it yet on mutation-heavy pages but I'll keep an eye on its performance...

@tophf
Copy link
Contributor

tophf commented Mar 4, 2015

Jason's 908d769 breaks automatic style injection on feedly.com, even though I've fixed the obvious typo. This PR, on the other hand, being applied instead of 908d769, doesn't break feedly.com. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Stylish does not affect dynamically generated iframes
3 participants
@hideheader @JasonBarnabe @tophf