Clean up document.documentElement.className (html class attribute) presence after injection #706

myfonj · 2019-04-29T01:17:30Z

I've observed that pages affected by Stylish have always <html class=""> (or class="[author classes]"). I assume it is result of

stylus/content/apply.js

Lines 351 to 364 in b40849a

    
               document.documentElement.classList.add(className); 
        
               const el = styleInjector.createStyle('transition-patch'); 
        
               // FIXME: this will trigger docRootObserver and cause a resort. We should 
        
               // move this function into style-injector. 
        
               document.documentElement.appendChild(el); 
        
               setStyleContent(el, ` 
        
                 ${docId}.${CSS.escape(className)}:root * { 
        
                   transition: none !important; 
        
                 } 
        
               `) 
        
                 .then(() => { 
        
                   setTimeout(() => { 
        
                     el.remove(); 
        
                     document.documentElement.classList.remove(className);

(I have some global userstyle that involves transition so this code most probably runs on every page I visit.)
There class is added and then removed from "HTML" element, what produces empty class="" in most cases, because of, as I have learnt consequently,

var d = document.createElement('div');
d.attributes.length === 0; // true
d.classList.add('foo');d.classList.remove('foo');
d.attributes.length === 1; // true

This is nothing grave and should not cause much trouble in sane pages, but I've ran into certain page that is really broken as a result of this [1]. This also seems to be trivially fixable by storing .getAttribute('class') and then removeAttribute('class') in case there wasn't any initially. (Will try and test and send PR if time permits.)

[1] https://goout.net/ - to some degree they deserve this, because they are in fact parsing HTML with regex (by doing something like '<html class="" lang="xx">'.match(/<html lang="(.*?)">/)[1]) and we all know what will happe͎̼n as a c̯͓̣o̞n̗̪s͍̠̣e̼qụ̭͍͇e̶͈̩͕̥̥̳̠n͖̯͇̰̝̩̲c̛̝e͏̮̣͚̫͔͉̙.͕̤̱̩.̹.͔

The text was updated successfully, but these errors were encountered:

Mottie · 2019-04-29T12:40:17Z

Hi @myfonj!

Yeah that site is crazy to be parsing HTML from inside of <br> tag content attributes, but I don't see how the class name of the <html> tag is causing any issues; especially an empty class name.

What browser are you using? What style is being used? Can you describe the steps to reproduce the issue you're describing.

myfonj · 2019-04-29T16:23:41Z

Hi @Mottie , nothing special, I'm using various Firefox versions (Developer and Nightly) and I have one global userstyle that involves transition (it is slightly updated ads-dimming utility but it is not important I suppose). I guess it would behave same way in Chrome. Problem is that aforementioned routine leaves empty class on html what gets in way of that "parsing" (cough cough) of the html@lang attribute what borkes their script:

(something what most probably simple document.documentElement.lang could do more efficiently and reliably in this context. Though I think same function is used for non-DOM string elsewhere, so this approach would not be universal in their case). So if I visit with that style enabled, content does not render. If I disable that style (or Stylus), page works.

narcolepticinsomniac · 2019-04-29T18:34:47Z

I couldn't repro in Chromium at all. I could consistently in FF, and even after the last commit in myf's PR, it still happens randomly if I paste & go half a dozen times in a row. I think, like 8 said, it'll always be a race. That blank space hack tophf suggested doesn't seem to work either AFAICT. I was injecting at the bottom of the html, and it seemed to have no effect in FF, or the Chromium popup, which is how I can consistently repro the issue the patch addresses. I suppose it's possible I wasn't doing it right though.

I was playing around with alternatives, and switching the function to something like this seems to work fine:

  function applyTransitionPatch() {
    // CSS transition bug workaround: since we insert styles asynchronously,
    // the browsers, especially Firefox, may apply all transitions on page load
    const patch = document.createElement('style');
    patch.textContent = 'html:not(#stylus-dummy-selector) :not(#stylus-dummy-selector) { transition: none !important; }';
    patch.id = 'stylus-transition-patch';
    patch.type = 'text/css';
    document.documentElement.appendChild(patch);
    document.onreadystatechange = () => {
      if (document.readyState === 'complete') {
        setTimeout(() => {
          document.getElementById('stylus-transition-patch').remove();
        });
      }
    }
  }

myfonj · 2019-05-05T00:35:13Z

Thanks for testing and fix proposal, anagrammar. I confess that race condition haven't even crossed my imagination when I identified initial problem. Solution with fewer DOM interventions is certainly better. (BTW, is giving that temporal style element ID necessary? I'd assume it could be addressed by the original patch reference.)

This also makes me wonder whether such fixups and injections are still necessary; isn't there more "native" way of introducing sheet into cascade yet? Something that just causes that such style sits in the cascade even before affected page even starts loading, isolated from page scripts, just like real userstyles were specified? Isn't #248 to the rescue?

narcolepticinsomniac · 2019-05-05T00:55:25Z

is giving that temporal style element ID necessary?

Nah, my js needs supervision. =) Should simply be:

  function applyTransitionPatch() {
    // CSS transition bug workaround: since we insert styles asynchronously,
    // the browsers, especially Firefox, may apply all transitions on page load
    const patch = document.createElement('style');
    patch.type = 'text/css';
    patch.textContent = 'html:not(#stylus-dummy-selector) :not(#stylus-dummy-selector) { transition: none !important; }';
    document.documentElement.appendChild(patch);
    document.onreadystatechange = () => {
      if (document.readyState === 'complete') {
        setTimeout(() => {
          patch.remove();
        });
      }
    }
  }

I noticed at least one additional positive side-effect since switching to this code. A site which occasionally had a FOUC, which I was always too lazy to investigate, seems not to have any issues anymore.

Isn't (tabs.insertCSS) to the rescue?

AFAIK, the issues tophf mentioned here are still problematic.

8's fix - closes #706, closes #707

myfonj mentioned this issue Apr 29, 2019

Remove entire html@class if there wasn't any. #707

Closed

narcolepticinsomniac added a commit that referenced this issue May 26, 2019

Avoid potential transition patch conflicts

e3e933d

8's fix - closes #706, closes #707

narcolepticinsomniac mentioned this issue May 26, 2019

Avoid potential transition patch conflicts #720

Merged

narcolepticinsomniac closed this as completed in #720 May 26, 2019

narcolepticinsomniac added a commit that referenced this issue May 26, 2019

Avoid potential transition patch conflicts (#720)

eb6888e

8's fix - closes #706, closes #707

eight04 mentioned this issue May 3, 2024

[Feature Request] PWA Titlebar Header Color #1772

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up document.documentElement.className (html class attribute) presence after injection #706

Clean up document.documentElement.className (html class attribute) presence after injection #706

myfonj commented Apr 29, 2019 •

edited

Loading

Mottie commented Apr 29, 2019

myfonj commented Apr 29, 2019

narcolepticinsomniac commented Apr 29, 2019

myfonj commented May 5, 2019

narcolepticinsomniac commented May 5, 2019 •

edited

Loading

Clean up document.documentElement.className (html class attribute) presence after injection #706

Clean up document.documentElement.className (html class attribute) presence after injection #706

Comments

myfonj commented Apr 29, 2019 • edited Loading

Mottie commented Apr 29, 2019

myfonj commented Apr 29, 2019

narcolepticinsomniac commented Apr 29, 2019

myfonj commented May 5, 2019

narcolepticinsomniac commented May 5, 2019 • edited Loading

myfonj commented Apr 29, 2019 •

edited

Loading

narcolepticinsomniac commented May 5, 2019 •

edited

Loading