Skip to content
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.

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

fix #12358 [HDPI] The "document" text in the “Generating Previews” dialog is truncated at >200% DPI #12363

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

Epica3055
Copy link
Member

@Epica3055 Epica3055 commented Oct 22, 2024

Fixes #12358

Proposed changes

Regression?

  • Yes

Risk

  • low

Screenshots

Before

Image

After

image
image
image

Test methodology

  • manual
Microsoft Reviewers: Open in CodeFlow

Copy link

codecov bot commented Oct 22, 2024

Codecov Report

Attention: Patch coverage is 5.71429% with 33 lines in your changes missing coverage. Please review.

Project coverage is 75.73717%. Comparing base (e8a65f9) to head (a23da97).
Report is 105 commits behind head on main.

Additional details and impacted files
@@                 Coverage Diff                 @@
##                main      #12363         +/-   ##
===================================================
+ Coverage   75.64857%   75.73717%   +0.08859%     
===================================================
  Files           3120        3159         +39     
  Lines         635396      636113        +717     
  Branches       46933       47001         +68     
===================================================
+ Hits          480668      481774       +1106     
+ Misses        151270      150868        -402     
- Partials        3458        3471         +13     
Flag Coverage Δ
Debug 75.73717% <5.71429%> (+0.08859%) ⬆️
integration 18.17861% <0.00000%> (-0.10380%) ⬇️
production 49.30080% <5.71429%> (+0.09681%) ⬆️
test 97.04530% <ø> (+0.01837%) ⬆️
unit 46.51736% <5.71429%> (+0.36854%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

---- 🚨 Try these New Features:

Copy link
Member

@LeafShi1 LeafShi1 left a comment

Choose a reason for hiding this comment

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

LGTM!

@lonitra
Copy link
Member

lonitra commented Oct 22, 2024

Changes seem fine to me, but could we do testing on this to make sure there are no other regressions

@lonitra lonitra added the 📬 waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) label Oct 22, 2024
@Epica3055 Epica3055 removed the 📬 waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) label Oct 30, 2024
This reverts commit c1c8ea3.
This reverts commit a7ff088.
…ng Previews” dialog is truncated at >200% DPI"

This reverts commit 7cf77c6.
This reverts commit 833c20a.
@Tanya-Solyanik Tanya-Solyanik added the 📬 waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) label Oct 30, 2024
@Philip-Wang01
Copy link
Contributor

Tested private dlls for this PR, here is the result.

For 11456, issue3(An error was detected by using Accessibility Insight tool for the "Page 1 of document" pattern) has been fixed. Currently only Narrator can't announce “Generating Previews”, NVDA and JAWS can announce it correctly, and isDialog and AccessibleName properties are correct.

For 12358, three regression HDPI issues (compared with the behaviors in .NET 8.0) still repro. See details in the team's channel.

@Philip-Wang01 Philip-Wang01 removed the 📬 waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) label Oct 31, 2024
@Tanya-Solyanik Tanya-Solyanik added the 📭 waiting-author-feedback The team requires more information from the author label Nov 1, 2024
@dotnet-policy-service dotnet-policy-service bot removed the 📭 waiting-author-feedback The team requires more information from the author label Nov 1, 2024
@LeafShi1 LeafShi1 added the 📬 waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) label Nov 1, 2024
@Tanya-Solyanik Tanya-Solyanik added the 📭 waiting-author-feedback The team requires more information from the author label Nov 1, 2024
@dotnet-policy-service dotnet-policy-service bot removed the 📭 waiting-author-feedback The team requires more information from the author label Nov 4, 2024
@Tanya-Solyanik
Copy link
Member

Tanya-Solyanik commented Nov 4, 2024

@Epica3055 - what accessible properties differ between the Label and the LinkLabel? Let's review them and any important property, like ControlType, can be fixed in a custom accessible object.

@Epica3055
Copy link
Member Author

@Tanya-Solyanik
I created a demo project.
image

@Tanya-Solyanik Tanya-Solyanik removed the 📬 waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) label Nov 5, 2024
@Tanya-Solyanik
Copy link
Member

Name and Automation Id properties are in your control, it will be set here - internal Label _messageLabel;
The problem is the child heirarchy. Could you please try to get rid of it, start by not having any links, then if that does not work, implement a custom accessible object that overrides the Navigation methods - FragmentNavigate, GetChild, GetChildCount

@Tanya-Solyanik Tanya-Solyanik added the 📭 waiting-author-feedback The team requires more information from the author label Nov 5, 2024
@dotnet-policy-service dotnet-policy-service bot removed the 📭 waiting-author-feedback The team requires more information from the author label Nov 6, 2024
@Epica3055 Epica3055 added the waiting-review This item is waiting on review by one or more members of team label Nov 7, 2024
Copy link
Member

@Tanya-Solyanik Tanya-Solyanik left a comment

Choose a reason for hiding this comment

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

Changes look good, could you please get them tested under different screen readers

@Tanya-Solyanik Tanya-Solyanik added 📬 waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) and removed waiting-review This item is waiting on review by one or more members of team labels Nov 12, 2024
@Philip-Wang01
Copy link
Contributor

Tested private dlls for this PR, here is the result.

For #11456, One behavior needs to be confirmed. Narrator still can't announce “Generating Previews”, NVDA announce it correctly, and isDialog and AccessibleName properties are correct.

For #12358, all HDPI issues have been fixed and there are no new regressions.

@Philip-Wang01 Philip-Wang01 removed the 📬 waiting-for-testing The PR is awaiting manual testing by the primary team; no action is yet required from the author(s) label Nov 12, 2024
@Tanya-Solyanik
Copy link
Member

Tanya-Solyanik commented Nov 13, 2024

Please investigate how to override AccessibleRole and ControlType for the new object

@LeafShi1 LeafShi1 added the waiting-review This item is waiting on review by one or more members of team label Nov 15, 2024
@Epica3055
Copy link
Member Author

@Tanya-Solyanik

When we focus on LinkLabel using AccessibleTool, we actually focus on Link.
So we need to change Link AccessibleRole , please review latest commit in this pr to see if that's acceptable.

@@ -142,6 +142,8 @@ private Color IEDisabledLinkColor
}
}

internal virtual AccessibleRole LinkAccessibleRole => AccessibleRole.Link;
Copy link
Member

Choose a reason for hiding this comment

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

Can we set the Control.AccessibleRole property instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we can.

JAWS announce “Page 1 of document” with the suffix changed from text to link, it announce “page 1 of document link”

The real problem here is in Link, not in LinkLabel.

It seems impossible to change Link.AccessibleRole from outside. So I changed our code to let Link.AccessibleRole rely on LinkLabel so that I can customize Link.AccessibleRole.


public partial class PrintControllerWithStatusDialog
{
private class FocusableLabel : LinkLabel
Copy link
Member

Choose a reason for hiding this comment

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

Note for the future - we might have other instances of dialogs where we need a focusable label, we'll "un-nest" this class if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-review This item is waiting on review by one or more members of team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[HDPI] The "document" text in the “Generating Previews” dialog is truncated at >200% DPI
6 participants