Skip to content

Add highContrast to FakeAccessibilityFeatures test #48486

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.

Sign up for GitHub

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

Jump to bottom
Merged
fluttergithubbot merged 4 commits into from
Feb 19, 2020
Merged

Add highContrast to FakeAccessibilityFeatures test #48486

fluttergithubbot merged 4 commits into from
Feb 19, 2020

Conversation

@miquelbeltran
Copy link
Member

@miquelbeltran miquelbeltran commented Jan 9, 2020 •
edited
Loading

Description

Add the highContrast property to the FakeAccessibilityFeatures class.

The highContrast is going to be added to the class AccessibilityFeatures in the PR: flutter/engine#15343

Design change document: https://docs.google.com/document/d/1kePVlqWvJu5Ph0RL6wgg67F3SATmsJ8QY5N0S1MvaGg/edit

Related Issues

Related to #48418
Depends on flutter/engine#15343

Tests

  • The value is added to FakeAccessibilityFeatures which is used in the Window and MediaQueryData tests.

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.

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I signed the [CLA].
  • I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

I don't know how to use Flutter Analyze with a custom Flutter Engine, so I can't make it pass.

Test pass when I use my custom Flutter Engine.

Breaking Change

Did any tests fail when you ran them? Please read [Handling breaking changes].

  • No, no existing tests failed, so this is not a breaking change.
  • Yes, this is a breaking change. If not, delete the remainder of this section.

@fluttergithubbot fluttergithubbot added the framework flutter/packages/flutter repository. See also f: labels. label Jan 9, 2020
@fluttergithubbot
Copy link
Contributor

fluttergithubbot commented Jan 9, 2020

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.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

This was referenced Jan 9, 2020
Merged
Closed
@fluttergithubbot fluttergithubbot added the a: tests "flutter test", flutter_test, or one of our tests label Jan 10, 2020
@Piinks Piinks added a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) platform-ios iOS applications specifically and removed a: tests "flutter test", flutter_test, or one of our tests labels Jan 10, 2020
@Piinks
Copy link
Contributor

Piinks commented Jan 10, 2020

Hi @miquelbeltran! Welcome 🎉 and thank you for the contribution! I've checked in on the engine change this depends on. :)

@fluttergithubbot fluttergithubbot added the a: tests "flutter test", flutter_test, or one of our tests label Jan 12, 2020
Piinks
Piinks reviewed Jan 13, 2020
View reviewed changes
@miquelbeltran miquelbeltran changed the title Add support for Increase Contrast on iOS Add highContrast to FakeAccessibilityFeatures test Jan 14, 2020
@miquelbeltran miquelbeltran mentioned this pull request Jan 14, 2020
Merged
Piinks
Piinks reviewed Jan 14, 2020
View reviewed changes
Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

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

Thanks for splitting this out to land without breaking! :)

Piinks
Piinks reviewed Jan 15, 2020
View reviewed changes
Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

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

I am inclined to approve. @LongCatIsLooong, you had mentioned this being similar to another feature in flutter/engine#15343. @miquelbeltran provided some feedback there on that, do you have any objections to this change moving forward?

@dnfield
Copy link
Contributor

dnfield commented Jan 15, 2020

We should land the change in the engine before landing this.

We should separately land a change to the framework to make FakeAccessibilityFeatures override noSuchMethod so that the engine will bea ble to roll the change into the framework. See the way we do it on TestWindow for an example.

@Piinks
Copy link
Contributor

Piinks commented Jan 15, 2020

@dnfield is right. I forgot that the engine will complain when you add highContrast and it isn't overridden.

Here is the example of NoSuchMethod @dnfield mentioned:

flutter/packages/flutter_test/lib/src/window.dart

Line 427 in 1be332e

/// This gives us some grace time when the dart:ui side adds something to

Adding that will actually open the door to making changes like this easier in the future! :)

@miquelbeltran
Copy link
Member Author

miquelbeltran commented Feb 17, 2020

Sorry for the delay, I have added now the noSuchMethod override.

@LongCatIsLooong
Copy link
Contributor

LongCatIsLooong commented Feb 18, 2020

Could you pull changes from upstream again? That should fix the test failure.

miquelbeltran and others added 4 commits February 19, 2020 08:06
@miquelbeltran
Add support for Increase Contrast on iOS
859b6a2 
Reference ticket: #48418

Requires changes from the flutter/engine project.

Instead of a hard-coded false, read the value from the
window.accessibilityFeatures.

Add highContrast to fake window test
@miquelbeltran @Piinks
Update packages/flutter/lib/src/widgets/media_query.dart
d3cb1f3 
Reverted highContrast value, will be included in a new Pull-Request

Co-Authored-By: Kate Lovett <katelovett@google.com>
@miquelbeltran
Removed the @override to avoid analysis issues
a3b9c72
@miquelbeltran
add noSuchMethod to FakeAccessibilityFeatures
58a1ab0
@miquelbeltran
Copy link
Member Author

miquelbeltran commented Feb 19, 2020

Done!

Piinks
Piinks approved these changes Feb 19, 2020
View reviewed changes
Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

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

This LGTM! Thank you for your continued work on this! 🎉

@Piinks
Copy link
Contributor

Piinks commented Feb 19, 2020

Once this lands, we should be able to move on to the follow-up engine change you have staged in flutter/engine#15343 :)

@fluttergithubbot fluttergithubbot merged commit d295fbb into flutter:master Feb 19, 2020
@miquelbeltran miquelbeltran deleted the mb-48418-highcontrast branch February 20, 2020 10:15
@Piinks Piinks mentioned this pull request Feb 20, 2020
Merged
9 tasks
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@Piinks Piinks Piinks approved these changes

+1 more reviewer

@dnfield dnfield dnfield left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) a: tests "flutter test", flutter_test, or one of our tests framework flutter/packages/flutter repository. See also f: labels. platform-ios iOS applications specifically

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@miquelbeltran @fluttergithubbot @Piinks @dnfield @LongCatIsLooong @googlebot