Snapshot: fix stuck focus in snapshots with hidden links

[abaevbog]

If some links are not currently visible (e.g. hidden in a collapsed menu), the focus won't land on them on Tab keypress. Then, moving focus into the content, or past it into context pane, is not possible. Here, we'll keep trying to shift focus forward or backward until the focus lands.

Fixes: zotero/zotero#5069

For reference, this is the new focus behavior in snapshot that currently gets stuck:

Screen.Recording.2025-02-27.at.5.31.52.PM.mov

[AbeJellinek]

Wouldn't this do the trick?

diff --git a/src/dom/common/dom-view.tsx b/src/dom/common/dom-view.tsx
index 8bb2b1a..ae5bee4 100644
--- a/src/dom/common/dom-view.tsx
+++ b/src/dom/common/dom-view.tsx
@@ -466,7 +466,8 @@ abstract class DOMView<State extends DOMViewState, Data> {
 				...this._annotationShadowRoot.querySelectorAll('[tabindex="-1"]')
 			] as (HTMLElement | SVGElement)[];
 			focusableElements = focusableElements.filter(
-				el => isPageRectVisible(getBoundingPageRect(el), this._iframeWindow, 0)
+				el => getComputedStyle(el).visibility === 'visible'
+					&& isPageRectVisible(getBoundingPageRect(el), this._iframeWindow, 0)
 			);
 			focusableElements.sort((a, b) => {
 				let rangeA;

[abaevbog]

The issue is that there are a few other instances when a focusable node would not actually be focusable, I think. Like if it's marked as [disabled], if it has 0 for width and height, if it has display:none, opacity: 0, if there is some layering happening with z-index, etc. In those cases, it would have visibility:visible but focus would still not land on them.

Since a snapshot can theoretically have any stylesheet, I thought it might be more error-resistant and future-proof to just keep going until focus actually lands. That way, it doesn't matter why the focus did not take. Otherwise, we would have to potentially enumerate all those instances that could interfere with focus, and it might be easy to miss some of them.

At the same time, maybe it is just easier to start with filtering based on a few most common properties (visibility, [disabled] and maybe display:none). And if there are snapshots that appear with more esoteric setups, we could adjust those filters.

[AbeJellinek]

Do the 0 width/height, opacity: 0, or low z-index conditions actually prevent focus? I don't think so! So this approach wouldn't catch those either.

In any case, we can keep the iterative approach, but let's:

• Move it inside _getFocusState()'s obj as moveFocus()
• Remove the focusableElements argument and use this.focusableElements
• Remove the index argument and use this.focusedElementIndex
• Right now this would get stuck if an unfocusable element were at the beginning/end, so move the wrapping logic ((this.focusedElementIndex + 1) % this.focusableElements.length for forward, (this.focusedElementIndex - 1 + this.focusableElements.length) % this.focusableElements.length for backward) into moveFocus(). Now that we're incrementing within the method, we should initially increment once above the for loop.
• In condition, check that i isn't equal to the starting index

[AbeJellinek]

If this isn't very clear, I can implement

[abaevbog]

Do the 0 width/height, opacity: 0, or low z-index conditions actually prevent focus? I don't think so! So this approach wouldn't catch those either.

You are correct! I must have misremembered the root of one of earlier VPAT issues when focus wouldn't land on the elements. opacity, z-index and width/height indeed do not prevent focus from landing on a node.

So then, the only instances when a link/button/etc. need to be skipped because they cannot be focused are: [disabled], visibility: hidden or display:none? If that's the case, it probably is easier to just tweak focusableElements.filter statement as you suggested to exclude elements in those 3 cases. Unless there is some addition benefit to the iterative approach (that I can't think of now). What do you think?

[AbeJellinek]

Yeah, let's just do that for now and see if anyone reports further issues.

We also don't need to handle disabled because <a> doesn't support it and we don't include <input>s in the tab order (they can't actually do anything in a snapshot), so we can do:

diff --git a/src/dom/common/dom-view.tsx b/src/dom/common/dom-view.tsx
index 8bb2b1a..173465d 100644
--- a/src/dom/common/dom-view.tsx
+++ b/src/dom/common/dom-view.tsx
@@ -465,9 +465,12 @@ abstract class DOMView<State extends DOMViewState, Data> {
 				...this._iframeDocument.querySelectorAll('a, area'),
 				...this._annotationShadowRoot.querySelectorAll('[tabindex="-1"]')
 			] as (HTMLElement | SVGElement)[];
-			focusableElements = focusableElements.filter(
-				el => isPageRectVisible(getBoundingPageRect(el), this._iframeWindow, 0)
-			);
+			focusableElements = focusableElements.filter((el) => {
+				let style = getComputedStyle(el);
+				return style.visibility === 'visible'
+					&& style.display !== 'none'
+					&& isPageRectVisible(getBoundingPageRect(el), this._iframeWindow, 0);
+			});
 			focusableElements.sort((a, b) => {
 				let rangeA;
 				if (a.getRootNode() === this._annotationShadowRoot && a.hasAttribute('data-annotation-id')) {

[abaevbog]

Yeah, that does exactly what we want!

One final question:

we don't include s in the tab order (they can't actually do anything in a snapshot)

In that specific wikipedia snapshot example, the buttons to show/collapse menus turn out to be actual <input>s, as odd as it is. Clicking on them will hide/expand this and other menus. It is currently excluded from taborder, though it probably should not be? Any reason to not include <input>s for cases like this? I think in this specific example, it doesn't really matter that much but perhaps there may be a snapshot where something important needs to be accessed by being able to click on an input-like element like this via keyboard?

[AbeJellinek]

Oh, funny, they're using a checkbox to toggle the menu using CSS. I've seen that in a few other places. The interactivity works without JavaScript, so it's still active in the reader.

I'm inclined not to support it, because I think supporting in-page interactions is confusing and goes against the spirit of having a consistent, unchanging snapshot of a page, but that's outside the scope of this PR.

[abaevbog]

Gotcha! That makes sense. I re-pushed just filtering out of unfocusable nodes with a brief comment

[AbeJellinek]

(Updated the commit message because this can affect EPUBs too.)