Skip to content

Commit ff8b5a2

Browse files
authored
Masterbar: fix invalid HTML markup (#98510)
* 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
1 parent 9b8a087 commit ff8b5a2

File tree

2 files changed

+117
-133
lines changed

2 files changed

+117
-133
lines changed

client/layout/masterbar/item.tsx

Lines changed: 16 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -60,8 +60,7 @@ class MasterbarItem extends Component< MasterbarItemProps > {
6060
isOpenForNonMouseFlow: false,
6161
};
6262

63-
componentButtonRef = React.createRef< HTMLButtonElement >();
64-
componentDivRef = React.createRef< HTMLDivElement >();
63+
wrapperRef = React.createRef< HTMLDivElement >();
6564

6665
_preloaded = false;
6766

@@ -191,12 +190,8 @@ class MasterbarItem extends Component< MasterbarItemProps > {
191190
}
192191

193192
// Check refs to see if the touch event started inside our component, if it didn't, close the menu.
194-
const isInComponentButtonRef = this.componentButtonRef.current?.contains(
195-
event.target as Node
196-
);
197-
const isInComponentDivRef = this.componentDivRef.current?.contains( event.target as Node );
198-
199-
if ( ! isInComponentButtonRef && ! isInComponentDivRef ) {
193+
const isInWrapper = this.wrapperRef.current?.contains( event.target as Node );
194+
if ( ! isInWrapper ) {
200195
this.setState( { isOpenForNonMouseFlow: false } );
201196
}
202197
};
@@ -221,49 +216,30 @@ class MasterbarItem extends Component< MasterbarItemProps > {
221216
disabled: this.props.disabled,
222217
};
223218

224-
if ( this.props.url && ! this.props.subItems ) {
225-
return (
219+
const MenuItem = ( props: React.HTMLAttributes< HTMLElement > ) =>
220+
this.props.url ? (
226221
<a
227-
{ ...attributes }
228222
href={ this.props.url }
229223
ref={ this.props.innerRef as LegacyRef< HTMLAnchorElement > }
230-
>
231-
{ this.renderChildren() }
232-
</a>
224+
{ ...props }
225+
/>
226+
) : (
227+
<button ref={ this.props.innerRef as LegacyRef< HTMLButtonElement > } { ...props } />
233228
);
234-
}
235-
236-
if ( this.props.url && this.props.subItems ) {
237-
return (
238-
<button
239-
{ ...attributes }
240-
ref={ this.componentButtonRef }
241-
onKeyDown={ this.toggleMenuByKey }
242-
>
243-
<a
244-
href={ this.props.url }
245-
ref={ this.props.innerRef as LegacyRef< HTMLAnchorElement > }
246-
onTouchEnd={ this.toggleMenuByTouch }
247-
tabIndex={ -1 }
248-
>
249-
{ this.renderChildren() }
250-
</a>
251-
{ this.renderSubItems() }
252-
</button>
253-
);
254-
}
255229

256230
return (
257-
<div className={ this.props.wrapperClassName } ref={ this.componentDivRef }>
258-
<button
231+
<div
232+
className={ clsx( 'masterbar__item-wrapper', this.props.wrapperClassName ) }
233+
ref={ this.wrapperRef }
234+
>
235+
<MenuItem
259236
{ ...attributes }
260-
ref={ this.props.innerRef as LegacyRef< HTMLButtonElement > }
261237
onKeyDown={ this.props.subItems && this.toggleMenuByKey }
262238
onTouchEnd={ this.props.subItems && this.toggleMenuByTouch }
263239
>
264240
{ this.renderChildren() }
265-
{ this.renderSubItems() }
266-
</button>
241+
</MenuItem>
242+
{ this.renderSubItems() }
267243
</div>
268244
);
269245
}

0 commit comments

Comments
 (0)