Skip to content
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

support Element.classList.add and Element.classList.remove #5913

Closed
pewgeuges opened this issue Dec 21, 2020 · 14 comments
Closed

support Element.classList.add and Element.classList.remove #5913

pewgeuges opened this issue Dec 21, 2020 · 14 comments
Assignees
Labels
Support Help with setup, implementation, or "How do I?" questions.

Comments

@pewgeuges
Copy link

The lean implementation of styled tooltips requires to add and remove classes for the shown and hidden status.

@Nantris
Copy link

Nantris commented Feb 24, 2021

Ah I figured these were supported since classList is listed as supported with no caveats. That's a bummer.

@kristoferbaxter
Copy link

This should be fully supported. DOMTokenLists are supported, and classList is one of them.

Can you provide a reproduction where it's not working?

@pewgeuges
Copy link
Author

@slapbox @kristoferbaxter The Footnotes plugin for WordPress has a hard links enabled mode since last year for AMP compatibilty, and alternative tooltips as well as an alternative reference container, both using plain JavaScript. The tooltips use this internal script:

<script content="text/javascript">
	function footnoteTooltipShow(footnoteTooltipId) {
		document.getElementById(footnoteTooltipId).classList.remove('hidden');
		document.getElementById(footnoteTooltipId).classList.add('shown');
	}
	function footnoteTooltipHide(footnoteTooltipId) {
		document.getElementById(footnoteTooltipId).classList.remove('shown');
		document.getElementById(footnoteTooltipId).classList.add('hidden');
	}
</script>

You see that the only functions used are classList.add() and classList.remove(). In spite of this simplicity and light-weight, the AMP plugin removes this script, (It also removes most of the CSS; the only remainder I can spot is .footnote_tooltip.hidden{visibility:hidden;opacity:0;transition-property:visibility opacity;transition-timing-function:linear}.)

To reproduce the issue, please install the plugin and set it to Enable alternative tooltips, and Script mode plain JavaScript. Enable hard links is automatic when Script mode is plain JavaScript. In this configuration, no libraries are loaded (neither jQuery Core, nor jQuery UI, all from WordPress, nor jQuery Tools shipped with the plugin). Looking at the page source I don’t see the document.getElementById(footnoteTooltipId).classList.add|remove functions. Moreover, the onmouseover|onmouseout events are removed alongside.

@kristoferbaxter kristoferbaxter transferred this issue from ampproject/worker-dom Feb 24, 2021
@kristoferbaxter
Copy link

Transferred this issue to amp-wp, since this doesn't have to do with the implementation of WorkerDOM, but rather in the conversion of a document using Wordpress Plugins.

@westonruter westonruter added the Support Help with setup, implementation, or "How do I?" questions. label Feb 24, 2021
@milindmore22
Copy link
Collaborator

Hello @pewgeuges

AMP pages do not allow custom javascript on AMP pages, so the AMP plugins remove them for you to produces valid AMP pages. You can however use amp-script component to use javascript on AMP pages with few restrictions

In your case I don't see why we need javascript to toggle footnote, this can be also achieved using a simple CSS tweak.

goto -> Dashboard-> Appearance -> Customize -> Addtional CSS -> paste bewow CSS

html[amp] span.footnote_referrer:hover > span.footnote_tooltip {
	visibility:visible;
	opacity:1;
}

let me know if this resolves your issue

@pewgeuges
Copy link
Author

pewgeuges commented Feb 24, 2021

In your case I don't see why we need javascript to toggle footnote, this can be also achieved using a simple CSS tweak.

goto -> Dashboard-> Appearance -> Customize -> Addtional CSS -> paste bewow CSS

html[amp] span.footnote_referrer:hover > span.footnote_tooltip {
	visibility:visible;
	opacity:1;
}

let me know if this resolves your issue

@milindmore22,

This is fabulous! Thanks a lot! No need for JS to display fully fledged tooltips!

The only change needed was remove the root element and its attribute selector, as even when using an argument present it failed. The now working is:

span.footnote_referrer:hover > span.footnote_tooltip {
	visibility:visible;
	opacity:1;
}

And the new issue is that fade-out parameters for delay and duration are now applied symmetrically to fade-in.

@pewgeuges
Copy link
Author

pewgeuges commented Feb 24, 2021

@milindmore22,

Solved also with asymmetric transitions. The rules are now:

span.footnote_referrer > span.footnote_tooltip {
	visibility: hidden;
	opacity: 0;
	transition-property: visibility, opacity;
	transition-delay: 500ms;
	transition-duration: 1s;
}
span.footnote_referrer:hover > span.footnote_tooltip {
	visibility: visible;
	opacity: 1;
	transition-property: visibility, opacity;
	transition-delay: 0;
	transition-duration: 200ms;
}

We’ll change the internal CSS accordingly (where values come in from settings).

Thank you very much for helping make the plugin AMP compatible!

@pewgeuges
Copy link
Author

Just a last thing @milindmore22: We acknowledge everyone contributing code or reporting issues. May we list you among the contributors, credit you in the changelog, and link the issue in a docblock or two? (Having hyperlinks in the changelog is also on the table.) (Our issue #‌41 is to complete the doc thoroughly.)

@westonruter
Copy link
Member

Once you've released the AMP-compatible version on WordPress.org, please let us know and we'll test and then add to our AMP-compatible plugin ecosystem directory: https://amp-wp.org/ecosystem/plugins/

@westonruter
Copy link
Member

westonruter commented Feb 24, 2021

@pewgeuges Also, in regards to :hover don't forget about ensuring it is accessible for users who are not using a mouse (or are on mobile). For them, you should probably also make use of :focus or :focus-within.

@pewgeuges
Copy link
Author

Once you've released the AMP-compatible version on WordPress.org, please let us know and we'll test and then add to our AMP-compatible plugin ecosystem directory: https://amp-wp.org/ecosystem/plugins/

Yes, it will be a pleasure, Thank you @westonruter!

@pewgeuges
Copy link
Author

pewgeuges commented Feb 24, 2021

@pewgeuges Also, in regards to :hover don't forget about ensuring it is accessible for users who are not using a mouse (or are on mobile). For the, you should probably also make use of :focus or :focus-within.

@westonruter Thanks a lot! I’ll dig into these pseudo-classes. FWIW with jQuery tooltips I can tap-and-hold a referrer, then the tooltip shows up on mobile. Hopefully :focus or :focus-within will perform the same — w/o JS!

@kristoferbaxter
Copy link

Makes me so happy to see such a great outcome. Thanks all!

@milindmore22
Copy link
Collaborator

@pewgeuges Glad to know the solution helped, feel free to reach us on WordPress Support Forums and we will more than happy to assist you further.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Support Help with setup, implementation, or "How do I?" questions.
Projects
None yet
Development

No branches or pull requests

5 participants