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 : Automatic focus jump during tab navigation #10109

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

Conversation

Darshan-upadhyay1110
Copy link
Contributor

Accessibility: Set focus on the first or selected tab in dialogs with tabs

  • Updated logic to check if the dialog contains tabs (tabcontrol).
  • If tabs are present, focus is now set on the first tab (tabcontrol-1) or the selected tab, instead of the first element of the entire widget.
  • This ensures that the user lands on the appropriate tab when interacting with tabbed dialogs.

Fix: Prevent focus on inner elements when tab control is present

  • Modified 'grab_focus' case to ignore focusing on specific elements if the widget contains a tab control.
  • When tabcontrol is present, focus will remain on the tab itself rather than any element inside the dialog.
  • Ensures better navigation and user experience in dialogs with tabs.

… tabs

- Updated logic to check if the dialog contains tabs (`tabcontrol`).
- If tabs are present, focus is now set on the first tab (`tabcontrol-1`) or the selected tab, instead of the first element of the entire widget.
- This ensures that the user lands on the appropriate tab when interacting with tabbed dialogs.

Signed-off-by: Darshan-upadhyay1110 <[email protected]>
Change-Id: Ia22734b39c414fa11b2948c86dee6a77fd72b5c8
- Modified 'grab_focus' case to ignore focusing on specific elements if the widget contains a tab control.
- When `tabcontrol` is present, focus will remain on the tab itself rather than any element inside the dialog.
- Ensures better navigation and user experience in dialogs with tabs.

Signed-off-by: Darshan-upadhyay1110 <[email protected]>
Change-Id: Idfefaf931f8a3a3f9354cf2a5930c0d035dec830
else
console.error('cannot get focus for widget: "' + instance.init_focus_id + '"');
// Check if the element with id 'tabcontrol' exists in the DOM
const isTabControlWidget = document.getElementById('tabcontrol') ? true : false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not use hardcoded ids of elements.
This can find any tabcontrol, not in the current dialog too.
If we want, we can scan the tree of widgets (JSON) for widget with type: tabcontrol

firstFocusableTab.focus();
}
else {
const focusWidget = instance.init_focus_id ? instance.container.querySelector('[id=\'' + instance.init_focus_id + '\']') : null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Here we have explicitly defined element which should be focused as wanted by the dialog designer.
I think it should be always prefered option.

What is the example of dialog/widget it didn't work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Menu "References" ( Dialog "Table of Contents")

control.focus();
// We will ignore focus on a specific element of the widget if the widget is tabControl.
// Focus should be on Tab and not on any element inside dialog if tabcontrols are present
if (!document.getElementById('tabcontrol'))
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not use hardcoded ids, also this looks for any element in the whole webpage, not in the current dialog's container.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes i think i can improve this here to only search for tabcontrol for open dialogs and not in entire document ?

@eszkadev
Copy link
Contributor

What is the example of wrong behavior and in which dialog? I think we shouldn't modify the core's dialog behavior. Only make sure it is exactly the same.

@Darshan-upadhyay1110
Copy link
Contributor Author

What is the example of wrong behavior and in which dialog? I think we shouldn't modify the core's dialog behavior. Only make sure it is exactly the same.

Menu "References" ( Dialog "Table of Contents")

Focus should be on Tab and not on the first element of the dialog. Arrow keys won't work if we do focus on first element of dialogs which have tabs.

@Darshan-upadhyay1110
Copy link
Contributor Author

I think we shouldn't modify the core's dialog behavior. Only make sure it is exactly the same

I think for some exception we need to change behavior here, otherwise the accessibility part will be broken for dialog. User can not move around tabs at intended.

@eszkadev
Copy link
Contributor

What is the example of wrong behavior and in which dialog? I think we shouldn't modify the core's dialog behavior. Only make sure it is exactly the same.

Menu "References" ( Dialog "Table of Contents")

Focus should be on Tab and not on the first element of the dialog. Arrow keys won't work if we do focus on first element of dialogs which have tabs.

Thanks. I see. As I said we should keep behavior consistent online vs core. I testes that dialog and in LO it opens with focus on a tab control. That means: we failed to focus the right element in online.

Could you check if:

  • in the JSON we receive correct init_focus_id ?
  • if not, the dialog has default focus widget set in the .ui in the core?
  • if we don't have defined default focus in JSON, probably we should take into account tabs as focusable widgets, so our logic will fint them as "first widget in the dialog"

@Darshan-upadhyay1110
Copy link
Contributor Author

Could you check if:

  • in the JSON we receive correct init_focus_id ?

it's => "title" and element will be div#title.ui-edit-container.jsdialog.ui-grid-cell
So basically focus widget is inside first tab and not the tab itself

@Darshan-upadhyay1110
Copy link
Contributor Author

Darshan-upadhyay1110 commented Sep 26, 2024

This is the patch that is sending info about first focusable element inside dialog ( maybe here we are not considering if tabs are present or not ) https://gerrit.libreoffice.org/c/core/+/126565

  • if we don't have defined default focus in JSON, probably we should take into account tabs as focusable widgets, so our logic will fint them as "first widget in the dialog"

@eszkadev even if there is defined default focus, can we make some change in online to see if tabs are there the force to focus on Selected tab ? it is a very basic requirement for accessibility to work (in any case we consider)

This can happen in future if someone adds a new UI file in CORE and messed up with init_focus_id over there, which ends up in online to break this focus thing again...

@eszkadev
Copy link
Contributor

This is the patch that is sending info about first focusable element inside dialog ( maybe here we are not considering if tabs are present or not ) https://gerrit.libreoffice.org/c/core/+/126565

  • if we don't have defined default focus in JSON, probably we should take into account tabs as focusable widgets, so our logic will fint them as "first widget in the dialog"

@eszkadev even if there is defined default focus, can we make some change in online to see if tabs are there the force to focus on Selected tab ? it is a very basic requirement for accessibility to work (in any case we consider)

This can happen in future if someone adds a new UI file in CORE and messed up with init_focus_id over there, which ends up in online to break this focus thing again...

ok, then we can do it, but please do not use hardcoded id's. Please search in JSON for widget type (tab control), get it's id and use.

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

Successfully merging this pull request may close these issues.

2 participants