Core 750: proper custom controls

playwright/src/fixtures/toc.ts

@@ -13,7 +13,7 @@ class TOC {
   constructor(page: Page) {
     this.page = page
     this.pageLocator = this.page.locator('[data-type="page"]')
-    this.tocDropdownLocator = this.page.locator('details[class*="NavDetails"]')
+    this.tocDropdownLocator = this.page.locator('a[class*="NavDetails"]')
     this.sectionNameLocator = this.page.locator('h1[class*="BookBanner"]')
     this.pageSlugLocator = this.page.locator('[data-type="page"] a')
     this.currentPageLocator = this.page.locator("[aria-label*='Current Page'] a")
@@ -33,7 +33,7 @@ class TOC {
 
   async unitIntroCount() {
     // Total number of unit introduction pages in the book
-    const unitIntroPageLocator = this.page.locator('//li[@data-type="unit"]/details/ol[1]/li[1][@data-type="page"]')
+    const unitIntroPageLocator = this.page.locator('//li[@data-type="unit"]/a/ol[1]/li[1][@data-type="page"]')
     return await unitIntroPageLocator.count()
   }
 
@@ -72,7 +72,7 @@ class TOC {
         await this.pageLocator.nth(pageNumber).click()
       } else {
         // expand the dropdowns in toc
-        await this.page.waitForSelector('details[class*="NavDetails"]')
+        await this.page.waitForSelector('a[class*="NavDetails"]')
         const tocDropdownCounts = await this.tocDropdownLocator.count()
         let tocDropdownCount: number
         for (tocDropdownCount = 0; tocDropdownCount < tocDropdownCounts; tocDropdownCount++) {
@@ -143,7 +143,7 @@ class TOC {
     // Return unit name of the current page
     const toc = this.page.locator('nav[data-testid=toc]')
     const unitLocator = toc
-      .locator('css=[data-type=unit] >> details', {
+      .locator('css=[data-type=unit] >> a', {
         has: this.page.locator(`[href="${await this.CurrentPageSlug()}"]`),
       })
       .first()

pytest-selenium/regions/toc.py

@@ -21,7 +21,7 @@ class TableOfContents(Region):
     _section_link_locator = (By.CSS_SELECTOR, "ol li a")
     _active_section_locator = (By.CSS_SELECTOR, "[aria-label='Current Page']")
 
-    _chapter_link_selector = "li details"
+    _chapter_link_selector = "li a"
 
     @property
     def active_section(self):

src/app/components/Details.tsx

@@ -50,3 +50,17 @@ export const Details = styled.details`
     }
   `}
 `;
+
+// Other components than ToC use Details, so we need to style them separately
+// tslint:disable-next-line:variable-name
+export const DetailsTree = styled.a`
+  ${/* suppress errors from https://github.com/stylelint/stylelint/issues/3391 */ css`
+    &[open] > ${ExpandIcon} {
+      display: none;
+    }
+
+    &:not([open]) > ${CollapseIcon} {
+      display: none;
+    }
+  `}
+`;

src/app/content/components/Assigned.spec.tsx

@@ -154,7 +154,7 @@ describe('Assigned', () => {
           'aria-label': 'Next Page',
           'href': 'books/book-slug-1/pages/3-test-page-4',
         })
-        .props.onClick({ preventDefault: jest.fn() });
+        .props.onClick({ preventDefault: jest.fn(), stopPropagation: jest.fn() });
 
       await services.promiseCollector.calm();
     });
@@ -209,7 +209,7 @@ describe('Assigned', () => {
           'aria-label': 'Next Page',
           'href': 'books/book-slug-1/pages/3-test-page-4',
         })
-        .props.onClick({ preventDefault: jest.fn() });
+        .props.onClick({ preventDefault: jest.fn(), stopPropagation: jest.fn() });
 
       await services.promiseCollector.calm();
     });
@@ -223,7 +223,7 @@ describe('Assigned', () => {
           'aria-label': 'Previous Page',
           'href': 'books/book-slug-1/pages/test-page-1',
         })
-        .props.onClick({ preventDefault: jest.fn() });
+        .props.onClick({ preventDefault: jest.fn(), stopPropagation: jest.fn() });
 
       await services.promiseCollector.calm();
     });

src/app/content/components/ContentLink.spec.tsx

@@ -45,6 +45,21 @@ describe('ContentLink', () => {
     return event;
   };
 
+  const onKeyDownMock = (event: React.KeyboardEvent<HTMLAnchorElement>, method: () => void) => {
+    event.preventDefault();
+    method();
+  };
+
+  const keyDown = async(component: renderer.ReactTestRenderer) => {
+    const event = {
+      preventDefault: jest.fn(),
+    };
+
+    await component.root.findByType('a').props.onKeyDown(event);
+
+    return event;
+  };
+
   describe('without unsaved changes', () => {
     // tslint:disable-next-line:variable-name
     let ConnectedContentLink: React.ElementType;
@@ -53,119 +68,151 @@ describe('ContentLink', () => {
       ConnectedContentLink = require('./ContentLink').default;
     });
 
-    it('dispatches navigation action on click', async() => {
+    it.each`
+      method     | description
+      ${click}   | ${'using onClick'}
+      ${keyDown} | ${'using onKeyDown'}
+    `('dispatches navigation action on click %description', async({ method }) => {
       const component = renderer.create(<TestContainer store={store}>
-        <ConnectedContentLink book={book} page={page} />
+        <ConnectedContentLink book={book} page={page} onKeyDown={onKeyDownMock} />
       </TestContainer>);
 
-      const event = await click(component);
+      const event = await method(component);
 
       expect(dispatch).toHaveBeenCalledWith(push({
-        params: {book: {slug: BOOK_SLUG}, page: {slug: PAGE_SLUG}},
+        params: { book: { slug: BOOK_SLUG }, page: { slug: PAGE_SLUG } },
         route: content,
-        state: { },
+        state: {},
       }));
       expect(event.preventDefault).toHaveBeenCalled();
     });
 
-    it('dispatches navigation action with search if there is a search', async() => {
+    it.each`
+      method     | description
+      ${click}   | ${'using onClick'}
+      ${keyDown} | ${'using onKeyDown'}
+    `('dispatches navigation action with search if there is a search %description', async({ method }) => {
       store.dispatch(requestSearch('asdf'));
       store.dispatch(receiveBook(book));
       const mockSearch = {
         query: 'asdf',
       };
       const component = renderer.create(<TestContainer store={store}>
-        <ConnectedContentLink book={book} page={page} queryParams={mockSearch} />
+        <ConnectedContentLink book={book} page={page} queryParams={mockSearch} onKeyDown={onKeyDownMock} />
       </TestContainer>);
 
-      const event = await click(component);
+      const event = await method(component);
 
       expect(dispatch).toHaveBeenCalledWith(push({
-        params: {book: {slug: BOOK_SLUG}, page: {slug: PAGE_SLUG}},
+        params: { book: { slug: BOOK_SLUG }, page: { slug: PAGE_SLUG } },
         route: content,
-        state: { },
+        state: {},
       }, { search: 'query=asdf' }));
       expect(event.preventDefault).toHaveBeenCalled();
     });
 
-    it('search passed as prop overwrites search from the redux state', async() => {
+    it.each`
+      method     | description
+      ${click}   | ${'using onClick'}
+      ${keyDown} | ${'using onKeyDown'}
+    `('search passed as prop overwrites search from the redux state %description', async({ method }) => {
       store.dispatch(requestSearch('asdf'));
       store.dispatch(receiveBook(book));
       const mockSearch = {
         query: 'search-from-direct-prop',
       };
       const component = renderer.create(<TestContainer store={store}>
-        <ConnectedContentLink book={book} page={page} queryParams={mockSearch} />
+        <ConnectedContentLink book={book} page={page} queryParams={mockSearch} onKeyDown={onKeyDownMock} />
       </TestContainer>);
 
-      const event = await click(component);
+      const event = await method(component);
 
       expect(dispatch).toHaveBeenCalledWith(push({
-        params: {book: {slug: BOOK_SLUG}, page: {slug: PAGE_SLUG}},
+        params: { book: { slug: BOOK_SLUG }, page: { slug: PAGE_SLUG } },
         route: content,
-        state: { },
+        state: {},
       }, { search: `query=${mockSearch.query}` }));
       expect(event.preventDefault).toHaveBeenCalled();
     });
 
-    it('dispatches navigation action with scroll target data and search if scroll target is passed', async() => {
-      const scrollTarget: SearchScrollTarget = { type: 'search', index: 1, elementId: 'anchor' };
-      store.dispatch(requestSearch('asdf'));
-      store.dispatch(receiveBook(book));
-      const mockSearch = {
-        query: 'asdf',
-      };
-      const component = renderer.create(<TestContainer store={store}>
-        <ConnectedContentLink book={book} page={page} queryParams={mockSearch} scrollTarget={scrollTarget} />
-      </TestContainer>);
-
-      dispatch.mockClear();
-
-      const event = await click(component);
-
-      expect(dispatch).toHaveBeenCalledWith(push({
-        params: {book: {slug: BOOK_SLUG}, page: {slug: PAGE_SLUG}},
-        route: content,
-        state: { },
-      }, {
-        hash: `#${scrollTarget.elementId}`,
-        search: queryString.stringify({
+    it.each`
+      method     | description
+      ${click}   | ${'using onClick'}
+      ${keyDown} | ${'using onKeyDown'}
+    `('dispatches navigation action with scroll target data and search if scroll target is passed %description',
+      async({ method }) => {
+        const scrollTarget: SearchScrollTarget = { type: 'search', index: 1, elementId: 'anchor' };
+        store.dispatch(requestSearch('asdf'));
+        store.dispatch(receiveBook(book));
+        const mockSearch = {
           query: 'asdf',
-          target: JSON.stringify(omit('elementId', scrollTarget)),
-        }),
-      }));
-      expect(event.preventDefault).toHaveBeenCalled();
-    });
-
-    it('dispatches navigation action without search when linking to a different book', async() => {
-      store.dispatch(requestSearch('asdf'));
-      store.dispatch(receiveBook({...book, id: 'differentid'}));
-      const component = renderer.create(<TestContainer store={store}>
-        <ConnectedContentLink book={book} page={page} />
-      </TestContainer>);
-
-      const event = await click(component);
-
-      expect(dispatch).toHaveBeenCalledWith(push({
-        params: {book: {slug: BOOK_SLUG}, page: {slug: PAGE_SLUG}},
-        route: content,
-        state: { },
-      }));
-      expect(event.preventDefault).toHaveBeenCalled();
-    });
-
-    it('calls onClick when passed', async() => {
+        };
+        const component = renderer.create(<TestContainer store={store}>
+          <ConnectedContentLink
+            book={book}
+            page={page}
+            queryParams={mockSearch}
+            scrollTarget={scrollTarget}
+            onKeyDown={onKeyDownMock}
+          />
+        </TestContainer>);
+
+        dispatch.mockClear();
+
+        const event = await method(component);
+
+        expect(dispatch).toHaveBeenCalledWith(push({
+          params: { book: { slug: BOOK_SLUG }, page: { slug: PAGE_SLUG } },
+          route: content,
+          state: {},
+        }, {
+          hash: `#${scrollTarget.elementId}`,
+          search: queryString.stringify({
+            query: 'asdf',
+            target: JSON.stringify(omit('elementId', scrollTarget)),
+          }),
+        }));
+        expect(event.preventDefault).toHaveBeenCalled();
+      });
+
+    it.each`
+      method     | description
+      ${click}   | ${'using onClick'}
+      ${keyDown} | ${'using onKeyDown'}
+    `('dispatches navigation action without search when linking to a different book %description',
+      async({ method }) => {
+        store.dispatch(requestSearch('asdf'));
+        store.dispatch(receiveBook({ ...book, id: 'differentid' }));
+        const component = renderer.create(<TestContainer store={store}>
+          <ConnectedContentLink book={book} page={page} onKeyDown={onKeyDownMock} />
+        </TestContainer>);
+
+        const event = await method(component);
+
+        expect(dispatch).toHaveBeenCalledWith(push({
+          params: { book: { slug: BOOK_SLUG }, page: { slug: PAGE_SLUG } },
+          route: content,
+          state: {},
+        }));
+        expect(event.preventDefault).toHaveBeenCalled();
+      });
+
+    it.each`
+      method     | description
+      ${click}   | ${'using onClick method'}
+      ${keyDown} | ${'using onKeyDown method'}
+    `('calls onClick when passed %description', async({ method }) => {
       const clickSpy = jest.fn();
       const component = renderer.create(<TestContainer store={store}>
-        <ConnectedContentLink book={book} page={page} onClick={clickSpy} />
+        <ConnectedContentLink book={book} page={page} onClick={clickSpy} onKeyDown={onKeyDownMock} />
       </TestContainer>);
 
-      const event = await click(component);
+      const event = await method(component);
 
       expect(dispatch).toHaveBeenCalledWith(push({
-        params: {book: {slug: BOOK_SLUG}, page: {slug: PAGE_SLUG}},
+        params: { book: { slug: BOOK_SLUG }, page: { slug: PAGE_SLUG } },
         route: content,
-        state: { },
+        state: {},
       }));
       expect(event.preventDefault).toHaveBeenCalled();
       expect(clickSpy).toHaveBeenCalled();
@@ -190,10 +237,11 @@ describe('ContentLink', () => {
     });
   });
 
-  describe('with unsaved changes' , () => {
+  describe('with unsaved changes', () => {
     // tslint:disable-next-line:variable-name
     let ConnectedContentLink: React.ElementType;
     const mockConfirmation = jest.fn()
+      .mockImplementationOnce(() => new Promise((resolve) => setTimeout(() => resolve(false), 300)))
       .mockImplementationOnce(() => new Promise((resolve) => setTimeout(() => resolve(false), 300)))
       .mockImplementationOnce(() => new Promise((resolve) => setTimeout(() => resolve(true), 300)));
 
@@ -206,21 +254,26 @@ describe('ContentLink', () => {
       ConnectedContentLink = require('./ContentLink').default;
     });
 
-    it('does not call onClick or dispatch if user decides not to discard changes' , async() => {
-      const clickSpy = jest.fn();
-      store.dispatch(setAnnotationChangesPending(true));
-      const component = renderer.create(<TestContainer store={store}>
-        <ConnectedContentLink book={book} page={page} onClick={clickSpy} />
-      </TestContainer>);
-
-      const event = await click(component);
-
-      expect(dispatch).not.toHaveBeenCalledWith(push(expect.anything()));
-      expect(event.preventDefault).toHaveBeenCalled();
-      expect(clickSpy).not.toHaveBeenCalled();
-    });
-
-    it('calls onClick and dispatch if user decides to discard changes' , async() => {
+    it.each`
+      method     | description
+      ${click}   | ${'using onClick method'}
+      ${keyDown} | ${'using onKeyDown method'}
+    `('does not call onClick or dispatch if user decides not to discard changes %description',
+      async({ method }) => {
+        const clickSpy = jest.fn();
+        store.dispatch(setAnnotationChangesPending(true));
+        const component = renderer.create(<TestContainer store={store}>
+          <ConnectedContentLink book={book} page={page} onClick={clickSpy} onKeyDown={onKeyDownMock} />
+        </TestContainer>);
+
+        const event = await method(component);
+
+        expect(dispatch).not.toHaveBeenCalledWith(push(expect.anything()));
+        expect(event.preventDefault).toHaveBeenCalled();
+        expect(clickSpy).not.toHaveBeenCalled();
+      });
+
+    it('calls onClick and dispatch if user decides to discard changes', async() => {
       const clickSpy = jest.fn();
       store.dispatch(setAnnotationChangesPending(true));
       const component = renderer.create(<TestContainer store={store}>