Skip to content

Conversation

vernard
Copy link
Contributor

@vernard vernard commented Oct 16, 2025

Description (*)

This toggleMenu function here used to exist in an old JS file js/varien/iehover-fix.js. That file doesn't exist anymore. Right now it's causing an Uncaught Reference error when I hover a menu item.

image

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)

@github-actions github-actions bot added the Component: Catalog Relates to Mage_Catalog label Oct 16, 2025
@sonarqubecloud
Copy link

@kiatng
Copy link
Contributor

kiatng commented Oct 19, 2025

I cannot replicate in rwd theme package. I don't have any theme that's fallback to base for testing. Not sure if this PR is BC.

@vernard Please provide replication steps. This will help the reviewers.

I agree with AI:

Let me investigate where this code is actually used and what the implications would be:

[11 tools called]

Analysis: NOT Safe to Remove (but it's effectively dead code)

Here's what I found:

Current Status:

  1. The toggleMenu() JavaScript function doesn't exist anywhere in the codebase
  2. No layout XML files reference catalog/navigation/top.phtml
  3. RWD theme uses a completely different system (Mage_Page_Block_Html_Topmenu)
  4. The code has a built-in disable flag ($noEventAttributes parameter on line 229)

The Issue:

This code generates broken JavaScript - it calls toggleMenu(this,1) but that function doesn't exist, so it would throw errors if executed.

Why NOT to Remove:

Backward compatibility - OpenMage maintains compatibility with:

  • Third-party themes that might use renderCategoriesMenuHtml()
  • Custom code that might call this method
  • Old Magento 1.x extensions

Recommended Options:

Option 1: Pass the disable flag (safest, no breaking changes)

<?php echo $this->renderCategoriesMenuHtml(0, 'level-top', '', true) ?>

Option 2: Deprecate instead of remove

if ($hasActiveChildren && !$noEventAttributes) {
    // @deprecated These event handlers reference non-existent toggleMenu() function
    // Will be removed in future version. Use CSS hover or modern JS instead.
    $attributes['onmouseover'] = 'toggleMenu(this,1)';
    $attributes['onmouseout'] = 'toggleMenu(this,0)';
}

Option 3: Remove the code (if you're certain no one is using it)

  • Check with the OpenMage community first
  • Add a note in CHANGELOG about the breaking change
  • Consider it for a major version bump

My Recommendation:

Since this is OpenMage (a community-maintained fork), I'd suggest opening a GitHub issue to discuss deprecating this with the community before removing it.

@kiatng
Copy link
Contributor

kiatng commented Oct 19, 2025

I am against this PR as it is BC. @vernard I suggest option 1, set $noEventAttributes = true in your code to bypass the errors. Alternatively, you can create a js function for toggeleMenu().

@sreichel
Copy link
Contributor

That file doesn't exist anymore.

@kiatng I cant reproduce too, but i agree with tis PR. The BC break was removing iehover-fix.js.

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

Labels

Component: Catalog Relates to Mage_Catalog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants