Fix #10814. Make support tests lazy and broken out to components. Fix #14084: attach the test div to documentElement, not body. #1342
Conversation
|
This seems more-or-less ready except points written in description. |
|
The errors are weird, nothing breaks purely in tests, they even can't get to the |
|
This is a great start. I have a suggestion though. We can still attach support properties to |
|
@timmywil Such a tactic would make it easier for us to test One solution I proposed in description would be to create a separate build option exposing the properties so that we can test it. Seems a little ugly, though. Any better suggestions? @timmywil, @dmethvin, @Orkel, @gibson042? |
|
I really don't think we need to protect ourselves here by actually hiding the support properties. We've been pretty clear about the fact that these are not public. If people continue to use them and then complain when they go away, we can just continue to point out that they're private. One big step in that direction would be to remove all details on http://api.jquery.com/jQuery.support/ and just have a page that says The bottom line is that if we're willing to drop all support properties today, then there's no reason we shouldn't be willing to drop individual properties at will. |
|
It does seem like a drastic change. I missed the conversation, but I don't think we need to make it completely private. It can certainly be useful if I know that the version of jQuery I am using already implements the exact feature test that I need. It's nice not to have to do it again, but I just know not to complain if that feature test goes away and I need to implement it myself in the future. Whatever we decide, I think we should still gather all of our features tests on |
|
If we don't expose Another hard thing to do is to demonstrate that we actually improve page initialization and did not heavily injured module performance. BTW, you changing a lot, so it might be good idea to fix code style issues on the lines that you touch and follow the code style in new files. All and all that's a good start. |
I'm good with that, and created a ticket. We can leave an entry saying it shouldn't be used. As for why, I think the logic was that others might need some of this info. However, it was definitely a mistake to expose things like this that cause forced layouts and then guarantee they are always present. Seems like this is a question of how much we're going to break things. We can stuff the results we do create into |
|
I don't care if the properties stay in |
As you probably realise, this is true for some, but not all, support tests. Sometimes the test indicates the the code should take a different path to the same result. In those cases, it's about performance. You get the correct test result either way. The only reliable way to test them without their exposure is to test for both correct results and performance regressions, both of which could be tricky and probably too much work right now. @scottgonzalez is right that the sanest way to test this stuff is to have it exposed somehow. Maybe |
You get the correct test result either way Do you have an example? But even if such test does not exist it might be in future. @scottgonzalez is right that the sanest way to test this stuff is to have it exposed somehow Yes, by this remark: Which is hard thing to do and it looks like it would be hard thing to maintain I was implying the same thought :-). |
|
Sorry, wrong account :-(, "markelog" is me |
Sure, |
|
I implemented requested changes. I'm using I still have these 3 strange test errors, I could use some help tracking them out. Besides these errors, is it OK to merge or is there anything else I need to fix? |
| input.type = "radio"; | ||
| support.radioValue = input.value === "t"; | ||
| })(); | ||
|
|
There was a problem hiding this comment.
Not a big deal, but you could return support here and just include ./attributes/support as the support var. The build will handle removing the return statement.
There was a problem hiding this comment.
I actually had sth like that at one point but I changed it not aware of the fact the build step will remove the return statement, good to know! I'll change that.
There was a problem hiding this comment.
@timmywil Should it be ./attributes/support or ./attributes/var/support?
There was a problem hiding this comment.
Seems like you figured it out already, but ./attributes/support is right because you're not adding another var, just adding to an existing one.
|
This is great, by the way. |
|
I added a test for http://bugs.jquery.com/ticket/14084 but it doesn't run for some reason. When I put it in |
| expected = { | ||
| "checkOn":true, |
There was a problem hiding this comment.
This is lots of line changes but I figured out since this diff will be large anyway...
We have a mess in lists of support properties here, they're in a completely random order. I made it alphabetical, it'd be good if we kept it that way.
|
@Orkel, we should be able to prove speed improvements with a couple of jsperfs showing results. @mzgol, this PR gets us separated out by module, which is awesome, but theoretically actually slows us down until we implement lazy loading strategy, right? @timmywil @scottgonzalez @dmethvin, in the past we documented jQuery.support and have had "trouble" deprecating these, like what happened when we tried to remove our broken quirks mode feature detect. Now that we have AMD, we no longer need to be burdened by the "public but not documented" conundrum. My vote would be to deprecate jQuery.support in the next release, with a path to removal. Besides, if we want lazy initialization (I.e. do the support test right before we need it), we'll likely need to change the jQuery.support object anyway. |
|
@mikesherov this does implement lazy loading. We only need to lazy load tests that require doc ready or a body. |
|
IMHO if we have to expose I'd still love to get this hidden (esp. that we have all of that under @mikesherov I only lazy-load tests that might trigger layout, i.e. those operating on elements attached to I'm working on an analogous PR for |
|
@mikesherov we can't do that, because this change will actually decrease speed of some methods but i yet would like to know by how much. @mzgol? Speed improvements might be considerable on page initialization time, at empty page with only jQuery included it's a 0.056 ms on my machine in Chrome. For 1.10.2 on some "heavy" page it's a ~150 ms. To know by exactly how much we could win i cc you and @paulirish. Because it's a main reason why support tests should became lazy-load. @mzgol i mention before BTW, you changing a lot, so it might be good idea to fix code style issues on the lines that you touch and follow the code style in new files. |
| @@ -797,6 +797,11 @@ test("css('width') and css('height') should respect box-sizing, see #11004", fun | |||
| equal( el_dis.css("height"), el_dis.css("height", el_dis.css("height")).css("height"), "css('height') is not respecting box-sizing for disconnected element, see #11004"); | |||
| }); | |||
|
|
|||
| testIframeWithCallback( "css('width') should works correctly before document ready", "css/cssWidthBeforeDocReady.html", function( cssWidthBeforeDocReady ) { | |||
There was a problem hiding this comment.
Could you mention ticket number here?
There was a problem hiding this comment.
BTW, any ideas, @Orkel, why this test does not fire unless I move it from here to unit/support?
There was a problem hiding this comment.
mmm, it does work in my browsers
|
@Orkel I corrected some style issues but not all. I looked over the files now and corrected the rest. Let me know if I missed anything. |
|
@Orkel how many tests current Chrome fire on this branch in the |
|
257 :-), in what browsers do you get such results? |
|
@Orkel It seems only in Chrome stable, works in Firefox, Safari, Opera Presto & Blink, Chrome Canary... EDIT: meh, that'll teach me to run tests ALWAYS in incognito mode, works there. :D |
|
:-))) |
|
Just to stress it, those 3 test failures exist in incognito mode as well. This is extremely weird as it seems it can't even get to the |
|
I've just created an analogous pull request for |
|
Since I don't experience the same failures on the Need to investigate more. |
|
BTW, this saves 19 bytes minified & gzipped. :) |
Support module broken up.
Things still to do:
(1) 3 tests are failing, I need to figure why (
documenElementissues?)Analogous PR for
1.x-master: #1353.