fix (a11y): add parent node check when removing existing aria describe #19951

HELiving · 2020-07-12T17:45:05Z

#19949

googlebot · 2020-07-12T17:45:10Z

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

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

HELiving · 2020-07-12T17:52:38Z

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

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).

The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.

The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

@googlebot I signed it!

googlebot · 2020-07-12T17:52:43Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

src/cdk/a11y/aria-describer/aria-describer.ts

crisbeto

LGTM.

Commit message fixup: should be changed to something like fix(a11y): add parent node check when removing existing aria describer container.

…into add-guard

HELiving · 2020-07-12T19:44:24Z

LGTM.

Commit message fixup: should be changed to something like fix(a11y): add parent node check when removing existing aria describer container.

browser stack failures are there, but they seem unrelated. Is this still merge-able with browser stack failure?

crisbeto · 2020-07-12T20:32:51Z

Looks like a flake. I've restarted it.

HELiving · 2020-07-12T21:47:17Z

Looks like a flake. I've restarted it.

It's still happening I think. Not sure how to debug that particular area.

HELiving · 2020-07-13T18:29:03Z

@jelbourn @devversion bump

devversion

LGTM. It's super surprising to me that the parent node is undefined (given we retrieved it from the document), but as noted in the issue, it shouldn't hurt having this check.

jelbourn · 2020-07-14T16:16:44Z

src/cdk/a11y/aria-describer/aria-describer.ts

+      if (preExistingContainer && preExistingContainer.parentNode) {
+        preExistingContainer.parentNode.removeChild(preExistingContainer);


Could this just be

preExistingContainer?.parentNode?.removeChild(preExistingContainer);

HELiving · 2020-07-14T16:37:27Z

I think its a stylistic preference. Sure feel free to make any updates :)

…

On Tue, Jul 14, 2020 at 12:17 PM Jeremy Elbourn ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/cdk/a11y/aria-describer/aria-describer.ts <#19951 (comment)>: > + if (preExistingContainer && preExistingContainer.parentNode) { + preExistingContainer.parentNode.removeChild(preExistingContainer); Could this just be preExistingContainer?.parentNode?.removeChild(preExistingContainer); — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#19951 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AQICDMDFVPYHM2RRFU2GCELR3SAH7ANCNFSM4OX43RQA> .

HELiving · 2020-07-21T14:09:58Z

I think its a stylistic preference. Sure feel free to make any updates :)
…
On Tue, Jul 14, 2020 at 12:17 PM Jeremy Elbourn @.> wrote: @.* commented on this pull request. ------------------------------ In src/cdk/a11y/aria-describer/aria-describer.ts <#19951 (comment)>: > + if (preExistingContainer && preExistingContainer.parentNode) { + preExistingContainer.parentNode.removeChild(preExistingContainer); Could this just be preExistingContainer?.parentNode?.removeChild(preExistingContainer); — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#19951 (review)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AQICDMDFVPYHM2RRFU2GCELR3SAH7ANCNFSM4OX43RQA .

@jelbourn bump

jelbourn · 2020-07-21T22:22:36Z

This PR has "merge ready", which means it's in the queue to be merged (it's not always fast because there are extra hoops to have to do with every PR to make sure they don't break anything inside Google)

HELiving · 2020-07-21T22:33:54Z

Thank you. This is my first time creating a PR so I was wondering if I had to ping to make sure it is in the queue. Thanks for the heads up :)

…

On Tue, Jul 21, 2020 at 6:22 PM Jeremy Elbourn ***@***.***> wrote: This PR has "merge ready", which means it's in the queue to be merged (it's not always fast because there are extra hoops to have to do with every PR to make sure they don't break anything inside Google) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#19951 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AQICDMFDZJTW747WGFB5VMDR4YILVANCNFSM4OX43RQA> .

wagnermaciel · 2020-07-24T19:44:36Z

@jelbourn This PR is passing presubmits but I couldn't merge it for the sync because it conflicts with 10.1.x

angular-automatic-lock-bot · 2020-08-28T00:11:56Z

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.}

added a null guard

79289b8

HELiving requested review from devversion and jelbourn as code owners July 12, 2020 17:45

googlebot added the cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla label Jul 12, 2020

googlebot added cla: yes PR author has agreed to Google's Contributor License Agreement and removed cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla labels Jul 12, 2020

crisbeto reviewed Jul 12, 2020

View reviewed changes

src/cdk/a11y/aria-describer/aria-describer.ts Outdated Show resolved Hide resolved

remove !

26b16cf

crisbeto approved these changes Jul 12, 2020

View reviewed changes

Shawn Yoon added 2 commits July 12, 2020 15:40

fix (a11y): add parent node check when removing existing aria describe

cd38374

Merge branch 'add-guard' of https://github.com/shawn-avrio/components …

e3014e4

…into add-guard

HELiving changed the title ~~added a null guard~~ fix (a11y): add parent node check when removing existing aria describe Jul 12, 2020

crisbeto removed the merge: fix commit message When the PR is merged, rewrites/fixups of the commit messages are needed label Jul 12, 2020

devversion approved these changes Jul 14, 2020

View reviewed changes

jelbourn added the merge: fix commit message When the PR is merged, rewrites/fixups of the commit messages are needed label Jul 14, 2020

jelbourn reviewed Jul 14, 2020

View reviewed changes

jelbourn added target: minor This PR is targeted for the next minor release and removed target: patch This PR is targeted for the next patch release labels Jul 28, 2020

jelbourn merged commit 16a3730 into angular:master Jul 28, 2020

angular-automatic-lock-bot bot locked and limited conversation to collaborators Aug 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix (a11y): add parent node check when removing existing aria describe #19951

fix (a11y): add parent node check when removing existing aria describe #19951

HELiving commented Jul 12, 2020 •

edited

googlebot commented Jul 12, 2020

HELiving commented Jul 12, 2020

What to do if you already signed the CLA

Individual signers

Corporate signers

googlebot commented Jul 12, 2020

crisbeto left a comment •

edited

HELiving commented Jul 12, 2020

crisbeto commented Jul 12, 2020

HELiving commented Jul 12, 2020

HELiving commented Jul 13, 2020

devversion left a comment

jelbourn Jul 14, 2020

HELiving commented Jul 14, 2020 via email

HELiving commented Jul 21, 2020

jelbourn commented Jul 21, 2020

HELiving commented Jul 21, 2020 via email

wagnermaciel commented Jul 24, 2020

angular-automatic-lock-bot bot commented Aug 28, 2020

		if (preExistingContainer && preExistingContainer.parentNode) {
		preExistingContainer.parentNode.removeChild(preExistingContainer);

fix (a11y): add parent node check when removing existing aria describe #19951

fix (a11y): add parent node check when removing existing aria describe #19951

Conversation

HELiving commented Jul 12, 2020 • edited

googlebot commented Jul 12, 2020

What to do if you already signed the CLA

Individual signers

Corporate signers

HELiving commented Jul 12, 2020

What to do if you already signed the CLA

Individual signers

Corporate signers

googlebot commented Jul 12, 2020

crisbeto left a comment • edited

Choose a reason for hiding this comment

HELiving commented Jul 12, 2020

crisbeto commented Jul 12, 2020

HELiving commented Jul 12, 2020

HELiving commented Jul 13, 2020

devversion left a comment

Choose a reason for hiding this comment

jelbourn Jul 14, 2020

Choose a reason for hiding this comment

HELiving commented Jul 14, 2020 via email

HELiving commented Jul 21, 2020

jelbourn commented Jul 21, 2020

HELiving commented Jul 21, 2020 via email

wagnermaciel commented Jul 24, 2020

angular-automatic-lock-bot bot commented Aug 28, 2020

HELiving commented Jul 12, 2020 •

edited

crisbeto left a comment •

edited