Fix the a11y scrolling issue #35899
Fix the a11y scrolling issue #35899
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
| // We neutralize the scroll position after all children have been | ||
| // updated. Otherwise the browser does not yet have the sizes of the | ||
| // child nodes and resets the scrollTop value back to zero. | ||
| semanticsObject.owner.addOneTimePostUpdateCallback(() { |
There was a problem hiding this comment.
Is addOneTimePostUpdateCallback still necessary?
There was a problem hiding this comment.
Several tests will fail if we call _neutralizeDomScrollPosition() directly here without using addOneTimePostUpdateCallback(). I think we still need to wait for the update to finish before attempting to neutralize the scroll position.
| // updated. Otherwise the browser does not yet have the sizes of the | ||
| // child nodes and resets the scrollTop value back to zero. | ||
| semanticsObject.owner.addOneTimePostUpdateCallback(() { | ||
| _neutralizeDomScrollPosition(); |
There was a problem hiding this comment.
_neutralizeDomScrollPosition updates verticalContainerAdjustment and horizontalContainerAdjustment. Do we need to call recomputePositionAndSize after this?
There was a problem hiding this comment.
I'm not sure. Was recomputePositionAndSize meant to be called every time we neutralize the position?
Currently we wait for everything to finish updating before we neutralize the scrolling position. Now is it possible that calling recomputePositionAndSize would cause unintended consequences and change the value of ScrollTop/ScrollLeft?
There was a problem hiding this comment.
Yes. More generally, recomputePositionAndSize should be called every time a parameter that affects it changes. Position and size depend on a whole bunch of stuff, like rect, transform, vertical and horizontal adjustments, etc. If any of them change and we don't call recomputePositionAndSize we risk the position and size to go out of sync with the current state of the semantics tree.
I see two ways to fix this:
- Remove
addOneTimePostUpdateCallback. This should work because roles are updated before callingrecomputePositionAndSize. However, as you mention above, this breaks some tests. - Call
recomputePositionAndSizeright after_neutralizeDomScrollPosition.
AFAICT, for the purposes of this bug fix either should work. Option 1 is probably a bigger yak shave though.
There was a problem hiding this comment.
I'll go with option 2 for now.
|
Gold has detected about 1 new digest(s) on patchset 9. |
yjbanov
left a comment
There was a problem hiding this comment.
Just a couple of non-blocking comments.
| }); | ||
| // We neutralize the scroll position after all children have been | ||
| // updated. Otherwise the browser does not yet have the sizes of the | ||
| // child nodes and resets the scrollTop value back to zero. |
There was a problem hiding this comment.
This comment is no longer accurate and can be removed. After this change we can set scrollTop to a non-zero value even on empty containers. We should find out why tests are failing when addOneTimePostUpdateCallback is removed, and if there's a valid reason then document it, otherwise update tests. This can be done later. No need to block this PR.
| // updated. Otherwise the browser does not yet have the sizes of the | ||
| // child nodes and resets the scrollTop value back to zero. | ||
| semanticsObject.owner.addOneTimePostUpdateCallback(() { | ||
| _neutralizeDomScrollPosition(); |
There was a problem hiding this comment.
Yes. More generally, recomputePositionAndSize should be called every time a parameter that affects it changes. Position and size depend on a whole bunch of stuff, like rect, transform, vertical and horizontal adjustments, etc. If any of them change and we don't call recomputePositionAndSize we risk the position and size to go out of sync with the current state of the semantics tree.
I see two ways to fix this:
- Remove
addOneTimePostUpdateCallback. This should work because roles are updated before callingrecomputePositionAndSize. However, as you mention above, this breaks some tests. - Call
recomputePositionAndSizeright after_neutralizeDomScrollPosition.
AFAICT, for the purposes of this bug fix either should work. Option 1 is probably a bigger yak shave though.
|
auto label is removed for flutter/engine, pr: 35899, due to - The status or check suite Linux Web Engine has failed. Please fix the issues identified (or deflake) before re-applying this label. |
* Fix the a11y scrolling issue * Add scroll overflow dom element * Add tests for scrollTop/scrollLeft * Documentation and code clean-ups * call recomputePositionAndSize() after neutralizing the scroll position * Remove unnecessary imports
Fixes the scrolling issue when using assistive technology.
When the assistive technology gets to the last item on a scrollable list, the browser sets the scrollTop/scrollLeft to zero and the user is not able to scroll back up/left anymore. This PR fixes the issue by adding a dummy DOM element to the end of the scrollable list so we can prevent scrollTop/scrollLeft from becoming zero and getting stuck.
Here is a test app with the current engine build: https://negarb-testapp.web.app/#/
And here is the test app using this fix: https://a11y-scroll-fixed.web.app/#/
Fixes flutter/flutter#104036
Pre-launch Checklist
writing and running engine tests.
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.