-
Notifications
You must be signed in to change notification settings - Fork 30
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
feat(components): Improve FormField Toolbar UX #2133
Conversation
Deploying atlantis with Cloudflare Pages
|
We don't need it configurable as of right now at least.
To reproduce: * focus the text input to show the toolbar * click a button in the toolbar * notice its active outline is on top of the gradient instead of behind it
Published Pre-release for ef61c4d with versions:
To install the new version(s) for Web run:
|
To reproduce this: * type in lots of lines of text * shrink the form field height by dragging the bottom right corner up higher * scroll to the bottom of the text area * try scrolling again: it shouldn't * BEFORE: after reaching the bottom, another scrollbar for the parent container would appear and firefox would scroll further down and the toolbar would move upwards inside the container.
In chrome/safari/firefox, when adding new lines to the bottom of the input, the browser doesn't keep scrolling and the new lines eventually go behind the toolbar. Using scroll padding actually fixes chrome and safari here. But firefox is stubborn and seems to have bugs around their scroll padding implementation.
See previous commit - firefox/chrome/safari had a bug with new lines becoming hidden behind the toolbar while typing. I fixed it for chrome/safari, but firefox was stubborn. This new implementation uses height instead of padding+scroll-padding. This works for firefox/chrome/safari so far.
In the previous commit, I forgot to test scrolling text below the toolbar and noticed the gradient blend wasn't working because the textarea height didn't include it. This commit fixes the textarea's height and adds padding-bottom/scroll-padding-bottom to restore the behaviour where you're adding new lines to the bottom and chrome/safari will make sure the text is always visible as you're typing. Things brings back the firefox issue I mentioned in that earlier commit RE: scroll padding. However, it's not as bad as before because the padded area is only the height of the gradient blend... so the text is partially visible. But to make it less bad, I decided to cut that gradient in half for firefox only, and it looks not too bad now.
This actually reverts some changes made in an earlier PR: #1856 In that PR, the resize handle was moved to the "textarea" class, but that's multiple levels above the actual textarea input element. This was the cause of the safari scrollbar bug, where the scrollbar overlapped the resize handle, blocking it from being used. However, as far as I can tell, that was done to avoid the firefox bug where the resize handle is sitting slightly above the toolbar... which is now back. This change allowed me to delete all of the safari-specific css which removed the bad bottom padding which allows the text to scroll/flow seamlessly below the bottom of the element, exactly like chrome. Given the minefield of browser compatibility issues around scroll-padding, scrollbar behaviour, resize behaviour, etc... I think firefox having the resize handle above the toolbar actually isn't terrible and is the least problematic of the alternatives. One browser-specific quirk is better than two.
To reproduce the bug before this change: * focus the textarea * click a different window outside the browser * notice the toolbar shifted 1px * focus the textarea again * notice the toolbar shifted 1px back
.wrapper.textarea { | ||
overflow: hidden; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved this from line 106 to here, to make css lint happy. Note resize
was moved back to the actual textarea element (.textarea .input
).
See this comment.
The overflow hidden on toolbarWrapper was cutting off button focus outlines. Adding a margin to the toolbar instead of the toolbarWrapper avoids this by providing extra space around the sides.
&.textarea { | ||
/* This avoids a shift of content when the textarea is blurred due to the relative positioning | ||
added above. We should look into whether that's necessary at all, but it requires testing many | ||
instances of FormField. */ | ||
position: initial; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing position:relative
3 lines above is too risky... it's 3 year old code and affects every form field input.
For now, I'm opting to make the safer change, only targeting multiline InputText
.
.textarea.safari .horizontalWrapper { | ||
margin-right: var(--field--padding-right); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All safari overrides have been removed! See this comment.
.input { | ||
height: 100%; | ||
padding-bottom: var(--field--toolbarTotalHeight); | ||
scroll-padding-bottom: var(--field--toolbarTotalHeight); | ||
} | ||
|
||
/* When you're adding new lines to the bottom of a textarea, firefox doesn't respect scroll-padding | ||
and the text ends up partially hidden behind the toolbar gradient, until you manually scroll again. | ||
To make this less bad, we're shrinking the size of the gradient blend so most of the text is visible. */ | ||
&.firefox { | ||
.input { | ||
padding-bottom: var(--field--toolbarGradientHeight); | ||
scroll-padding-bottom: var(--field--toolbarGradientHeight); | ||
} | ||
|
||
.childrenWrapper { | ||
height: calc( | ||
100% - var(--field--toolbarTotalHeight) + | ||
var(--field--toolbarGradientHeight) | ||
); | ||
} | ||
|
||
.toolbar::before { | ||
--firefoxGradientHeight: calc(var(--field--toolbarGradientHeight) / 2); | ||
top: calc(var(--firefoxGradientHeight) * -1); | ||
height: var(--firefoxGradientHeight); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So here's where the implementations differ, due to Firefox's bugs around scroll-padding.
Chromium/Safari
When you add new lines to the bottom of the text input, chromium & safari take the scroll-padding-bottom into account and keep up with the new lines, ensuring they are always visible as you are typing.
chrome.mov
Firefox
In firefox, if you disable this .firefox
class override, when you add new lines to the bottom they end up getting hidden behind the toolbar. This is because firefox isn't taking the scroll-padding-bottom into account.
To fix this, I ended up overriding the implementation to do this:
- shrunk the height of the
childrenWrapper
so there's space for the toolbar and the textarea doesn't scroll behind it, it only scrolls behind the gradient blend - shrunk the height of the gradient so that MOST of the new line of text is visible, while still providing a decent smooth blending affect
So these are the 2 tradeoffs I think we should live with until firefox fixes this issue:
- a very small amount of the gradient covers the bottom of the new line of text, unless you scroll down further of course
- the right
resize
handle is ABOVE the toolbar
I think these 2 tradeoffs are much better than the alternative issues we had to work around. Chromium+Safari being great experiences, leaving FF to be slightly worse but still completely functional and accessible.
firefox.mov
import { InputValidation } from "../InputValidation"; | ||
import { isFirefox } from "../utils/getClientBrowser"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW we had a useIsSafari
hook which we don't need anymore, so I deleted it. Note we already had an isSafari
function inside this util file ^
@@ -99,15 +99,10 @@ function InputTextInternal( | |||
})); | |||
|
|||
useSafeLayoutEffect(() => { | |||
if ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment at the bottom of this file for context!
This check was preventing us from setting an explicit height on the textarea input. We need to set the height, otherwise it will auto-expand when the toolbar becomes visible.
wrapper.style.height = "auto"; | ||
textArea.style.flexBasis = textAreaHeight(textArea) + "px"; | ||
function resize(textArea: HTMLTextAreaElement) { | ||
textArea.style.height = textAreaHeight(textArea) + "px"; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reverts back to behaviour we had prior to ~7 months ago and fixes a safari scrollbar issue we had been living with.
See this commit for details: d68d4a4
This actually reverts some changes made in an earlier PR: #1856
In that PR, the resize handle was moved to the "textarea" class, but that's multiple levels above the actual textarea input element. This was the cause of the safari scrollbar bug, where the scrollbar overlapped the resize handle, blocking it from being used. However, as far as I can tell, that was done to avoid the firefox bug where the resize handle is sitting slightly above the toolbar... which is now back.
This change allowed me to delete all of the safari-specific css which removed the bad bottom padding which allows the text to scroll/flow seamlessly below the bottom of the element, exactly like chrome.
Given the minefield of browser compatibility issues around scroll-padding, scrollbar behaviour, resize behaviour, etc... I think firefox having the resize handle above the toolbar actually isn't terrible and is the least problematic of the alternatives. One browser-specific quirk is better than two.
👉 Before/After: Safari
Before
Before this change, we had the scrollbar offset from the right edge to avoid overlapping the resize
handle, which was blocking it from being used.
safari.before.mov
After
Because I restored the resize
handle on the textarea input, Safari correctly handles the scrollbar overlapping it.
safari.after.mov
👉 Before/After: Firefox
Before
Firefox had a double scrollbar due to the old implementation. The good thing was that the resize
handle was in a consistent spot, like other browsers.
firefox.before.mov
After
No more double scrollbar! But the resize
handle sits above the toolbar now, and I think that's completely fine as a tradeoff.
firefox.after.mov
Without this, if you pass rows with min=5 and height=5 for example, there's not enough room for the toolbar so the text flows behind it.
const IS_FIREFOX = isFirefox(navigator.userAgent); | ||
const TOOLBAR_HEIGHT = tokens["space-larger"]; | ||
const TOOLBAR_PADDING_BOTTOM = tokens["space-base"]; | ||
export const TOOLBAR_TOTAL_HEIGHT = TOOLBAR_HEIGHT + TOOLBAR_PADDING_BOTTOM; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The toolbar height has to be explicitly set to a fixed value to work correctly. For now, it is set to --space-larger
and we can easily extend this in the future via a prop to override this variable if we need it to be configurable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when I put a
Menu
inside the toolbar in the webstory, the border bottom gets thinner than the other sides
@ZakaryH this is because the Menu component appears to have a larger button height (40px) than our normal Buttons (32px)
The toolbar height currently is hardcoded, but as I mentioned above we can make this a prop if necessary, so the consumer can set the height to be larger.
I noticed this when clicking on content that would be where to toolbar appears (happens on all browsers). Not sure if this is a must fix. Toolbar.test.mov |
Without this, in firefox when the scrollHeight is smaller than the maxHeight, it was shrinking the textarea as you were typing.
If we render in SSR mode, the navigator check will fail.
Interesting find.. I'll look into it.
Yes, I need to add min-height there likely. |
another small detail, when the happens with both rows and no rows prop |
@@ -144,12 +136,8 @@ function InputTextInternal( | |||
} | |||
} | |||
|
|||
function resize(textArea: HTMLTextAreaElement, wrapper: HTMLDivElement) { | |||
if (rowRange.min === rowRange.max) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
previously when you didn't provide a rows prop for a multiline, it doesn't grow. it'll always stay the same size. here's how it behaves on this branch for a standard mutliline with no rows.
@ZakaryH good catch. That's due to me removing this line. When you don't supply rows, the default is min:3/max:3 so we never explicitly set the height in that case.
If I restore this, browsers will auto-expand the textarea to fit the toolbar.
I'm not sure I have a solution to address the issue you found yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah ok, I didn't have a chance to read over the code yet. I just wanted to do a few of the problematic flows I encountered on v1 of this.
yeah I'd be hesitant to remove the resize logic since that applies to normal, non toolbar multiline text inputs too. I can't remember if it's documented or not that we offer the not-growing behavior from omitting rows but I remember seeing a few instances like that in product which may be expecting or relying on that behavior
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be hesitant to remove the resize logic since that applies to normal, non toolbar multiline text inputs too
Great point here. This definitely could be fixed to not affect inputs without toolbars.
Yeah I think this is another example of the wall I hit here. The problems you're finding make me think we should stop going down this path.
|
||
const scrollHeight = | ||
textArea.scrollHeight + | ||
parseFloat(borderTopWidth) + | ||
parseFloat(borderBottomWidth); | ||
parseFloat(borderBottomWidth) + | ||
firefoxOffset; | ||
|
||
return Math.min(scrollHeight, maxHeight); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another small detail, when the
toolbarVisibility
is set toalways
there's still some change in the dimensions of the input when you start typing despite it looking like there's plenty of space to fit the new texthappens with both rows and no rows prop
@ZakaryH that's also related to resizing in general. I don't have a solution yet... this might be my wall for the path I took to get to this solution.
We've reached the limits of this UI approach. We have a new ticket (JOB-110270) for a different direction. |
@jdeichert Let's also chat. I ran into some issues when attempting to deal with the toolbar ux with magic fields and can provide another perspective on use case! |
Motivations
This PR aims to improve our UX around
InputText
when atoolbar
is supplied.The Problem
The InputText height grows upon focus to accommodate the toolbar. The problem with this is that it leads to lots of UI jumping around, causing a bad experience.
The Solution
Stop auto-expanding the textarea height and instead slide the toolbar up from the bottom of the input with absolute positioning so it doesn't affect the layout.
While making these adjustments, I also had to solve a number of bugs around scrollbar behaviours. I went through a few different iterations to land on the current solution and there are 2 very small firefox quirks I believe we should live with. These quirks are much more ideal than the alternatives I ran into while building towards this final solution. See this comment and this comment for before/after screen recordings and details.
Before / After
See comments for more screenshots/recordings of safari & firefox.
before.mov
after.mov
Changes
Changed
InputText
no longer auto-expands on focus when atoolbar
is suppliedresize
handle has moved back to the textarea input elementFixed
resize
handledisabled
Testing
TODO: I'm working on a JO draft PR to fix tests next.
To test this:
resize
handle is not blocked/still functionalresize
handle is above the toolbar when focused (see comment)Browserstack
I tested various browsers on windows.
Edge
windows.edge.mov
Firefox
windows.firefox.mov
Chrome
windows.chrome.mov