fix(material-experimental/mdc-form-field): notched-outline should include prefixes and suffixes #18381
Conversation
…lude prefixes and suffixes When we landed the initial MDC form-field implementation, we intentionally left the outline appearance with prefixes and suffixes in a visually bad state. We did this because it was unclear how to proceed due to a limitation with the MDC notched-outline. After chatting more with the MDC team about this, and experimenting with more alternatives, the form-field will now follow MDC as close as possible. Instead of requiring changes upstream to MDC which aren't necessarily reasonable (due to us supporting arbitrary prefix/suffix content), we just use the notched-outline as intended. To summarize what happened before: Our form-field supports abitrary prefixes and suffixes. This is different in MDC, which always assumes a fixed width/size for prefixes and suffixes. Ultimately, this causes problems for us since in the outline appearance, the label should be relative to the infix container (not overlap prefixes), but still be aligned vertically over the prefix container. MDC achieves this by simply shifting the label by a fixed amount of pixels (since they only allow a specific width for prefixes and suffixes). This won't work for us since our prefix container can scale as much as possible. To reasonably fix this issue in a way that leverages/follows MDC as much as possible, we determine the prefix container width programmatically and shift the floating label horizontally based on the measured width. This is basically the same approach MDC takes.. just that we have a dynamic label offset measured at runtime. For more context on the issue, refer to the original feature request in MDC: material-components-web#5326.
devversion
added
P3
googlebot
added
the
cla: yes
| // If the directionality is RTL, the x-axis transform needs to be inverted. This | ||
| // is because `transformX` does not change based on the page directionality. | ||
| const labelHorizontalOffset = | ||
| (this._dir.value === 'rtl' ? -1 : 1) * prefixContainer.getBoundingClientRect().width; |
There was a problem hiding this comment.
We could also account for cases where the prefix containers start of invisible here, but I think that this is out-of-scope and very unlikely (it will also affect performance).
I saw that something like this has been done in the non-MDC based form-field for the outline gap. Though IMO we should not complicate these things, and rather provide/expose methods, so that people can refresh if needed.
There was a problem hiding this comment.
Am I reading this incorrectly, or is this calling getBoundingClientRect on every zone stable?
There was a problem hiding this comment.
getBoundingClientRect will be called only if the projected prefixes change, or if the appearance is set to outline from fill. The goal is to call this as less as possible.
The update is just scheduled on the next zone stable when the prefixes change, or if the appearance has been changed. The client rect won't be retrieved on every zone stable.
There was a problem hiding this comment.
I see, you're guarding it on the boolean. It seems like it would be more idiomatic rxjs to combine the _prefixChildren.changes stream with the zone stable stream somehow (though I don't actually know the right operators off the top of my head)
There was a problem hiding this comment.
I'm unsure if it's achievable with RxJS in a less complicated way. Combining both observables would complicate things since we also want to update if the appearance input changes.
We need logic like: Whenever prefix changes, or the appearance input changes, and the Zone becomes stable, update the offset. And important would be to only update once if both prefix and input change. FWIW: The same pattern is used in the non MDC-based form-field.
| // If the directionality is RTL, the x-axis transform needs to be inverted. This | ||
| // is because `transformX` does not change based on the page directionality. | ||
| const labelHorizontalOffset = | ||
| (this._dir.value === 'rtl' ? -1 : 1) * prefixContainer.getBoundingClientRect().width; |
There was a problem hiding this comment.
Am I reading this incorrectly, or is this calling getBoundingClientRect on every zone stable?
|
@jelbourn Thanks for trying the change in the dev-app. The vertical alignments are expected, since the prefix and suffixes align with the input baseline. This can be observed if you type into the form-fields with simple text prefixes/suffixes. For the icons we currently do not provide any specific styles to adjust the baseline (like it has been done in the non MDC-based one). The baseline is incorrect for the icons right now. The prefix and suffix containers currently are plain containers, and projected content must have a correct baseline to correctly align with the input element. I want to work on better alignment for |
…uld include prefixes and suffixes Address feedback; and handle SSR better
|
@jelbourn I addressed the feedback, and also found a way to make arbitrary prefixes render properly on the server. I think the overall arbitrary prefix/suffix API we need to support for backwards compatibility is not ideal. Personally, for me It makes sense to support it now for backwards compatibility (given that it's not too much additional bloat), but long-term we should limit the prefix/suffix to either:
This would simplify the MDC-based form-field, and would allow us to rely on MDC as much as possible. Also it would allow us better align with the Material Design specification since text prefixes/suffixes actually should behave differently. |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
When we landed the initial MDC form-field implementation, we
intentionally left the outline appearance with prefixes and suffixes in
a visually bad state. We did this because it was unclear how to proceed
due to a limitation with the MDC notched-outline.
After chatting more with the MDC team about this, and experimenting with
more alternatives, the form-field will now follow MDC as close as
possible. Instead of requiring changes upstream to MDC which aren't
necessarily reasonable (due to us supporting arbitrary prefix/suffix
content), we just use the notched-outline as intended.
To summarize what happened before: Our form-field supports abitrary
prefixes and suffixes. This is different in MDC, which always assumes a
fixed width/size for prefixes and suffixes. Ultimately, this causes
problems for us since in the outline appearance, the label should
be relative to the infix container (not overlap prefixes), but still
be aligned vertically over the prefix container. MDC achieves this by
simply shifting the label by a fixed amount of pixels (since they only
allow a specific width for prefixes and suffixes). This won't work for
us since our prefix container can scale as much as possible.
To reasonably fix this issue in a way that leverages/follows MDC as much
as possible, we determine the prefix container width programmatically
and shift the floating label horizontally based on the measured width.
This is basically the same approach MDC takes.. just that we have a
dynamic label offset measured at runtime.
For more context on the issue, refer to the original feature request in
MDC: material-components/material-components-web#5326.