Skip to content

Commit

Permalink
[bento][amp-sidebar] SSR compatible design for Sidebar Toolbar (#34158)
Browse files Browse the repository at this point in the history 
* SSR compatible design for Toolbar

* Various code review comments from Caroline

* Additional review comments

* Review comments

* Remove unused import

* Add sanitation

* Update sanitization method

* Update return type for helper function

* Remove truthiness checks that are redundant

* Add unit tests
  • Loading branch information
@krdwan
krdwan committed May 7, 2021
1 parent df5b6a1 commit 68394c2
Show file tree
Hide file tree
Showing 2 changed files with 100 additions and 104 deletions.
81 changes: 53 additions & 28 deletions extensions/amp-sidebar/1.0/component.js
View file
Expand Up @@ -18,7 +18,8 @@ import * as Preact from '../../../src/preact';
import {ContainWrapper, useValueRef} from '../../../src/preact/component';
import {Keys} from '../../../src/core/constants/key-codes';
import {Side} from './sidebar-config';
import {createPortal, forwardRef} from '../../../src/preact/compat';
import {escapeCssSelectorIdent} from '../../../src/core/dom/css';
import {forwardRef} from '../../../src/preact/compat';
import {isRTL} from '../../../src/dom';
import {
useCallback,
Expand Down Expand Up @@ -186,49 +187,73 @@ export {Sidebar};
*/
export function SidebarToolbar({
toolbar: mediaQueryProp,
toolbarTarget,
toolbarTarget: toolbarTargetProp,
children,
...rest
}) {
const ref = useRef();
const [matches, setMatches] = useState(false);
const ref = useRef(null);
const [mediaQuery, setMediaQuery] = useState(null);
const [toolbarTarget, setToolbarTarget] = useState(null);
const [targetEl, setTargetEl] = useState(null);

useEffect(() => {
const window = ref.current?.ownerDocument?.defaultView;
if (!window) {
const doc = ref.current?.ownerDocument;
if (!doc) {
return;
}

const mediaQueryList = window.matchMedia(mediaQueryProp);
const updateMatches = () => setMatches(mediaQueryList.matches);
mediaQueryList.addEventListener('change', updateMatches);
setMatches(mediaQueryList.matches);
return () => mediaQueryList.removeEventListener('change', updateMatches);
const sanitizedToolbarTarget = escapeCssSelectorIdent(toolbarTargetProp);
setToolbarTarget(sanitizedToolbarTarget);
setTargetEl(doc.getElementById(sanitizedToolbarTarget));
}, [toolbarTargetProp]);

useEffect(() => {
const win = ref.current?.ownerDocument?.defaultView;
if (!win) {
return;
}

setMediaQuery(sanitizeMediaQuery(win, mediaQueryProp));
}, [mediaQueryProp]);

useEffect(() => {
const document = ref.current?.ownerDocument;
if (!document) {
const element = ref.current;
const doc = ref.current?.ownerDocument;
if (!doc || !targetEl || mediaQuery == null) {
return;
}

const selector = `#${CSS.escape(toolbarTarget)}`;
const newTargetEl = document.querySelector(selector);
setTargetEl(newTargetEl);
}, [toolbarTarget]);
const clone = element.cloneNode(true);
const style = doc.createElement('style');
style./*OK*/ textContent =
`#${toolbarTarget}{display: none;}` +
`@media ${mediaQuery}{#${toolbarTarget}{display: initial;}}`;

targetEl.appendChild(clone);
targetEl.appendChild(style);
return () => {
targetEl.removeChild(clone);
targetEl.removeChild(style);
};
}, [mediaQuery, toolbarTarget, targetEl]);

return (
<>
<nav
ref={ref}
toolbar={mediaQueryProp}
toolbar-target={toolbarTarget}
{...rest}
>
{children}
</nav>
{matches && targetEl && createPortal(children, targetEl)}
</>
<nav
ref={ref}
toolbar={mediaQueryProp}
toolbar-target={toolbarTargetProp}
{...rest}
>
{children}
</nav>
);
}

/**
* @param {!Window} win
* @param {string|undefined} query
* @return {string}
*/
function sanitizeMediaQuery(win, query) {
return win.matchMedia(query).media;
}
123 changes: 47 additions & 76 deletions extensions/amp-sidebar/1.0/test/test-component.js
View file
Expand Up @@ -681,7 +681,7 @@ describes.sandboxed('Sidebar preact component', {}, (env) => {
document.body.removeChild(target);
});

it('toolbar target receives content when media query is true', () => {
it('toolbar target should receive expected content from toolbar', () => {
// this media query is always true
mediaQuery = '';
wrapper = mount(
Expand All @@ -698,13 +698,17 @@ describes.sandboxed('Sidebar preact component', {}, (env) => {
</>
);

// Toolbar target should have children
// Toolbar nodes were appended to the target
expect(target.hasChildNodes()).to.be.true;
expect(target.childElementCount).to.equal(2);
expect(target.firstElementChild.nodeName).to.equal('NAV');
expect(target.lastElementChild.nodeName).to.equal('STYLE');
});

it('toolbar target does not receive content when media query is false', () => {
it('existing children in toolbar target should not be overwritten', () => {
// this media query is always true
mediaQuery = 'false';
mediaQuery = '';
target.innerHTML = '<span>hello world<span>';
wrapper = mount(
<>
<Sidebar ref={ref} side="left">
Expand All @@ -719,17 +723,17 @@ describes.sandboxed('Sidebar preact component', {}, (env) => {
</>
);

// Toolbar target should have children
expect(target.hasChildNodes()).to.be.false;
// Toolbar target now has 3 nodes, 1 existing, 2 appended from toolbar
expect(target.hasChildNodes()).to.be.true;
expect(target.childElementCount).to.equal(3);
expect(target.firstElementChild.nodeName).to.equal('SPAN');
expect(target.children[1].nodeName).to.equal('NAV');
expect(target.lastElementChild.nodeName).to.equal('STYLE');
});

it('toolbar target content changes when sidebar is opened and closed', () => {
// disable animations for synchronous testing
const animateFunction = Element.prototype.animate;
Element.prototype.animate = null;

// this media query is always true
mediaQuery = '';
it('toolbar target content should be removed on unmount', () => {
// this media query is always false
mediaQuery = '(max-height: 0px)';
wrapper = mount(
<>
<Sidebar ref={ref} side="left">
Expand All @@ -741,68 +745,50 @@ describes.sandboxed('Sidebar preact component', {}, (env) => {
</ul>
</SidebarToolbar>
</Sidebar>
<button id="open" onClick={() => ref.current.open()}></button>
<button id="close" onClick={() => ref.current.close()}></button>
</>
);

const openButton = wrapper.find('#open');
const closeButton = wrapper.find('#close');

// verify sidebar is closed
let sidebarElement = wrapper.find(Sidebar).getDOMNode();
expect(isOpened(sidebarElement)).to.be.false;

// Toolbar target should have children
expect(target.hasChildNodes()).to.be.true;
expect(target.childElementCount).to.equal(2);

// click to open the sidebar
openButton.simulate('click');

// verify sidebar is opened and toolbar has children
sidebarElement = wrapper.find(Sidebar).getDOMNode();
expect(isOpened(sidebarElement)).to.be.true;
expect(target.hasChildNodes()).to.be.true;
wrapper.unmount();
expect(target.hasChildNodes()).to.be.false;
});

// click to close the sidebar
closeButton.simulate('click');
it('toolbar should sanitize an invalid media query', () => {
// this is an invalid media query
mediaQuery = 'foo {}';
wrapper = mount(
<>
<Sidebar ref={ref} side="left">
<div>Content</div>
<SidebarToolbar toolbar={mediaQuery} toolbarTarget="toolbar-target">
<ul>
<li>Toolbar Item 1</li>
<li>Toolbar Item 2</li>
</ul>
</SidebarToolbar>
</Sidebar>
</>
);

// verify sidebar is closed and toolbar has children
sidebarElement = wrapper.find(Sidebar).getDOMNode();
expect(isOpened(sidebarElement)).to.be.false;
expect(target.hasChildNodes()).to.be.true;

Element.prototype.animate = animateFunction;
expect(target.childElementCount).to.equal(2);
const styleElementText = target.lastElementChild.textContent;
expect(styleElementText).to.include('not all'); //sanitized media query
expect(styleElementText).not.to.include('foo'); //unsanitized media query
});

it('toolbar target content updates when media query truthiness change', () => {
// disable animations for synchronous testing
const animateFunction = Element.prototype.animate;
Element.prototype.animate = null;

// mock the media query to manually update matching
let mockMediaQueryList;
window.matchMedia = (media) => {
const mql = matchMediaFunction(media);
mockMediaQueryList = {
matches: mql.matches,
addEventListener: function (type, callback) {
this.callback = callback;
},
removeEventListener: function () {
this.callback = null;
},
};
return mockMediaQueryList;
};

it('toolbar should sanitize the toolbar target attribute', () => {
// this media query is always true
mediaQuery = '';
const toolbarTarget = 'toolbar-target:.';
const getElementByIdSpy = env.sandbox.spy(document, 'getElementById');
wrapper = mount(
<>
<Sidebar ref={ref} side="left">
<div>Content</div>
<SidebarToolbar toolbar={mediaQuery} toolbarTarget="toolbar-target">
<SidebarToolbar toolbar={mediaQuery} toolbarTarget={toolbarTarget}>
<ul>
<li>Toolbar Item 1</li>
<li>Toolbar Item 2</li>
Expand All @@ -812,24 +798,9 @@ describes.sandboxed('Sidebar preact component', {}, (env) => {
</>
);

// media query is true so toolbar-target should have content
expect(target.hasChildNodes()).to.be.true;

// update the media query to be false (mocked)
// verify toolbar-target no longer has children
mockMediaQueryList.matches = false;
mockMediaQueryList.callback();
wrapper.update();
expect(getElementByIdSpy).to.be.calledOnce;
expect(getElementByIdSpy).to.be.calledWith('toolbar-target\\:\\.');
expect(target.hasChildNodes()).to.be.false;

// update the media query to be true (mocked)
// verify toolbar-target has children
mockMediaQueryList.matches = true;
mockMediaQueryList.callback();
wrapper.update();
expect(target.hasChildNodes()).to.be.true;

Element.prototype.animate = animateFunction;
});
});
});

0 comments on commit 68394c2

Please sign in to comment.