nico3333fr / jquery-accessible-accordion-aria

jQuery Accessible Accordion System, using ARIA
MIT License
60 stars 19 forks source link

Copy the html instead of text from header to button ? #21

Closed oilvier closed 7 years ago

oilvier commented 7 years ago

Hi,

I was just wondering if there's a reason to copy the text content of the header to the button, instead of the HTML ?

var $button = this.options.button.clone().text($header.text());

It prevents the potential styling applied on the original header element (like the use of tags (strong, span, etc...) or graphics (SVG icons)) to be replicated to the new button element.

But maybe there's an issue I don't see in keeping the HTML ?

nico3333fr commented 7 years ago

Hi,

to be honest, it is more a habit coming from my work: I don't use often font icons, and most of the time I prefer styling directly on the button generated. :)

The only risk that I see is to have bad-formed HTML in the button. II guess I will have to test if there is a difference, and if not, maybe it could be an option of the plugin ? (tell me what you think about it)

Cheers, Nicolas

oilvier commented 7 years ago

Yes, it could be an option like "generatedContent: text / html"

kotyy commented 7 years ago

I've been using this plugin in a lot of sites recently. Thank you for developing and maintaining it. It certainly makes my life easier. :)

I'd like to request this feature, too. I can't think of any reason this shouldn't be allowed from a accessibility or semantics perspective, and the spec does allow for phrasing content within buttons.

Changing the line mentioned in oliver's comment, however introduces another problem. clickButtonEventHandler assumes that the target is the button element. If you click on an element within a button, it is the target.

My quick fix was to modify the first line of clickButtonHandler to be

var $target = $(e.target);
var $button = $target.is('button') ? $target : $target.parent();
chrisgrabinski commented 7 years ago

@kotyy Thanks for this great solution. I tried it out and ran into one more, tiny problem that might occur if – for example – you were to use encapsulated phrasing content (i.e. span within span). It's ugly but some designs might call for it. :(

accordion-example

Using jQuery's .closest() instead of .parent() makes sure that the button will always be targeted by the clickButtonHandler.

var $target = $(e.target);
var $button = $target.is('button') ? $target : $target.closest('button');
nico3333fr commented 7 years ago

This is great, I'm finishing some tests and the fix will be release this week-end, thanks to all of you. You're all great 👍

nico3333fr commented 7 years ago

Hi there,

the issue has been fixed in https://github.com/nico3333fr/jquery-accessible-accordion-aria/commit/1607dbfe447fbae053d7a3f1dd17085b8152f022 .

The documentation has been updated https://a11y.nicolas-hoffmann.net/accordion/ (also in Readme with a special example ;) https://github.com/nico3333fr/jquery-accessible-accordion-aria/blob/master/README.md#all-options-of-the-plugin )

Thanks all for your help on it, really appreciated. 👍