Masterbar: fix invalid HTML markup #98510

ciampo · 2025-01-16T23:10:05Z

Related to #98365

Proposed Changes

Move master bar subitems from being children of the parent menu item to being siblings
Do not render interactive elements such as button and a as children of button
Normalize the markup, always rendering the item wrapper — this makes it more predictable

Why are these changes being made?

The current structure generates invalid (button rendered as a child of another button) and generally inaccessible (interactive elements like a as children of button) HTML markup

Note

This PR only aims at fixing the most critical issue (invalid markup) with the masterbar markup. As documented in #98365, the master bar semantics and general accessibility are still far from ideal — but that should be tackled in separate PRs. Given the effort required for that potential refactor, we should discuss our options carefully before embarking on that endeavour, especially given how we're slowly showing wp-admin screens over calypso ones.

Testing Instructions

Load Calypso in a variety of pages/scenarios:
- Logged out: eg. /discover
- Logged in: eg. home page
- With paid items in the cart
Test on multiple viewport sizes
Test both pointer, keyboard, and touchscreen interactions

In all of these scenarios:

The master bar should not include invalid HTML markup (button or a as a child of button)
The error in the browser console about button rendered inside a button currently displayed on trunk should be gone
The look&feel and the UX should be the same as on trunk

Pre-merge Checklist

Has the general commit checklist been followed? (PCYsg-hS-p2)
Have you written new tests for your changes?
Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
Have you checked for TypeScript, React or other console errors?
Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
- For UI changes, have we tested the change in various languages (for example, ES, PT, FR, or DE)? The length of text and words vary significantly between languages.
For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-aUh-p2)?

github-actions · 2025-01-16T23:10:18Z

Calypso Live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-130284

Jetpack Cloud live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-130284&env=jetpack

Automattic for Agencies live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-130284&env=a8c-for-agencies

matticbot · 2025-01-16T23:16:55Z

This PR modifies the release build for the following Calypso Apps:

For info about this notification, see here: PCYsg-OT6-p2

notifications
wpcom-block-editor

To test WordPress.com changes, run install-plugin.sh $pluginSlug fix/masterbar-invalid-markup on your sandbox.

matticbot · 2025-01-16T23:20:12Z

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

App Entrypoints (~69 bytes removed 📉 [gzipped])

name         parsed_size           gzip_size
entry-main        -411 B  (-0.0%)      -69 B  (-0.0%)
entry-login       -411 B  (-0.0%)      -69 B  (-0.0%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Async-loaded Components (~70 bytes removed 📉 [gzipped])

name                                        parsed_size           gzip_size
async-load-calypso-my-sites-checkout-modal       -411 B  (-0.0%)      -70 B  (-0.0%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

ciampo · 2025-01-19T17:20:57Z

client/layout/masterbar/item.tsx

-	componentButtonRef = React.createRef< HTMLButtonElement >();
-	componentDivRef = React.createRef< HTMLDivElement >();


By always rendering the wrapper, and either and a or a button, componentButtonRef is not necessary anymore, and componentDivRef was renamed to wapperRef for better clarity

ciampo · 2025-01-19T17:22:28Z

client/layout/masterbar/item.tsx

-		const isInComponentButtonRef = this.componentButtonRef.current?.contains(
-			event.target as Node
-		);
-		const isInComponentDivRef = this.componentDivRef.current?.contains( event.target as Node );
-
-		if ( ! isInComponentButtonRef && ! isInComponentDivRef ) {


Related to the comment above, since the wrapper button was removed, the logic can be simplified to only check whether we're in the wrapper div (which is now always rendered)

ciampo · 2025-01-19T17:26:37Z

client/layout/masterbar/item.tsx

@@ -221,49 +216,30 @@ class MasterbarItem extends Component< MasterbarItemProps > {
 			disabled: this.props.disabled,
 		};

-		if ( this.props.url && ! this.props.subItems ) {


Moved from

if ( this.props.url && ! this.props.subItems ) { // render anchor tag } if ( this.props.url && this.props.subItems ) { // render button with anchor tag and sub-menu items as children } // else, if props.url is not defined // render a wrapper div > button > menu items as children (no anchor tag)

to:

always rendering a wrapper div

render either a button or an a depending on props.url

render the sub-menu items (when defined) as sibling of the button/a

ciampo · 2025-01-19T17:27:22Z

client/layout/masterbar/item.tsx

-						href={ this.props.url }
-						ref={ this.props.innerRef as LegacyRef< HTMLAnchorElement > }
-						onTouchEnd={ this.toggleMenuByTouch }
-						tabIndex={ -1 }


Since we don't render a/button nested in a parent button, the tabindex={-1} shouldn't be necessary anymore

ciampo · 2025-01-19T17:28:19Z

client/layout/masterbar/item.tsx


 		return (
-			<div className={ this.props.wrapperClassName } ref={ this.componentDivRef }>
-				<button
+			<div


Always rendering the div could cause some styles to break, but I hopefully managed to catch and amend them all. Lots of testing needed to make sure I didn't miss any edge case

ciampo · 2025-01-19T18:03:21Z

E2E test failures could be related, I'll look into them and report.

lsl

Nice simplification, I don't see any major regressions.

lsl

Sorry. Monday brain, I just realized that this wasn't changing the sidebar but the masterbar. Testing that, there is a blocking regression on mouse hover here:

Peek.2025-01-20.09-50.mp4

Please dismiss the "needs changes" review once resolved.

- avoid nesting a inside button: render one or the other - always render wrapper div for consistency - render subitems as siblings of the parent menuitem (and not nested) - less forks in the logic - tweak styles to follow the new markup

ciampo · 2025-01-20T12:34:16Z

@lsl 's bug should be addressed with abd76a2, and e2e tests are also passing.

Can I get another round of review? Given the breadth of the changes, I'd feel better with more people reviewing this PR.

Blocking bug should be fixed

tyxla

LGTM and tests well 👍

A few unrelated potential follow-ups I've noticed while testing on more narrow views (<782px):

The sites menu item (the top left one) doesn't have a pointer cursor when hovering
The "log out" item when hovering the Me item appears misaligned:

ciampo · 2025-01-21T10:01:40Z

Follow-up PRs:

The sites menu item (the top left one) doesn't have a pointer cursor when hovering

#98651

The "log out" item when hovering the Me item appears misaligned

#98656

* Reorganize and simplify markup: - avoid nesting a inside button: render one or the other - always render wrapper div for consistency - render subitems as siblings of the parent menuitem (and not nested) - less forks in the logic - tweak styles to follow the new markup * Add back subitems, tweak CSS selectors to match new markup * Remove code comments * Revert to && instead of ternary * Keep submenu opened while hovering

sirbrillig · 2025-01-24T16:29:20Z

client/layout/masterbar/item.tsx

@@ -221,49 +216,30 @@ class MasterbarItem extends Component< MasterbarItemProps > {
 			disabled: this.props.disabled,
 		};

-		if ( this.props.url && ! this.props.subItems ) {
-			return (
+		const MenuItem = ( props: React.HTMLAttributes< HTMLElement > ) =>


FYI it's not a good idea to create components inside a render method. It causes the component to be recreated on every render which can have side effects (like this). See #98897 which should fix this issue by moving the component's definition outside the render method.

ciampo mentioned this pull request Jan 16, 2025

Masterbar: Add a label for the Reader icon. #98479

Merged

8 tasks

ciampo force-pushed the fix/masterbar-invalid-markup branch 3 times, most recently from 6b6d4cc to b2eb737 Compare January 17, 2025 17:22

ciampo requested review from Addison-Stavlo and fushar January 17, 2025 17:46

matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jan 17, 2025

ciampo requested review from okmttdhr, xavier-lc, a team and lsl January 17, 2025 17:46

ciampo self-assigned this Jan 17, 2025

ciampo added [Type] Bug When a feature is broken and / or not performing as intended Masterbar labels Jan 17, 2025

ciampo marked this pull request as ready for review January 17, 2025 17:47

ciampo mentioned this pull request Jan 17, 2025

Masterbar produces incorrect and inaccessible markup #98365

Open

ciampo force-pushed the fix/masterbar-invalid-markup branch 2 times, most recently from 1096e2a to 3d58fb3 Compare January 19, 2025 17:19

ciampo commented Jan 19, 2025

View reviewed changes

lsl approved these changes Jan 19, 2025

View reviewed changes

lsl previously requested changes Jan 19, 2025

View reviewed changes

ciampo added 5 commits January 20, 2025 11:15

Reorganize and simplify markup:

433cccb

- avoid nesting a inside button: render one or the other - always render wrapper div for consistency - render subitems as siblings of the parent menuitem (and not nested) - less forks in the logic - tweak styles to follow the new markup

Add back subitems, tweak CSS selectors to match new markup

dadcbd6

Remove code comments

27215d8

Revert to && instead of ternary

7c01260

Keep submenu opened while hovering

abd76a2

ciampo force-pushed the fix/masterbar-invalid-markup branch from 136c854 to abd76a2 Compare January 20, 2025 10:58

ciampo requested a review from lsl January 20, 2025 12:33

tyxla approved these changes Jan 20, 2025

View reviewed changes

ciampo merged commit ff8b5a2 into trunk Jan 21, 2025
14 checks passed

ciampo deleted the fix/masterbar-invalid-markup branch January 21, 2025 08:54

github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jan 21, 2025

This was referenced Jan 21, 2025

Masterbar: display a pointer cursor for the sidebar menu toggle button #98651

Merged

Masterbar: fix misaligned account sub-menu items #98656

Merged

ciampo mentioned this pull request Jan 23, 2025

Untangle Admin Bar #98079

Open

This was referenced Jan 24, 2025

Masterbar shopping cart dropdown no longer appears near cart button #98893

Closed

Masterbar: Do not recreate masterbar button element on every render #98897

Merged

sirbrillig reviewed Jan 24, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Masterbar: fix invalid HTML markup #98510

Masterbar: fix invalid HTML markup #98510

ciampo commented Jan 16, 2025 •

edited

Loading

github-actions bot commented Jan 16, 2025 •

edited by matticbot

Loading

matticbot commented Jan 16, 2025 •

edited

Loading

matticbot commented Jan 16, 2025 •

edited

Loading

ciampo Jan 19, 2025

ciampo Jan 19, 2025

ciampo Jan 19, 2025

ciampo Jan 19, 2025

ciampo Jan 19, 2025

ciampo commented Jan 19, 2025

lsl left a comment

lsl left a comment

ciampo commented Jan 20, 2025

tyxla left a comment

ciampo commented Jan 21, 2025 •

edited

Loading

sirbrillig Jan 24, 2025

		componentButtonRef = React.createRef< HTMLButtonElement >();
		componentDivRef = React.createRef< HTMLDivElement >();

Masterbar: fix invalid HTML markup #98510

Masterbar: fix invalid HTML markup #98510

Conversation

ciampo commented Jan 16, 2025 • edited Loading

Proposed Changes

Why are these changes being made?

Testing Instructions

Pre-merge Checklist

github-actions bot commented Jan 16, 2025 • edited by matticbot Loading

matticbot commented Jan 16, 2025 • edited Loading

matticbot commented Jan 16, 2025 • edited Loading

ciampo Jan 19, 2025

Choose a reason for hiding this comment

ciampo Jan 19, 2025

Choose a reason for hiding this comment

ciampo Jan 19, 2025

Choose a reason for hiding this comment

ciampo Jan 19, 2025

Choose a reason for hiding this comment

ciampo Jan 19, 2025

Choose a reason for hiding this comment

ciampo commented Jan 19, 2025

lsl left a comment

Choose a reason for hiding this comment

lsl left a comment

Choose a reason for hiding this comment

ciampo commented Jan 20, 2025

tyxla left a comment

Choose a reason for hiding this comment

ciampo commented Jan 21, 2025 • edited Loading

sirbrillig Jan 24, 2025

Choose a reason for hiding this comment

ciampo commented Jan 16, 2025 •

edited

Loading

github-actions bot commented Jan 16, 2025 •

edited by matticbot

Loading

matticbot commented Jan 16, 2025 •

edited

Loading

matticbot commented Jan 16, 2025 •

edited

Loading

ciampo commented Jan 21, 2025 •

edited

Loading