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

Proposal: new object structure for test definitions #1246

Open
stucox opened this issue Mar 2, 2014 · 4 comments
Open

Proposal: new object structure for test definitions #1246

stucox opened this issue Mar 2, 2014 · 4 comments

Comments

@stucox
Copy link
Member

stucox commented Mar 2, 2014

Not expecting us to act on this any time soon, but thought I’d throw my brain-dump out there.

Ok, so I got a bit carried away while looking at #1224. Basically there’s a limit to how much we can optimise things like testStyles() with our tests as they are at the moment: just a chunk of code to test a thing. I’ll carry on working on it though, there are definitely some savings which can be made, but I had some other ideas too.


Problem:

Modernizr’s current addTest() test structure makes it hard to separate DOM manipulation and DOM checking. Interleaving these things is poor for performance, because it results in a large number of restyle/layout operations.

Solution:

Restructure how tests are defined, such that DOM manipulation and DOM checking can be separated. The runner can then perform all DOM state setup for all tests together, and then perform all the necessary checking together.

Proposal: new test format

Modernizr.addTest() could be adapted to accept an object as a sole argument:

Modernizr.addTest({
    name: 'propname',
    setUp: function (elem) {
        // Do DOM manipulation here; `elem` is a container
        // for you to use
    },
    tearDown: function (elem) {
        // Any special teardown actions here, although rarely needed:
        // `elem` and its children will be removed automatically
    },
    test: function (elem) {
        // Interrogate the DOM and return test result
    }
});

The test runner would then be able to run each test’s setUp phase, one after the other: if these are written to avoid reading from the DOM, they’ll rattle through without any expensive restyle/layout operations.

It would then do a single repaint, before running all of the test phases; again, as long as these don’t modify the DOM in any way, the browser won’t have to restyle/layout.

Of course there will be some which can’t be split like this, but I think they’re rare.

Example: (Compare with the current version)

Modernizr.addTest({
    name: 'localizednumber',
    setUp: function (elem) {
        var input;

        elem.innerHTML = '<input type="number" value="1.0" step="0.1"/>';
        input = elem.childNodes[0];
        input.focus();

        try {
            document.execCommand('InsertText', false, '1,1');
        }
        catch(e) { } // prevent warnings in IE
    },
    test: function (elem) {
        var input = elem.childNodes[0];
        var diff = input.type === 'number' &&
            input.valueAsNumber === 1.1 &&
            input.checkValidity();
        return diff;
    }
});

Simple tests

Like the current API, test could still just be a Boolean value:

Modernizr.addTest({name: 'geolocation', test: 'geolocation' in navigator});

Built-in helpers

Common test patterns could be invoked using special attributes. These could be designed to split their DOM manipulation and interrogation efficiently.

testProp / testValue

testProp performs a test for the named CSS property, using testValue if provided (and CSS.supports() if available); the result is passed to the test method:

Modernizr.addTest({
    name: 'boxsizing',
    testProp: 'box-sizing',
    testValue: 'border-box',
    test: function (elem, result) {
        return result && (document.documentMode === undefined || document.documentMode > 7);
    }
});

Or omit the test function for the Boolean result of the property check will be used implicitly:

Modernizr.addTest({
    name: 'bgsizecover',
    testProp: 'background-size',
    testValue: 'cover'
});

It would test for prefixed versions too by default, unless you’ve set the noPrefixes global option, or you set usePrefixes: false for the specific test:

Modernizr.addTest({
    name: 'textshadow',
    testProp: 'text-shadow',
    testValue: '1px 1px',
    usePrefixes: false
});

useStyles

useStyles can be used to prime the test with some style rules. The special token #m! will be replaced with a selector targeting the elem (which is determined at runtime):

Modernizr.addTest({
    name: 'hiddenscroll',
    useStyles: '#m! {width:100px;height:100px;overflow:scroll}',
    test: function (elem) {
        return elem.offsetWidth === elem.clientWidth;
    }
});

prefixed

prefixed can be used too, and its result would be passed to test, like testProp; we can name argument something more useful:

Modernizr.addTest({
    name: 'lowbattery',
    prefixed: 'battery',
    prefixedObj: navigator,
    test: function (elem, battery) {
        var minLevel = 0.20;
        return !!(battery && !battery.charging && battery.level <= minLevel);
    }
});

If test is omitted then the test result will be the result of prefixed(), cast to a Boolean:

Modernizr.addTest({
    name: 'batteryapi',
    prefixed: 'battery',
    prefixedObj: navigator
});

And there are inevitable combinations of these – we’d have to agree an order for the arguments passed to test, etc.

Async tests

I think this structure better facilitates async tests too, with a helpful result method:

Modernizr.addTest({
    name: 'webp',
    test: function (elem) {
        var image = new Image();

        image.onerror = function() {
            this.result(false);
        };
        image.onload = function() {
            this.result(image.width == 1);
        };

        image.src = 'data:image/webp;base64,UklGRiQAAABXRUJQVlA4IBgAAAAwAQCdASoBAAEAAwA0JaQAA3AA/vuUAAA=';
    }
});

Interdependencies

We’d still have our AMD wrappers around all of these (I’ve omitted them so far for brevity), but they’d be used as they are now when a test depends on another:

define(['Modernizr', 'test/canvas'], function ( Modernizr ) {
    Modernizr.addTest({
        name: 'canvasblending',
        test: function (elem) {
            if (Modernizr.canvas === false) {
                return false;
            }
            var ctx = createElement('canvas').getContext('2d');
            ctx.globalCompositeOperation = 'screen';
                return ctx.globalCompositeOperation === 'screen';
        }
    });
});
@patrickkettner
Copy link
Member

I think this is exactly the type of thing that has to happen. Thank you so much for writing all of this up.
It would make using benchmark.js to profile all fo the detects a lot simpler, as well.

I would be concerned with accidentally leaking globals, since variables would have to be defined multiple times.

Also, would we be able to unset all vars between tests? We would risk having something be defined that shouldn't be depending on the order the tests run, right?

Built-in helpers

Common test patterns could be invoked using special attributes.

I would be concerned about making tests difficult for newcomers to grok. keeping the amount of properties to an absolute minimum would be a good goal to have. If its just a matter of a small amount of sugar, I would probably be against it.

prefixed

Modernizr.addTest({
    name: 'lowbattery',
    prefixed: 'battery',
    prefixedObj: navigator,
    test: function (elem, battery) {
        var minLevel = 0.20;
        return !!(battery && !battery.charging && battery.level <= minLevel);
    }
});

I know this is just pseudo code, but I don't see why we would need to pass in navigator. we could just look that up when needed.

If test is omitted then the test result will be the result of prefixed(), cast to a Boolean

again, worried about tests seeming too magical to newbies. but that is clean as hell.

async

Async stuff looks wonderful. much cleaner than the current. though it seems like "result" should be the "test" code. so more like

 Modernizr.addTest({
        name: 'asyncTest',
        async: function() {
           var i = new Image();
           i.onload = this.test;
           i.onerror = this.test;
        },
        test: function (event) {
            if (event.type == 'load') return true;
            return false;
})

that way test is always the way to test code.

@SlexAxton
Copy link
Member

I love all of this, but I have to be the one to ask if we're solving a
problem that doesn't have to exist.

When I build websites these days, I can rarely use 3 tests without getting
ahead of myself. I think all the tests are useful, but is there a benefit
to the majority of sites if they were only running the tests that they use?

I spose I'd like to see the perf impact of this with our most popular 6
plugins vs. a build today. It'd probably be easy to do by hand and would
likely be indicative of the actual perf impact vs. educational impact of
just running fewer tests.

(Forgive my thumbs/2am brain.)

@stucox
Copy link
Member Author

stucox commented Mar 5, 2014

It’s worth remembering that a lot of sites still end up in the wild with the dev version, and as much as we harp on at people about custom builds (including throwing warnings in the console), I’d feel more comfortable if we were doing everything in our power to minimise this hit.

One tweak I’d make – specifying the property name as the first arg (as it is now), so the object syntax just becomes an alternative way of defining your test “function”:

Modernizr.addTest('propname', {
    ...
});

But even with this, I appreciate the structure might be more complex to newcomers, which is a definite downside.

Science is the way to approach this. I’ll build a prototype on a branch, do some tests with various different builds, and we can look at the numbers and decide what to do.

@robwierzbowski
Copy link
Contributor

This structure is much more straightforward to me. ✨ it.

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

4 participants