frend / frend.co

Frend — A collection of accessible, modern front-end components.
http://frend.co
MIT License
635 stars 25 forks source link

SVG in off-canvas buttons #76

Closed yellowled closed 8 years ago

yellowled commented 8 years ago

Using SVG icon sprites in the button opening or closing the off-canvas component produces a JS error.

Markup used for the button:

<button class="open-nav" type="button" aria-controls="site-nav"><svg class="icon-bars" role="img" title="Open" viewbox="0 0 1792 1792" width="1792" height="1792"><use xlink:href="img/sprite.svg#bars"></use></svg></button>

If the mouse click is on the part of the button that is larger than the SVG icon (due to i.e. padding), it works, but clicking on the actual icon produces a JS error in the console:

froffcanvas.min.js:1 Uncaught TypeError: Cannot read property 'style' of null`

Toggling the button with the keyboard works just fine. Same example works just fine with button text.

See http://s9y.netzgestaltung.net (currently) for a live example.

thomasdigby commented 8 years ago

Thanks @yellowled, I'll look into it as soon as poss. I have absolutely no idea why that would affect the script at all...

yellowled commented 8 years ago

Well, there's only two instances of style in the unminified offcanvas.js, both trying to set the visibility of panel. So I would assume that panel is null for whatever reason when showPanel is called. As far as I see, in that case, panel is ultimately determined by e.target.getAttribute('aria-controls') in _eventOpenPointer.

I'm no JS expert, but could it be that the script just does not take into account that there could be another element inside button? So that maybe the event trickles down to the <svg>, which doesn't have an attribute aria-controls, resulting in panel being null? (I'm really no JS expert, as you can probably tell by now. :D)

yellowled commented 8 years ago

Also possibly interesting: for the close button (which does not have an aria-controls attribute), it does not matter where the click is placed.

thomasdigby commented 8 years ago

Yep you're probably right, I think we employ a method on another component to detect elements within the button so it's probably an easy fix. I'll investigate what's going on with the close button as well.

yellowled commented 8 years ago

Appreciate it. Just to avoid misunderstanding, the close button works as intended even with an <svg> inside.

adamduncan commented 8 years ago

This happens when the element that has the event bound to it - in this case, the close button - has child elements.

To ensure the click event bubbles up to the correct element, we can use e.currentTarget instead of e.target

Actually - should double check that other components aren't affected too. Can imagine it's not unlikely that things like tabs and accordion headers couldn't have span or i elements nested within them.

thomasdigby commented 8 years ago

78 @adamduncan

thomasdigby commented 8 years ago

This fix should be up on NPM and merged into the master branch.

@yellowled Let me know if you have any issues after trying this fix.

yellowled commented 8 years ago

@thomasdigby Already tested this, works like a charm. 👍