Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Remove more asserts and fix a11y check #8896

Merged
dnfield merged 3 commits into from
May 8, 2019
Merged

Remove more asserts and fix a11y check #8896

dnfield merged 3 commits into from
May 8, 2019

Conversation

@dnfield
Copy link
Contributor

@dnfield dnfield commented May 8, 2019

This converts the rest of the AssertionError throws from my previous PR to Log.e.

Also fixes a Log.e in the accessibility bridge so that it won't itself throw if the index is out of range.

Makes flutter/flutter#32047 not be a crasher/TODAY bug.

@dnfield dnfield mentioned this pull request May 8, 2019
Closed
@dnfield dnfield requested review from jason-simmons and mklim May 8, 2019 06:54
@dnfield dnfield requested review from goderbauer and jonahwilliams and removed request for jason-simmons and mklim May 8, 2019 08:00
@dnfield
Copy link
Contributor Author

dnfield commented May 8, 2019 •
edited
Loading

I'm updating this PR to remove some accessibility logs entirely. They seem wrong to me at this point.

The assert wants to believe that scrollIndex is relative to childrenInHitTestOrder, when really it's relative to scrollChildren. In a sliver list, the scrollChildren might some large number and the childrenIn*Order is only the children being painted (+ a few on either end for hidden ones). We have no good way to go to or from the scrollIndex to an index in childrenInHitTestOrder. The assert here never functioned before, and it doesn't make sense to start logging about this now.

It is definitely possible to have the accessibility bridge get something wrong about what's visible, but this doesn't catch that properly.

/cc @goderbauer @jonahwilliams @yjbanov

@goderbauer
Copy link
Member

goderbauer commented May 8, 2019

This converts the rest of the AssertionError throws from my previous PR to Log.e.

Can you link to the previous PR in this PR's description for future archeologists?

goderbauer
goderbauer approved these changes May 8, 2019
View reviewed changes
Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dnfield
Copy link
Contributor Author

dnfield commented May 8, 2019

Related: #8821, #8881

@dnfield dnfield merged commit 5d1c1a0 into flutter:master May 8, 2019
@dnfield dnfield deleted the no_throw branch May 8, 2019 08:19
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 8, 2019
@engine-flutter-autoroll
5d1c1a0 Remove more asserts and fix a11y check (flutter/engine#8896)
bc19fc5
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 8, 2019
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 8, 2019
@engine-flutter-autoroll
5d1c1a0 Remove more asserts and fix a11y check (flutter/engine#8896)
a52e2e0
engine-flutter-autoroll added a commit to flutter/flutter that referenced this pull request May 8, 2019
@engine-flutter-autoroll
Roll engine ac4f3c9..1604a41 (3 commits) (#32316)
d6c4e36 
flutter/engine@ac4f3c9...1604a41

git log ac4f3c9..1604a41 --no-merges --oneline
1604a41 Roll src/third_party/skia f90bfd72174b..15c91422339a (1 commits) (flutter/engine#8900)
3fccf83 Roll src/third_party/skia 4497ac179949..f90bfd72174b (2 commits) (flutter/engine#8897)
5d1c1a0 Remove more asserts and fix a11y check (flutter/engine#8896)

The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md

If the roll is causing failures, please contact the current sheriff (chinmaygarde@google.com), and stop
the roller if necessary.
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 8, 2019
Merged
huqiuser pushed a commit to huqiuser/engine that referenced this pull request Jun 12, 2019
@dnfield
Remove more asserts and fix a11y check (flutter#8896)
c00f55f
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jonahwilliams jonahwilliams Awaiting requested review from jonahwilliams

1 more reviewer

@goderbauer goderbauer goderbauer approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dnfield @goderbauer @googlebot