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

[Firefox] Weird transition behavior in current master #177

Closed
narcolepticinsomniac opened this issue Aug 29, 2017 · 32 comments
Closed

[Firefox] Weird transition behavior in current master #177

narcolepticinsomniac opened this issue Aug 29, 2017 · 32 comments

Comments

@narcolepticinsomniac
Copy link
Member

narcolepticinsomniac commented Aug 29, 2017

Noticed it with my editor style: https://pastebin.com/GuDs0d3A

Not sure if it's specific to editor styles.

Transitions are applied on page load. Doesn't occur in stable AMO version. Tested the same in WF54, W55, and FF Nightly.

@tophf Btw, I switched up the buttons to a slightly more classic look. You like em any better?

@tophf
Copy link
Member

tophf commented Aug 29, 2017

The only relevant difference I can think of is that now the options are initialized with the actual values during page load i.e. before the page is even shown. I don't know how and why it influences animations so your guess is as good as mine.

@narcolepticinsomniac
Copy link
Member Author

My guess is not typically anywhere near as good as yours. Whenever that's the case, we're in trouble. =)

@tophf
Copy link
Member

tophf commented Aug 29, 2017

I mean I don't know CSS any better than you, quite likely worse.

@narcolepticinsomniac
Copy link
Member Author

I don't know CSS any better than you

Neither of our CSS skills matter much here. There's nothing wrong with the CSS, and CSS cannot fix this behavior.

This bug only occurs in FF, only in our internal pages, and only began recently in our master. The way our moz-extension:// pages are loading is bugged. Whether that has to do with recent changes in how options are initialized, or some other recent change to storage, IDK.

Browsers used to be far more erratic, and occasionally apply transitions on page loads in a similar way, but they've been pretty solid for the last couple years, at least. Probably still not perfect with elaborate transitions on drastic layout shifts, but simple colors shouldn't have issues like this.

This bug occurs in all versions, including Nightly, so this isn't something that will resolve itself any time soon. Leaving it like this would be pretty terrible, so hopefully you can pinpoint the cause. It's something altered recently. If it's something essential that can't be fixed, perhaps we could implement a workaround like what was required years ago. Obviously, eliminating the bug is always preferable to a workaround, but the old method is this. It'd certainly be better than leaving it as it is.

@tophf tophf closed this as completed in 72e8213 Sep 2, 2017
@tophf
Copy link
Member

tophf commented Sep 2, 2017

I did a git bisect and the culprit is 9a55e64 which autosizes filter select elements. It uses offsetWidth which forces the browser to calculate the entire page layout while it's still loading. Chrome differentiates between layouting and actually rendering/presenting so it doesn't flicker. Firefox erroneously starts the presentation phase and applies transitions to irrelevant elements so this definitely looks like a bug in Firefox, but I won't report it as I don't care about FF. I've used the workaround you found, thanks, it was a right move for me to rely on your knowledge of CSS!

@narcolepticinsomniac
Copy link
Member Author

Nice. Not sure why :not(body) is necessary though.

Now that I've been looking into transition behavior in FF, we seem to have some similar issues that have nothing to do with anything recent, because I can repro in the AMO version.

I tested the same code on Google's home page with Stylus and the legacy Stylish in WF55 with our current AMO stable, and it works as expected in Stylish, but doesn't in Stylus.

Screen record:
transition bug.zip

Code:

input[type="submit"] {
    -moz-appearance: none!important;
    background: red!important;
    transition: 1s !important;
}

input[type="submit"]:hover {
    background: cyan!important;
}

Transitions being erroneously applied on page load isn't universal. The same code on a different site can work fine. I first noticed with my Google style in Nightly, so this isn't unique to WF or v55 either. Don't know what the difference is, but the same code works in the legacy Stylish regardless.

At the end of the screen record, I also demonstrated that export to moz-format didn't appear to be working either, which is odd.

@tophf
Copy link
Member

tophf commented Sep 3, 2017

Well, apparently something triggers the bug in Firefox. I'll investigate.

@tophf
Copy link
Member

tophf commented Sep 3, 2017

AFAIK the classic Stylish registers styles via some internal service provided to the full addons (deprecated in FF57), which is why it doesn't exhibit the bug.

I've added a global fix in FF that supersedes the local one in manage.html. Please test: indexeddb-fallback.zip (I've added the fix to that branch to make testing in FF easier).

Chrome sometimes applies the transition too, but I don't think there's anything I can do about it in Stylus as Chrome doesn't provide an API to remove my CSS patch that is injected as a user stylesheet thus overriding any other DOM-level author stylesheets. I can report the bug on crbug, I guess.

@tophf
Copy link
Member

tophf commented Sep 3, 2017

Hmm, actually this is not a browser bug. The behavior perfectly conforms to the standard: initially there was no background color and then we asynchronously add it along with a transition property. The issue is the asynchronicity, which we can circumvent only using the Tampermonkey approach, but it'll spam the console with deprecation warnings about synchronous XHR. I'm inclined to remove the global workaround I've added and only use the one for the manage page in FF because it's caused by a browser bug.

BTW, it's easy to avoid the issue by specifying transition only in :hover rule.

@tophf
Copy link
Member

tophf commented Sep 3, 2017

Another solution would be to use the current global patch in FF and a different patch in Chrome inside apply.js, but that requires inventing some super-high specificity * selector and I don't know how to do it reliably.

@narcolepticinsomniac
Copy link
Member Author

narcolepticinsomniac commented Sep 3, 2017

I think it's a huge improvement to the current behavior. I very rarely catch Chrome slipping up on applying transitions, but it does happen. Difference is, you can tell Chrome is trying and getting it right 99% of the time, whereas FF fails every page load on problematic sites. Still, I wouldn't think there's any real drawbacks to this workaround, so it's a shame it wouldn't work in Chrome as well.

Weird that FF has an API that Chrome doesn't, no? I get that removing it is good form, but it's not like the CSS that might get left behind will affect anything anyway.

BTW, it's easy to avoid the issue by specifying transition only in :hover rule.

Maybe I am better at CSS than you. Putting transitions on :hover is a rookie mistake. Transitions belong on the elements themselves, otherwise there is no "fade out".

@narcolepticinsomniac
Copy link
Member Author

The behavior perfectly conforms to the standard

IDK what you mean by that, but the standard behavior browsers aim to achieve is not applying transitions on page load. There are exceptions where explicitly specified, like animations and keyframes. but I know that's the goal for ordinary transitions, because they've gotten consistently better at it over the years. Even FF does for the most part, it's just far buggier still.

@tophf
Copy link
Member

tophf commented Sep 3, 2017

Well, okay then, we'll use the global patch in FF for now.
Let's worry about Chrome only if someone pesters us.

otherwise there is no "fade out".

Indeed, I forgot about that.

@tophf
Copy link
Member

tophf commented Sep 3, 2017

IDK what you mean by that

I mean we're not part of the page load process. We add the styles after the browser decided on appearance. For some reason FF decides earlier or it's slower in receiving the styles we add.

@tophf
Copy link
Member

tophf commented Sep 3, 2017

Maybe there's a time threshold in the browsers which makes them exclude our added styles from the initial calculation of appearance. And the threshold is different in Chrome and FF.

@narcolepticinsomniac
Copy link
Member Author

I think transitions fall into the category where once they have them worked out, they make some incremental improvements to speed, and it becomes a balancing act. FF is just buggier because it's buggier in general, I would guess. We could apply the same to Chrome, but the CSS couldn't be removed, correct? Is that really such a big deal?

@tophf
Copy link
Member

tophf commented Sep 3, 2017

Is that really such a big deal?

It would disable all transitions on all pages.
Personally, I don't mind.

@tophf
Copy link
Member

tophf commented Sep 3, 2017

Ah, I see what you mean. We should add a class/whatnot to html tag and remove it so the leftover CSS would not affect anything. Gonna try it.

@tophf
Copy link
Member

tophf commented Sep 3, 2017

Ugh, Chrome doesn't offer the "origin" parameter in insertCSS so we can't use it to add CSS with a higher-than-page-priority, which means we would have to add a <style> element with the fix before the page starts loading - on all pages! I just did it and the only concern I have is that rules with * affect the already slow CSSOM calculation. However, the patch is removed before any page content is displayed so I guess it's fine. BTW, this is what would be bad were we to leave it hanging around on the heavy complex pages.

The same zip link above is updated. Please test.

@narcolepticinsomniac
Copy link
Member Author

I actually have a good test for Chrome. If I set my New Tab Redirect extension page to Google, with my style which has transitions on those buttons, the transition efficiency drops from 99% to like 85-90%. Something about the redirect process, probably because they're inserting a blank html placeholder during the redirect. Initial testing tells me it's good, but I'll know for sure after I test it for a while.

I don't see any code leftover, but there's an empty [style] attribute on body. Is that problematic if a site has a style attribute in their html? If not, I'd say it's pretty clean, considering.

I gotta take off for a while, but I'll test some more when I get back.

@tophf
Copy link
Member

tophf commented Sep 3, 2017

We don't add style attribute on body.

@narcolepticinsomniac
Copy link
Member Author

narcolepticinsomniac commented Sep 3, 2017

Yeah, the style attribute is added by Lastpass. Never noticed, because I wasn't looking for it. Apparently they don't think it's problematic. That being the case, I'm not seeing any obvious leftovers from this little hack, and it appears to work very well, even on the redirect extension example I mentioned above. With the extension opening Google's home page in Chrome, I can pretty reliably repro transition loading issues about 10% of the time, but even with the weird redirect loading, the workaround initializes and prevents it from occurring.

This fix is pretty necessary for FF IMO, because the buggish behavior on some sites looks pretty terrible, but I'm also a big fan of applying it in Chrome. AFAICT, it very reliably prevents frequent page load transition bugs in FF, and the rare instances in Chrome.

Only issue I saw was a syntax error, which doesn't even look right:

syntax

@tophf
Copy link
Member

tophf commented Sep 3, 2017

syntax error

That's from an "old" version.
I've rewritten the fix several times today.

@narcolepticinsomniac
Copy link
Member Author

Went back and tested in WF, and it appears the original issue on our moz-extension:// is back. I re-downloaded from the link above, and the error is gone, so I'm assuming it's the updated version, but maybe not.

@tophf
Copy link
Member

tophf commented Sep 3, 2017

I've added an improved version of the initial hack for all Stylus pages. See if you can break it.

@narcolepticinsomniac
Copy link
Member Author

See if you can break it.

Challenge was accepted, but it fails at failing. I am excellent at breaking things, so I blame you. =)

We do, however, have a Stylelint issue in FF. Triggered by the ridiculous size of data-images in my Google style, if I had to guess. Not sure how relevant it is since I haven't heard much about the linting since you said the current implementation was "too dumb". Shame, I thought it was very close to good, but I guess certain scenarios couldn't be accounted for?

Code: https://pastebin.com/wHVyQEYN

Error:

stylelint

@tophf
Copy link
Member

tophf commented Sep 3, 2017

Yeah, I've noticed these warnings in a few rare cases while importing/linting complex styles. Apparently, FF runs some javascript libraries much slower, like 10 times slower, even in Nightly. The difference is noticeable only on a relatively fast multi-core computer, though, where the mature multi-process architecture of Chrome really shines. I remember using FF a lot ~5 years ago on an old, much slower, 2-core CPU and back then both browsers seemed more or less the same in terms of performance, sometimes Chrome was slower, sometimes FF was slower.

@narcolepticinsomniac
Copy link
Member Author

New error. Looks like an unnecessary negation operator, but I had to google it to even know what to call it, so what do I know?

null

@tophf
Copy link
Member

tophf commented Sep 4, 2017

Yes, and I've fixed it earlier without announcing.

@tophf
Copy link
Member

tophf commented Sep 4, 2017

I've merged all the fixes in master branch and deleted indexeddb-fallback.

@tophf
Copy link
Member

tophf commented Mar 6, 2019

There's a simpler fix which I didn't test: https://crbug.com/332189#c20

Add a <script> element containing a single space to the footer of the HTML page.

@tophf
Copy link
Member

tophf commented May 16, 2021

Chrome 92 has finally fixed the bug: transitions no longer occur on page load. I'll see if we can get rid of our suppressor hack in Chrome 92+.

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