Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Patch fixes for latest sync #20401

Merged
merged 2 commits into from
Aug 26, 2020
Merged

Patch fixes for latest sync #20401

merged 2 commits into from
Aug 26, 2020

Conversation

wagnermaciel
Copy link
Contributor

@wagnermaciel wagnermaciel commented Aug 24, 2020 •
edited
Loading

Revert the latest change to src/material/icon/icon-registry.ts and src/material/icon/icon.ts (See PR #20146).
Add a type to src/material/form-field/form-field.ts to ensure userAriaDescribedBy is a string before calling .split.

These changes resolved internal failures where 1. icons stopped showing up and were caught by a screenshot test, and 2. calling .split was causing an error and it was discovered that userAriaDescribedBy was an object.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Aug 24, 2020
@jelbourn
Copy link
Member

Can you amend your commits to include more information about why the change is being made?

@wagnermaciel
revert(mat-icon): this reverts two files from commit 79e4d28
e00e7a1 
* These checks caused failures in pantheon where icons were not loading
@wagnermaciel wagnermaciel force-pushed the patch-fixes-for-latest-sync branch from 9fa0c28 to 08c33c6 Compare August 24, 2020 17:30
@wagnermaciel
Copy link
Contributor Author

@jelbourn Updated my commit messages. Let me know if those are descriptive enough

@wagnermaciel wagnermaciel requested a review from jelbourn August 24, 2020 17:31
devversion
devversion reviewed Aug 24, 2020
View reviewed changes
src/material/form-field/form-field.ts Outdated
@@ -507,7 +507,7 @@ export class MatFormField extends _MatFormFieldMixinBase
if (this._control) {
let ids: string[] = [];

if (this._control.userAriaDescribedBy) {
if (typeof this._control.userAriaDescribedBy === 'string') {
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment on why this assertion is needed? It's not clear to me in what cases userAriaDescribedBy becomes an object.

Copy link
Contributor Author

@wagnermaciel wagnermaciel Aug 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure why it happens but I am sure that it does happen. I just noticed that this was happening to an internal project and causing failures so I tried adding a check to see if .split was defined before calling it

Copy link
Member

Choose a reason for hiding this comment

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

My hypothesis would be that the lack of template type checking somewhere is letting someone bind an invalid value into something that then makes its way into aria-describedby. What we would typically want to do here is get to the root cause of the issue and potentially fix the offending app rather than accommodating an invalid value since the type system is supposed to provide this kind of protection.

Copy link
Member

@devversion devversion Aug 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, that seems more likely to me. That's where I was heading. It seems wrong to change form-field in favor of something we don't fully understand yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unless we can find the root cause of this type error, is the only other alternative to revert the entire PR?

Here's the PR that added this change
#19587

Copy link
Member

Choose a reason for hiding this comment

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

Who can look into this? I don't think I have the ability to help with that, unfortunately. To me it also sounds like someone explicitly passes an object to aria-describedby using an Angular input binding. It should be possible to filter out this app from the stack trace where the form-field threw an error. How many instances of these errors did you see?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just one. I'm also on vacation rn but I can track it down in my spare time. I'm not sure how long it'll take, though. Would we be ok with leaving this patch fix for now and adding a todo saying once this is tracked down we should remove this check?

Copy link
Member

@devversion devversion Aug 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, for sure. Works for me. I'll leave that to you. just wanted to make sure that this is temporary/fixed up in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll take the responsibility for this. You guys can call me out for it if this isn't resolved or at least updated within the next 2 weeks. I also created an issue to make sure we don't lose track of this #20403

Copy link
Member

Choose a reason for hiding this comment

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

More information: it's the Google Fonts app where this fails, and nowhere in the app do they set an aria-describedby, so it seems like it's something that's coming from us.

@wagnermaciel wagnermaciel force-pushed the patch-fixes-for-latest-sync branch from 08c33c6 to 35b3b73 Compare August 24, 2020 18:08
@wagnermaciel wagnermaciel force-pushed the patch-fixes-for-latest-sync branch from 35b3b73 to 147689a Compare August 24, 2020 19:40
@wagnermaciel wagnermaciel mentioned this pull request Aug 24, 2020
Open
@jelbourn jelbourn added the target: minor This PR is targeted for the next minor release label Aug 26, 2020
@wagnermaciel @jelbourn
fix(form-field): add a type check for userAriaDescribedBy
5552abe 
* Internal test was failing because userAriaDescribedBy showed up as an object so
  calling .split was throwing errors.
@jelbourn jelbourn force-pushed the patch-fixes-for-latest-sync branch from 147689a to 5552abe Compare August 26, 2020 16:07
jelbourn
jelbourn approved these changes Aug 26, 2020
View reviewed changes
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

I pushed a change to one of your commits to remove a leftover console.log

@jelbourn jelbourn added action: merge The PR is ready for merge by the caretaker merge safe merge: preserve commits When the PR is merged, a rebase and merge should be performed labels Aug 26, 2020
@jelbourn jelbourn merged commit 7a2a732 into angular:master Aug 26, 2020
@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 Sep 26, 2020
@wagnermaciel wagnermaciel deleted the patch-fixes-for-latest-sync branch January 14, 2021 19:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@crisbeto crisbeto crisbeto left review comments

@jelbourn jelbourn jelbourn approved these changes

@devversion devversion devversion approved these changes

@mmalerba mmalerba Awaiting requested review from mmalerba

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement merge: preserve commits When the PR is merged, a rebase and merge should be performed target: minor This PR is targeted for the next minor release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@wagnermaciel @jelbourn @crisbeto @devversion @googlebot