Skip to content

Fix: Radio focus handler bound to onChange instead of onFocus#5468

Open
TranscenderNing wants to merge 116 commits into
google:masterfrom
TranscenderNing:fix/radio-focus-handler-wrong-binding
Open

Fix: Radio focus handler bound to onChange instead of onFocus#5468
TranscenderNing wants to merge 116 commits into
google:masterfrom
TranscenderNing:fix/radio-focus-handler-wrong-binding

Conversation

@TranscenderNing

Copy link
Copy Markdown

Summary

  • Bug: MaterialRadio.prototype.init() line 230: boundFocusHandler_ was incorrectly bound to this.onChange_ instead of this.onFocus_
  • Impact: Every focus event on a radio button (Tab key navigation) incorrectly triggers the change handler, causing O(n) DOM scan + potential visual state glitches
  • Fix: Single word change: .onChange_.onFocus_

Root Cause Analysis

In src/radio/radio.js line 230:

// BEFORE (buggy):
this.boundFocusHandler_ = this.onChange_.bind(this);  // ← copy-paste error

// AFTER (fixed):
this.boundFocusHandler_ = this.onFocus_.bind(this);   // ← correct method

This is a clear copy-paste error from line 229, where boundChangeHandler_ is correctly bound to onChange_. The same pattern is correctly implemented in sibling components:

  • checkbox.js line 251: this.boundFocusHandler_ = this.onFocus_.bind(this);
  • switch.js line 252: this.boundFocusHandler_ = this.onFocus_.bind(this);
  • radio.js line 230: this.boundFocusHandler_ = this.onChange_.bind(this);

Reproduction Steps

  1. Create a page with MDL Radio buttons
  2. Use Tab key to navigate/focus a radio button
  3. Before fix: Focus triggers onChange_ which does a full DOM query via document.getElementsByClassName() and calls updateClasses_() on every radio button on the page
  4. After fix: Focus only triggers lightweight onFocus_ handler

Test Plan

  • Verified onFocus_ method exists at line 97 of radio.js
  • Confirmed binding pattern matches checkbox.js and switch.js
  • Manual test: Tab-navigate through radio buttons - no unexpected DOM scanning
  • Existing test suite should continue to pass (backward-compatible change)

Additional Context

This was found during a systematic code audit comparing event handler initialization across all MDL form components. All three primary form components (checkbox, radio, switch) share nearly identical initialization patterns, making this inconsistency easy to spot.

katranci and others added 30 commits May 30, 2016 11:59
* Fixes element.MaterialRadio.[un]check() calls on radio lists
This commit adds an [Issue Template](https://github.com/blog/2111-issue-and-pull-request-templates) to this repo alerting users to
the status of features / breaking 1.x changes by the core team, as well
as bug reporting guidelines.

This will hopefully help stabilize the work being done for google#4462
…plate

Add issue template outlining feature request / bug reporting guidelines
….0.0

Update gulp-connect to version 5.0.0 🚀
….3.1

Update gulp-flatten to version 0.3.1 🚀
sgomes and others added 29 commits December 15, 2016 16:26
_radio.scss: scale() instead of scale3d()
Since we're unsure of what this value should be and it has existed this long, simply removing and not making valid as to try and not break existing applications.
When using "background" property CSSO not working correctly ( if not replacing sass property ). Separating "background" to "background-color" and "background-image" solve this problem
Uncheck the main checkbox when unchecking a checkbox if is checked
Thanks for your comments.
I hope that this solution seems to you better
Correction of syntax
set is_dirty when placeholder exists
In MaterialRadio.prototype.init() (line 230), boundFocusHandler_ was
incorrectly bound to this.onChange_ instead of this.onFocus_. This
caused every focus event on a radio button (e.g. via Tab key) to
trigger the change handler logic, which performs an O(n) scan of ALL
radio buttons in the DOM via document.getElementsByClassName.

Impact:
- Wrong behavior: Focus triggers change-handler logic meant only for
  actual value changes, causing potential visual state glitches
- Performance: Every focus event triggers a full DOM query + iteration
  over all radio buttons on the page

Root cause: This appears to be a copy-paste error from line 229 where
boundChangeHandler_ is correctly bound to onChange_. The same pattern
is correctly implemented in checkbox.js (line 251) and switch.js (line 252).

Fix: Change .onChange_ to .onFocus_ on line 230 (single word change).
@google-cla

google-cla Bot commented May 28, 2026

Copy link
Copy Markdown

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.