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.

Sign up for GitHub

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

Jump to bottom

delegatesFocus interacts poorly with CSS scroll-margin #7033

Open
emilio opened this issue Sep 6, 2021 · 5 comments
Open

delegatesFocus interacts poorly with CSS scroll-margin #7033

emilio opened this issue Sep 6, 2021 · 5 comments
Labels
topic: focus topic: shadow Relates to shadow trees (as defined in DOM)

Comments

@emilio
Copy link
Contributor

emilio commented Sep 6, 2021

The way delegatesFocus is implemented means that some css properties like e.g. scroll-margin don't work as authors expect. When I do something like:

<!doctype html>
<style>
  my-component {
    scroll-margin-top: 100px;
  }
</style>
<my-component></my-component>

And my-component delegates focus to something inside its shadow tree, I'd expect myComponent.focus() to honor that scroll margin. However we'd only look at the scroll margin of the scrolled-to element, which would be the delegated element (and which would have scroll-margin set to all zeros).

The component would have to explicitly inherit through scroll-margin, which seems bad? Perhaps scroll-margin should be inherited? Perhaps we should look at scroll-margin on shadow hosts and add it to the children scroll margin? Perhaps something else?

This came up because Firefox uses something somewhat similar to delegatesFocus (but hackier) in <input type=date> and somebody reported this bug.

cc @sefeng211 @whatwg/css @whatwg/components
@rniwa
Copy link

rniwa commented Sep 6, 2021

Is this an issue specific to delegatesFocus though? How does normal scrolling avoid this issue?

@annevk annevk added topic: focus topic: shadow Relates to shadow trees (as defined in DOM) labels Sep 7, 2021
@emilio
Copy link
Contributor Author

emilio commented Sep 7, 2021

What do you mean with normal scrolling? This is because of the scrollIntoView call that focusing implicitly does. When calling scrollIntoView on something, you use the scroll-margin set on that thing.

@rniwa
Copy link

rniwa commented Sep 7, 2021

What do you mean with normal scrolling? This is because of the scrollIntoView call that focusing implicitly does. When calling scrollIntoView on something, you use the scroll-margin set on that thing.

So this is an issue with scrollIntoView right, not necessarily just focus?

@emilio
Copy link
Contributor Author

emilio commented Sep 13, 2021

Well, sorta, I guess? When you scrollIntoView something, that doesn't do any retargeting, but focusing does.

@emilio
Copy link
Contributor Author

emilio commented Sep 13, 2021

Perhaps scroll-margin should be accumulated through ancestors though, like regular CSS margin is, so that if you have

<div style="scroll-margin-top: 100px">
  <div style="scroll-margin-top: 100px">

The effective scroll-margin-top would be 200px rather than 100?

i.e.,

<!doctype html>
<style>
  body { margin: 0 }
  .padding {
    height: 400vh;
  }
  .margin {
    scroll-margin-top: 100px;
  }
  #content {
    background-color: green;
    width: 100px;
    height: 100px;
  }
</style>
<a href="#content">Scroll</a>
<div class="padding"></div>
<div class="margin">
  <div class="margin" id="content"></div>
</div>
<div class="padding"></div>

@tabatkins @fantasai I wonder why scroll-margin wasn't defined to accumulate using ancestor boxes, like regular margin is? I guess it does make the implementation quite a bit trickier, but seems to me that'd be a more sensible behavior?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
topic: focus topic: shadow Relates to shadow trees (as defined in DOM)
Milestone
No milestone
Development

No branches or pull requests
3 participants
@rniwa @emilio @annevk