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

Conversation

@ferhatb
Copy link
Contributor

@ferhatb ferhatb commented Jan 11, 2021

Description

Voiceover fails to order elements if transform, left, top is not specified on semantics nodes. This PR disables the optimization to skip translation styles when matrix is identity and offset is 0.

PR also removes semantics test skips for Edge due to style order.

Related Issues

flutter/flutter#73347
flutter/flutter#50754

Tests

I added the following tests:

semantics_test.dart

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 [C++, Objective-C, Java style guides] for the engine.
  • I read the [tree hygiene] wiki page, which explains my responsibilities.
  • I updated/added relevant documentation.
  • All existing and new tests are passing.
  • I am willing to follow-up on review comments in a timely manner.

Reviewer Checklist

  • I have submitted any presubmit flakes in this PR using the [engine presubmit flakes form] before re-triggering the failure.

Breaking Change

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

@ferhatb ferhatb requested a review from yjbanov January 11, 2021 22:10
transform == null || isIdentityFloat32ListTransform(transform);

// On Mac OS and iOS, VoiceOver requires left=0 top=0 value to correctly
// handle order. See https://github.com/flutter/flutter/issues/73347.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this comment be closer to the _resetContainerOffsets and _resetElementOffsets functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

} else {
element.style
..top = '0px'
..left = '0px';
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if we use top/left in all browsers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea was that layout faster with transform.

@yjbanov
Copy link
Contributor

yjbanov commented Jan 12, 2021

lgtm

Comment on lines 856 to 857
_resetElementOffsets();
_resetContainerOffsets(containerElement);
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like these two methods do the exact same thing but on different html elements. Maybe consolidate them into one method that takes the element as an argument?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Done.

@ferhatb ferhatb merged commit 3f691f0 into flutter:master Jan 14, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 14, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 14, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 14, 2021
cbracken pushed a commit to flutter/flutter that referenced this pull request Jan 14, 2021
* d7b25f9 [web] Reland: Enable the new rich paragraph implementation (flutter/engine#23659)

* 507a887 fix ax unique id flake (flutter/engine#23518)

* e5a4c7c Roll Fuchsia Linux SDK from BUsKF6z4t... to a7ezEWPM5... (flutter/engine#23668)

* 7d1e4e3 Roll Skia from 0e4a29af9db2 to af9b58e287b5 (10 revisions) (flutter/engine#23670)

* 635f64a Roll Skia from af9b58e287b5 to f435ada424df (1 revision) (flutter/engine#23671)

* 6957611 Roll Fuchsia Mac SDK from BsUY1yjWh... to BoBy7Eobf... (flutter/engine#23672)

* 37131f1 Use non-deprecated SkImageFilter factory functions (flutter/engine#23649)

* 38f08ff Roll Fuchsia Linux SDK from a7ezEWPM5... to erwxDS3kf... (flutter/engine#23676)

* a56183c Roll Skia from f435ada424df to 8f78d5528438 (7 revisions) (flutter/engine#23680)

* 9d75996 Revert "[web] Enable the new rich paragraph implementation (#23162)" (flutter/engine#23687)

* b030bc8 Roll Dart SDK from 724d9e5e7d71 to 3629a798353d (1 revision) (flutter/engine#23681)

* a8a7534 Support Mice in iPadOS 13.4+ (flutter/engine#23636)

* 3f691f0 [web] Fix semantic node order for webkit (flutter/engine#23601)

* 70d08c6 Roll Skia from 8f78d5528438 to 890b2b406a60 (9 revisions) (flutter/engine#23692)

* 5459565 Roll Fuchsia Mac SDK from BoBy7Eobf... to DVYrV15dq... (flutter/engine#23693)

* 3fd4c63 [web] Fix letter spacing for rich paragraphs (flutter/engine#23683)

* effb529 Added missing export for the flutter engine group. (flutter/engine#23689)
gspencergoog pushed a commit to gspencergoog/engine that referenced this pull request Jan 20, 2021
@ferhatb ferhatb deleted the voiceover_order branch February 11, 2021 17:41
hjfreyer pushed a commit to hjfreyer/engine that referenced this pull request Mar 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants