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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions modules/popup-stack/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,47 @@ considered to be "contained" by an element under the following conditions:
PopupStack.contains(element: HTMLElement, eventTarget: HTMLElement): boolean
```

### pushStackContext

```tsx
PopupStack.pushStackContext(element: HTMLElement): void
```

Add a new stack context for popups. This method could be called with the same element multiple
times, but should only push a new stack context once. The most common use-case for calling
`pushStackContext` is when entering fullscreen, but multiple fullscreen listeners could be pushing
the same element which is very difficult to ensure only one stack is used. To mitigate, this method
filters out multiple calls to push the same element as a new stack context.

### popStackContext

```tsx
PopupStack.popStackContext(element: HTMLElement): void
```

Remove the topmost stack context. The stack context will only be removed if the top stack
context container element matches to guard against accidental remove of other stack contexts you
don't own.

### transferToCurrentContext

```tsx
PopupStack.transferToCurrentContext(item: PopupStackItem): void
```

Transfer the popup stack item into the current popup stack context.

An example might be a popup that is opened and an element goes into fullscreen. The default popup
stack context is `document.body`, but the
[Fullscreen API](https://developer.mozilla.org/en-US/docs/Web/API/Fullscreen_API) will only render
elements that are children of the fullscreen element. If the popup isn't transferred to the current
popup stack context, the popup will remain open, but will no longer be rendered. This method will
transfer that popup to the fullscreen element so that it will render. Popups created while in a
fullscreen context that need to be transferred back when fullscreen is exited should also call this
method. While popups may still render when fullscreen is exited, popups will be members of different
popup stack contexts which will cause unspecified results (like the escape key will choose the wrong
popup as the "topmost").

### createAdapter

Create an adapter for the PopupStack. Any method provided will override the default method of
Expand Down
157 changes: 141 additions & 16 deletions modules/popup-stack/lib/PopupStack.ts
Original file line number Diff line number Diff line change
@@ -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.


/**
* This type is purposely an interface so that it can be extended for a specific use-case.
*/
Expand Down Expand Up @@ -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.

zIndex: {
min: number;
max: number;
Expand Down Expand Up @@ -183,13 +186,23 @@ const setToWindow = (path: string, value: any) => {
// defined on the page, we need to use that one. Never, ever, ever change this variable name on
// window
const stack: Stack = getFromWindow('workday.__popupStack') || {
description: 'Global popup stack from @workday/canvas-kit-popup-stack',
items: [] as PopupStackItem[],
description: 'Global popup stack from @workday/canvas-kit/popup-stack',
container: () => document.body,
items: [],
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.

_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.


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.


function getTopStack() {
return stacks[stacks.length - 1];
}

export const PopupStack = {
/**
* Create a HTMLElement as the container for the popup stack item. The returned element reference
Expand All @@ -198,6 +211,7 @@ export const PopupStack = {
* should be added to this element.
*/
createContainer(): HTMLElement {
const stack = getTopStack();
if (stack._adapter?.createContainer) {
return stack._adapter.createContainer();
}
Expand All @@ -212,31 +226,37 @@ export const PopupStack = {
* method when the event triggers.
*/
add(item: PopupStackItem): void {
const stack = getTopStack();
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.


setZIndexOfElements(PopupStack.getElements());
},

/**
* Removes an item from the stack by its `HTMLElement` reference. This should be called when a
* popup is "closed" or when the element is removed from the page entirely to ensure proper memory
* cleanup. This will not automatically be called when the element is removed from the DOM. Will
* reset z-index values of the stack
* Removes an item from a stack by its `HTMLElement` reference. This should be called when a popup
* is "closed" or when the element is removed from the page entirely to ensure proper memory
* cleanup. A popup will be removed from the stack it is a part of. This will not automatically be
* called when the element is removed from the DOM. This method will reset z-index values of the
* stack.
*/
remove(element: HTMLElement): void {
if (stack._adapter?.remove) {
stack._adapter.remove(element);
return;
// Find the stack the popup belongs to.
const stack = find(stacks, stack => !!find(stack.items, item => item.element === element));
if (stack) {
if (stack._adapter?.remove) {
stack._adapter.remove(element);
return;
}
stack.items = stack.items.filter(item => item.element !== element);
(stack.container?.() || document.body).removeChild(element);

setZIndexOfElements(PopupStack.getElements(stack));
}
stack.items = stack.items.filter(item => item.element !== element);
document.body.removeChild(element);

setZIndexOfElements(PopupStack.getElements());
},

/**
Expand All @@ -245,6 +265,7 @@ export const PopupStack = {
* reference that was passed to `add`
*/
isTopmost(element: HTMLElement): boolean {
const stack = getTopStack();
if (stack._adapter?.isTopmost) {
return stack._adapter.isTopmost(element);
}
Expand All @@ -261,7 +282,8 @@ export const PopupStack = {
* elements in the order of lowest z-index to highest z-index. Some popup behaviors will need to
* make decisions based on z-index order.
*/
getElements(): HTMLElement[] {
getElements(stackOverride?: Stack): HTMLElement[] {
const stack = stackOverride || getTopStack();
if (stack._adapter?.getElements) {
return stack._adapter.getElements();
}
Expand All @@ -281,6 +303,7 @@ export const PopupStack = {
* the top of the stack.
*/
bringToTop(element: HTMLElement): void {
const stack = getTopStack();
if (stack._adapter?.bringToTop) {
stack._adapter.bringToTop(element);
return;
Expand Down Expand Up @@ -320,6 +343,7 @@ export const PopupStack = {
* is not inside `element`).
*/
contains(element: HTMLElement, eventTarget: HTMLElement): boolean {
const stack = getTopStack();
if (stack._adapter?.contains) {
return stack._adapter.contains(element, eventTarget);
}
Expand All @@ -344,6 +368,87 @@ export const PopupStack = {
}
return false;
},

/**
* Add a new stack context for popups. This method could be called with the same element multiple
* times, but should only push a new stack context once. The most common use-case for calling
* `pushStackContext` is when entering fullscreen, but multiple fullscreen listeners could be
* pushing the same element which is very difficult to ensure only one stack is used. To mitigate,
* this method filters out multiple calls to push the same element as a new stack context.
*/
pushStackContext(container: HTMLElement): void {
const stack = getTopStack();

if (stack._adapter?.pushStackContext) {
return stack._adapter.pushStackContext(container);
}
// Don't push if the container already exists. This removes duplicates
if (stack.container?.() === container) {
return;
}

const newStack: Stack = {
items: [],
zIndex: stack.zIndex,
container: () => container,
_adapter: {},
};
stacks.push(newStack);
},

/**
* Remove the topmost stack context. The stack context will only be removed if the top stack
* context container element matches to guard against accidental remove of other stack contexts
* you don't own.
*/
popStackContext(container: HTMLElement): void {
const stack = getTopStack();

if (stack._adapter?.popStackContext) {
return stack._adapter.popStackContext(container);
}

if (stack.container?.() === container && stacks.length > 1) {
stacks.pop();
}
},

/**
* Transfer the popup stack item into the current popup stack context.
*
* An example might be a popup
* that is opened and an element goes into fullscreen. The default popup stack context is
* `document.body`, but the [Fullscreen
* API](https://developer.mozilla.org/en-US/docs/Web/API/Fullscreen_API) will only render elements
* that are children of the fullscreen element. If the popup isn't transferred to the current
* popup stack context, the popup will remain open, but will no longer be rendered. This method
* will transfer that popup to the fullscreen element so that it will render. Popups created while
* in a fullscreen context that need to be transferred back when fullscreen is exited should also
* call this method. While popups may still render when fullscreen is exited, popups will be
* members of different popup stack contexts which will cause unspecified results (like the escape
* key will choose the wrong popup as the "topmost").
*/
transferToCurrentContext(item: PopupStackItem): void {
const stack = getTopStack();

if (stack._adapter?.transferToCurrentContext) {
return stack._adapter.transferToCurrentContext(item);
}

if (find(stack.items, i => i.element === item.element)) {
// The element is already in the stack, don't do anything
return;
}

// Try to find the element in existing stacks. If it exists, we need to first remove from that
// stack context
const oldStack = find(stacks, stack => !!find(stack.items, i => i.element === item.element));
if (oldStack) {
PopupStack.remove(item.element);
}

PopupStack.add(item);
},
};

/**
Expand All @@ -361,3 +466,23 @@ export function resetStack() {
export const createAdapter = (adapter: Partial<typeof PopupStack>) => {
stack._adapter = adapter;
};

// keep track of the element ourselves to avoid accidentally popping off someone else's stack
// context
let element: HTMLElement | null = null;

// Where should this go? Each version of `PopupStack` on a page will add a listener. The
// `PopupStack` should guard against multiple handlers like this simultaneously and there is no
// lifecycle here.
if (screenfull.isEnabled) {
screenfull.on('change', () => {
if (screenfull.isFullscreen) {
if (screenfull.element) {
element = screenfull.element as HTMLElement;
PopupStack.pushStackContext(element);
}
} else if (element) {
PopupStack.popStackContext(element);
}
});
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.

}
7 changes: 5 additions & 2 deletions modules/popup-stack/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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.

"types": "dist/es6/index.d.ts",
"repository": {
"type": "git",
Expand Down Expand Up @@ -41,5 +41,8 @@
"canvas-kit",
"workday",
"popup-stack"
]
],
"dependencies": {
"screenfull": "^5.2.0"
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -129,8 +129,6 @@ export const DropdownMenu = ({
}
};

console.log('dropdownItems', dropdownItems);

return (
<div css={menuStyles}>
<MenuList {...elemProps}>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ export const useFocusActiveItemElement = <E extends HTMLElement>(
activeDropdownItemRef: React.RefObject<E>
) => {
useLayoutEffect(() => {
console.log('useFocusActiveItem');
if (activeDropdownItemRef.current) {
return activeDropdownItemRef.current.focus();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ export const CollapsibleList = ({
const {shouldCollapseList} = useCollapse(listRef, maxWidth);
const {collapsedItems, collapsedItemIndices} = useBuildCollapsedList(listRef, children, maxWidth);
const {rootItem, collapsibleItems, currentItem} = parseListItems(children);
console.log(collapsedItems, collapsedItemIndices);
return (
<BreadcrumbsList ref={listRef} {...props}>
{rootItem}
Expand Down
2 changes: 2 additions & 0 deletions modules/preview-react/menu/stories/examples/ContextMenu.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
usePopupModel,
useAlwaysCloseOnOutsideClick,
useCloseOnEscape,
useTransferOnFullscreenExit,
} from '@workday/canvas-kit-react/popup';

const ContextMenuTarget = createComponent('div')({
Expand Down Expand Up @@ -40,6 +41,7 @@ export const ContextMenu = () => {

useAlwaysCloseOnOutsideClick(model);
useCloseOnEscape(model);
useTransferOnFullscreenExit(model);

return (
<Popup model={model}>
Expand Down
2 changes: 2 additions & 0 deletions modules/preview-react/select/lib/SelectMenu.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
useCloseOnOutsideClick,
usePopupModel,
useReturnFocus,
useTransferOnFullscreenExit,
} from '@workday/canvas-kit-react/popup';
import {colors, borderRadius, inputColors} from '@workday/canvas-kit-react/tokens';

Expand Down Expand Up @@ -271,6 +272,7 @@ const SelectMenu = ({
useCloseOnEscape(model);
useCloseOnOutsideClick(model);
useReturnFocus(model);
useTransferOnFullscreenExit(model);

return (
<Popper
Expand Down
1 change: 1 addition & 0 deletions modules/react/common/lib/utils/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,6 @@ export * from './useMount';
export * from './components';
export * from './models';
export * from './elements';
export * from './useIsFullscreen';
export * from './useWindowSize';
export * from './StaticStates';
20 changes: 20 additions & 0 deletions modules/react/common/lib/utils/useIsFullscreen.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import React from 'react';
import screenfull from 'screenfull';

export const useIsFullscreen = () => {
const [isFullscreen, setIsFullscreen] = React.useState(false);

const handler = React.useCallback(() => {
setIsFullscreen(screenfull.isFullscreen);
}, []);

React.useEffect(() => {
screenfull.on('change', handler);

return () => {
screenfull.off('change', handler);
};
}, [handler]);

return isFullscreen;
};
Loading