Reverse the semantics order of modal barrier and modal scope #59290
Conversation
fluttergithubbot
added
the
framework
|
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. Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
@goderbauer would you take a look at this pr? I don't have test yet since i would like to have some feedback if this the approach we want to take. |
There was a problem hiding this comment.
Will this mess up hit testing on e.g. Android? I would expect that on Android whenever you try to touch anything in the alert to a11y focus it, you’ll actually focus the modal barrier now...
|
Can you use sortKey wrapped around the barrier and the modal scope to achieve the same affect in a simpler way (and without potentially messing up hittest order? |
chunhtai
force-pushed
the
issues/46625
branch
2 times, most recently
from
798e49a to
8b469a8
Compare
|
Hi @goderbauer I used the sort key, and it seemed to be a cleaner way to solve this problem. This however, broke a lot of semantics test because we add a new semantics node above each modal route. Before I proceed to fix all the failing tests, do you think this is the approach we want to take? |
goderbauer
left a comment
goderbauer
left a comment
There was a problem hiding this comment.
Using sortKey is the right fix for this (can you verify on real devices (Android and iOS) that this all works as expected?).
This will probably also break tests internally in google. We'll have to come up with a migration path.
There was a problem hiding this comment.
nit: revert formatting change?
There was a problem hiding this comment.
nit: remove this empty line?
|
@goderbauer can you take another look at this? |
goderbauer
left a comment
goderbauer
left a comment
There was a problem hiding this comment.
LGTM after comments are resolved.
We really need to find a better way to write semantics tests to make them less brittle. This change shouldn't really require so many unrelated test changes...
There was a problem hiding this comment.
Should this be semanticsDismissible instead of barrierDismissible?
There was a problem hiding this comment.
nice catch, actually it should check both. the semanticsDismissible name is confusing, it means the semantics should include this modal barrier, barrierDismissible is the actual one whether the barrier is selectable. If the barrierDismissible = false and semanticsDismissible = true, the barrier is still excluded from the semantics tress.
There was a problem hiding this comment.
Maybe add a comment here: // To be sorted after the _ModalScope.
There was a problem hiding this comment.
Maybe add a comment here: // To be sorted before the ModalBarrier
There was a problem hiding this comment.
Please add: "// Regression test for ."
There was a problem hiding this comment.
Doesn't the barrier have a label and/or action we can use to identify it better? This could just be any stray semantics node...
…#59290) * Add semantics sort key for modal scope and modal barrier * fix test * fix test * fix test * fix space * fix more tests * addressing comments
4 participants
Description
This will change the order that semantics focus traverse.
Related Issues
Fixes #46625
Tests
I added the following tests:
TBD, will add test once we are ok with this approach.
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.///).flutter analyze --flutter-repo) does not report any problems on my PR.Breaking Change
Did any tests fail when you ran them? Please read Handling breaking changes.