RevisionGrid: Set accessibility values #9887

mstv · 2022-03-06T17:46:49Z

Fixes #9852

Proposed changes

Set missing accessibility Value for RevisionGrid columns:
- message
- commit id
Set empty string for accessibility property Name of all RevisionGrid cells
because it contained redundant information "Message Row nnn"

Screenshots

Before

After

Test methodology

manual: run "...\Windows Kits\10\bin\10.0.19041.0\x86\inspect.exe" and hover the grid cells

Test environment(s)

Git Extensions 33.33.33
Build 687a8e3
Git 2.35.1.windows.1
Microsoft Windows NT 10.0.19044.0
.NET 5.0.12
DPI 96dpi (no scaling)

Merge strategy

I agree that the maintainer squash merge this PR (if the commit message is clear).

✒️ I contribute this code under The Developer Certificate of Origin.

for message and commit id columns

gerhardol

Have not checked accessibility

gerhardol · 2022-03-06T20:42:04Z

GitUI/UserControls/RevisionGrid/Columns/CommitIdColumnProvider.cs

+        public override void OnCellFormatting(DataGridViewCellFormattingEventArgs e, GitRevision revision)
+        {
+            // Set the grid cell's accessibility text
+            if (!revision.IsArtificial)


Why limit artificial?
Is this displayed somewhere other than for accessibility?
(I do not see that when commenting out the test.)

Why limit artificial?

We do not want to display "111...1" anywhere.

Is this displayed somewhere other than for accessibility? (I do not see that when commenting out the test.)

No, it isn't. OnCellPainting draws the cell. I have just restored setting e.Value, which was removed by mistake in #8550.

RussKie · 2022-03-07T00:11:20Z

I don't think this change is addressing the original issue as the accessible name value remains "Message Zeile 2".

RussKie · 2022-03-07T00:30:35Z

@vladimir-krestov @SergeySmirnov-Akvelon @guybark do you think you could give us a hand here? I struggle to find a way to override DataGridViewTextBoxCell.DataGridViewTextBoxCellAccessibleObject's properties such as Name, Description, etc. Any thoughts on whether it is possible?
Thank you.

RussKie · 2022-03-07T00:35:31Z

I tried the following, and it didn't work:

diff --git a/GitUI/UserControls/RevisionGrid/RevisionGridControl.cs b/GitUI/UserControls/RevisionGrid/RevisionGridControl.cs
index 51c1561cc..aed592d60 100644
--- a/GitUI/UserControls/RevisionGrid/RevisionGridControl.cs
+++ b/GitUI/UserControls/RevisionGrid/RevisionGridControl.cs
@@ -345,6 +345,8 @@ internal bool MultiSelect
             set => _gridView.MultiSelect = value;
         }
 
+        internal DataGridViewCell GetCell(int row, int column) => _gridView.Rows[row].Cells[column];
+
         private static void FillMenuFromMenuCommands(IEnumerable<MenuCommand> menuCommands, ToolStripDropDownItem targetItem)

diff --git a/GitUI/UserControls/RevisionGrid/Columns/CommitIdColumnProvider.cs b/GitUI/UserControls/RevisionGrid/Columns/CommitIdColumnProvider.cs
index c1ba632c5..ea3d58350 100644
--- a/GitUI/UserControls/RevisionGrid/Columns/CommitIdColumnProvider.cs
+++ b/GitUI/UserControls/RevisionGrid/Columns/CommitIdColumnProvider.cs
@@ -37,6 +37,8 @@ public CommitIdColumnProvider(RevisionGridControl grid)
 
         public override void OnCellPainting(DataGridViewCellPaintingEventArgs e, GitRevision revision, int rowHeight, in CellStyle style)
         {
+            DataGridViewCell cell = _grid.GetCell(e.RowIndex, e.ColumnIndex);
+
             var monospaceFont = style.MonospaceFont;
             if (!_widthByLengthByFont.TryGetValue(monospaceFont, out var widthByLength))
             {
@@ -51,11 +53,19 @@ public override void OnCellPainting(DataGridViewCellPaintingEventArgs e, GitRevi
 
                 if (i == -1 && Column.Width > widthByLength[widthByLength.Length - 1])
                 {
-                    _grid.DrawColumnText(e, revision.ObjectId.ToString(), monospaceFont, style.ForeColor, e.CellBounds, useEllipsis: false);
+                    string hash = revision.ObjectId.ToString();
+                    cell.AccessibilityObject.Name = hash;
+                    cell.AccessibilityObject.Value = hash;
+
+                    _grid.DrawColumnText(e, hash, monospaceFont, style.ForeColor, e.CellBounds, useEllipsis: false);
                 }
                 else if (i > 1)
                 {
-                    _grid.DrawColumnText(e, revision.ObjectId.ToShortString(i - 1), monospaceFont, style.ForeColor, e.CellBounds, useEllipsis: false);
+                    string hash = revision.ObjectId.ToShortString(i - 1);
+                    cell.AccessibilityObject.Name = hash;
+                    cell.AccessibilityObject.Value = hash;
+
+                    _grid.DrawColumnText(e, hash, monospaceFont, style.ForeColor, e.CellBounds, useEllipsis: false);
                 }
             }
         }

IIUIC, any descendant of Control can set AccessibleName property, however DataGridViewTextBoxCell is not a descendant of Control type.

mstv · 2022-03-07T19:43:52Z

I don't think this change is addressing the original issue as the accessible name value remains "Message Zeile 2".

The issue description was unclear / confusing: #9852 (comment).

The Name property has always been "Message Row xxx". Up to v3.4.3 / #8550, the property Value contained the commit message. I have restored this.

@justanotheranonymoususer, please confirm whether https://ci.appveyor.com/api/buildjobs/cqfaxktufd5d845u/artifacts/artifacts%2FRelease%2Fpublish%2FGitExtensions-Portable-4.0.0.13552-f95404ed5.zip works for you.

justanotheranonymoususer · 2022-03-07T20:18:51Z

Yes, now it works as before. I still get "Message Row xxx" which I'd rather not get, but I also get the actual text which is what's important.

SergeySmirnov-Akvelon · 2022-03-10T12:18:53Z

@vladimir-krestov @SergeySmirnov-Akvelon @guybark do you think you could give us a hand here? I struggle to find a way to override DataGridViewTextBoxCell.DataGridViewTextBoxCellAccessibleObject's properties such as Name, Description, etc. Any thoughts on whether it is possible? Thank you.

Hi @RussKie. I was able to do this using inherited classes.

        ...
        this.Column1 = new DataGridViewTextBoxColumn() { CellTemplate = new SubDataGridViewTextBoxCell() };
        ...

        public class SubDataGridViewTextBoxCell : DataGridViewTextBoxCell
        {
            protected class SubDataGridViewTextBoxCellAccessibleObject : DataGridViewTextBoxCellAccessibleObject
            {
                public SubDataGridViewTextBoxCellAccessibleObject(DataGridViewCell? owner) : base(owner)
                {
                }

                public override string Name => $"Test Name {Owner.Value}";

                public override string Description => $"Test Description {Owner.Value}";
            }

            protected override AccessibleObject CreateAccessibilityInstance()
            {
                return new SubDataGridViewTextBoxCellAccessibleObject(this);
            }
        }

mstv · 2022-03-10T19:34:09Z

Thank you, @SergeySmirnov-Akvelon!

So, @justanotheranonymoususer, which properties should be set how? E.g.:

Name or Value = Commit Subject or Full Commit Message (incl. body)
Description = empty?

justanotheranonymoususer · 2022-03-10T19:36:56Z

Looking at other elements, e.g. browser tabs, taskbar button, I see that Name is set, and Value and Description are empty or not implemented. Thanks.

RussKie · 2022-03-11T07:24:01Z

Thank you @SergeySmirnov-Akvelon!

@mstv @justanotheranonymoususer: according to the docs, AccessibleName describes the element such as the text in a Button, the name of a MenuItem, or a label displayed next to a TextBox control, and AccessibleDescription convey information that is less tangible, such as "A button that shows a picture of a cactus".

So based on this information, I'd say for a row

AccessibleName would be something like "Commit Row 123",
AccessibleDescription would be something like "<Subject> by <Author> made on <date>. Hash <hash>",

...and each cell would have the following:

AccessibleName would be something like "<ColumnType> for Commit Row 123", where ColumnType is Author, Author avatar, Date, Commit hash, Subject, etc.
AccessibleDescription - I think we could leave it empty, because the name and the value make it redundant.
Value would contains the actual text the cell has, e.g., author name, the subject or the date.

We also have branches and tags information, and I'm not sure how these should be presented, i.e., should they be announced at the row level or the cell level?

The above is a description of a desired state, and I don't expect this to be implemented in this PR. We can start with the select columns and set the properties at the cell level.

Thoughts?

justanotheranonymoususer · 2022-03-11T09:57:07Z

As I said, I'd rather have only the content and not have "Commit Row 123" at all. That's also what I see in most other programs. They don't say "taskbar button" or "browser tab", they just have the content in "Name" or "Value", e.g. "Notepad" or "Google Search". "Description" is usually not set at all, and usually only one of "Name" or "Value" is used. I think it's better to look at popular programs and parts of the OS and see what they implement. I think it's a better guide than the docs, since that's what tools work with and optimized for in practice.

because it contained redundant information

mstv · 2022-03-12T20:47:02Z

Name is empty now. Value contains the major grid contents.

RussKie

Works for me if works for users with accessibility needs.
Thank you for doing this.

RevisionGrid: Set accessibility values

58163fd

for message and commit id columns

mstv self-assigned this Mar 6, 2022

gerhardol approved these changes Mar 6, 2022

View reviewed changes

RevisionGrid: Empty accessibility property Name

687a8e3

because it contained redundant information

RussKie approved these changes Mar 13, 2022

View reviewed changes

mstv merged commit 77a19b6 into gitextensions:master Mar 13, 2022

ghost added this to the vNext milestone Mar 13, 2022

mstv deleted the fix/9852_accessibility branch March 13, 2022 21:34

gerhardol mentioned this pull request May 12, 2022

Refresh regardless of formProcess results #9992

Merged

RussKie mentioned this pull request Jun 7, 2022

Refactoring AutomationId at the accessible object classes #6603 dotnet/winforms#7206

Merged

Uh oh!

RevisionGrid: Set accessibility values #9887

RevisionGrid: Set accessibility values #9887

Uh oh!

Conversation

mstv commented Mar 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes

Screenshots

Before

After

Test methodology

Test environment(s)

Merge strategy

Uh oh!

gerhardol left a comment

Choose a reason for hiding this comment

Uh oh!

gerhardol Mar 6, 2022

Choose a reason for hiding this comment

Uh oh!

mstv Mar 6, 2022

Choose a reason for hiding this comment

Uh oh!

RussKie commented Mar 7, 2022

Uh oh!

RussKie commented Mar 7, 2022

Uh oh!

RussKie commented Mar 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mstv commented Mar 7, 2022

Uh oh!

justanotheranonymoususer commented Mar 7, 2022 via email • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SergeySmirnov-Akvelon commented Mar 10, 2022

Uh oh!

mstv commented Mar 10, 2022

Uh oh!

justanotheranonymoususer commented Mar 10, 2022 via email • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RussKie commented Mar 11, 2022

Uh oh!

justanotheranonymoususer commented Mar 11, 2022 via email • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mstv commented Mar 12, 2022

Uh oh!

RussKie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mstv commented Mar 6, 2022 •

edited

Loading

RussKie commented Mar 7, 2022 •

edited

Loading

justanotheranonymoususer commented Mar 7, 2022 via email •

edited by ghost

Loading

justanotheranonymoususer commented Mar 10, 2022 via email •

edited by ghost

Loading

justanotheranonymoususer commented Mar 11, 2022 via email •

edited by ghost

Loading