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

SingleFile in Safari does not save deferred images. #1022

Closed
adomasven opened this issue Aug 12, 2022 · 23 comments
Closed

SingleFile in Safari does not save deferred images. #1022

adomasven opened this issue Aug 12, 2022 · 23 comments

Comments

@adomasven
Copy link

adomasven commented Aug 12, 2022

Hi. At Zotero we had two branches of Zotero Connector extension code, separate for Safari and other browsers, that we have now rebased back onto a single branch. Safari didn't use to support SingleFile before, but now it does. We run SingleFile completely standalone on the page and only provide a shimmed fetch function that ignores CORS. In Chrome SingleFile loads and saves deferred images on https://enviragallery.com/demo/lazy-loading-demo/, but it does not In Safari. I've tried increasing the deferred load timeout and enabling loadDeferredImagesDispatchScrollEvent, but Safari still doesn't load deferred images.

@gildas-lormeau
Copy link
Owner

gildas-lormeau commented Aug 12, 2022

Hi, that's strange. Unfortunately, I don't have a Mac so I cannot test SingleFile in Safari. Did you try to set loadDeferredImagesKeepZoomLevel to true? By doing so, you are supposed to see the page zoomed out in order to show it completely at the beginning of the process. Do you confirm that you see this behavior? Does it change anything?

@adomasven
Copy link
Author

No, that doesn't seem to do anything in Safari from my observation and it doesn't fix deferred image saving. Could you point me to the codepoints that govern this and the description on how it's supposed to work? I'll try to troubleshoot it more thoroughly myself then and submit a PR.

@gildas-lormeau
Copy link
Owner

gildas-lormeau commented Aug 12, 2022

Thank you for the feedback. I may be wrong but I have the impression that the problem is that SingleFile would not be able to override the clientHeight property of documentElement. This is done here: https://github.com/gildas-lormeau/single-file-core/blob/f919a47104948cf32ca199059ac17391f908faf0/processors/hooks/content/content-hooks-frames-web.js#L85. I think this can be verified by logging scrollingElement.clientHeight just after this code is executed.

@adomasven
Copy link
Author

We only inject single-file-bootstrap.js and single-file.js into Zotero Connector, so that code does not run in either plugin.

@gildas-lormeau
Copy link
Owner

gildas-lormeau commented Aug 12, 2022

Once built, this code is present in single-file-hooks-frames.js. So, you have to inject this script (in the "main" world, i.e. the page itself) to make the feature work.

@adomasven
Copy link
Author

Sure, but deferred images load without it in Chrome, how come? If we don't want to load frames, is it ok to inject the script in the top frame?

@gildas-lormeau
Copy link
Owner

gildas-lormeau commented Aug 12, 2022

Are you sure you're not injecting this script in Chrome? The feature can't work if this script is not injected. Note that MV3 prevents you to inject scripts dynamically in the page (this is what SingleFile tries to do with MV2, see https://github.com/gildas-lormeau/SingleFile/blob/142eb26f961dd0a346834b58cb39931c2e42e216/src/lib/single-file/core/content/content-hooks-frames-extension-injection.js). You have to declare it in the manifest file, see https://github.com/gildas-lormeau/SingleFile-Lite/blob/main/manifest.json#L27.

@adomasven
Copy link
Author

adomasven commented Aug 12, 2022

I am sure. Testing with MV2 extension for Chrome with this code: https://github.com/zotero/zotero-connectors/blob/safari-app-extension-maintenance/src/browserExt/background.js#L335-L340

Idk where this code comes from in single-file.js, but could it be responsible for it working in Chrome:

if (element.tagName == "IMG") {
			const imageData = {
				currentSrc: elementHidden ?
					EMPTY_RESOURCE$1 :
					(options.loadDeferredImages && element.getAttribute(LAZY_SRC_ATTRIBUTE_NAME)) || element.currentSrc
			};

@adomasven
Copy link
Author

adomasven commented Aug 12, 2022

Either way, I injected that script into Safari and got:

[Log] Pre override
[Log] 375
[Log] Post override
[Log] 4049

for the override line, so the override works fine. Deferred images are still not saved though. Although the fact that it works in Chrome without this code raises more questions than it answers.

@gildas-lormeau
Copy link
Owner

gildas-lormeau commented Aug 12, 2022

The code in your previous comment is not directly related to deferred loading. It's a piece of code that tries to find the currentSrc of images (and which works in conjunction with the script that loads deferred images).

@gildas-lormeau
Copy link
Owner

Regarding the logs, they look fine. Do you confirm that images were not loaded when you injected the script?

@adomasven
Copy link
Author

Ok, I had the SingleFile extension enabled in Chrome, and if I disable it deferred image loading no longer works there either. Thanks for helping me through this confusion. So what do I need to inject into the main page beyond single-file.js and single-file-bootstrap.js to get deferred image saving to work? We don't need frame saving. Also you should probably update the Wiki with this information.

@adomasven
Copy link
Author

Regarding the logs, they look fine. Do you confirm that images were not loaded when you injected the script?

Yes, the images are not loaded.

@gildas-lormeau
Copy link
Owner

gildas-lormeau commented Aug 12, 2022

When you set loadDeferredImagesKeepZoomLevel to true and inject the script, do you see the page that is zoomed out (briefly)?

@adomasven
Copy link
Author

Yep, that works and actually saves all deferred images, but we cannot use it with this UX in Safari.

@gildas-lormeau
Copy link
Owner

gildas-lormeau commented Aug 12, 2022

I understand, it's still a step forward though and it allows me to understand the issue. I think the simplest way for you to debug this problem is to put a debugger; at the beginning of loadDeferredImagesStart (in content-hooks-frames-web.js) and compare the execution of the code in Safari and Chrome (with loadDeferredImagesKeepZoomLevel set to false).

@adomasven
Copy link
Author

I'm not sure if I will be able to troubleshoot this successfully today, and I am going away for a week, so if there's no more comments on my part -- sorry! Thanks for the help so far. Could you confirm whether we need to inject single-file-frames.js for any other functionality aside from frame saving, like deferred image loading, or is single-file-hooks-frames.js enough?

@adomasven
Copy link
Author

Ok, so it actually doesn't work in chrome even if I load single-file-hooks-frames.js. But it does work with the SingleFile extension installed and enabled in Chrome. Ideas?

@gildas-lormeau
Copy link
Owner

gildas-lormeau commented Aug 12, 2022

I did some debugging in Chrome. You have to know that loading deferred images is painful, so SingleFile combines multiple techniques to do so. It looks like the code below is the one that makes the feature work on this page. This code overrides the Image constructor in order to detect when the src property is updated. If I remove this code and save the page with SingleFile, the images are not saved.
https://github.com/gildas-lormeau/single-file-core/blob/f919a47104948cf32ca199059ac17391f908faf0/processors/hooks/content/content-hooks-frames-web.js#L108-L139

You need to inject single-file-frames.js as a content script in the page itself and all the frames in order to get all their contents. If you don't save frames, you shouldn't need to inject it.

You need to inject single-file-hooks-frames.js as a regular script (i.e. "main" in MV3) in the page itself (and all the frames ideally) in order to save deferred images and fonts loaded with the FontFace API (this can be tested with www.theverge.com). The role of this script is to override (or "hook") native APIs when necessary.

I guess it works with SingleFile installed on your end because SingleFile injects the single-file-hooks-frames.js script for you and Zotero can interact with it. It probably means that you're unable to inject single-file-hooks-frames.js in Zotero, for some reasons. I recommend you to disable SingleFile when you're testing your extension to avoid unwanted interactions.

@adomasven
Copy link
Author

Ok, I'm getting back to this. I'm now testing on Chrome with SingleFile extension disabled, since this turned out to not be just the Safari issue. I've injected both single-file-frames.js and single-file-hooks-frames.js via manifest.json with this config:

{
	"matches": ["http://*/*", "https://*/*"],
	"run_at": "document_start",
	"all_frames": true,
	"js": [
		"lib/SingleFile/single-file-hooks-frames.js",
		"lib/SingleFile/single-file-frames.js"
	]
}

I see the scripts loaded via developer tools for the page, and I see loadDeferredImagesStart being executed. However, the monkey-patched globalThis.Image constructor is never called. Saving works with loadDeferredImagesKeepZoomLevel = true, but not without it.

If I step through loadDeferredImagesStart in the debugger, I see the document get zoomed out, but nothing actually loads during the debugger steps, and then it zooms back in.

Unfortunately, I cannot compare this to what happens when the SingleFile chrome extension is enabled, because that code is minified. Any other debugging steps that I may perform here?

@gildas-lormeau
Copy link
Owner

gildas-lormeau commented Aug 23, 2022

Following the discussion #1027, are you able to build a non-minified version of the code?

@adomasven
Copy link
Author

Ok, I figured it out. The issue is NOT doing what you've been telling me to do, hah. I wasn't injecting single-file-hooks-frames.js as a regular script. Additional stress in the docs about this file would be beneficial, explicitly stating that injecting this code in a privileged scope will make deferred image loading not work. Like in Safari we have a privileged extension JS scope/world and then the regular execution scope, similar to web extensions. In MV3 you can indeed specify the execution world in manifest.json, not sure about MV2. So in MV2, you need code like

_injectSingleFileHooks: function() {
  const scriptElement = document.createElement("script");
  scriptElement.src = getExtensionURL("lib/SingleFile/single-file-hooks-frames.js");
  scriptElement.async = false;
  document.body.appendChild(scriptElement);
  scriptElement.remove();
}

to correctly manually inject the file. Injecting via web extension APIs is wrong.

Thanks a ton for help debugging this!

@gildas-lormeau
Copy link
Owner

Thank you for the feedback, I'm glad to hear you were able to find the cause of the issue :)

adomasven added a commit to zotero/zotero-connectors that referenced this issue Aug 24, 2022
Previously deferred image saving didn't work unless the SingleFile
extension was also installed in the browser.
See gildas-lormeau/SingleFile#1022
for detailed discussion
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

2 participants