0
\$\begingroup\$

Currently, I'm working on a project where I needed to run some code every time the page is hidden.

I searched high and low and found this question: Is there a way to detect if a browser window is not currently active?. The accepted answer did an amazing job, but it only sets a class, doesn't run a function.

So, with that need, I took off from that code and made an API around it:

;(function(window){
    //Name of the property used by the browser
    var hidden = 'hidden';
    //Internal status, to know if the page is visible or not
    //This will hold the current page visibility status, in case someone runs .fire()
    var status;

    var handlers = {
        visible: [],
        hidden: [],
        change: []
    };

    var fire = function(event){

        var fired_status = event === 'change' ? status : event;

        //Prevents external changes from causing off-by-n bugs
        var list = handlers[event];
        var length = list.length;

        for(var i = 0; i < length; i++)
        {
            if(list[i])
            {
                list[i].call(window, fired_status);
            }
        }
    };

    //=========================================================================================
    // Core code, taken from https://stackoverflow.com/q/1060008/
    // Changed to my own taste and needs

    //Only runs when the status changes, so, we trigger the change here too
    var onchange = function(event){
        var map = {
            focus: 'visible',
            focusin: 'visible',
            pageshow: 'visible',
            blur: 'hidden',
            focusout: 'hidden',
            pagehide:'hidden'
        };

        event = event || window.event;

        //We need to keep the internal status updated
        status = map[event.type] || (this[hidden] ? 'hidden' : 'visible');

        fire(status);
        fire('change');
    };
    if (hidden in document)
    {
        document.addEventListener('visibilitychange', onchange);
    }
    else if ((hidden = 'mozHidden') in document)
    {
        document.addEventListener('mozvisibilitychange', onchange);
    }
    else if ((hidden = 'webkitHidden') in document)
    {
        document.addEventListener('webkitvisibilitychange', onchange);
    }
    else if ((hidden = 'msHidden') in document)
    {
        document.addEventListener('msvisibilitychange', onchange);
    }
    else if ('onfocusin' in document)
    {
        document.onfocusin = document.onfocusout = onchange;
    }
    else
    {
        window.onpageshow = window.onpagehide = window.onfocus = window.onblur = onchange;
    }

    status = document[hidden] === undefined ? null : (document[hidden] ? 'hidden' : 'visible');
    //=========================================================================================

    if(status !== null)
    {
        var fn = function(){
            if(status === 'hidden')
            {
                fire('hidden');
            }
            window.removeEventListener('load', fn);
        };

        window.addEventListener('load', fn);
    }

    window.PageVisibility = {
        //jQuery - style handling
        visible: function(handler){
            if(handler instanceof window.Function)
            {
                handlers['visible'][handlers['visible'].length] = handler;
            }
            else
            {
                fire('visible');
            }
        },

        //plain style (eg: obj.onevent = handler)
        set onvisible(handler){
            handlers['visible'] = handler instanceof window.Function ? [handler] : [];
        },
        get onvisible(){
            return handlers['visible'][0];
        },

        hidden: function(handler){
            if(handler instanceof window.Function)
            {
                handlers['hidden'][handlers['hidden'].length] = handler;
            }
            else
            {
                fire('hidden');
            }
        },
        set onhidden(handler){
            handlers['hidden'] = handler instanceof window.Function ? [handler] : [];
        },
        get onhidden(){
            return handlers['hidden'][0];
        },

        visibilitychange: function(handler){
            if(handler instanceof window.Function)
            {
                handlers['change'][handlers['change'].length] = handler;
            }
            else
            {
                fire('change');
            }
        },
        set onvisibilitychange(handler){
            handlers['change'] = handler instanceof window.Function ? [handler] : [];
        },
        get onvisibilitychange(){
            return handlers['change'][0];
        },

        //Modern style
        addEventListener: function(event, handler){
            if(!(event in handlers) || !(handler instanceof window.Function))
            {
                return false;
            }

            handlers[event][handlers[event].length] = handler;

            return true;
        },
        removeEventListener: function(event, handler){
            if(!(event in handlers) || !(handler instanceof window.Function))
            {
                return false;
            }

            if(handler)
            {
                for(var i = 0, l = handlers[event].length; i < l; i++)
                {
                    if(handlers[event][i] === handler)
                    {
                        delete handlers[event][i];
                    }
                }
            }
            else
            {
                handlers[event] = [];
            }

            return true;
        },
        //Should return null only when it is impossible to check if the page is hidden
        isVisible: function(){
            return status === null ? null : status === 'visible';
        },
        fire: function(){
            if(status !== null)
            {
                fire(status);
            }
        },
        //May be useful for someone
        hiddenPropertyName: hidden
    };

})(Function('return this')());

The code is quite long but most of it is just whitespace.

The code allows to use jQuery-style event handling, direct-attribution style (or old style) and the "modern" addEventListener style.

I know the code isn't compatible with IE8, but I only need to support IE9 and up. Any other browser that works is perfectly fine.


My major area of concert is all the repeated code on the event handling. I couldn't think of a way to keep it easy and clean.

Performance might be an issue as well.

In terms of readability, clarity and performance, what else can I improve?

\$\endgroup\$
  • 1
    \$\begingroup\$ looks like 'getters' in window.PageVisibility object don't return anything, they just read an array value get onvisible(){ handlers['visible'][0]; } . Could you explain why ? \$\endgroup\$ – RomanPerekhrest Mar 27 '16 at 19:21
  • \$\begingroup\$ @RomanPerekhrest Because I forgot the return -.- I've ninja-edited it. \$\endgroup\$ – Ismael Miguel Mar 27 '16 at 20:06
1
\$\begingroup\$

Currently, there is a small bug on removeEventListener.
The method is prepared to remove all event listeners. It does so by having no value on the 2nd parameter.
To remove a single event listener, just pass the function.

An example:

PageVisibility.removeEventListener('change', function(){}); //works, removes the event listener
PageVisibility.removeEventListener('change'); //doesn't work, returns `false`

There are 2 ways to fix this:

  • Change the if inside the method:

    if(!(event in handlers) || (handler && !(handler instanceof window.Function)))
    {
        return false;
    }
    
  • Rewrite the code to remove multiple event listeners:

    [...]
    removeEventListener: function(event, handler){
        if(!(event in handlers) || !(handler instanceof window.Function))
        {
            return false;
        }
    
        for(var i = 0, l = handlers[event].length; i < l; i++)
        {
            if(handlers[event][i] === handler)
            {
                delete handlers[event][i];
            }
        }
    
        return true;
    },
    removeEventListeners: function(event){
        if(!(event in handlers))
        {
            return false;
        }
    
        handlers[event] = [];
    
        return true;
    },
    [...]
    

I've decided to stick with the first option since it is easier to use.
Some may disagree and prefer the second option due to separation of tasks and avoid surprises if they forget the 2nd parameter.

|improve this answer|||||
\$\endgroup\$
0
\$\begingroup\$

I would suggest refactoring, at least, two "moments":
- check for 'hidden' property support among browsers;
- window.PageVisibility object

The goal is to eliminate redundancy and duplicate logic.

var hiddenMap = {'hidden': '', 'mozHidden': 'moz', 'webkitHidden': 'webkit', 'msHidden': 'ms'},
    foundSupport = false, // indicates whether '~hidden' was determined in current browser (depending on browser's type)
    prop;

// check for '~hidden' property existence among major browsers
for (prop in hiddenMap) {
    if ((hidden = prop) in document) {
        document.addEventListener(hiddenMap[prop] + 'visibilitychange', onchange);
        foundSupport = true;
        break;
    }
}
if (!foundSupport) {
    if ('onfocusin' in document) {
        document.onfocusin = document.onfocusout = onchange;
    } else {
        window.onpageshow = window.onpagehide = window.onfocus = window.onblur = onchange;
    }
}

/**
* Sets/Fires passed 'PageVisibility' handler 
* @param handler expects function object
* @param eventType event type
* @param setter whether the method id delegated to set passed handler into list of handlers
* return void
**/
var setVisibilityHandler = function(handler, eventType, setter) {
    if (handler && type) {
        if (setter) { // setter action
            handlers[eventType] = handler instanceof window.Function ? [handler] : [];
        } else {
            if (handler instanceof window.Function) {
                handlers[eventType][handlers[eventType].length] = handler;
            } else {
                fire(eventType);
            }
        }
    }
};

window.PageVisibility = {
    //jQuery - style handling
    visible: function (handler) {
        setVisibilityHandler(handler, 'visible');
    },
    //plain style (eg: obj.onevent = handler)
    set onvisible(handler) {
        setVisibilityHandler(handler, 'visible', true);
    },
    get onvisible() {
        return handlers['visible'][0];
    },
    hidden: function (handler) {
        setVisibilityHandler(handler, 'hidden');
    },
    set onhidden(handler) {
        setVisibilityHandler(handler, 'hidden', true);
    },
    get onhidden() {
        return handlers['hidden'][0];
    },
    visibilitychange: function (handler) {
        setVisibilityHandler(handler, 'change');
    },
    set onvisibilitychange(handler) {
        setVisibilityHandler(handler, 'change', true);
    },
    get onvisibilitychange() {
        return handlers['change'][0];
    },
    //Modern style
    addEventListener: function (event, handler) {
        if (!(event in handlers) || !(handler instanceof window.Function)) {
            return false;
        }
        handlers[event][handlers[event].length] = handler;

        return true;
    },
    removeEventListener: function (event, handler) {
        if (!(event in handlers) || !(handler instanceof window.Function)) {
            return false;
        }

        if (handler) {
            var len = handlers[event].length;
            while (len--) {
                if (handlers[event][len] === handler) {
                    delete handlers[event][len];
                }
            }
        } else {
            handlers[event] = [];
        }

        return true;
    },
    //Should return null only when it is impossible to check if the page is hidden
    isVisible: function () {
        return status === null ? null : status === 'visible';
    },
    fire: function () {
        if (status !== null) {
            fire(status);
        }
    },
    //May be useful for someone
    hiddenPropertyName: hidden
};
|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ I'm really sorry, but I'm very confused with your review. Why does checkHandler does everything but checking the handler? And why is it a named function, on the global scope? Why it doesn't return anything? And why that foundSupport variable? I'm not saying your review is bad. I'm saying that it is confusing and needs some clarification. I really don't know your logic and why you wrote it the way you did. \$\endgroup\$ – Ismael Miguel Mar 28 '16 at 22:21
  • \$\begingroup\$ 1)Why does checkHandler does everything but checking the handler? - Doesn't checkHandler check passed handler with this handler instanceof window.Function or handler instanceof window.Function? don't you think? 2) And why is it a named function, on the global scope? - Ok. I had replaced it with variable; 3) Why it doesn't return anything? - Functions don't always have to return something, it isn't strictly necessary. It's an entirely subjective decision. 4) why that foundSupport variable? - It's a flag, indicating whether ~hidden property was determined among major browsers \$\endgroup\$ – RomanPerekhrest Mar 29 '16 at 6:12
  • \$\begingroup\$ The problem isn't that it is a named function. The problem is that it is on the global scope and does the oposite of checking the handler. I see what you have gone for with the function,but the name isn't the right one. I like the idea, not the name. But the big problem is that you simply put the code without any explanation. A review should explain that my code sucks, why it sucks and hoe to make it non-sucky. I don't get that from yours. Please, try to explain yourself a little on the answer. So everybody can understand why you went that way and +1 your answer. I see the code, not the "why". \$\endgroup\$ – Ismael Miguel Mar 29 '16 at 7:55
  • \$\begingroup\$ ` The problem is that it is on the global scope` - var fire, var onchange, window.PageVisibility are also in global scope. As for the name that handler - the problem is that it acts like a handler 'setter' in one case, and like a handler launcher - in another case. So, the name should be pretty creative, or checkHandler should split into two separate functions. \$\endgroup\$ – RomanPerekhrest Mar 29 '16 at 14:32
  • 4
    \$\begingroup\$ @RomanPerekhrest I've noticed that you have posted 6 answers and only have 3 upvotes here on Code Review, but you have 11K on stackoverflow.com. We are a little different on code review, generally a written explanation rather than an alternate solution is considered a better answer here on code review. You have at least one answer here on code review that is good at codereview.stackexchange.com/questions/118184/…. It might help improve your scores if you read the help page codereview.stackexchange.com/help/how-to-answer. \$\endgroup\$ – pacmaninbw Sep 20 '16 at 14:34

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.