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

More performant YouTube Logo hide option. #135

Closed
wants to merge 2 commits into from

Conversation

tomikaka22
Copy link

This pull request adds an option to hide the YouTube logo from the top right corner of the app.

Me and the creator of youtube-webos-cobalt modified the code of #56 a while ago to work without the deprecated DOMNodeInserted event, thus making it more performant and up to date.

@fire332
Copy link

fire332 commented Mar 26, 2024

Is it possible to just inject a stylesheet that hides the CSS class instead of re-applying the style change every X seconds? Does YouTube allow 'unsafe-inline' for style tags in its CSP header?

@tomikaka22
Copy link
Author

I don't think I'm applying anything X seconds unless I'm missing something.

@fire332
Copy link

fire332 commented Mar 26, 2024

Apologies. I've misinterpreted the code in my haste. Regardless, my question still stands out of curiosity at the very least.

@tomikaka22
Copy link
Author

It'd probably be possible, but I think making the logo container opaque directly in the JavaScript is the simplest.

Unless there is already a mechanism that loads stylesheets based on config options.

@throwaway96
Copy link
Member

throwaway96 commented Mar 27, 2024

Is it really necessary to call logoHideShow() three times?

I'm assuming this is from the first call:

VM22:4250 Uncaught TypeError: Cannot read property 'style' of null
    at logoHideShow (<anonymous>:4250:64)
    at Module../src/ui.js (<anonymous>:4286:1)
    ...

@fire332
Copy link

fire332 commented Mar 27, 2024

We already achieve the same goal of waiting for an element to be added before doing something to it here.

@throwaway96
Copy link
Member

We already achieve the same goal of waiting for an element to be added before doing something to it here.

That makes a lot more sense to me. Why does it look at attributes though?

@throwaway96
Copy link
Member

throwaway96 commented Mar 27, 2024

This works for me.

async function logoHideShow() {
  const logo = await waitForChildAdd(
    document.getElementById('container'),
    (node) => node.nodeName === 'YTLR-REDUX-CONNECT-YTLR-LOGO-ENTITY'
  );

  logo.style.opacity = configRead('hideLogo') ? '0' : '1';
}
logoHideShow();

document.getElementById('container') could probably just as well be document.body, since it seems like nearly every element added is in #container.

I also disabled the attribute stuff in waitForChildAdd(), and it still finds the video and logo elements. It doesn't appear to be necessary for current uses of waitForChildAdd(). Is there a rationale for leaving it in?

@fire332
Copy link

fire332 commented Mar 27, 2024

If the Mutation Observer doesn't fire on attribute change, one must carefully craft the predicate to only rely on properties that exist when the node is added.

It's not worth trading the principle of least surprise for a handful of milliseconds faster startup.

@throwaway96
Copy link
Member

The name and description of waitForChildAdd() imply to me that the checks will be made when a child is added, not later on when its attributes are modified.

In the single existing use of waitForChildAdd() and this new one, the tests (which are both fairly intuitive) work fine without attributes, so it doesn't seem like predicates have to be that carefully crafted.

I might feel differently if anyone were actually using the attribute functionality. If it's not removed, it should at least be made optional. I'm not sure whether it should be enabled by default though.

@fire332 fire332 mentioned this pull request Mar 27, 2024
@throwaway96
Copy link
Member

Does YouTube allow 'unsafe-inline' for style tags in its CSP header?

I'm not actually seeing a Content-Security-Policy header (or <meta http-equiv>), but it looks like all <style> elements have a nonce attribute. Still, I just tested injecting <style> without nonce (on webOS 6.3.2), and it worked.

The version I'm currently using is based on waitForChildAdd()—which I only learned of yesterday. If someone wants to write a version that injects a stylesheet, please let me know (otherwise I may look into doing it myself). I think we should probably be avoiding setTimeout()/setInterval() unless they're truly necessary.

@fire332
Copy link

fire332 commented Mar 28, 2024

In a perfect world, we would do this:

body:has(#__hide_logo:checked) .ytlr-redux-connect-ytlr-logo-entity {
  visibility: hidden !important;
}

Which would mean we wouldn't need JS at all for this functionality.

But too bad we're not in a perfect world because :has is only supported in Chrome 105+ and the polyfill looks expensive.

Method 1: toggle entire stylesheet

Create a new CSS file:
./hide-ytlogo.css

.ytlr-redux-connect-ytlr-logo-entity {
  visibility: hidden !important;
}

And use it like so:

const ele = document.createElement('link')
ele.rel = 'stylesheet'
// URL asset: https://webpack.js.org/guides/asset-modules/#url-assets
ele.href = new URL('./hide-ytlogo.css', import.meta.url)
document.head.appendChild(ele)

// `disabled` is available on `link[rel="stylesheet"]`
// See: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/link#disabled
ele.disabled = !configRead('hideLogo')

Method 2: toggle class on body

./hide-ytlogo.css

body.logo-hidden .ytlr-redux-connect-ytlr-logo-entity {
  visibility: hidden !important;
}
import './hide-ytlogo.css'
if (configRead('hideLogo'))
  document.body.classList.add('logo-hidden')
else
  document.body.classList.remove('logo-hidden')

@throwaway96
Copy link
Member

I don't think a separate CSS file is necessary. This seems to work in my testing:

/**
 * Initialize ability to hide YouTube logo in top right corner.
 */
function initHideLogo() {
  const style = document.createElement('style');
  document.head.appendChild(style);

  /** @type {(hide: boolean) => void} */
  const setHide = (hide) => {
    const opacity = hide ? '0' : '1';
    style.textContent = `ytlr-redux-connect-ytlr-logo-entity { opacity: ${opacity}; }`;
  };

  setHide(configRead('hideLogo'));

  configAddChangeListener('hideLogo', (evt) => {
    setHide(evt.detail.newValue);
  });
}

@fire332
Copy link

fire332 commented Mar 28, 2024

That also works too. Though visibility should be used instead of opacity so the element isn't interactable.

@throwaway96 throwaway96 added the enhancement New feature or request label Mar 29, 2024
@throwaway96
Copy link
Member

Any objections to closing this in favor of #149?

@tomikaka22
Copy link
Author

Looks good to me, happy to see this feature finally making it after all this time.

@tomikaka22 tomikaka22 closed this Mar 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants