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

hasEvent() - add tagName capability and fail faster #636

Merged
merged 9 commits into from Mar 24, 2013

Conversation

Projects
None yet
6 participants
@ryanve
Contributor

ryanve commented Jul 19, 2012

I made 2 changes to Modernizr.hasEvent()

1 - The optional elem param can now be an element or a tag name:

Modernizr.hasEvent('click', document.createElement('a'));
Modernizr.hasEvent('click', 'a');

2 - It now detects the need for the setAttribute fix ahead of time so that it can return false faster in browsers that don't need the fix. I tested this in FF3 / FF12 / IE7 / IE8 / Chrome / Opera / Safari and in all cases tried it behaves like the Modernizr.hasEvent() from version 2.5.3.

('onblur' in document.documentElement) // true => ok // false => needs fix
@paulirish

This comment has been minimized.

Member

paulirish commented Jul 19, 2012

my my. cc @kangax

@sindresorhus

This comment has been minimized.

Contributor

sindresorhus commented Jan 24, 2013

@kangax ping

@kangax

This comment has been minimized.

kangax commented Jan 29, 2013

Looks OK from quick glance. Checking presence of property upfront should be fine here. And 2nd param sugar sounds useful.

Only FIX is a bad name. Something like hasEventHandlerProperty would be better.

@stucox

This comment has been minimized.

Member

stucox commented Jan 29, 2013

@ryanve - could you update your PR? Ideally pull from master first - we've just moved to an AMD structure, so if you could make this fit in with that, that'd be great.

@ryanve

This comment has been minimized.

Contributor

ryanve commented Jan 30, 2013

@kangax @stucox Thanks—I'll touch it up and put in into the AMD structure.

ryanve added some commits Jan 30, 2013

// Detect whether event support can be detected via `in`. Use a DOM element
// for the test, with the `blur` event b/c it should always be there. If this
// is `false` then we need the fallback technique. (bit.ly/event-detection)
hasEventHandlerProperty = 'onblur' in document.documentElement;

This comment has been minimized.

@staabm

staabm Jan 30, 2013

IMO hasEventHandlerSupport would fit better

This comment has been minimized.

@ryanve

ryanve Feb 5, 2013

Contributor

@staabm I'm agreeable to either name. hasEventHandlerProperty seems more literal and less likely to be misconstrued.

This comment has been minimized.

@staabm

staabm Feb 5, 2013

hasEventHandlerProperty describes the technical fact/background and hasEventHandlerSupport describes the meaning and intension why it is used.

This comment has been minimized.

@stucox

stucox Feb 6, 2013

Member

canTestEventOnRootElement?

Edit: sorry, think I missed the point. canTestEventInElement?

This comment has been minimized.

@ryanve

ryanve Feb 7, 2013

Contributor

@staabm @stucox Let's just inverse it and call it needsFallback. That's the greater point of it. It'll be evident that it relates to isEventSupported because it's in the closure.

@ryanve

This comment has been minimized.

Contributor

ryanve commented Feb 7, 2013

Does this need any functional tweaks, or new tests? It's pretty solid now. Consider:

  • If there were no backwards compatibility concerns, then I'd omit the TAGNAMES lookup feature entirely b/c it has potential to give unexpected results and it's harder to document—the currents docs don't mention it anyway. The 2nd param lets users be explicit.
  • I replaced is() with typeof. Is there a protocol on that? typeof is more straightforward, performant, and it loosens coupling between the modules. If "is" is used, then I think it should be in the dependency array.
  • I'm curious as to whether nullifying element is needed. It doesn't hurt to leave it. The closure compiler removes that line whereas uglify keeps it.
@stucox

This comment has been minimized.

Member

stucox commented Feb 9, 2013

I think we should drop the TAGNAMES lookup. If we get this in before we release 3.0, then we can stick it in the release notes and backward compatibility isn't a problem - this is our chance to tidy up stuff like this. I've added this to the v3 milestone.

typeof works for me - I was never convinced by wrapping it, but @paulirish may disagree 👅 - I would make it a triple equals though.

I think we can lose the element = null. It's only needed if you attach events to the element, which we're not doing here. Turns out Closure Compiler is well clever and will work out if you're attaching an event, and will leave element = null if so... it should be safe to follow its lead.

@kangax

This comment has been minimized.

kangax commented Feb 9, 2013

If there were no backwards compatibility concerns, then I'd omit the TAGNAMES lookup feature entirely b/c it has potential to give unexpected results and it's harder to document—the currents docs don't mention it anyway. The 2nd param lets user be explicit.

Which unexpected results? If anything, it would seem that NOT using lookup would give unexpected results :)

The lookup is used to associate event name with element type. Certain elements implement certain events, so obviously in-based detection only works when you test event on a "proper" element. At least, that's how things were when I wrote the original function (wow, almost 4 years? time flies...)

I'm curious as to whether nullifying element is needed. It doesn't hurt to leave it. The closure compiler removes that line whereas uglify keeps it.

I added it originally as a safe precaution. But it's totally ok to drop it. There's no closure over an element here, so no chance of leak.

@ryanve

This comment has been minimized.

Contributor

ryanve commented Feb 10, 2013

@stucox Agreed. Version 3 sounds like perfect timing. Thanks. I'll update the points mentioned and wait a bit to see if we can come to a consensus about the dropping the TAGNAMES.

@ryanve

This comment has been minimized.

Contributor

ryanve commented Feb 10, 2013

@kangax Thanks for explaining =] By unexpected I mean that a dev using it may not know about the lookup or remember its details. They may wrongfully expect defaults for other events. It's easier to document as it defaults div than as it defaults to a div, except when this or that.

Omitting the lookup would help bulletproof against any possible usage, custom event names, or DOM changes. Something inherited like Modernizr.hasEvent('toString') would otherwise need an extra check or an Object.create(null) hash to be totally safe. I'd rather avoid that entirely.

@stucox

This comment has been minimized.

Member

stucox commented Feb 10, 2013

Agree with Ryan - the TAGNAMES behaviour might be "useful" but it's unexpected and hence unpredictable unless carefully documented; and raises the question of "how far do we go? do we pick appropriate elements for every event we currently know about?" - which we could do without.

@ryanve

This comment has been minimized.

Contributor

ryanve commented Feb 10, 2013

isEventSupported('click', {}) is false via in but true via the fallback b/c it reverts to a div. Do we want to normalize that like:

if ( !element.nodeType && element !== window )
    return false;
@stucox

This comment has been minimized.

Member

stucox commented Feb 11, 2013

Well that wouldn't be a documented use... the 2nd arg should either be an element or a tag name (string) - if someone wants to pass something else in, then behaviour's going to be unpredictable - so I wouldn't worry.

@ryanve

This comment has been minimized.

Contributor

ryanve commented Feb 12, 2013

@stucox Sounds fine. We could even drop the typeof element !== 'object' to loosen it further. I was erring on the safe side.

@stucox

This comment has been minimized.

Member

stucox commented Feb 12, 2013

Yep, sounds like a plan. Then we'll get this merged in :)

@stucox

This comment has been minimized.

Member

stucox commented Feb 12, 2013

@kangax - still reckon we should keep the TAGNAMES lookup? If so, we should document it.

@stucox

This comment has been minimized.

Member

stucox commented Feb 13, 2013

(but I still think it should go!)

@stucox stucox referenced this pull request Mar 23, 2013

Closed

v3.0 release notes #805

@stucox stucox merged commit 3991aea into Modernizr:master Mar 24, 2013

1 check passed

default The Travis build passed
Details
@ryanve

This comment has been minimized.

Contributor

ryanve commented Mar 27, 2013

@stucox Thanks, looking forward to 3.0 =)

patrickkettner pushed a commit to patrickkettner/Modernizr that referenced this pull request Feb 22, 2015

patrickkettner pushed a commit to patrickkettner/Modernizr that referenced this pull request Feb 22, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment