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

fix(popup-stack): Add support for the fullscreen API #1403

Merged

Conversation

NicholasBoll
Copy link
Member

Summary

Add fullscreen support to all popups. New hooks were added if your popup should transfer or close when entering/exiting a fullscreen context.

Fixes: #978
Fixes: #891

category

Release Note

Fullscreen support was added to all Popups. 3 new hooks were added to help support fullscreen in whatever way you see fit:

  • useTransferOnFullscreenEnter: Use if your popup should remain open and be transfer into the fullscreen element
  • useTransferOnFullscreenExit: Use if your popup should remain open and transfer out of the fullscreen element back to the body element
  • useCloseOnFullscreenExit: Use if your popup should close when fullscreen is exited

This fix is added to v5 with the option for popup stack adapters to opt into additional support for fullscreen. By default, a popup adapter does not need to support fullscreen. If fullscreen is entered, the PopupStack will use its own popup stack methods and when fullscreen is exited, control is returned back to the adapter. When a fullscreen popup stack context is entered, a new stack is pushed onto window.__popupStackOfStacks. If the adapter choses to support multiple stacks, an adapter must be pushed into this stack. This is how PopupStack keeps track of adapters on each stack.

A few components were updated to handle fullscreen:

  • Tooltip
    • Closes when fullscreen is exited
  • Select
    • Transfers into/out of the fullscreen context. Select menus can stay open during a transfer in/out of fullscreen

No Cypress tests were added to verify Fullscreen functionality. Cypress isn't able to test entering/exiting of fullscreen due to not using native events.

@NicholasBoll NicholasBoll added bug Something isn't working 5.x labels Dec 21, 2021
@@ -1,3 +1,5 @@
import screenfull from 'screenfull';
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

screenfull was chosen because it is as lightweight as possible and already used by some teams.

@@ -106,6 +108,7 @@ function getChildPopups(item: PopupStackItem, items: PopupStackItem[]): PopupSta

interface Stack {
items: PopupStackItem[];
container?: () => HTMLElement;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional. Falls back to document.body if not defined.

const stacks: Stack[] = getFromWindow('workday.__popupStackOfStacks') || [stack];

(stacks as any).description = 'Global stack of popup stacks from @workday/canvas-kit/popup-stack';
setToWindow('workday.__popupStackOfStacks', stacks);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the new variable added to window. This can be observed to see what stack we're currently in. Adapters should still manipulate this object.

if (stack._adapter?.add) {
stack._adapter.add(item);
return;
}
stack.items.push(item);
document.body.appendChild(item.element);
(stack.container?.() || document.body).appendChild(item.element);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the || document.body is backwards compatible code. If this version of PopupStack isn't the first loaded, the container would not be defined. The PopupStack tries to register a global shared window.workday.__popupStack variable. This code cannot be removed.

@@ -6,7 +6,7 @@
"license": "Apache-2.0",
"main": "dist/commonjs/index.js",
"module": "dist/es6/index.js",
"sideEffects": false,
"sideEffects": true,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should have always been true. Since we manipulate window outside of a function call, this is required to support tree shaking. Proper tree shaking would break previous versions of PopupStack. I'm surprised we haven't received any bug reports.

@cypress
Copy link

cypress bot commented Dec 22, 2021



Test summary

625 0 1 0


Run details

Project canvas-kit
Status Passed
Commit 5bc8de6 ℹ️
Started Jan 3, 2022 6:12 PM
Ended Jan 3, 2022 6:20 PM
Duration 07:06 💡
OS Linux Ubuntu - 20.04
Browser Electron 85

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

modules/popup-stack/README.md Outdated Show resolved Hide resolved
modules/popup-stack/lib/PopupStack.ts Outdated Show resolved Hide resolved
zIndex: {min: 30, max: 50, getValue: getValue},
_adapter: {} as Partial<typeof PopupStack>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we removing the typing here for _adapter and items?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stack is already declared as type Stack which includes typing for _adapter, so this casting is not necessary.

zIndex: {min: 30, max: 50, getValue: getValue},
_adapter: {} as Partial<typeof PopupStack>,
_adapter: {},
};
setToWindow('workday.__popupStack', stack);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe too early to consider this, but is the plan to eventually deprecate the singular stack? If so, we should note this is only here for backwards compatibility as well

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's possible, but a single stack needs to be created as the base stack and that needs to be shared. It could be that the first import of PopupStack is a version before this change and this variable would still exist. It could be that this code is removed and an old version that uses this still adds the global variable. It is important that all instances of this file use the same reference.

It is possible to deprecate, but that would take a lot of thought and testing. I have no plans to remove it at the moment.

PopupStack.popStackContext(element);
}
console.log('change', screenfull.isFullscreen, stacks.length);
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps we can also instantiate this using the initialization of the stack at line

aka

const stack: Stack = getFromWindow('workday.__popupStack') || {
  description: 'Global popup stack from @workday/canvas-kit/popup-stack',
  container: () => document.body,
  items: [],
  zIndex: {min: 30, max: 50, getValue: getValue},
  _adapter: {},
 activateFullScreenListener: () => { 
   const initialPopupStack = getFromWindow('workday.__popupStack');
   if (initialPopupStack.activateFullScreenListener) {
      // do all the logic above from lines 477 - 488
      initialPopupStack.activateFullScreenListener = null;
   } 
 } 
}


Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something would still need to call this method. Setting it to null would mean the caller would have to do a nullable check. A global state variable would be the only way to ensure this method does not get called more than once, but it does put a weird cross-version dependency.

The way it is done here doesn't require a cross-version dependency, but does require pushStackContext to ignore requests from the same element. This requirement isn't only for this call though as pushing a new stack context could be from any number of components that could be placed on the same element. I think multiple calls to push a new stack context on the same element should be silently ignored (the side-effect is the same whether this was the first call or the 10th) rather than an error or other attempts to make sure the application only pushes a stack context element only once.

The question about the location of this call is around where could it be called where lifecycle is controlled. There are a few problems:

  • How do we handle only adding a full screen listener if we have popups?
  • How do we remove a full screen listener if there are no more popups?

I thought about other places this could go:

  • In Popper.tsx in a useEffect hook
    • The problem here is the listener is added many more times than once per PopupStack import instance
  • In Popper.tsx outside the component (side-effect)
    • The problem here is tree shaking. Tree shaking will theoretically remove non-reachable code. In practice, this probably isn't a problem because abuse of tree-shaking is so common. Many tree-shaking bundlers also need a special "pure" comment to flag removal and a side effect list this would not be flagged as "pure" and therefore not removed. There are higher optimizer settings that could still remove this code as "dead code" since there is no caller. For this reason, @workday/canvas-kit-popup-stack has been changed to a side-effect dependency to help remove chances of side-effect removal
  • Only a method that is truly side-effect free
    • This would be a breaking change as the PopupStack library already relies on side effects to create a globally shared stack. It would mean the application would need to opt in to full screen support and could not be released as a patch. The reason the PopupStack relies on side-effects to remove the coordination required by a global call. It isn't obvious in larger application who is responsible for making this call. In a single app like Workday, that can be a platform team, but what about independent application development where an app may or may not be running in a context of the Workday application? That complexity would be pushed onto many developers. We made a decision to hide this complexity and simply using any popups would mean the system worked and it was designed with the multi-app use-case in mind.

I haven't thought of a better place to add the listener than PopupStack.ts. The lifecycle is tied to the page. This can mean slow memory leaks as any additional dynamic imports could add listeners that never get cleaned up, but that would be a very slow leak. The only real change that could mitigate slow memory leaks would be an additional global variable that checks if the listener is added, but that runs into the same multi-version dependency issue where the listener could never be changed by later versions. I still think this is the best place for this listener to be added to add support for full-screen without pushing the complexity to add development teams.

modules/react/popup/stories/Popup.stories.mdx Outdated Show resolved Hide resolved
@workday-canvas-kit workday-canvas-kit enabled auto-merge (squash) January 3, 2022 21:37
@workday-canvas-kit workday-canvas-kit merged commit fe36834 into Workday:support Jan 3, 2022
@NicholasBoll NicholasBoll deleted the feat/popup-stack-full-screen branch January 3, 2022 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5.x automerge bug Something isn't working
Projects
None yet
4 participants