Skip to content

Magazine block tab and panel ids are not unique per block instance #127

Description

@Herm71

Found while clearing the lint backlog in #116, step 5. The remaining eslint error is the symptom, not the problem.

Problem

Magazine block tab and panel ids are not unique per block instance. make_tab_key() composes them from the item title and its position only:

return sprintf(
	'%s-%s%s',
	sanitize_title( $magazine[ Magazine_Block::ITEM_TITLE ] ),
	$index + 1,
	$element ? '__' . esc_html( $element ) : ''
);

Two Magazine blocks on the same page whose items share a title therefore emit duplicate id attributes, and duplicate aria-controls / aria-labelledby targets pointing at them.

The symptom

getTabPanel() takes a block argument and ignores it, resolving the panel globally instead:

const getTabPanel = ( tab, block ) => {
	return document.getElementById( tab.getAttribute( 'aria-controls' ) );
};

Its sibling does scope correctly:

const getBlockTabs = ( block ) =>
	Array.from( block.querySelectorAll( '[role="tab"]' ) );

So the unused parameter looks like scoping that was intended and never written. With duplicate ids, document.getElementById returns the first match in the document — meaning clicking a tab in the second Magazine block can open the panel in the first.

This is why no-unused-vars still fires on src/views/magazine-block/index.js:27 after #116 step 5; the parameter was deliberately left in place rather than deleted, because deleting it would erase the evidence.

Scope

Only reachable with two or more Magazine blocks on one page sharing an item title. That may never have happened in practice, which is presumably why it has gone unnoticed. Worth confirming before prioritising.

Fix

Two halves, and the JS half alone is not sufficient:

  1. PHP — include the block instance id in the key, the way Press_Inquiries_Controller::get_panel_id() already does with $this->block['id']. This is the part that actually fixes the invalid markup and the ambiguous ARIA relationships for assistive technology.
  2. JS — scope the lookup to the block, matching getBlockTabs:
    const getTabPanel = ( tab, block ) =>
        block.querySelector( `#${ CSS.escape( tab.getAttribute( 'aria-controls' ) ) }` );

Doing only (2) leaves duplicate ids in the DOM, which is invalid HTML and still confuses screen readers even if the click handler behaves.

Acceptance

  • Tab and panel ids are unique across multiple Magazine blocks on one page
  • getTabPanel() uses its block argument, clearing the no-unused-vars error
  • Two Magazine blocks with identical item titles operate independently
  • aria-controls and aria-labelledby resolve to elements within the same block
  • npm run lint:js is clean

Location: src/Components/Magazine_Block_Controller.php — make_tab_key(), src/views/magazine-block/index.js:27

Related: #116 (lint backlog), where this was found.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    P2Performance & lifecyclebugSomething isn't working

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions