Create a new component: Toggletip

1. Should non-js users be able to access these tips? I don't think they are essential so maybe it's fine, but we need to be aware that non-js users might not be able to access these.
2. I think we should move the attribute conversion outside from \Drupal\Core\Render\Renderer::doRender because it feels that \Drupal\Core\Render\Renderer::doRender is too low level code for it to support this. Could this be moved to template_preprocess?
3. The toggle isn't vertically aligned to middle on headings
   😅
4. The positioning should probably take displace into account:
   

1. Should non-js users be able to access these tips? I don't think they are essential so maybe it's fine, but we need to be aware that non-js users might not be able to access these.

   I'm thinking no. Perhaps the eventual documentation for this can include a mention that system-critical information should not be provided in the toggletip (it really shouldn't anyway)

2. I think we should move the attribute conversion outside from \Drupal\Core\Render\Renderer::doRender because it feels that \Drupal\Core\Render\Renderer::doRender is too low level code for it to support this. Could this be moved to template_preprocess?

   I moved it. This also required a modification to the html_tag render element since it doesn't uses a template, but it does seem like a more appropriate place than the low level Render.php

3. The toggle isn't vertically aligned to middle on headings

   This was pretty tricky, especially making sure it works when the text wraps, but I think it's in good shape.

4. The positioning should probably take displace into account:

   Top displace was supported already, and I added side support. That was tricky! I also added positioning options for the toggle button

I'm sure there's more to do, but pushing what I have so far so folks can have a look.

Anyone wanting to give this a spin can enable the toggletip_test module and go to /toggletip-test/toggletips. It's a test module, so $settings['extension_discovery_scan_tests'] = TRUE; must be configured.

I'm switching this to "Needs Review" to make it easier to find, but it's probably more accurate to say this needs architectural review as there aren't tests etc. The functionality works and should be evaluated to what parts we keep and what should be changed. Instructions how to see tooltips locally at the beginning of the issue summary, and a video of them in action can be found here: https://youtu.be/8l6cEVwgGXw

At this moment

• **YES** Reviewing the approach, design, implementation details, etc
• **NO** Reviewing as if this is a candidate for RTBC. This is a work in progress and we're not there yet. We don't need screenshots, code optimizations, typo correction, etc until there's more agreement on how we move forward

1. Links in Tooltip: should be yellow as in #3248239: Links in the tour tip body are visually the same as the rest of the text - Claro theme → to have enough contrast. Until we get the yellow palette, the color to use here is --color-sunglow (#ffd23f).
   
2. Not adding a max-length results on really long lines of text. The legibility of this text is really bad, so I would suggest adding a max-with setting by default. Since this is using a 14px font-size, and the maximum legible line-length is around 80 characters, I would suggest a length of 35rem/560px.


   Also, to compensate long first lines, I would suggest using text-wrap: balance;.

As discussed in Slack, I would recommend to use an icon instead of using the "i" character. The problem on a UI/design perspective is that it isn’t really recognizable, we shouldn’t rely on the font from the system for understandable icons. so here's an SVG free of any copyright or license.

Both items in #16 are addressed and most items in the MR. I'd prefer to wait on the theme functions until things are further along so iterating is easier as we discover new things.

Also tagging with needs tests. There's a test module already built with many toggletip use cases that tests can use.

Setting back to needs review, fully aware that at the very least theme functions and tests must be added, but making this available again for architectural review to see if any interesting feedback comes in before a day is spent on tests.

Been back n forth to this ticket. One tooltip I think would be the format tags on a text editor. People love to complain about that list of tags at the bottom, this would just make it better but still be there.

Maybe should be tagged for accessibility as well.

Should we be looking to leverage the popover API here with this polyfill for browsers yet to support it? https://github.com/oddbird/popover-polyfill

@larowlan re #21 I think this is a good route. Seems like this will be standard pretty soon. It looks like the popover API already works in most supported browsers and the Firefox support is far enough along that it can be enabled via the dev console. The one thing we'd need to check for is that the Polyfill works for Opera, as the repo does not specify it, and that appears to be the furthest one out from introducing the feature.

It's tempting to also use the CSS Anchor Positioning polyfill which seems like a nice way to handle positioning, but it seems a little too early to be sure the spec will be widely adopted in its current form. Figured I'd mention it on the off chance that this issue takes long enough to get in that there's better browser support for this (believe it or not some Drupal issues take a while 😛) Floating UI is a pretty solid library in the meantime.

I'm just putting forward this example from the USWDS. It's better than most I've tested:
https://designsystem.digital.gov/components/tooltip/

That said, I'm looking for an explicit example like I've called for here:
https://github.com/uswds/uswds/issues/5343

I don't know how often buttons stand in isolation with tooltips.

JS tests have been added. Looks like there's something wrong with the right, right-start, right-end toggle tips so the test is expected to fail on those assertions right now

tim.plunkett → made their first commit to this issue’s fork.

Did a small test with the test module. This one tooltip seemed off.

Think the failure was random so restarted the tests to be sure.

But would this be merged as is and follow ups made?

Failure did appear to be random.

I'm confused whether this is now a tooltip or a toggletip, everyone seems to be talking about tooltip including the issue title but the code is toggletip?

Also, doesn't this require documentation and a change record on how to use this?

I think we need to guard against XSS here, left some comments on the MR

Appears threads have been addressed so moving back to RTBC.

Posted comments on the MR

This is hitting #3211992: TestSiteApplicationTest::testInstallWithNonExistingFile() fails when another test creates database tables during the test run → which was supposedly fixed but was not

Looks like all the feedback has been addressed now. Waiting for 🐛 TestSiteApplicationTest::testInstallWithNonExistingFile() fails when another test creates database tables during the test run Fixed to go in that address the last test fail.

Tests passed hooray

I think the CR could still use some more information about what has been added, and how to use it.

Tagging for JavaScript subsystem maintainer review since this is adding a new JavaScript API.

Updated the CR.

Nice to see this getting so close to the final line, but I'd love to see a few more changes on the CSS on the line of 🌱 [PLAN] Drupal CSS Modernization Initiative Active to start using modern CSS in core.
Some thing I would restructure on that sense would be:

• Use units strategically, switching to use rem instead of px whenever possible. Since core/misc/toggletip.css is not using PostCSS here so we want to ensure the final CSS keeps the right unit we need to define it directly.
• Define per component CSS Custom Properties (aka CSS variables). Things like colors could be defined at the beginning of the document for this component, which would let other themes override it easily.

Few comments on the MR

I see some places where we can improve it. Gonna work on it.

Hello!
Great initiative!

For my part, I want to note that I see two components here...

The first is a button with the letter "I" which is added to Drupal elements in different positions and which depends on another component

The second component is a completely independent Tooltip. To call it, the minimum requirement is a couple of attributes:

data-drupal-tooltip-toggle-button="Tooltip Text"
popovertarget="very_unical_id"

Which can be used by developers for example of the Navigation module:
https://www.drupal.org/project/navigation/issues/3393067 ✨ Added tooltip component. Needs review

As additional attributes I added:
- [data-drupal-tooltip-placement] from https://floating-ui.com/docs/computePosition#placement-1
- [data-drupal-tooltip-on-hover]

We also need to add:
- [data-drupal-tooltip-hide-arrow]
- [data-drupal-tooltip-extra-css-class]

I think this will make the component more flexible and allow for simple evolution when we want to use for example:
https://developer.chrome.com/blog/tether-elements-to-each-other-with-css...

I added some examples in core/modules/system/tests/modules/toggletip_test/src/Form/ToggletipForm.php

Everything is working:
https://gyazo.com/eff3ddefa40ae15908de552819c75d7d

Please take a look!

Gauravvvv → made their first commit to this issue’s fork.

bnjmnm → closed merge request !3854

bnjmnm → closed merge request !5024

I like the idea for an additional tooltip component alongside the toggletip being built here, but it shouldn't be added within the scope of this issue for two significant reasons:

• Before work even started on this, we confirmed there is an accessible way to implement toggletips. This isn't the case for tooltips, and in some cases the recommendation to make truly accessible tooltips was to use toggletips instead
• Even if a fully accessible tooltip approach was discovered, it is best implemented in a separate issue. I's not necessary to introduce them at the same time, and keeping them separate means less of a review burden and avoids the scenario where one render element is fine but the issue can't land because there are concerns about the other one.

I closed the MR to avoid additional work going into something that, while a good idea, should not be happening in the scope of this issue. The branch still exists, though, and can be used to port the code to a dedicated tooltip issue.

@bnjmnm
Hello!

I can't see problem here if we build it with 2 components still.
Even if it will be used only as toggle tip only. I can provide MR with same functional but splitted. And without extra features. Then later it will be easier to switch. What you think?

I can't see problem here if we build it with 2 components still.

The problem with providing two components in this issue instead of the one proposed is

• Scope creep
• Adding a component that has not been vetted as accessible

Maybe I explained it wrong.

But:

1. Positioning a button and positioning a tooltip are different tasks in Javascript. And they should not be in one monolithic script.

2. From the point of view of BEM, these styles are also easily separated.

A11y has absolutely nothing to do with it

These components are still in one package. So scope not a reason aswell.

I just pushed tooltip without hover attribute. I still think that placement needed.

@bnjmnm can you please take a look?

How it affects a11y and scope?

Did you check this MR?

Admittedly I didn't get very far as I saw the addition of things labeled "tooltip" when we are decidedly going the toggletip route, not tooltip. For more on that distinction, this article explains it pretty well. Since you're apparently not adding a tooltip per that understanding of it, this can be looked into further - but if this is determined to be a good approach the naming should be something other than "tooltip".

@bnjmnm Thank you for article link. Sorry but i still see no reason to merge `i` button positioning with tooltip component.

Sorry but i still see no reason to merge `i` button positioning with tooltip component.

I haven't stated a position on the merging / separation of these components one way or the other so no apology needed. Any reservations expressed thus far are specific to the terminology "tooltip", which should not be used as that is not what is being created here. This is hiding/showing additional information via a dedicated disclosure button, which makes it a toggletip. A tooltip would be something that offers additional info that is triggered by interacting with an element/text on the page that has a purpose outside of being a disclosure for the tip content. Something like tipTrigger and tipContent would be more accurate labels for what I think is being attempted here.

Yes. Sorry for the misunderstanding.

This new component may be called
WhatLooksLikeTooltipButNotTooltip
or the more neutral "Drupal tooltip"

But the most important things that are obvious to me:

1. The component name should not be a blocking issue.

2. "If it looks like a duck, swims like a duck, and quacks like a duck, then it probably is a duck." so maybe the term tooltip will be more understandable for developers and users.

3. Positioning the trigger is a separate task for the script and styles.

That is why I believe that we can safely split this large monolithic script into something simpler, more obvious, component-based and better supported in the future.

The splitting itself makes sense since the toggle tips consists of two parts, the button that user can click and the box that displays the message. I agree with @bnjmnm that tooltip as a name is confusing since it's often used to refer the behavior where user hovers over an element and a box with a message appears. Maybe we could try to rename the component into something that would only describe the box with a message instead of the tooltip behavior? The first idea that comes in my mind is "arrowBox" since it's essentially a box with an arrow. If we ever end up creating a tooltip component, it could reuse the arrow box component. Thoughts? 😊

Name not critical for me if it will be possible to use it somewhere outside of circle buttons logic and styles

Good names IMO.

InfoButton
Tip

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

RE: #65

laurii i think arrowbox not cool. because
i believe in can be triggered with optional arrow. as i suggested in navigation module usage.
So now we have T...tip :)

As suggested i renamed Tooltip to Tip.

ToggleTip now controls position of `i` button
Tip is only about positioning and style of T..tip

Seems logical to me.

Navigation module is in core now. https://www.drupal.org/node/3438895 →

It has same tooltip on board. And it works and looks more or less same.
So here we can move `tTip` outside of Navigation and add extra attribute [data-drupal-tooltip-on-focus] or something to cover both cases.

Popover API is now part of baseline https://web.dev/blog/popover-api 🙌

finnsky → changed the visibility of the branch toggletip-11x to hidden.

finnsky → changed the visibility of the branch 3197758-create-a-new to hidden.

Hidden outdated branches.

Some issues with RTL, but it close to finish now.

Now seems all works as expected.
Please review.