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.

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

Add the IAccessibleTextSelectionContainer interface #13400

Closed
wants to merge 1 commit into from
Closed

Add the IAccessibleTextSelectionContainer interface #13400

wants to merge 1 commit into from

Conversation

OzancanKaratas
Copy link
Collaborator

Link to issue number:

None

Summary of the issue:

None

Description of how this pull request fixes the issue:

@michaelDCurran commited in iaccessible2 repository. I updated it in the NVDA repository.

Testing strategy:

This is a submodule change. Test may not be necessary.

Known issues with pull request:

None

Change log entries:

Please refer the pull request in LinuxA11y/IAccessible2/#17.

@OzancanKaratas OzancanKaratas requested a review from a team as a code owner February 28, 2022 14:07
@LeonarddeR
Copy link
Collaborator

I guess it doesn't make that much sense to implement this as long as it is not used in NVDA"s code base.

@OzancanKaratas
Copy link
Collaborator Author

You're right, but it can be used in the future. @michaelDCurran, what do you think?

@AppVeyorBot
Copy link

See test results for failed build of commit dad7dd871f

@AppVeyorBot
Copy link

See test results for failed build of commit 41df307dae

@OzancanKaratas OzancanKaratas force-pushed the updateIAccessible2API branch from af7edf9 to 6f08ffa Compare March 3, 2022 18:59
@OzancanKaratas
Copy link
Collaborator Author

@michaelDCurran, tests fail. What should I do?

@AppVeyorBot

This comment was marked as off-topic.

Sign in to view
@seanbudd
Copy link
Member

seanbudd commented Mar 3, 2022

@OzancanKaratas - don't worry about unrelated test failures, we can retrigger the build as needed. I've done that now.

@AppVeyorBot
Copy link

See test results for failed build of commit c0868b92a3

@OzancanKaratas
Copy link
Collaborator Author

@seanbudd, thanks for your interest. I couldn't find the reason for the failure. Can you help me please?

@OzancanKaratas OzancanKaratas force-pushed the updateIAccessible2API branch from 2ce612b to 4c3a67b Compare March 6, 2022 03:56
@michaelDCurran
Copy link
Member

Note that the build failed due to a bug in the IAccessible2 IDL.
I have addressed this in LinuxA11y/IAccessible2#19

There is no immediate need to include IAccessibleTextSelectionContainer in NVDA, until Chrome or another web browser have implemented it.

@OzancanKaratas OzancanKaratas reopened this Mar 6, 2022
@AppVeyorBot
Copy link

See test results for failed build of commit e04120a8a1

@OzancanKaratas
Copy link
Collaborator Author

There is no immediate need to include IAccessibleTextSelectionContainer in NVDA, until Chrome or another web browser have implemented it.

Yes, but I wanted to prepare NVDA for the future.

@seanbudd seanbudd marked this pull request as draft March 9, 2022 05:07
@seanbudd
Copy link
Member

seanbudd commented Mar 9, 2022

Keeping this as a draft until this is ready

@OzancanKaratas OzancanKaratas force-pushed the updateIAccessible2API branch from 6ae50a5 to feb04bf Compare March 31, 2022 03:51
@OzancanKaratas OzancanKaratas marked this pull request as ready for review March 31, 2022 04:09
@OzancanKaratas
Copy link
Collaborator Author

@seanbudd, I apologize for the inconveniences. It is now ready.

@AppVeyorBot

This comment was marked as off-topic.

Sign in to view
@seanbudd seanbudd added the conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review. label Apr 19, 2022
@lukaszgo1
Copy link
Contributor

Given this comment by @michaelDCurran shouldn't this PR be closed as the new interface is not supported by any of the browsers currently.

@seanbudd seanbudd requested review from seanbudd and removed request for michaelDCurran June 14, 2022 04:20
@seanbudd
Copy link
Member

@OzancanKaratas - what does merging this solve? Is there a summary of the changes introduced by changing commits?

@seanbudd seanbudd added the blocked/needs-info The issue can not be progressed until more information is provided. label Jun 17, 2022
@seanbudd seanbudd added blocked/needs-external-fix and removed blocked/needs-info The issue can not be progressed until more information is provided. labels Jun 28, 2022
@seanbudd
Copy link
Member

Waiting on browsers to implement this

@seanbudd seanbudd marked this pull request as draft June 28, 2022 05:34
@seanbudd
Copy link
Member

seanbudd commented Jan 16, 2023 •
edited
Loading

It appears that chromium has implemented this:

I don't think it has been implemented in Firefox yet

seanbudd
seanbudd reviewed Jan 16, 2023
View reviewed changes
nvdaHelper/ia2_sconscript Outdated
@@ -44,11 +44,11 @@ idlFiles = [
'AccessibleStates.idl',
'Accessible2.idl',
'Accessible2_2.idl',
'Accessible2_3.idl',
Copy link
Member

Choose a reason for hiding this comment

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

why is this removed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because this has been removed from the original repository.

Copy link
Member

Choose a reason for hiding this comment

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

No it hasn't.
If you check the diff of this PR, that file has not been deleted in the submodule update.
The file is also on the master branch
https://github.com/LinuxA11y/IAccessible2/blob/master/api/Accessible2_3.idl

@seanbudd seanbudd marked this pull request as ready for review January 16, 2023 06:05
@AppVeyorBot
Copy link

See test results for failed build of commit ee13dca6c5

seanbudd
seanbudd approved these changes Jan 17, 2023
View reviewed changes
Copy link
Member

@seanbudd seanbudd left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @OzancanKaratas, I am going to hold off on merging until we can discuss and create an issue to make use of this on NVDA's side, now that chromium supports this

Edit: this is being tracked internally, just going to await @michaelDCurran to confirm this is ok to merge.

@seanbudd seanbudd added the blocked/needs-product-decision A product decision needs to be made. Decisions about NVDA UX or supported use-cases. label Jan 18, 2023
@seanbudd seanbudd added blocked and removed blocked/needs-product-decision A product decision needs to be made. Decisions about NVDA UX or supported use-cases. labels Feb 13, 2023
@seanbudd
Copy link
Member

Blocked by further work on the implementation by Chrome/Firefox

@robertknight robertknight mentioned this pull request Apr 4, 2023
Open
@seanbudd
Copy link
Member

seanbudd commented Jun 9, 2023

I'm closing this as invalid. There still needs to be work done on the chromium side

@seanbudd seanbudd closed this Jun 9, 2023
@OzancanKaratas OzancanKaratas deleted the updateIAccessible2API branch October 31, 2023 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seanbudd seanbudd seanbudd approved these changes

@michaelDCurran michaelDCurran Awaiting requested review from michaelDCurran

Assignees
No one assigned
Labels
blocked conceptApproved Similar 'triaged' for issues, PR accepted in theory, implementation needs review.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@OzancanKaratas @LeonarddeR @AppVeyorBot @seanbudd @michaelDCurran @lukaszgo1