Make getHeaderHeightWithoutNav() more robust to avoid errors seen in the wild.

[techanvil]

Feature Description

Since the release of version 1.95.0 a handful of errors have been reported from installed sites (four at the time of writing).

These evidently relate to the work done via #6109 which was included in this release.

Although it's not been possible to recreate these issues in our testing we should add some defensive logic to getHeaderHeightWithoutNav() to prevent these errors.

---

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

Acceptance criteria

• The getHeaderHeightWithoutNav() function should be updated so that it doesn't return anything other than a positive number or zero.
• It should make a best-effort attempt to calculate the correct height for the sticky Site Kit header & WordPress admin bar combination based on the information available.
• It should return zero for any unexpected results.
• Going forward, we should not see any more of these errors in the production logs.

Implementation Brief

• Improve the check for header stickiness by checking the header's position style property via getComputedStyle().
• Add some checks to ensure getHeaderHeightWithoutNav() cannot return anything but a finite positive integer or 0.
• Refactor getHeaderHeightWithoutNav() to be more readable.
• Add a guard at the point of using the return value of getHeaderHeightWithoutNav() as a failsafe - use Math.abs(), similar to how it's used in WidgetAreaRenderer.
• Rename getHeaderHeightWithoutNav() and getHeaderHeight() to getStickyHeaderHeightWithoutNav() and getStickyHeaderHeight() to be more explicit.
• Finish off the PR Make the getHeaderHeightWithoutNav() function more defensive. #6672 which implements the above.

Test Coverage

• Add some tests for getStickyHeaderHeightWithoutNav() and backfill test coverage for getStickyHeaderHeight().

QA Brief

• The bug described here should no longer occur.

I am running through the dashboard navigation tooltip tour and if you click 'back' on the final tip, a fatal error appears.

• Run through the smoke/regression tests described under Additional QA for followup PR(s) in view_notification GA event being sent for Banner Notifications that are dismissed or hidden #6109.

Changelog entry

• Fix potential IntersectionObserver error in banner notifications.

[aaemnnosttv]

Moving forward to CR

[aaemnnosttv]

@techanvil I left a review on the PR that I reopened as a draft. Let's use that to inform a new IB, which could involve keeping the same PR (which is now against develop) if you'd like. Depending on what direction we take, it might make more sense to start fresh but I'm curious to get your thoughts.

[techanvil]

@techanvil I left a review on the PR that I reopened as a draft. Let's use that to inform a new IB, which could involve keeping the same PR (which is now against develop) if you'd like. Depending on what direction we take, it might make more sense to start fresh but I'm curious to get your thoughts.

Thanks @aaemnnosttv, you raised some good points there. I've replied on the PR, and also made the discussed changes. I've also updated the IB - I think it makes sense to continue using this branch considering the amount of detailed discussion that has already taken place on it.

[aaemnnosttv]

@techanvil there's a duplicate conflicting section for test coverage – I'll remove the older one.

IB ✅

[mohitwp]

QA Update ✅

• Tested on dev.
• Verified errors related to feature tour bug described Release 1.96.0 #6655 (comment) not appearing now.
• Done additional smoke/regression tests as described under Additional QA for followup PR(s) in view_notification GA event being sent for Banner Notifications that are dismissed or hidden #6109.

Recording.273.mp4