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

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

Closed
aaemnnosttv opened this issue Oct 24, 2023 · 6 comments
Closed

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

aaemnnosttv opened this issue Oct 24, 2023 · 6 comments
Labels
P0 High priority Type: Bug Something isn't working

Comments

@aaemnnosttv
Copy link
Collaborator

aaemnnosttv commented Oct 24, 2023

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

  • WP 6.4

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

Acceptance criteria

  • The admin menu tooltip should work on both mobile and desktop on all supported versions of WordPress, including the soon to be latest 6.4

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:

  • Change the call to adminMenuToggle.click() to instead invoke the click on adminMenuToggle.firstChild.
    • This will ensure the menu isn't subsequently closed in WordPress' new document click handler, as event.target will match the #wp-admin-bar-menu-toggle contains check.
    • Invoking the click on adminMenuToggle.firstChild should be a backward-compatible change across all supported WP versions, so it shouldn't need to be conditional.
  • Update the hack we introduced for 6.2 so it's only applied for WP versions >=6.2 and <6.4, as it's no longer needed or correct with the removal of the related focusout handler in 6.4.

Update Puppeteer version:

  • Update the version of Puppeteer to 13.1.0 or above. This is the earliest version with a working Chromium version and was tested during the IB investigation with a PoC to verify it works with our CI workflows, however a higher version could be installed if it works OK. The Chromium version that comes with this version of Puppeteer is 98.0.4758.0.

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

  • Fix the failing test in headless mode.
    • Please note the described fix does work with the above version of Chromium, however while the interactive test passes immediately (and it doesn't without the update), the headless test still fails. So a bit of work is still needed to fix the headless test, but in theory it should work.

There is a PoC PR which may be continued.

Test Coverage

  • Covered in the main body of the IB.

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

  • Fix display of admin menu tooltip on WP 6.4+ in mobile viewports.
@aaemnnosttv aaemnnosttv added Type: Bug Something isn't working P0 High priority labels Oct 24, 2023
@aaemnnosttv aaemnnosttv assigned techanvil and unassigned techanvil Oct 24, 2023
@zutigrm zutigrm assigned zutigrm and techanvil and unassigned zutigrm Oct 30, 2023
@techanvil techanvil removed their assignment Nov 1, 2023
@tofumatt tofumatt self-assigned this Nov 6, 2023
@tofumatt
Copy link
Collaborator

tofumatt commented Nov 6, 2023

IB ✅

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

@tofumatt tofumatt assigned tofumatt and unassigned tofumatt Nov 6, 2023
@aaemnnosttv
Copy link
Collaborator Author

@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
Copy link
Collaborator

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 #7862 to address this.

@wpdarren wpdarren self-assigned this Nov 13, 2023
@wpdarren
Copy link
Collaborator

wpdarren commented Nov 14, 2023

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
Copy link
Collaborator

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

@techanvil techanvil removed their assignment Nov 14, 2023
@wpdarren
Copy link
Collaborator

QA Update: ✅

Verified:

  • The admin menu/tooltip menu on the left-hand side is displayed.
    • I tested this on various mobile and tablet viewports to ensure the tooltip appeared on the menu.
    • I also tested this on the AdSense CTA, AdBlocking Recovery, and Enhanced Measurement banner, which have a 'Maybe later' link and the same behavior with the tooltip.
Screenshots

image
image
image
image
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P0 High priority Type: Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants