Selma: Content script sometimes gets injected after DOMContentLoaded event on OperaGX

[A-K-O-R-A]

Pull Request

Description

This resolves an issue where the Selma improvement script won't activate when it gets injected after the DOMContentLoaded event. It also resolves a small layout issue with the credit banner.
See: https://stackoverflow.com/a/39993724

Type of change

• Bug fix (non-breaking change which fixes a bug)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that might cause existing functionality to not work as expected)

Further info

• This change requires a documentation update
• I updated the documentation accordingly, if required
• I commented my code where its useful

Testing

We have 1500+ Users. Please test your changes thoroughly.

• Tested my changes on Firefox
• Tested my changes on Chromium-Based-Browsers
• Successfully run npm run test locally

[C0ntroller]

Seems like a fix, but there is some strange weirdness in this file.

First of all: why is the Css injected via js? This could also be declared in the manifest.json, couldn't it?

Also, the reason this happens is, that we try to inject before the page is loaded and sometime it already is once the code is run. Correct?
But why even inject the script before the loading is complete? Because we could also just set the content script to load at "document_idle" (also in the manifest). Is there a reason the script should even be run before?

[A-K-O-R-A]

First of all: why is the Css injected via js? This could also be declared in the manifest.json, couldn't it?

Because the goal was to give the user the chance to disable the stylesheet, as it only works with the changes the JS script makes to the page

Also, the reason this happens is, that we try to inject before the page is loaded and sometime it already is once the code is run. Correct?
But why even inject the script before the loading is complete? Because we could also just set the content script to load at "document_idle" (also in the manifest). Is there a reason the script should even be run before?

The initial goal was to prevent a flickering effect when the style sheet gets applied after the page is loaded, but with the current solution it does get applied after the page is loaded so I am not sure what I wrote there 😅

[C0ntroller]

Because the goal was to give the user the chance to disable the stylesheet, as it only works with the changes the JS script makes to the page

This is probably nitpicking, but wouldn't be the better way to do this to inject the Css, prefix all styles in the Css with a specific class and or remove the class to the body? I just think injecting files is way less safe and more prone to being blocked in the future 😅
But well, I guess is works, so I won't give it any priority :)

The initial goal was to prevent a flickering effect when the style sheet gets applied after the page is loaded

Why isn't the reading of the setting and the css injection (based on the result) done while waiting for the event? Because these operations should be working even without the fully loaded Dom, shouldn't it?

[A-K-O-R-A]

This is probably nitpicking, but wouldn't be the better way to do this to inject the Css, prefix all styles in the Css with a specific class and or remove the class to the body? I just think injecting files is way less safe and more prone to being blocked in the future 😅 But well, I guess is works, so I won't give it any priority :)

Yes changing a flag class on the root would be a better solution, I did not think of that back then but if I find the time for it I will change it.

Why isn't the reading of the setting and the css injection (based on the result) done while waiting for the event? Because these operations should be working even without the fully loaded Dom, shouldn't it?

Yes they could be done in parallel, I think in reality this does not really change anything. Weirdly this entire issue is not present on firefox, but can also be observed on safari

[A-K-O-R-A]

Ok so I just tested it with document_idle, using that does make the flicker noticeably worse. So I would be in favor of keping the (admittedly finicky) current solution for now. I did move the promise for setting to the top of the file and it is now executed in parallel to the rest

[C0ntroller]

It still looks like injectCSS() get only called after DOMContentLoaded. We should probably think about this as two separate actions: applying styles and applying DOM changes.

So probably you could do something like this:

//... 
// Inject CSS
storage.get(['improveSelma'], result => {
    if(!result.improveSelma) return;
    injectCSS('base');
    //... Inject the other styles based on path
});

// Running this outside the async makes it way more predicable
if (document.state !== 'loading') main();
else appendListener('DOMContentLoaded', main);

async main() {
    const enabled = await new Promise(resolve => storage.get(['improveSelma', r => resolve(r.improveSelma));
    if (!enabled) return;
    addCreditsBanner();
    applyChanges();
    //... 
} 

//... All the other definitions

This would fix two things:

• the event listener should be way more predictable, maybe it will even work cross browser
• Css get's injected at earliest moment possible, this should reduce flicker to an absolute minimum for this method.
  • Using the class approach could even more reduce this, as the files do not need to be loaded anymore

[A-K-O-R-A]

Injecting the CSS before the DOMContentLoaded unfortunately is not possible, because that way the CSS styles will get overridden by the CSS styles from selma :/

[C0ntroller]

Injecting the CSS before the DOMContentLoaded unfortunately is not possible, because that way the CSS styles will get overridden by the CSS styles from selma :/

Even when using !important for the styles in question?

[A-K-O-R-A]

Even when using !important for the styles in question?

Yes, for some reason the alternating colors in the table get messed up, it's all a bit strange. Maybe I did something wrong though, I am not really a web dev expert to be honest