Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Gmail had this awesome feature and it was discontinued for some reason. Nowadays I have installed a nice Google Chrome plugin for this job.

A couple of months ago I had to develop a similar feature for a project and I think that could be improved but I don't know how.

I'm here asking for a code review or suggestions.

(function($, undefined) {

    $.fn.mousePointer = function(options) {

        var settings = $.extend({}, {
            size : 95,
            bgColor : "#000",
            arrColor : "#FFF",
            btnLeft : null,
            btnRight : null,
            btnUp : null,
            btnDown : null,
            className : "p_c_arr_" + new Date().getTime()
        }, options);

        var fncCreateButton = function(target, degrees, left, top) {

            var r = settings.size, w = r / 5;

            var c = document.createElement("canvas");
            c.className = settings.className;
            c.height = r, c.width = r;

            var ctx = c.getContext("2d");

            // CREATE THE BACKGROUND BUTTON
            ctx.beginPath();
            ctx.arc(r, r, r, Math.PI, 1.5 * Math.PI);
            ctx.arc(r, r, w * 2, 1.5 * Math.PI, Math.PI, true);
            ctx.closePath();

            ctx.strokeStyle = settings.bgColor;
            ctx.stroke();

            ctx.fillStyle = settings.bgColor;
            ctx.fill();

            // CREATE THE ARROW INSIDE THE BUTTON
            ctx.beginPath();
            ctx.moveTo(r / 2 - w / 2, r / 2 - w / 2);
            ctx.lineTo(r / 2, r / 2 + w);
            ctx.lineTo(r / 2 + w, r / 2);
            ctx.closePath();

            ctx.strokeStyle = settings.arrColor;
            ctx.stroke();

            ctx.fillStyle = settings.arrColor;
            ctx.fill();

            // SET THE ANGLE AND POSITION
            var canvas = $(c);
            canvas.css({
                "-webkit-transform" : 'rotate(' + degrees + 'deg)',
                "-moz-transform" : 'rotate(' + degrees + 'deg)',
                "-ms-transform" : 'rotate(' + degrees + 'deg)',
                "transform" : 'rotate(' + degrees + 'deg)',
                "position" : 'absolute',
                "opacity" : 0,
                "left" : left,
                "top" : top
            }).data("target", target);

            $("body").append(c);

            return canvas;

        }
        var fncRemoveButtons = function() {

            $("." + settings.className).remove();
            $(document).off("mousemove.mousePointer");
            $(document).off("mouseup.mousePointer");

        }
        var fncDoAction = function(button, opacity) {

            if ( target = button.data("target")) {
                button.css("opacity", opacity);
                if (opacity >= 1) {
                    fncRemoveButtons();

                    var target = $(target);
                    var events = $._data(target[0], "events");

                    if (events && typeof events.click == "object") {
                        target.trigger("click");
                    } else if ( typeof target[0].onclick == "function") {
                        target[0].onclick();
                    } else {
                        window.location = target.attr("href");
                    }

                }
            }

        }

        $(document).on("mousedown.mousePointer", function(e) {

            if (e.which == 3) {

                $(document).on("contextmenu", function(e) {
                    e.preventDefault();
                });

                $(document).on("selectstart.mousePointer", function(e) {
                    e.preventDefault();
                });

                $(document).on("mouseup.mousePointer", function(e) {
                    fncRemoveButtons();
                });

                var x = e.clientX, y = e.clientY, r = settings.size * 0.75, degrees = 45;

                var btnUp = fncCreateButton(settings.btnUp, (degrees), (x - r / 2), (y - r * 1.5));
                var btnRight = fncCreateButton(settings.btnRight, (degrees * 3), (x + r / 2), (y - r / 2));
                var btnDown = fncCreateButton(settings.btnDown, (degrees * 5), (x - r / 2), (y + r / 2));
                var btnLeft = fncCreateButton(settings.btnLeft, (degrees * 7), (x - r * 1.5), (y - r / 2));

                $(document).on("mousemove.mousePointer", function(e) {

                    var _x = e.clientX, _y = e.clientY;

                    fncDoAction(btnLeft, (1 - (_x - (x - r)) / r));
                    fncDoAction(btnRight, (1 - ((x + r) - _x ) / r));
                    fncDoAction(btnUp, (1 - (_y - (y - r)) / r));
                    fncDoAction(btnDown, (1 - ((y + r) - _y ) / r));

                });

            }

        });

    }
})(jQuery);

And if you want to contribute, you can find it here.

share|improve this question

1 Answer 1

up vote 11 down vote accepted

First, a word about usage: $().mousePointer is sub-optimal. Why instantiate a jQuery object that you never even touch? Instead, you can attach your plugin as $.mousePointer = ... and call it as $.mousePointer(...).

In line 5 you do $.extend({}, {...}, options). The point of passing an empty object as the first argument is to prevent modifying an existing object. Since you're passing an object literal (i.e. an object that isn't used anywhere else) as the second argument, this is not needed. $.extend({...}, options) will do. If you decide to move the defaults definition to the top of the file (a good idea), $.extend({}, defaults, options) does make sense.

Use Date.now() instead of new Date().getTime() to avoid an extra allocation and also to make the code more readable. Or rather, don't rely on classes to identify your buttons.

In lines 16-68 you create a canvas, paint a static image to it, then rotate it using CSS and return the canvas. Instead, consider using SVG:

  • Smaller size and easier to change (graphics designers are more likely to be savvy in Inkscape than in canvas javascript). It can also be stored in an external file and not clutter up code. Also note that the client might want to use a PNG image instead. You don't give him that option.
  • It has about the same support as canvas does (IE8 supports neither, other desktop browsers support both).
  • SVG is a vector format. Canvas renders onto a bitmap. This means higher quality when doing transformations (scaling, rotating by anything else than a multiple of 90 degrees, displaying on a retina display ...)
  • SVG works with javascript disabled (not a benefit in this case, I admit)
  • As far as I can tell, browsers will prefer to allocate CSS-transformed canvas buffers on the GPU. I don't think these buttons need, or can benefit from, GPU acceleration. Browsers are getting better, but there's only so much they can guess.

The downside of external SVG is that javascript cannot easily tweak the view. Even then, an inline SVG still has the benefit of higher graphics quality. Even if you don't want to learn SVG or use external editors, I recommend rotating the button in code and rendering an already rotated version onto the canvas.

Also, out of desktop browsers, only Safari needs a prefix for CSS transformations.

Also, not sure what's the point of stroking and filling the same shape with the same color, except maybe to provide a more solid outline if the color used happens to be semitransparent.

L64: Use $(document.body) instead of $("body"). It looks better and it's slightly faster.

L72-73: It's great that you namespace your event handlers. Note that you can supply multiple events at once: $(document).off("mousemove.mousePointer mouseup.mousePointer"). Similarly for $().on. Alternatively, you can pass the event handlers themselves to off.

L78: if ( target = button.data("target")) -- This looks like a typo. If you do want to do an assignment inside a condition while testing for truthiness, you can denote that using an extra pair of parentheses: if((target = button.data("target"))). Better yet, assign first, then test. var target = button.data("target"); if(target) .... Also, your target variable was never declared an is accidentally global. This mistake is harder to make when assigning first and testing then.

L84-91: What is that? The absence of an onclick property doesn't mean the DOM node doesn't have any event handlers. Also, don't use $._data. It's a private function and can be removed at any time. If you want to provide a default event handler, accept an alternate event handler, attach it yourself and if one isn't provided, attach your own instead. This also prevents an awkward situation when someone wants to detect click events on your buttons without disrupting their functionality. In short, what event handlers are attached to a DOM node is none of your business.

L101: if(e.which == 3) surely deserves a comment: // right mouse button. Also, === is generally preferred over ==.

L124: x and _x aren't very descriptive variable names. menuCenterX and menuClickX? Even firstClickX and secondClickX are easier to understand.

Finally, the math can be simplified:

  1 - (_x - (x - r)) / r
= 1 - (_x - x + r) / r
= 1 - (_x - x)/r - r/r
= 0 - (_x - x) / r
= (x - _x) / r

Concerning the signature of fncCreateButton: the left and top arguments do not respect the button symmetry. I would probably choose the arc center as the reference point. It would simplify the caller code and possibly also the callee code. The amount of responsibilities of this function seems acceptable. The function can be split further, although the comments do serve their purpose and code duplication is not an issue here. Not sure if the same function should both create an element and append it to the document - even if the creation is separated into a different function. The single responsibility principle is violated. I am sometimes guilty of the same. Note that factoring out a function is not too hard of a thing to do.

What's the point of creating a canvas that is never visible? Both fncCreateButton and its caller know if the target element is null.

It's also better performance-wise to not keep creating and destroying the buttons repeatedly - especially since canvas creation involves a graphic buffer allocation.

fncDoAction surely does too much. It:

  • determines if there's a DOM node to click
  • sets the opacity of a button
  • determines if the opacity is reached
  • removes the context menu buttons
  • tries to pinpoint a single event handler and call it
  • provides a default action in case it doesn't find a handler

Button removal is separated into its own function called from here and I have already mentioned the handler faulty magic issue. I also realise its point is to avoid code duplication. But the function name doesn't really say anything. Renaming it would preserve readability. Perhaps moving it closer to its only usage and giving it a name along the lines of handleMouseMove would be better?

I can see you're using the types hungarian notation to denote functions (fncSomething; not sure what "c" stands for) but not for anything else. Hungarian notation isn't commonly used nowadays, and, since function names are verbs rather than nouns, I'm not sure why you feel the need to use a prefix for functions in particular. The most common use of hungarian notation in javascript is to denote jQuery objects as opposed to vanilla DOM nodes ($this = $(this)).

I don't even know what the target element is for, or if it's ever visible, but you seem to require the client to supply one, and you seem to only use it as a placeholder for event handlers and to disable buttons that shouldn't be doing anything. Instead, let the client supply their own event handlers (instead of event handling DOM nodes) and optional boolean arguments to disable individual buttons.

Finally, have you tested if your plugin works when:

  • the user scrolls after you show the context menu before you dismiss it?
  • the user operates on a zoomed in / zoomed out page?
  • the user attempts touch screen or the menu key to invoke the context menu?
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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