Skip to content

fix(material/form-field): outline not updated if required state changes #26909

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

Closed
wants to merge 1 commit into from
Closed

fix(material/form-field): outline not updated if required state changes #26909

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/material/form-field/form-field.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@
<label matFormFieldFloatingLabel
[floating]="_shouldLabelFloat()"
*ngIf="_hasFloatingLabel()"
(cdkObserveContent)="_refreshOutlineNotchWidth()"
[cdkObserveContentDisabled]="!_hasOutline()"
(cdkObserveContent)="_labelContentChanged()"
[cdkObserveContentDisabled]="!_shouldObserveLabelContentChanges()"
[id]="_labelId"
[attr.for]="_control.id"
[attr.aria-owns]="_control.id">
Expand Down
24 changes: 21 additions & 3 deletions src/material/form-field/form-field.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,7 @@ export class MatFormField
private _isFocused: boolean | null = null;
private _explicitFormFieldControl: MatFormFieldControl<any>;
private _needsOutlineLabelOffsetUpdateOnStable = false;
private _isInitialized = false;

constructor(
public _elementRef: ElementRef,
Expand Down Expand Up @@ -332,6 +333,7 @@ export class MatFormField
// Because the above changes a value used in the template after it was checked, we need
// to trigger CD or the change might not be reflected if there is no other CD scheduled.
this._changeDetectorRef.detectChanges();
this._isInitialized = true;
}

ngAfterContentInit() {
Expand Down Expand Up @@ -552,10 +554,26 @@ export class MatFormField

/** Refreshes the width of the outline-notch, if present. */
_refreshOutlineNotchWidth() {
if (!this._hasOutline() || !this._floatingLabel) {
return;
if (this._hasOutline() && this._floatingLabel) {
this._labelWidth = this._floatingLabel.getWidth();
}
this._labelWidth = this._floatingLabel.getWidth();
}

/** Invoked when the content of the label has changed. */
_labelContentChanged(): void {
this._refreshOutlineNotchWidth();
// Required since the changed might have happened after check,
// e.g. if the `required` state of the control has changed.
this._changeDetectorRef.detectChanges();
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the underlying control instead emit stateChanges?

Copy link
Member Author

Choose a reason for hiding this comment

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

The control already emits to stateChanges, but the asterisk itself doesn't show up. I think that some of the data bindings in the form field need to be re-evaluated.

Copy link
Member

Choose a reason for hiding this comment

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

That seems surprising to me, but I also haven't played with the form field in a while. I think the flow should be:

Control changes required -> emits state changes -> form field is marked for check -> on re-render, asterisk should render -> content change causes outline refresh (via cdk observe content).

I don't understand why we need an explicit detectChanges here (and no markForCheck), and also why this is needed at all. Just trying to make sure we understand it before forcing an explicit change detection. Although it's likely fair to say that the content of a label doesn't change often.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that your sequence of events is correct, but I believe that the MutationObserver is synchronous so we need to detect changes for the change to be picked up immediately instead of waiting for the next change detection.

Copy link
Member

Choose a reason for hiding this comment

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

What would be the problem with waiting for the next change detection? You mean, that might not happen "soon" because nothing asynchronous is triggering Zone/a new application tick? If so, that seems reasonable to me

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah so the problem is that it may take until the next user interaction for it get picked up. This could be problematic if required is based on a data binding.

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha. Good that we have this captured here. Ideally the comment would reflect some of these comments, but the PR is also sufficient I think.

}

/** Determines whether the content of the label should be observed for size changes. */
_shouldObserveLabelContentChanges(): boolean {
// Note that this could be simplified by checking `_isInitialized` in `_labelContentChanged`.
// We don't do it, because it doesn't allow us to skip the first emit which is unnecessary
// since we already measure the label or init. We want the observer to be set up *after*
// the initial content is in place.
return this._isInitialized && this._hasOutline();
}

/** Does any extra processing that is required when handling the hints. */
Expand Down
2 changes: 2 additions & 0 deletions tools/public_api_guard/material/form-field.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ export class MatFormField implements AfterContentInit, AfterContentChecked, Afte
_labelChildNonStatic: MatLabel | undefined;
// (undocumented)
_labelChildStatic: MatLabel | undefined;
_labelContentChanged(): void;
// (undocumented)
readonly _labelId: string;
_labelWidth: number;
Expand All @@ -137,6 +138,7 @@ export class MatFormField implements AfterContentInit, AfterContentChecked, Afte
_shouldForward(prop: keyof AbstractControlDirective): boolean;
// (undocumented)
_shouldLabelFloat(): boolean;
_shouldObserveLabelContentChanges(): boolean;
_subscriptAnimationState: string;
get subscriptSizing(): SubscriptSizing;
set subscriptSizing(value: SubscriptSizing);
Expand Down