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

contentWrapper breaks CSS rules and videos #207

Open
alterphp opened this Issue Sep 10, 2018 · 8 comments

Comments

Projects
None yet
5 participants
@alterphp
Copy link

alterphp commented Sep 10, 2018

Hi,

I've this strange behavior when integrating TarteAuCitron : my <video> on homepage is frozen and some CSS rules are broken...

I've finally found a culprit : div#contentWrapper that embeds all html of the body by TarteAuCitron... Is the only reason of that wrapper the aria-hidden attribute ?

@AmauriC

This comment has been minimized.

Copy link
Owner

AmauriC commented Sep 10, 2018

Yes, this is a part of the last accessibility update.

This can be an ID conflict. Send me the url at amauri@wpmobile.app and I will have a look.

@alterphp

This comment has been minimized.

Copy link
Author

alterphp commented Sep 10, 2018

I changed the ID in tarteaucitron.js to check => same result.

It's clearly the fact to move every DOM node of the <body> that causes the problem. I don't know the problem for the <video> tags, but for CSS it's clear that every rules like body > * will fail with this wrapper.

@rubycon

This comment has been minimized.

Copy link

rubycon commented Sep 20, 2018

@AmauriC, wrapping all the body content implicitly is not great! It can breaks CSS selectors, such as body > header or body > * in stylesheets and JS selector query (I suppose it's the issue with <video>). You shouldn't assume how the user's gonna use his selectors.

For now, a better way could be to make this accessibility feature an opt-in parameter.

Also contentWrapper is a pretty common id name, better to namespaced it (e.g. tarteaucitronBodyWrapper) or/and let the user set it as a parameter.

@EmmaNatales

This comment has been minimized.

Copy link

EmmaNatales commented Oct 3, 2018

Bonjour et merci beaucoup pour tarte au citron ;-)
J'ai aussi un problème avec ce 'contentWrapper', j'ai des animations css sur une page (des blocs qui apparaissent en bougeant) et le fait de déplacer tout le contenu du body dans ce div rejoue l'animation.
J'ai viré le code du contentWrapper et plus de problème.

@AmauriC AmauriC closed this in c31b2a1 Oct 3, 2018

@AmauriC

This comment has been minimized.

Copy link
Owner

AmauriC commented Oct 3, 2018

Ce contentWrapper pose beaucoup de problème.. je viens de la supprimer.

Si quelqu'un à un conseil pour garder un haut niveau d'accessibilité en ne cassant pas la structure du site, ne pas hésiter 👍

@AmauriC AmauriC reopened this Oct 3, 2018

@alterphp

This comment has been minimized.

Copy link
Author

alterphp commented Oct 3, 2018

Si j'ai bien compris, le contentWrapper servait à encapsuler tous les blocs sous la balise <body> pour leur ajouter l'attribut aria-hidden. Le container de tarteaucitron est ajouté ensuite en fin de <body>;

Puisqu'on ne doit pas changer la hiérarchie des éléments du DOM, je propose le fonctionnement suivant :

Affichage de tarteaucitron

  • Pour chaque élément enfant de premier niveau de la balise <body> :
    • S'il n'a pas déjà l'attribut aria-hidden à true => on lui ajoute l'attribut aria-hidden="true" + une classe nous indiquant que l'attribut a été ajouté par TarteAuCitron (ex: tac_original_content_hidden)
  • Afficher TarteAuCitron (comme actuellement)

Dissimuler TarteAuCitron

  • Cacher les éléments TarteAuCitron
  • Retirer l'attribut aria-hidden aux éléments de la classe tac_original_content_hidden ainsi que la classe elle-même.
@rubycon

This comment has been minimized.

Copy link

rubycon commented Oct 3, 2018

La solution proposée par @alterphp est simple et sans inconvénients. Par contre je pense qu'utiliser un data attribute au lieu d'une classe (element.dataset['tarteaucitron-hidden'] = true;) pour identifier les éléments modifiés serait moins invasif.

Cependant la fonctionnalité d'accessibilité devrait aussi être modifiable depuis tarteaucitron.init().

@stevenMouret

This comment has been minimized.

Copy link

stevenMouret commented Oct 3, 2018

The best way is to not use aria-hidden but a dialog role.

Here the Dialog (modal) design pattern.
And an example.

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