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

Rewrite Alt+Right next-element functions as a tree-traversal #23890

Open
shoogle opened this issue Aug 4, 2024 · 4 comments
Open

Rewrite Alt+Right next-element functions as a tree-traversal #23890

shoogle opened this issue Aug 4, 2024 · 4 comments
Assignees
Labels
accessibility Issues related to accessibility internal Issues for the internal team P2 Priority: Medium Tech debt

Comments

@shoogle
Copy link
Contributor

shoogle commented Aug 4, 2024

The EngravingItem::nextElement() and EngravingItem::previousElement() virtual functions define the order in which elements are visited by the Alt+Right and Alt+Left navigation shortcuts.

The way these functions are currently written, they must be overridden by every EngravingItem subclass to determine what comes next in the sequence. However, there is no guarantee that the sequence is consistent, correct, or complete. Ideally, all interactive elements should be included, while all non-interactive elements (e.g. Page, System, etc.) should be excluded.

These functions could be greatly simplified by rewriting them as a tree-traversal, which could look something like this:

EngravingItem* EngravingItem::nextElement()
{
    // Return first child if there is one.
    for (EngravingItem* child : children()) {
        if (child) {
            return child;
        }
    }
    // Otherwise return next sibling in ancestor element.
    EngravingItem* current = this;
    while (EngravingItem* parent = current->parent()) {
        EngravingItemList siblings = parent->children();
        auto it = siblings.cbegin();
        for (;; ++it) {
            IF_ASSERT_FAILED(it != siblings.cend()) {
                break;
            }
            if (*it == current) {
                ++it;
                break;
            }
        }
        for (; it != siblings.cend(); ++it) {
            if (EngravingItem* sibling = *it) {
                return sibling;
            }
        }
        current = parent;
    }
    return nullptr;
}

EngravingItem* EngravingItem::nextNavigableElement()
{
    for (EngravingItem* next = nextElement(); next; next = next->nextElement()) {
        if (!next->flag(ElementFlag::NOT_SELECTABLE)) {
            return next;
        }
    }
    return nullptr;
}

But which tree should we traverse? There are several possible candidates for the parent() and children() functions.

Navigation Parent Navigation Children
EngravingObject::parent()
EngravingObject::explicitParent()
EngravingObject::children()
EngravingObject::scanParent() EngravingObject::scanChildren()
EngravingItem::parentItem(false /*explicit*/)
EngravingItem::parentItem(true /*explicit*/)
EngravingItem::childrenItems()
AccessibleItem::accessibleParent() AccessibleItem::accessibleChild(i)
AccessibleItem::accessibleChildCount()

We'll need to use the one that gives the closest match to the current next-element sequence.

We may still need to override nextElement in a few subclasses, for example, to ensure that navigation stays in the current staff rather than going up and down the segments.

@shoogle shoogle added P2 Priority: Medium accessibility Issues related to accessibility Tech debt labels Aug 4, 2024
@shoogle shoogle self-assigned this Aug 4, 2024
@shoogle shoogle unassigned Eism Aug 4, 2024
@shoogle
Copy link
Contributor Author

shoogle commented Aug 6, 2024

Advantages of making this change:

  1. Elements that are currently ignored by Alt+Right would now be included:
  2. All future engraving elements would be navigable with Alt+Right automatically.
    • We wouldn't need to remember to add things to the next-element functions.
    • Everything would be automatically included, unless we explicitly exclude it by making it non-selectable.
  3. Code would be easier to read, understand, and maintain.

@Leo-Cal
Copy link

Leo-Cal commented Aug 9, 2024

Hi! I would like to start contributing to MuseScore by tacking this issue. Is this alredy being worked on?

@shoogle shoogle added the internal Issues for the internal team label Aug 10, 2024
@shoogle
Copy link
Contributor Author

shoogle commented Aug 10, 2024

Hi @Leo-Cal! This is a core issue for accessibility and will set the direction for future development in this area. As such, I'd like to reserve this one for the internal team, but please feel free to tackle other issues, such as the ones on our community projects board. Thanks!

@MarcSabatella
Copy link
Contributor

Timing of this is interesting as I just saw a request from a blind user on the MuseScoreAccessibility mailing list at groups.io to have new commands to allow more direct access to the tree, much as screen readers usually present the accessibility tree of any window or web page and allow direct access to it. This came up in a discussion about having a "next element of same type" command that would allow navigation directly from, say, one tempo marking to the next. it would be nice to design this new tree traversal scheme in a way that could easily support such commands, or even present the accessibility tree directly to screen readers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility Issues related to accessibility internal Issues for the internal team P2 Priority: Medium Tech debt
Projects
Status: First release after the upcoming one
Development

No branches or pull requests

4 participants