adobe / spectrum-web-components

Spectrum Web Components
https://opensource.adobe.com/spectrum-web-components/
Apache License 2.0
1.24k stars 199 forks source link

[Bug]: Initial Focus Issue with Tabs Component in Spectrum Web Components #4590

Closed jianliao closed 1 month ago

jianliao commented 1 month ago

Code of conduct

Impacted component(s)

Tabs

Expected behavior

Preconditions:

  1. Tabs component has an initial selected tab set.
  2. TabsOverflow is enabled to allow horizontal scrolling when tabs exceed the available width.

Expected Behavior:

  1. The initial selected tab should be highlighted as selected.
  2. Horizontal scrolling arrow buttons should be displayed correctly if tabs exceed the width.
  3. Tabs component should not auto-focus

Actual behavior

  1. All UI elements render correctly.
  2. Upon initialization, the Tabs component automatically grabs the current focus.
  3. If the Tabs component is placed at the bottom of a long page, the page scrolls to the bottom on load, focusing on the Tabs component.

Screenshots

https://spectrum.corp.adobe.com/page/action-bar/

https://github.com/adobe/spectrum-web-components/assets/1207520/5c81086e-2197-4b65-9c4d-45d8a2d8086f

What browsers are you seeing the problem in?

Chrome

How can we reproduce this issue?

https://studio.webcomponents.dev/edit/Sw2YyLG4GfgevvzkYbm8/src/index.ts?p=stories

Sample code that illustrates the problem

<div class="container" style="max-width: 250px">
    <sp-tabs-overflow>
      <sp-tabs selected="1" size="m">
        <sp-tab label="Tab 1" value="1"></sp-tab>
        <sp-tab label="Tab 2" value="2"></sp-tab>
        <sp-tab label="Tab 3" value="3"></sp-tab>
        <sp-tab label="Tab 4" value="4"></sp-tab>
        <sp-tab label="Tab 5" value="5"></sp-tab>
        <sp-tab label="Tab 6" value="6"></sp-tab>
        <sp-tab label="Tab 7" value="7"></sp-tab>
        <sp-tab label="Tab 8" value="8"></sp-tab>
        <sp-tab-panel value="1">Content for Tab 1</sp-tab-panel>
        <sp-tab-panel value="2">Content for Tab 2</sp-tab-panel>
        <sp-tab-panel value="3">Content for Tab 3</sp-tab-panel>
        <sp-tab-panel value="4">Content for Tab 4</sp-tab-panel>
        <sp-tab-panel value="5">Content for Tab 5</sp-tab-panel>
        <sp-tab-panel value="6">Content for Tab 6</sp-tab-panel>
        <sp-tab-panel value="7">Content for Tab 7</sp-tab-panel>
        <sp-tab-panel value="8">Content for Tab 8</sp-tab-panel>
      </sp-tabs>
    </sp-tabs-overflow>
  </div>

Logs taken while reproducing problem

No response

jianliao commented 1 month ago

Related issue: #4032 cc: @Rocss

Rocss commented 1 month ago

@Rajdeepc me and Westbrook thought of making this behaviour optional, but did not see at that moment this edge case.

In this case, would this initial proposal be an accepted alternative? I know Westbrook had some comments on doing DOM calculations in this workflow, but I don't see a solution right now without them: https://github.com/adobe/spectrum-web-components/pull/4032/commits/1c1d7d1ec3d823cd55efd4b7b944a4a0d05a0be9#diff-b00b1c2026d53e2627110a96b16bd9a5a8247140577ab2a411d7cf2895fe4eb5R254

Open to any suggestions

Rajdeepc commented 1 month ago

I feel that auto-scroll into view should be auto detected at load and not optional for e.g if you make this optional how would you handle selected views in multiple tab views? But what Jian has pointed out would be a bad UX for many consumers of <sp-tabs-overflow/> element with this feature enabled by default. Do you feel we can check for scroll direction and stop the vertical scroll into view coz we would only want to enable this for horizontal tab views?

Rocss commented 1 month ago

Idk how we could stop the vertical scroll with the current scrollIntoView implementation, from my knowledge you can not specify the direction using this method.

An alternative here is to use the scrollTabs method from the Tabs and specify the delta it needs to (horizontally) scroll if the selected tab is not into view.

najikahalsema commented 1 month ago

Will be fixed in the next release by #4613