Storybook: S and A hotkeys are swapped after entering Fullscreen mode

[jalovatt]

Describe the bug
After entering full-screen mode, the first press of s or a is swapped - s opens the addon panel, and a opens the sidebar. Subsequent presses behave correctly, but the issue repeats if "Go Fullscreen" is selected again.

To Reproduce

1. Create a temporary folder and navigate to it.
2. npx create-react-app my-react-app, cd my-react-app, npx -p @storybook/cli sb init
3. yarn storybook
4. Press a; the addon panel toggles.
5. Press s; the sidebar toggles.
6. Press f to enter full-screen mode.
7. Press a; the sidebar toggles.
8. Press a again; the addon panel toggles.
9. Press f to reset into full-screen mode again.
10. Press s; the addon panel toggles.
11. Press s; the sidebar toggles.

Expected behavior
The shortcut keys should always target the correct panel.

System:

• OS: Ubuntu 18.04
• Device: 2018 Asus Vivobook
• Browser: Chrome 72, Firefox Quantum 66
• Framework: React
• Version: Storybook 5.0.6, React 16.8.6

[Phoenixton]

Ok so I believe that's not exactly what's happening.

After testing it extensively, I think that what happens is the following (For reference, showNav refers to the stories on the left and showPanel to the addon panel)

1) You start by not being in Fullscreen, hence {isFullScreen == false, showNav == true, showPanel == true}.
2) You go FullScreen by pressing F. This sets {isFullScreen to true, but does not set showNav and showPanel to false}.
3) Now you try pressing A because you want your panels to pop-up. Well storybook is right in believing that means that you want to exit fullscreen mode so it does, and now it executes what you asked: you pressed A, so it's going to togglePanel(). Except showPanel is still true, so it's now going to be set to false. Remember that showNav is still true too? Well that means that your booleans are now {isFullScreen == false, showNav == true, showPanel == false}. Hence why you feel like showNav is popping up :) It isn't popping up, it's reclaiming its rightful place from before the fullscreen while your input is making the addon panel disappear.

This is a direct consequence of toggleFullScreen not setting showNav and showPanel to false, but setting them to false would mean that you can't remember the old layout from before the user.

@shilman I would argue that this is working as intended but I agree that to someone expecting a more sensible result, it might seem awkward. What's your take on this?

[shilman]

@phoenixton thanks for looking into this! Your analysis sounds spot on. I think the UI should do what the user expects, which use to show the add-ons panel when the user presses A, or the sidebar when user presses S. i think we can achieve that by updating the handler logic in full screen mode without losing the state.e

What do you think?

[Armanio]

Easy, guys, eeeeasy.

[Armanio]

@shilman, @jalovatt and @Phoenixton catch fix. 🔝

[Phoenixton]

@Armanio Mind you, I can't test your code until tomorrow evening, apologies if I am way off base, but is that all that's needed? I would think it sidesteps the problem. To me, in togglePanel(),

         if (isFullscreen) {
            fullApi.toggleFullscreen();
          }

makes logical sense. As in, if you're full screen and you want the addon panel to pop up, well... you're no longer fullscreen, see what I mean? (I also wonder why the change is only made for togglePanel() and not for toggleNav() too?)

I think there's a bit of modulability to consider here, too. The fullscreen could be defined as "the state where everything else disappears", instead of just "the state where the addon panel and the sidebar are no longer visible". @shilman How would you define a fullscreen in Storybook?

[Armanio]

Hmm, now I rebuild storybook and see that I broke toggle panel. I investigate this issue today.

[Phoenixton]

I tried a couple of obvious fixes this morning and nothing worked well, I'll take a look at it again tomorrow evening if it's still open, no pressure. But I really believe it's worth considering a case where we redefine what being fullscreen really means (to plan for potential future panels we might want to have by default outside of addons and sidebar, or even if we want the toolbar to disappear when we go full screen since it doesn't currently). Maybe introducing another variable to differentiate between being currently shown to the user (false when in fullscreen) and being open as a whole (which would be akin to a "last known position before fullscreen") is a possibility.

[jalovatt]

Thanks for your explanation @Phoenixton , it makes much more sense of why Storybook is doing what it's doing.

IMO if a user toggles to full-screen and then toggles one of the panels or the toolbar, they're implicitly leaving full-screen mode and establishing a new state where all of the panels are closed except This One.

Allowing full-screen mode with toggleable panels _and_ remembering their layout before they entered full-screen seems counterintuitive to me:

a) I go full-screen
b) I mess with the panels for a bit
c) I press F again

I would expect it to return to full-screen and not return to how things were prior to a).

Entering full-screen and then immediately choosing to leave it should, on the other hand, remember the panels' state because the user hasn't done anything to modify it.

[Phoenixton]

@jalovatt You're very much welcome :) I agree with your proposal on the whole, it seems quite logical to me.

Something like this would correspond to your proposal, imo:

1) Go to fullscreen -> isFullscreen = true, just the way it is right now
2) TogglePanel()/ToggleNav() -> if (isFullscreen), leave fullscreen, close everything (addon panel, sidebar) and open the appropriate window (addon Panel in TogglePanel(), sidebar in ToggleNav() and so on)

This handles the fact that if you just ToggleFullscreen() and then toggle it again without touching anything else, you would revert to your old state, which is the behavior I would expect, and this doesn't make us declare additional variables. Whaddya think?

[jalovatt]

Yup, I think that would do it.

[Phoenixton]

I'll do that tomorrow then :)

[Armanio]

Guys, could you check fix here? It's expected behavior or not? 🤔

[Phoenixton]

@Armanio The behavior is still broken for me on your link. I can fix it tomorrow if it's not done today, don't worry :)

[Armanio]

Still broken? 😕 What is wrong?

Ok, of course, waiting for you solution. Thank you!

[Phoenixton]

@Armanio So if you try to press F, it goes full screen, then you press A and it opens the Sidebar instead of the add-on panel, this is what we refer to when we say it's broken :)

Go ahead if you wanna fix it, don't worry mate! I just don't want you to be pressured about it, if it's still there tomorrow, I'll do it :)

[jalovatt]

@Armanio In that build, pressing either S or A in full-screen just opens both of them again.

[Armanio]

Oh, you are right, guys, still broken.

Ok, @Phoenixton fix it if you can, I absolutely confused. Your move buddy!

[Phoenixton]

No pb, thanks ^_^

[shilman]

Ta-da!! I just released https://github.com/storybooks/storybook/releases/tag/v5.1.0-alpha.28 containing PR #6525 that references this issue. Upgrade today to try it out!

Because it's a pre-release you can find it on the @next NPM tag.

Closing this issue. Please re-open if you think there's still more to do.

[shilman]

Yowza!! I just released https://github.com/storybooks/storybook/releases/tag/v5.0.9 containing PR #6525 that references this issue. Upgrade today to try it out!