delegatesFocus interacts poorly with CSS scroll-margin

emilio commented 3 years ago

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 commented 3 years ago

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

emilio commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

emilio commented 3 years ago

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?

whatwg / html

delegatesFocus interacts poorly with CSS scroll-margin #7033