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

computeStyleTests() throws in Firefox because window.getComputedStyle(div) returns null #3514

Closed
tedald opened this Issue Jan 25, 2017 · 19 comments

Comments

Projects
None yet
7 participants
@tedald
Copy link

tedald commented Jan 25, 2017

Description

jquery 3.1.1: Seeing occasional errors on our production site due to Firefox returning null for window.getComputedStyle(div) inside jquery support function computeStyleTests(). Related Firefox bug: https://bugzilla.mozilla.org/show_bug.cgi?id=548397

workaround in computeStyleTests: var divStyle = window.getComputedStyle( div ) || {};

Link to test case

No test case yet. Perhaps as the Firefox bug states, iframes are involved.

@gibson042

This comment has been minimized.

Copy link
Member

gibson042 commented Jan 25, 2017

We can prevent the exception, but our CSS methods won't actually work without gCS. Further, computeStyleTests is called on-demand, and just loading jQuery doesn't constitute "demand". So what is happening that triggers the computation in a hidden iframe, and how much of a "fix" would a change be that prevents the exception but yields incorrect behavior?

@gibson042 gibson042 added the CSS label Jan 25, 2017

@tedald

This comment has been minimized.

Copy link

tedald commented Jan 25, 2017

what is happening that triggers the computation in a hidden iframe

We are getting the errors via an error reporting service. It doesn't appear that the problem pages are actually in an iframe, but I don't yet have enough info.

how much of a "fix" would a change be that prevents the exception but yields incorrect behavior?

An environment that returns null for getComputedStyle may indeed have other issues. But throwing an exception stops javascript block execution. I think fixing and limiting the issue to something more specific like "jquery css methods fail", while perhaps not always better, is generally preferable.

@dmethvin

This comment has been minimized.

Copy link
Member

dmethvin commented Jan 25, 2017

We are getting the errors via an error reporting service. It doesn't appear that the problem pages are actually in an iframe, but I don't yet have enough info.

In that case this is definitely not something we should be sweeping under the rug just to keep the error log clean. If you determine that this particular error is harmless for your case, you can always filter it out of your error reporting.

@markelog

This comment has been minimized.

Copy link
Member

markelog commented Jan 29, 2017

Sounds like Gecko people not gonna fix that, moreover CSS specification might change too – w3c/csswg-drafts#571

@timmywil

This comment has been minimized.

Copy link
Member

timmywil commented Feb 6, 2017

FF's insistence not to fix is an issue: hakimel/reveal.js#1546

@timmywil timmywil removed the Needs review label Feb 6, 2017

@timmywil

This comment has been minimized.

Copy link
Member

timmywil commented Feb 6, 2017

We'd like FF to address this. Dave to comment on the FF ticket.

@bzbarsky

This comment has been minimized.

Copy link

bzbarsky commented Feb 6, 2017

Sounds like Gecko people not gonna fix that

Fix what? The "getComputedStyle throws exceptions or returns null in display:none iframe" thing we are aiming to fix. https://bugzilla.mozilla.org/show_bug.cgi?id=548397 tracks that, as noted, and I have been working on it (albeit with limited success so far). Unfortunately, it's a big architectural change and I've run into a huge number of test failures with my attempted fixes, so I can't give a timeframe for it being fixed at the moment. :(

The csswg-drafts discussion linked above would not affect getComputedStyle except for the small set of properties listed as returning used styles at https://drafts.csswg.org/cssom/#resolved-value (for which the used value would become equal to the computed value if box construction in the subframe were suppressed). That said, computeStyleTests does in fact check some of those properties, so may be affected.

@markelog

This comment has been minimized.

Copy link
Member

markelog commented Feb 7, 2017

Some bits are confusing here, like

would not affect getComputedStyle

vs

so may be affected.

So sounds like we will be affected :)

Fix what? The "getComputedStyle throws exceptions or returns null in display:none iframe" thing we are aiming to fix.

Iframe "situation" in FF was always interesting from my experience, remembering stuff like that – https://bugzilla.mozilla.org/show_bug.cgi?id=797029, so when you say

Unfortunately it's failures in all sorts of different parts of the code, not just a case of the same failure happening in lots of test suites.

I presumed situation is the similar (with some degree of doubt though, hence the "Sounds like" from original comment), especially considering that FF ticket was filled seven years ago.

I think I understand what you saying though, right now it seems we need clarify what to do or don't do on our side, should we wait for FF resolution and clarify exactly how css spec changes could affect us.

Is that about right?

@bzbarsky

This comment has been minimized.

Copy link

bzbarsky commented Feb 7, 2017

So sounds like we will be affected :)

Yes, if you actually rely on layout in display:none iframes. But that's really unrelated to the Gecko bug being cited, which breaks attempts to get style inside display:none iframes.

@markelog

This comment has been minimized.

Copy link
Member

markelog commented Feb 7, 2017

if you actually rely on layout in display:none iframes

That would happen if jQuery itself loaded in such iframe, right?

@bzbarsky

This comment has been minimized.

Copy link

bzbarsky commented Feb 7, 2017

Yes, indeed.

@timmywil

This comment has been minimized.

Copy link
Member

timmywil commented Feb 13, 2017

I think we're going to count on a fix from FF here. Our support tests are pointless otherwise. Thank you @bzbarsky. Keep at it!

@timmywil timmywil closed this Feb 13, 2017

@bzbarsky

This comment has been minimized.

Copy link

bzbarsky commented Feb 13, 2017

You could assume that exception means "supported". Better than just completely failing....

@tedald

This comment has been minimized.

Copy link

tedald commented Feb 13, 2017

Yes, or re-write the test to defer until successful. Completely failing breaks the script block.

@timmywil

This comment has been minimized.

Copy link
Member

timmywil commented Feb 13, 2017

I'm not a fan of assuming support when the value doesn't even exist. However, I'm fine with deferring.

@timmywil

This comment has been minimized.

Copy link
Member

timmywil commented Dec 17, 2018

I started creating a test for this and it seems it's been fixed in FF?

@mgol

This comment has been minimized.

Copy link
Member

mgol commented Jan 7, 2019

@timmywil Yup, it seems that's the case since Firefox 62, see https://www.fxsitecompat.com/en-CA/docs/2018/getcomputedstyle-no-longer-returns-null-when-style-can-t-be-retrieved/.

We still support Firefox ESR (currently 60) but since this issue has been open for such a long time, perhaps ESR users can live with it for the time being.

Should we close the issue?

@timmywil

This comment has been minimized.

Copy link
Member

timmywil commented Jan 7, 2019

Now that someone else has confirmed, I say yes.

@timmywil timmywil closed this Jan 7, 2019

@mgol mgol removed this from the 3.4.0 milestone Jan 7, 2019

@mgol mgol added the Works For Me label Jan 7, 2019

@mgol

This comment has been minimized.

Copy link
Member

mgol commented Jan 7, 2019

@timmywil I removed the milestone & added the "Works For Me" label so that it doesn't get pulled into the changelog.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment