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

Sufficient Technique C43 is not a sufficient technique #3187

Closed
brothercake opened this issue May 12, 2023 · 6 comments · Fixed by #3277
Closed

Sufficient Technique C43 is not a sufficient technique #3187

brothercake opened this issue May 12, 2023 · 6 comments · Fixed by #3277

Comments

@brothercake
Copy link

This technique doesn't work properly, in that:

  • it doesn't work for caret browsing
  • it incorrectly assumes that the footer height is known and fixed

A sufficient technique would use scroll-padding and would re-calculate it dynamically using a resize observer.

See the following for notes I made about this approach, based on Alastair's original proof of concept: https://www.tpgi.com/prevent-focused-elements-from-being-obscured-by-sticky-headers/#an-alternative-approach

@alastc
Copy link
Contributor

alastc commented May 26, 2023

Hi James,

We'll take another look at the technique, thanks for posting this.

It might be a case of qualifying the current technique, e.g. to be careful of wrapping text.

If you'd like to boil your approach down to a technique, we'd happily take a PR, or we could create one based on your approach if that's ok?

Given that the SC is focused on navigating interactive elements, I don't think that the lack of support when caret browsing is a problem.

@brothercake
Copy link
Author

If you'd like to boil your approach down to a technique, we'd happily take a PR, or we could create one based on your approach if that's ok?

I'm quite happy for you to use what I've done as it suits.

Given that the SC is focused on navigating interactive elements, I don't think that the lack of support when caret browsing is a problem.

Why not? Caret browsing still reaches interactive elements, isn't that something a user who needs it would have turned on all the time?

@fstrr
Copy link
Contributor

fstrr commented Jul 2, 2023

@brothercake We (well, mostly @adampage) took your example and adapted it to what's there for C43 at the moment. Can you take a look at this codepen and see if you have any feedback. Thanks.

fstrr added a commit that referenced this issue Jul 11, 2023
This update addresses issue 3187
1. Adds support for dialog element of unknown height.
2. Adds support for caret browsing.
3. Retitled C43 and working example as they now use `scroll-padding` instead of `margin-padding`.
4. Updated Resources links

Closes #3187

Signed-off-by: Francis Storr <francis.storr@intel.com>
@brothercake
Copy link
Author

@fstrr @adampage This looks great, works perfectly.

The only potential gap in its functionality is where viewport resizing happens on the fly (e.g. when an element is already focused, and resizing the window causes it to disappear behind the dialog), this doesn't trigger an immediate update, not until the focus moves to another element. But I'm not sure that's really necessary anyway.

@adampage
Copy link
Member

adampage commented Aug 1, 2023

Thank you, @brothercake.

Oof, that’s a good point. It’d be handy if the browser had some sort of RecalculateScrollPosition() helper for that situation, then I could just throw it into the ResizeObserver. 🤔

I suppose we could detect which element is currently focused, do some math, and then set documentElement.scrollTop accordingly, but that does seem like quite a bit of work for a scenario that I suspect will be uncommon?

@brothercake
Copy link
Author

brothercake commented Aug 2, 2023

@adampage Yeah I did this by evaluating boundary overlap in cases where the ResizeObserver fires, and an element is already focused, and that element is not inside the header, then calling scrollIntoView()

let focused = document.activeElement || document.body;
if(focused !== document.body && !header.contains(focused)) {
    if(focused.getBoundingClientRect().top < height) {
        focused.scrollIntoView();
    }
}

But the scenario is not particularly common, and most likely to only happen when a layout is being tested for reflow, rather than a user doing this. Even then, it's only a temporary situation that wouldn't fail 2.4.7 or 2.4.11, so I don't think it's essential.

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

Successfully merging a pull request may close this issue.

4 participants