Terra Table Resize Fixes #1885
Terra Table Resize Fixes #1885
Conversation
…s for column resizing. Remove width updating in style for column header cells.
| @@ -138,6 +138,11 @@ const ColumnResizeHandle = (props) => { | |||
| resizeHandleRef.current.style.height = `${height}px`; | |||
| }; | |||
|
|
|||
| const handleFocus = () => { | |||
| setIsActive(true); | |||
There was a problem hiding this comment.
Why do we need add this change now?
There was a problem hiding this comment.
We were not accounting for other ways that the resize handle could be approached. We were only accounting for the datagrid approach (arrows). This logic accounts for other ways that resize handles are reached in components.
| @@ -105,7 +105,7 @@ describe('ColumnResizeHandle', () => { | |||
| it('sets the appropriate prop values on space keydown', () => { | |||
| const wrapper = mountWithIntl( | |||
| <ColumnContext.Provider value={{ setColumnHeaderAriaLiveMessage: jest.fn() }}> | |||
| <ColumnResizeHandle /> | |||
| <ColumnResizeHandle setIsActive={jest.fn()} /> | |||
There was a problem hiding this comment.
We are adding a new prop to pass a function, but it does not seem like we are testing to ensure it was called. What are we attempting to validate?
There was a problem hiding this comment.
I made this change because we were previously only setting this on keyboard navigation, but this should always be active regardless of how it was focused (besides mouse events). I have added comments and a mock to clarify the purpose of adding this parameter.
|
You don't have a JIRA in your PR template. |
There was a problem hiding this comment.
There have been recent changes in main, so make sure that the CHANGELOG entries are in the right section.
|
@mjpalazzo To validate accessibility, I set up a new example with Resizeable columns in Table: https://engineering.cerner.com/terra-framework/pull/1885/components/cerner-terra-framework-docs/table/examples/resizable-columns It should announce aria values when the columns are reached via tab, but not with mouse events. We should also test the resize handle behavior in WorklistDataGrid as well. It should not take on aria values when the resize is focused with the mouse, but should take on aria values when reached by the arrow keys. |
There was a problem hiding this comment.
Don't we already have example for column resizing? We should not need a new example to show this functionality.
There was a problem hiding this comment.
Turns out we did not have them previously. I added them because we did not have an example for a core feature that should have an example.
There was a problem hiding this comment.
We already have tests for column resizing. We should not need a new test file.
| @@ -15,6 +15,11 @@ afterAll(() => { | |||
| }); | |||
|
|
|||
| describe('ColumnResizeHandle', () => { | |||
| const mockSetIsActive = jest.fn(); | |||
There was a problem hiding this comment.
Why is this not just defined in the tests that require it? This does not seem like something we should have to validate every test.
There was a problem hiding this comment.
Since we're now calling setIsActive on every non-mouse focus event, we are required to set it in the test suites. I figured we ought to validate that the function is set/not set when appropriate on all the tests but if we feel strongly about it, then we can discuss this further.
| @@ -108,6 +108,8 @@ const ColumnResizeHandle = (props) => { | |||
|
|
|||
| // Ref variable for native resize handle element | |||
| const resizeHandleRef = useRef(); | |||
| // Ref variable to prevent active state on mouse drag | |||
| const handleFocus = useRef(true); | |||
There was a problem hiding this comment.
Can't we use the isActive prop? It seems like that would work without this extra logic.
There was a problem hiding this comment.
We do a similar implementation in DataGrid where we use the ref to control the onFocus callback. Since mouseDown also calls focus, we have to set this in order to prevent focus from being called as well.
There was a problem hiding this comment.
In that scenario, we don't have the isActive prop that we can leverage. Also, focus is being given to the DataGrid externally. Why is the isActive prop not sufficient would be my question.
|
@kenk2 - I tried validating the resizable columns example with JAWS, but the keyboard navigation was not working for me. I could Tab to the column resizing tool. JAWS announced "Press Enter to use. Column header, Patient , column 1 row" I pressed Enter and I no longer had focus on the resizing tool. Focus seemed to go to the frame around the table. Note also that in the example the Illness Severity and Visit column headers are buttons and can receive focus, but the other column headers are not interactive. |
Summary
What was changed:
Added tests for column resizing in table. Addressed an issue of column resizing not announcing aria values when in non-grid contexts. Also added examples for showing resizable columns in tables.
Why it was changed:
To fix some potential aria issues and to test the behavior of resizing columns for tables.
Testing
This change was tested using:
Reviews
In addition to engineering reviews, this PR needs:
Additional Details
This PR resolves:
UXPLATFORM-9864
Thank you for contributing to Terra.
@cerner/terra