Skip to content

fix(material/radio): clear names from host nodes #15422

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Mar 8, 2019

Along the same lines as #15368. Clears the name from the host node of the radio button and radio group, because they end up being forwarded to the underlying input and can cause double results when using something like getElementsByName.

@crisbeto crisbeto added the target: patch This PR is targeted for the next patch release label Mar 8, 2019
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Mar 8, 2019
jelbourn
jelbourn previously approved these changes Mar 9, 2019
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Mar 9, 2019
crisbeto added a commit to crisbeto/material2 that referenced this pull request Mar 16, 2019
Along the same lines as angular#15422 and angular#15368. Clears the static `name` attribute from the slide toggle's host node in order to prevent both the underlying input and the host from showing up in `document.getElementsByName` or `By.name`.
crisbeto added a commit to crisbeto/material2 that referenced this pull request Mar 23, 2019
Along the same lines as angular#15422 and angular#15368. Clears the static `name` attribute from the slide toggle's host node in order to prevent both the underlying input and the host from showing up in `document.getElementsByName` or `By.name`.
@mmalerba mmalerba added aaa and removed aaa labels Apr 25, 2019
crisbeto added a commit to crisbeto/material2 that referenced this pull request May 4, 2019
Along the same lines as angular#15422 and angular#15368. Clears the static `name` attribute from the slide toggle's host node in order to prevent both the underlying input and the host from showing up in `document.getElementsByName` or `By.name`.
@crisbeto crisbeto force-pushed the radio-host-node-names branch from 225cbc8 to a6b2616 Compare July 1, 2019 20:16
crisbeto added a commit to crisbeto/material2 that referenced this pull request Jul 21, 2019
Along the same lines as angular#15422 and angular#15368. Clears the static `name` attribute from the slide toggle's host node in order to prevent both the underlying input and the host from showing up in `document.getElementsByName` or `By.name`.
@crisbeto crisbeto force-pushed the radio-host-node-names branch from a6b2616 to 5c2c31b Compare October 1, 2019 19:32
crisbeto added a commit to crisbeto/material2 that referenced this pull request Nov 29, 2019
Along the same lines as angular#15422 and angular#15368. Clears the static `name` attribute from the slide toggle's host node in order to prevent both the underlying input and the host from showing up in `document.getElementsByName` or `By.name`.
@crisbeto crisbeto force-pushed the radio-host-node-names branch from 5c2c31b to d298469 Compare May 2, 2020 15:08
@crisbeto crisbeto added the P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent label Jun 7, 2020
@crisbeto crisbeto force-pushed the radio-host-node-names branch from d298469 to f7e96d2 Compare June 7, 2020 09:12
@mmalerba mmalerba removed the lgtm label Jul 31, 2020
crisbeto added a commit to crisbeto/material2 that referenced this pull request Jan 10, 2021
Along the same lines as angular#15422 and angular#15368. Clears the static `name` attribute from the slide
toggle's host node in order to prevent both the underlying input and the host from showing
up in `document.getElementsByName` or `By.name`.
@devversion devversion removed their request for review August 18, 2021 12:54
@andrewseguin andrewseguin added needs rebase and removed cla: yes PR author has agreed to Google's Contributor License Agreement labels Dec 29, 2021
crisbeto added a commit to crisbeto/material2 that referenced this pull request Mar 7, 2022
Along the same lines as angular#15422 and angular#15368. Clears the static `name` attribute from the slide
toggle's host node in order to prevent both the underlying input and the host from showing
up in `document.getElementsByName` or `By.name`.
@crisbeto crisbeto changed the title fix(radio): clear names from host nodes fix(material/radio): clear names from host nodes Mar 7, 2022
@crisbeto crisbeto force-pushed the radio-host-node-names branch from f7e96d2 to bbbc038 Compare March 7, 2022 08:45
crisbeto added a commit that referenced this pull request Mar 9, 2022
Along the same lines as #15422 and #15368. Clears the static `name` attribute from the slide
toggle's host node in order to prevent both the underlying input and the host from showing
up in `document.getElementsByName` or `By.name`.

(cherry picked from commit 5db1df0)
crisbeto added a commit that referenced this pull request Mar 9, 2022
Along the same lines as #15422 and #15368. Clears the static `name` attribute from the slide
toggle's host node in order to prevent both the underlying input and the host from showing
up in `document.getElementsByName` or `By.name`.
@andrewseguin andrewseguin removed the P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent label Mar 28, 2022
forsti0506 pushed a commit to forsti0506/components that referenced this pull request Apr 3, 2022
Along the same lines as angular#15422 and angular#15368. Clears the static `name` attribute from the slide
toggle's host node in order to prevent both the underlying input and the host from showing
up in `document.getElementsByName` or `By.name`.
@andrewseguin andrewseguin assigned crisbeto and unassigned crisbeto Apr 22, 2022
@amysorto amysorto self-assigned this May 11, 2022
@amysorto
Copy link
Contributor

Shoud the name be assigned to the inner input? In a test within google3, the input within the radio had this name "mat-radio-group-0" instead of what was being passed in.

@andrewseguin
Copy link
Contributor

@crisbeto Heads up that Amy has a quick question about going forward on this change

@crisbeto
Copy link
Member Author

crisbeto commented Aug 15, 2022

I think that we talked through this over DM at some point. The fix is correct, but it ended up breaking internally because apps were projecting all sorts of unrelated content into the radio buttons.

@andrewseguin
Copy link
Contributor

Gotcha thanks - @amysorto Do you have a good idea on how to get those tests passing with this change? Feel free to pair with myself or Kristiyan to get this in

Along the same lines as angular#15368. Clears the `name` from the host node of the radio button and radio group, because they end up being forwarded to the underlying `input` and can cause double results when using something like `getElementsByName`.
@crisbeto crisbeto force-pushed the radio-host-node-names branch from bbbc038 to fe0f0e8 Compare August 23, 2022 16:21
@andrewseguin andrewseguin assigned devversion and unassigned devversion and amysorto Jun 5, 2023
@crisbeto
Copy link
Member Author

Looks like this is really breaking internally. I'll close it for now since we haven't gotten any issue reports about it.

@crisbeto crisbeto closed this Feb 28, 2024
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Mar 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants