Instant Search: change visual on close button

[keoshi]

Addresses issue reported in #14616

Changes proposed in this Pull Request:

This PR originated from an issue reported in #14616 that mentioned the close button was placed behind the system scrollbar. This happens because we're applying overflow-y: hidden on the body and adding overflow on the overlay itself.

The design team weighed in and the solution here is to get rid of the background-color on the close button, and to simply have a floating ×. As a nice side-effect, this should give more focus to the content itself since the close button, depending on the color applied, could demand too much attention in some cases.

Here's the breakdown of all changes:

• Transforms close button into a selectable a.
• Moves the close button closer to the content on larger screens.
• Updates styling.
• Removes all “close button” options from the customizer.

Light theme:

Dark theme:

Placement reference on desktop:

Is this a new feature or does it add/remove features to an existing part of Jetpack?

• No.

Testing instructions:

1. Follow these setup instructions to set up Jetpack Instant Search on your site.
2. Search your site to trigger the Jetpack Search overlay.
3. Ensure that the Jetpack Search close button works as expected, can be selected by tabbing to it, and that it's accessible.

Proposed changelog entry for your changes:

• None.

[jeffgolenski]

@keoshi This is gunna sound crazy, but on larger screens (15 inch macbook) the X icon is way far away. I'm wondering if it's too far. If I sound crazy, ignore me. Otherwise, I'm wondering if there's a way to max the distance from the search results / widgets?

proposed (question mark)

More importantly, the div looks good, but it's not visually selectable when I use tab to navigate around

[jsnmoon]

I like the rationale for the change. However, it looks like the close button becomes invisible in two scenarios:

• When using the dark theme (screenshot omitted)
• On viewports narrower than the 768 breakpoint:

I also agree with @jeffgolenski's suggestion on bringing the close button closer to the sidebar area.

[jsnmoon]

Nit: Changing a div to button, even when given a button role, decreases accessibility. Are we doing this to avoid styling issues from the theme?

[keoshi]

Yes, exactly. Since almost all themes include default styling for the button element it would be very hard for us to target it in order to remove all styling.

For example, Twenty Twenty targets it as button:not(.toggle) which is highly specific and nearly impossible to target, even with a narrow button.jetpack-instant-search__overlay-close.

To be clear though, I've transformed it into a div because that's what my skills allowed me to do. I imagine a link (a) would be a better fit, but it requires more wiring and functions that I could do on friday.

[keoshi]

@jeffgolenski Great feedback! I'll try to see if I can include the close button on a wrapping div. As for

More importantly, the div looks good, but it's not visually selectable when I use tab to navigate around

You should be able to select it by navigating with the tab key; that's what this line is for:

https://github.com/Automattic/jetpack/pull/14680/files#diff-d5fcdcd6ea017c86b19f22b381ef4001R45

[keoshi]

@jsnmoon Yep, I still have to add styles for the dark variant. Thanks for catching all those scenarios, will get started on those right away.

[keoshi]

@jsnmoon Updated this PR but I couldn't do it all so it still needs some work. I brought in the close button from overlay.jsx to search-results.jsx to more easily control its position, but that means we have to move the event listeners too along with the closeOverlay functionality.

I've added a temporary placeholder here so I could test it without errors: https://github.com/Automattic/jetpack/pull/14680/files#diff-577c3009c4d36d28fd2e71d0adb283d8R146

Let me know if this is an efficient use of both of our time, or if you'd prefer to handle these on your own. And apologies in advance if the code is not up to par.

[folletto]

Seems good. Specifically to this PR, just one thing: any reason to not color the × of the same color of clickable links? It seems it would differentiate it enough, and contrast should always be ok as it's the same as text.

---

More broadly... I know we are still working on this, but I want to make sure we don't ignore it: I think we need to be a better in terms of grid here. The alignments are really all over and it seems it would benefit from a better grid to put in earlier :)

[keoshi]

@folletto

any reason to not color the × of the same color of clickable links?

That's what I'm hoping @jsnmoon can do by using a regular link as discussed here. The only problem there would be ensuring solid contrast with the background on both our light and dark themes on the overlay, which could be an issue.

I think we need to be a better in terms of grid here. The alignments are really all over and it seems it would benefit from a better grid to put in earlier :)

Agreed, but this is a side-effect of inheriting so many styles and markup from the theme itself. The grid is all broken in your screenshot, but it might look good on a different theme. Unfortunately there's no way to fully control that unless we use 100% custom markup and styles.

By the way, the position of the close button was done purposefully so half of it is always placed outside the content; see screenshot on the OP.

[folletto]

The only problem there would be ensuring solid contrast with the background on both our light and dark themes on the overlay, which could be an issue.

Sure, but if it's the same color as the link text...

Agreed, but this is a side-effect of inheriting so many styles and markup from the theme itself. The grid is all broken in your screenshot, but it might look good on a different theme. Unfortunately there's no way to fully control that unless we use 100% custom markup and styles.

I'm a bit perplexed. If we take for example the top area and make a full witdh piece, so the sidebar doesn't go as high up, then at least that alignment is preserved and would work regardless of the theme, no?

What are our reference themes to make sure it works well on these?

[keoshi]

Sure, but if it's the same color as the link text...

The link color is optimized for a single background color more often than not. Take Twenty Seventeen for example: the link color is black because it was only considered for its white background and that works nicely with our JPS “Light” theme. However, when the JPS “Dark” theme is applied, the cross is virtually invisible. See #14666 for a related issue and possible solution that doesn't require manual overrides.

I'm a bit perplexed. If we take for example the top area and make a full witdh piece, so the sidebar doesn't go as high up, then at least that alignment is preserved and would work regardless of the theme, no?

Yep, there's more we can do on that front and we'll do it.

What are our reference themes to make sure it works well on these?

Our solution should ideally be universal as advertised. So with that in mind I'm personally testing changes on ~15 themes, including the latest Twenty * and some of the more popular ones on .org. But with JPS audience in mind, I'd say we're more likely to see it being used with custom themes.

[jsnmoon]

Updated this PR but I couldn't do it all so it still needs some work. I brought in the close button from overlay.jsx to search-results.jsx to more easily control its position, but that means we have to move the event listeners too along with the closeOverlay functionality.

Let me know if this is an efficient use of both of our time, or if you'd prefer to handle these on your own. And apologies in advance if the code is not up to par. @keoshi

Your work made it super easy for me to bind the necessary functionality, thanks!

I went ahead and pushed two commits: One for hooking up the close overlay function to SearchResults and another for changing the div into an anchor.

[keoshi]

@jsnmoon Thanks for hooking everything up! Works as expected, merging now.