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 Palette Properties Dialog Accessibility #25038

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

shubham-shinde-442
Copy link
Contributor

Resolves: #18880

I believe these changes improve the accessibility of this dialog to some extent, except for the spin box.

  • I signed the CLA
  • The title of the PR describes the problem it addresses
  • Each commit's message describes its purpose and effects, and references the issue it resolves
  • If changes are extensive, there is a sequence of easily reviewable commits
  • The code in the PR follows the coding rules
  • There are no unnecessary changes
  • The code compiles and runs on my machine, preferably after each commit individually
  • I created a unit test or vtest to verify the changes I made (if applicable)

@cbjeukendrup
Copy link
Contributor

It should be possible to do it for the spinboxes as well. Of course, that's slightly more complicated because they are in a Repeater, but that's easy to solve: you can use model.index to get the index.
So, you would do

IncrementalPropertyControl {
    …
    navigation.panel: navigationPanel
    navigation.order: 2 + model.index
    navigation.accessible.name: modelData["title"]
    …
}

    // And then in some control below…
    navigation.order: 2 + repeater.count

@shubham-shinde-442
Copy link
Contributor Author

Thanks for the suggestion @cbjeukendrup. I tried this approach earlier, but after navigating to the first spin box (i.e. Width), it's not possible to navigate to the other spin boxes using the arrow keys, which leads to issue #20515

@cbjeukendrup
Copy link
Contributor

As a workaround, one can use the left/right arrows instead of up/down, and sometimes one needs to press Esc to be able to move to the next one.
Since this is the case for all spin boxes currently in the app, I think it is preferable to add navigation support to those in this dialog too; then, when #20515 etc. are fixed, it will work automatically for this dialog too.
(Otherwise, if we don't add navigation support for those spin boxes now, it is annoying that we have to remember to do it later. And having them keyboard-accessible in some way is better than in no way at all.)

@shubham-shinde-442
Copy link
Contributor Author

@cbjeukendrup Ok, Updated.

}

ButtonBox {
width: parent.width

buttons: [ ButtonBoxModel.Cancel, ButtonBoxModel.Ok ]

navigationPanel.section: root.navigationSection
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we set the order too, so that it doesn't collide with the navigationPanel of the other items?

@cbjeukendrup
Copy link
Contributor

I found that the "palette cell properties" dialog (right-click on a specific palette cell to find it) also doesn't have navigation yet. Since that dialog is so similar, it might make sense to fix that one too in this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Keyboard arrows do not affect Palette cell properties text fields
2 participants