Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.I'm currently looking at a website in production (with caches and all) but with no JS and CSS aggregation.
Once every 4 request or so javascript crashes at if (jQuery.support.positionFixed === undefined) because jQuery.support is actually undefined and you can't look for .positionFixed in undefined. The tests done in jQuery to build the support object are not trivial and can take time and since this piece of code is contained in a self-executing closure it can be executed before jQuery had time to build $.support and crashes all the js.
Since this is only required by tableheader which is far from being added to every page, I'm invoking the same reasoning as to why we don't need a jQuery.support.resizeCSS : #20 and that we should use the same approach as #15.
The crash described is pretty rare and js aggregation would most likely avoid the problem, but it's still an issue and that code certainly doesn't need to run on every page.
| Comment | File | Size | Author |
|---|---|---|---|
| #30 | 1439462_position_fixed.patch | 1.11 KB | catch |
| #22 | core-js-remove-fixedPosition-1439462-22.patch | 402 bytes | Jelle_S |
| #6 | core-js-remove-positionFixed-1439462-5.patch | 1.04 KB | nod_ |
| core-js-remove-positionFixed-D7.patch | 1.83 KB | nod_ | |
| core-js-remove-positionFixed.patch | 1.73 KB | nod_ | |
Comments
Comment #1
mikemadison CreditAttribution: mikemadison commentedFor what it's worth, I ran into a problem on a development site today. Your D7 patch solved my issue.
I'm not sure exactly what caused the issue, I can tell you I'm running D7.10. The page in question was trying to execute the Colorbox module, as well as the QuickTabs module.
QuickTabs was loading, Colorbox was not, so I assume that it was somehow related to Colorbox. Once I did the patch, everything was fine.
Comment #2
nod_That's nice to know but unless you're sure it's the same error you've been experiencing it can't really count as a test.
Glad that ended up helping you though :)
Comment #4
nod_What's wrong with the bot? since when does it test -D7 patches?
Comment #5
Damien Tournoud CreditAttribution: Damien Tournoud commentedLet's try to understand why this happens. The patch in here looks like just a workaround.
Our addition is in
.ready()because it relies on the<body>tag being defined. On the other hand, the code in jQuery that initializesjQuery.supportis directly in the main scope ofjquery.js(and as a consequence should be executed when jQuery is loaded).So the only reason for
jQuery.supportto be undefined would be thatdrupal.jsis loaded beforejquery.js. Why would that happen?Comment #6
nod_Actually…
D7 patch still stands.
Comment #7
nod_Comment #8
nod_Another no-brainer for D8.
Comment #9
nod_Comment #10
nod_The jQuery documentation is not up to date, here is the relevant ticket adding this thing to $.support: http://bugs.jquery.com/ticket/6809
Comment #11
amateescu CreditAttribution: amateescu commentedI went through this issue today with @nod_ and neither of us couldn't reproduce the initial bug report in Chrome 17 (or FF 11 fwiw) so I suppose it was a bug in an earlier Chrome version.
With the bug report out of the way, I'd say this is a nice and small improvement for D8, now that we have jQuery 1.7.
Comment #12
catchLooks likes great clean-up.
I've not come across the original bug report, if it's no longer possible to reproduce then I'm happy to ignore that and just take the clean-up patch as is, so I've committed/pushed it to 8.x.
This will need a change notification.
Comment #13
ifriz CreditAttribution: ifriz commentedI am going to write the change notification.
Comment #14
ifriz CreditAttribution: ifriz commentedChange record was created.
http://drupal.org/node/1497012
Comment #15
nod_Thanks for taking that on :)
The text could use some work. try to make a somewhat pleasant sentence, it's a bit dry like that.
Also a change notif needs review as well, don't just fix it without second opinion.
The important thing here is that we don't make our own check anymore since jQuery already provide the information we need. it's a renaming.
Comment #16
nod_Made a few changes, hopefully that'll be clearer.
Comment #17
amateescu CreditAttribution: amateescu commentedYep, looks cleaner.
Comment #18
nod_Updated the title as well, forgot to change it.
Comment #20
seutje CreditAttribution: seutje commentedLooks like updating jQuery broke this, as it seems like they dropped
$.support.fixedPosition-> https://github.com/jquery/jquery/commit/77536f5cb2ab042ac8be40ba59f36d8f...also, since we support only IE8 and up, can we just get rid of this test and assume
position: fixed;is always supported?Comment #21
nod_+1 to that.
Comment #22
Jelle_SShould be a simple patch... Don't think testbot will have problems with it.
Comment #24
Jelle_SFailed 1 test:
Retesting because this doesn't seem related...
Comment #25
Jelle_S#22: core-js-remove-fixedPosition-1439462-22.patch queued for re-testing.
Comment #26
seutje CreditAttribution: seutje commentedOdd, why wouldn't it pass? o.O
Looks good to me!
Comment #27
Jelle_S@seutje because the bot doesn't like me :-( ;-)
Now we just have to wait for one of the core committers to swing by...
Comment #28
Dries CreditAttribution: Dries commentedAgreed that this can be removed. Committed.
Comment #29.0
(not verified) CreditAttribution: commentedortho
Comment #30
catchFound this doing some performance optimization of a 7.x site - causes a recalculate styles/layout due to the adding/checking height of/removing element.
Here's a 7.x backport for sites that don't support IE8. Don't see a way to backport this, unfortunately we made that property part of the 7.x API, when it really should have been contained in tableheader.js.