google / site-kit-wp

Site Kit is a one-stop solution for WordPress users to use everything Google has to offer to make them successful on the web.
https://sitekit.withgoogle.com
Apache License 2.0
1.23k stars 284 forks source link

Admin menu tooltip does not work properly on mobile as of WP 6.4 #7738

Closed aaemnnosttv closed 10 months ago

aaemnnosttv commented 11 months ago

Bug Description

As recently raised by a failing E2E test in WP nightly, the admin menu tooltip we show in various places (usually when a user dismisses a CTA) to highlight Site Kit's settings page in the admin menu no longer appears.

Steps to reproduce

  1. Set up Site Kit on WP 6.4 (currently RC2)
  2. In a mobile viewport (e.g. width 375px) click the Maybe Later link of the AdSense setup CTA widget
  3. Notice no admin tooltip is displayed

Screenshots

image

Additional Context


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Implementation Brief

This is easily fixable for recent versions of the evergreen browsers that we support (I did some pretty thorough cross-browser testing while investigating this IB, and we should also ensure that all of our supported browsers are given a good test during QA). There is a complication when it comes to our test stack though, as the menu won't show as expected in the version of Chromium (97.0.4691.0) that our current Puppeteer version uses. For some reason, the click events that are dispatched during the test are handled in a different order on this version of Chromium compared to recent browser versions.

Seeing as we have no requirement to support this old version of Chromium, we should apply the fix, and update Puppeteer to a version which provides a more recent Chromium version where it does work.

Within assets/js/components/AdminMenuTooltip/useShowTooltip.js:

Update Puppeteer version:

Within tests/e2e/specs/modules/adsense/show-admin-menu-tooltip.test.js:

There is a PoC PR which may be continued.

Test Coverage

QA Brief

  1. Set up Site Kit on WP 6.4
  2. In a mobile viewport (e.g. width 375px) click the Maybe Later link of the AdSense setup CTA widget
  3. The admin menu/tooltip menu on the left-hand-side should be displayed

This is just one example of the tooltip which is used in a few places such as Key Metrics CTA, ABR CTA, and Enhanced Measurement notification CTAs.

Changelog entry

tofumatt commented 10 months ago

IB ✅

Thanks for all of the context in the IB here, @techanvil 👍🏻

aaemnnosttv commented 10 months ago

@techanvil @tofumatt E2E seems to be consistently failing with these now, it might only be from the upgrade to Puppeteer but I'm not sure – I don't see how the change to the tooltip we made would have broken these. Please take another look and let's open a follow-up issue for this so long as the AC is still met here.

image
techanvil commented 10 months ago

Thanks for spotting that @aaemnnosttv. It's a bit of an odd one, as these tests weren't failing for Latest/Nightly on the E2E test run for the PR. But, in my testing, with the tests failing locally, reverting to the previously used version of Puppeteer does fix them. I've created https://github.com/google/site-kit-wp/issues/7862 to address this.

wpdarren commented 10 months ago

QA Update: ⚠️

@techanvil the tooltip is now appearing on mobile. I wonder if I am too picky, though 😄 The tooltip feels a few pixels lower than the 'Settings' menu option. I have included screenshots below and wondered what you think. It's possible it was always like this, and I haven't noticed before.

techanvil commented 10 months ago

Hi @wpdarren, that is indeed how it's always been - here it is on 1.111.0. The tooltip is pointing to the Settings menu item element with its default spacing, it's just the menu item has a bit of padding on it, resulting in the tooltip position being slightly further out than might be expected.

We can probably improve the position of the tooltip - please feel free to create a separate issue for this, but it's out of scope for the current one.

image

wpdarren commented 10 months ago

QA Update: ✅

Verified:

Screenshots ![image](https://github.com/google/site-kit-wp/assets/73545194/47296457-d7c8-4820-b70a-608425d8e7fb) ![image](https://github.com/google/site-kit-wp/assets/73545194/5f835374-cf90-4a60-ab84-875c95918da5) ![image](https://github.com/google/site-kit-wp/assets/73545194/95af52a9-2512-4e5a-89d3-e1c7da8fd40d) ![image](https://github.com/google/site-kit-wp/assets/73545194/a0488d53-1024-4509-a982-59074955aa2b) ![image](https://github.com/google/site-kit-wp/assets/73545194/52ae8797-63c6-4574-afc6-8f711223c2cb)