Contenteditable table headers by SaschaCowley · Pull Request #15977 · nvaccess/nvda

SaschaCowley · 2023-12-28T11:20:21Z

Link to issue number:

Summary of the issue:

When navigating tables in contenteditable HTML elements in focus mode, table row and column headers are not reported.

Description of user facing changes

Table row and column headers are now reported when navigating tables in contenteditable HTML elements in focus mode.

Description of development approach

Added appropriate fields to the TextInfos.ControlField returned by CompoundTextInfo._getControlFieldForObject. Also added special case handling for Chromium, which erroneously reports heder cells as being their own headers.

Testing strategy:

Ran unit tests. Manually tested with a basic contenteditable table with row and column headers in Firefox and Edge.

Known issues with pull request:

None.

Code Review Checklist:

Documentation:
- Change log entry
- User Documentation -- unnecessary
- Developer / Technical Documentation -- unnecessary
- Context sensitive help for GUI changes -- unnecessary
Testing:
- Unit tests
- System (end to end) tests
- Manual testing
UX of all users considered:
- Speech
- Braille
- Low Vision
- Different web browsers
- Localization in other languages / culture than English
API is compatible with existing add-ons.
Security precautions taken. -- None should be needed

…eldForObject now has table row and column headers set when appropriate

Adriani90 · 2023-12-28T12:32:20Z

@SaschaCowley welcome to the NVDA project, and thank you very much for the contribution so far.
ctrl+alt+arrow keys navigates html tables in browse mode, not in focus mode. I guess the description in the referenced issue is talking about browse mode. Or am I wrong?

This should be tested in the Wordweb as well, so using an MS Office 365 account with Word Online should report table headers both in focus and in browse mode ideally.
Word Online is build based on content editable HTML elements, so it is a quite good use case to test content editable HTML elements support in NVDA.

Adriani90 · 2023-12-28T12:37:51Z

In Edge, row and column headers are reported even when on the row header or column header cell, meaning the cell's contents are spoken twice.

Is this also happening when using UIA for Edge and Chrome in NVDA advanced settings?

SaschaCowley · 2023-12-29T06:03:18Z

ctrl+alt+arrow keys navigates html tables in browse mode, not in focus mode. I guess the description in the referenced issue is talking about browse mode. Or am I wrong?

Ctrl+Alt+arrow keys navigates focused tables in focus mode, at least in contenteditable tables. See for example the demo provided by Mick in the original issue

This should be tested in the Wordweb as well, so using an MS Office 365 account with Word Online should report table headers both in focus and in browse mode ideally.

Great, I'll give this a try and report back.

In Edge, row and column headers are reported even when on the row header or column header cell, meaning the cell's contents are spoken twice.

Is this also happening when using UIA for Edge and Chrome in NVDA advanced settings?

When forcing the use of UIA in Edge, the double speaking of row and column headers does not occur. However, navigation by Alt+Ctrl+arrow keys stops working. I suspect this is a case of those scripts not being bound in that context. I can't seem to find an issue about this, does one exist already that we know of, or should I go ahead and creat one?

michaelDCurran · 2023-12-30T00:45:50Z

I don't think this PR needs to worry about Edge with UIA -- there are multiple issues there, which should be handled separately. Specifically:

NVDA does not provide table navigation commands on editable text or contenteditable nodes in Chromium when using UIA. This could in theory be solved by having NVDAObjects.UIA.chromium.ChromiumUIA also inherit from documentBase.DocumentWithTableNavigation.
A quick test also shows that the _getTableCellAt method which DocumentWithTableNavigation expects, may not yet be implemented for UIA.
The same code that was made in this PR (exposing row header and column header text) would also need to be made in NVDAObjects.UIA.UIATextInfo._getControlFieldForUIAObject.
So don't worry about Edge with UIA for this PR.

However, the issue with multiple reporting of row and column headers on header elements in Edge / chrome (without using UIA) probably should be handled in this PR (or another that blocks this one), as it becomes more noticeable.
Technically I think the bug is with Chromium for inappropriately exposing the same IAccessible2 object as its own column or row header.
But we could work around this perhaps in NVDAObjects.IAccessible.IAccessible._tableHeaderTextHelper by filtering out headers that have the same IAccessible2::uniqueID as the object whose headers we are fetching.

…le headers whose IAccessible::uniqueID matches that of the current cell

SaschaCowley · 2024-01-03T07:54:36Z

However, the issue with multiple reporting of row and column headers on header elements in Edge / chrome (without using UIA) probably should be handled in this PR (or another that blocks this one), as it becomes more noticeable. Technically I think the bug is with Chromium for inappropriately exposing the same IAccessible2 object as its own column or row header. But we could work around this perhaps in NVDAObjects.IAccessible.IAccessible._tableHeaderTextHelper by filtering out headers that have the same IAccessible2::uniqueID as the object whose headers we are fetching.

I have implemented this, and all seems to be working properly (based on manual and unit testing).

michaelDCurran

Looking good. But a few review comments.

AppVeyorBot · 2024-01-11T04:46:59Z

PASS: Translation comments check.
PASS: Unit tests.
FAIL: Lint check. See test results for more information.
PASS: System tests (tags: installer NVDA).
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/1bhle5398sp2krxf/artifacts/output/nvda_snapshot_pr15977-30619,e45c9844.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 1.0,
INSTALL_END 1.1,
BUILD_START 0.0,
BUILD_END 28.0,
TESTSETUP_START 0.0,
TESTSETUP_END 0.4,
TEST_START 0.0,
TEST_END 16.6,
FINISH_END 0.1

See test results for failed build of commit e45c984413

…terface to Iaccessible.IA2

SaschaCowley · 2024-01-12T04:09:32Z

This should be tested in the Wordweb as well, so using an MS Office 365 account with Word Online should report table headers both in focus and in browse mode ideally. Word Online is build based on content editable HTML elements, so it is a quite good use case to test content editable HTML elements support in NVDA.
I am getting errors unrelated to this PR (Identical when running from source or 2023.3) when attempting to navigate tables in focus mode in Word Web in Edge, so I suggest this not be a major point of testing for now. The PR seems to be working otherwise.

michaelDCurran · 2024-01-15T02:15:50Z

My understanding is that Word 365 online exposes tables as ARIA live regions and not as proper contenteditable structure yet. Office 365 Online still has a ways to go. Thus, I agree with Sascha that this pr should not worry about that. Message ID: ***@***.***>

…ble header

Co-authored-by: gerald-hartig <153188145+gerald-hartig@users.noreply.github.com>

gerald-hartig · 2024-01-31T03:29:06Z

Although I've approved it, please make sure you get @seanbudd to sign off on it too!

michaelDCurran

Looks like some extra blank lines were added to changes.t2t?

seanbudd · 2024-01-31T04:15:45Z

Please add "Fixes" before the issue number so GitHub automatically links the PR and issue.
Also please mark the final code review checklist items as checked, I assume you have manually tested the latest code changes?

Co-authored-by: Michael Curran <mick@nvaccess.org>

seanbudd · 2024-01-31T04:20:03Z

This PR can be merged when you are ready, the build passes and the description is up to date.
When merging PRs we generally perform a "squash merge" and copy paste the PR description into the commit message.
We use the following sections in the commit message:

Link to issue number
Summary of the issue
Description of user facing changes
Description of development approach
And if there are known issues, Known issues with pull request

So usually this is just copying the first 4 items, and sometimes the known issues.
You can check the master commit history for examples

AppVeyorBot · 2024-01-31T05:43:15Z

PASS: Translation comments check.
PASS: Unit tests.
PASS: Lint check.
FAIL: System tests (tags: installer NVDA). See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/tkpcwovfw76grf6y/artifacts/output/nvda_snapshot_pr15977-30902,367e7a51.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 1.0,
INSTALL_END 1.0,
BUILD_START 0.0,
BUILD_END 27.4,
TESTSETUP_START 0.0,
TESTSETUP_END 0.4,
TEST_START 0.0,
TEST_END 10.5,
FINISH_END 0.2

See test results for failed build of commit 367e7a517b

SaschaCowley · 2024-01-31T06:12:41Z

Please add "Fixes" before the issue number so GitHub automatically links the PR and issue.
Where do you mean?

seanbudd · 2024-01-31T06:53:08Z

No problem:

For example:

### Link to issue number:
Fixes #14113

AppVeyorBot · 2024-01-31T07:36:38Z

PASS: Translation comments check.
PASS: Unit tests.
PASS: Lint check.
FAIL: System tests (tags: installer NVDA). See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/wr7s1103hextircj/artifacts/output/nvda_snapshot_pr15977-30903,367e7a51.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 1.1,
INSTALL_END 1.0,
BUILD_START 0.0,
BUILD_END 28.8,
TESTSETUP_START 0.0,
TESTSETUP_END 0.4,
TEST_START 0.0,
TEST_END 10.4,
FINISH_END 0.2

See test results for failed build of commit 367e7a517b

AppVeyorBot · 2024-02-01T01:41:40Z

PASS: Translation comments check.
PASS: Unit tests.
PASS: Lint check.
FAIL: System tests (tags: installer NVDA). See test results for more information.
Build (for testing PR): https://ci.appveyor.com/api/buildjobs/a8m8wja0t63tsp37/artifacts/output/nvda_snapshot_pr15977-30908,367e7a51.exe
CI timing (mins):
INIT 0.0,
INSTALL_START 0.9,
INSTALL_END 0.9,
BUILD_START 0.0,
BUILD_END 27.4,
TESTSETUP_START 0.0,
TESTSETUP_END 0.4,
TEST_START 0.0,
TEST_END 10.6,
FINISH_END 0.1

See test results for failed build of commit 367e7a517b

stale

josephsl · 2024-02-07T02:00:35Z

Hi,

Wait, how was the merge possible (was Sascha approved to merge this PR)? Isn't it usually NV Access folks who can merge PR's? Just curious.
Thanks.

seanbudd · 2024-02-07T02:15:07Z

@SaschaCowley is our newest member of NV Access - I think further introductions will come in future

josephsl · 2024-02-07T02:19:21Z

Ah, makes sense – welcome, Sascha, and congratulations on the first PR merge!

cary-rowen · 2024-02-07T02:26:13Z

wow, welcome @SaschaCowley

SaschaCowley · 2024-02-07T04:22:55Z

Thanks, @josephsl and @cary-rowen, I'm excited to be here.

XLTechie · 2024-02-07T08:33:11Z

Welcome, @SaschaCowley! A welcome surprise!

Fixes nvaccess#14113 Summary of the issue: When navigating tables in contenteditable HTML elements in focus mode, table row and column headers are not reported. Description of user facing changes Table row and column headers are now reported when navigating tables in contenteditable HTML elements in focus mode. Description of development approach Added appropriate fields to the `TextInfos.ControlField` returned by `CompoundTextInfo._getControlFieldForObject`. Also added special case handling for Chromium, which erroneously reports heder cells as being their own headers.

SaschaCowley added 2 commits December 28, 2023 21:52

The textInfos.ControlField returned by CompoundTextInfo._getControlFi…

9d1dd23

…eldForObject now has table row and column headers set when appropriate

Updated changelog to note resolution of nvaccess#14113

69e5f42

Sascha Cowley added 4 commits January 3, 2024 16:20

Merge branch 'master' into conteditable_table_headers

e99cb8b

Updated IAccessible.IAccessible._tableHeaderTextHelper to exclude tab…

971088d

…le headers whose IAccessible::uniqueID matches that of the current cell

Updated copyright headers

5bb4e55

Fixed minor formatting issues

69de175

michaelDCurran requested changes Jan 4, 2024

View reviewed changes

Comment thread source/NVDAObjects/IAccessible/__init__.py Outdated

Comment thread source/NVDAObjects/IAccessible/__init__.py Outdated

Comment thread source/NVDAObjects/IAccessible/__init__.py Outdated

Comment thread source/NVDAObjects/IAccessible/__init__.py Outdated

seanbudd added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Jan 8, 2024

Sascha Cowley added 2 commits January 11, 2024 12:55

Merge branch 'master' into conteditable_table_headers

79fd263

Removed duplication and moved unnecessary code out of try blocks

65b491e

Sascha Cowley added 3 commits January 11, 2024 22:19

Resolved lint error

4c0ce6c

Merge branch 'master' into conteditable_table_headers

cbe9f66

Refactored IAccessible._tableHeaderTextHelper to use a single QueryIn…

fab4018

…terface to Iaccessible.IA2

michaelDCurran previously requested changes Jan 15, 2024

View reviewed changes

Comment thread source/NVDAObjects/IAccessible/__init__.py Outdated

gerald-hartig reviewed Jan 30, 2024

View reviewed changes

Comment thread source/NVDAObjects/IAccessible/__init__.py Outdated

Comment thread source/compoundDocuments.py Outdated

Sascha Cowley and others added 3 commits January 31, 2024 13:12

Merge branch 'master' into conteditable_table_headers

6baf45d

Added error checking when getting the initial IA2 pointer for each ta…

212b78c

…ble header

Apply suggestions from code review

86fc867

Co-authored-by: gerald-hartig <153188145+gerald-hartig@users.noreply.github.com>

SaschaCowley marked this pull request as ready for review January 31, 2024 02:27

SaschaCowley requested a review from a team as a code owner January 31, 2024 02:27

SaschaCowley requested a review from michaelDCurran January 31, 2024 02:27

gerald-hartig approved these changes Jan 31, 2024

View reviewed changes

michaelDCurran reviewed Jan 31, 2024

View reviewed changes

Comment thread user_docs/en/changes.t2t Outdated

Comment thread user_docs/en/changes.t2t Outdated

Apply suggestions from code review

0dd2100

Co-authored-by: Michael Curran <mick@nvaccess.org>

seanbudd approved these changes Jan 31, 2024

View reviewed changes

Merge branch 'master' into conteditable_table_headers

7baab88

SaschaCowley merged commit 65ddfb6 into nvaccess:master Feb 7, 2024

nvaccessAuto added this to the 2024.2 milestone Feb 7, 2024

SaschaCowley deleted the conteditable_table_headers branch August 22, 2024 05:03

Uh oh!

Conversation

SaschaCowley commented Dec 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Link to issue number:

Summary of the issue:

Description of user facing changes

Description of development approach

Testing strategy:

Known issues with pull request:

Code Review Checklist:

Uh oh!

Adriani90 commented Dec 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Adriani90 commented Dec 28, 2023

Uh oh!

SaschaCowley commented Dec 29, 2023

Uh oh!

michaelDCurran commented Dec 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SaschaCowley commented Jan 3, 2024

Uh oh!

michaelDCurran left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AppVeyorBot commented Jan 11, 2024

Uh oh!

SaschaCowley commented Jan 12, 2024

Uh oh!

Uh oh!

michaelDCurran commented Jan 15, 2024 via email

Uh oh!

Uh oh!

Uh oh!

gerald-hartig commented Jan 31, 2024

Uh oh!

michaelDCurran left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

seanbudd commented Jan 31, 2024

Uh oh!

seanbudd commented Jan 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AppVeyorBot commented Jan 31, 2024

Uh oh!

SaschaCowley commented Jan 31, 2024

Uh oh!

seanbudd commented Jan 31, 2024

Uh oh!

AppVeyorBot commented Jan 31, 2024

Uh oh!

AppVeyorBot commented Feb 1, 2024

Uh oh!

josephsl commented Feb 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seanbudd commented Feb 7, 2024

Uh oh!

josephsl commented Feb 7, 2024 via email

Uh oh!

cary-rowen commented Feb 7, 2024

Uh oh!

SaschaCowley commented Feb 7, 2024

Uh oh!

XLTechie commented Feb 7, 2024 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

SaschaCowley commented Dec 28, 2023 •

edited

Loading

Adriani90 commented Dec 28, 2023 •

edited

Loading

michaelDCurran commented Dec 30, 2023 •

edited

Loading

seanbudd commented Jan 31, 2024 •

edited

Loading

josephsl commented Feb 7, 2024 •

edited

Loading