Core 750: proper custom controls #2433

jomcarvajal · 2025-02-19T22:29:13Z

Card: https://openstax.atlassian.net/browse/CORE-750

…er-custom-controls

…/rex-web into CORE-750-proper-custom-controls

src/app/content/components/TableOfContents/index.tsx

RoyEJohnson

I've made some notes. It's looking good.

src/app/content/components/TableOfContents/keyboardSupport.hook.tsx

RoyEJohnson

Looks good to me.

jomcarvajal · 2025-04-08T14:35:49Z

@TomWoodward could you help me with a review here ?

TomWoodward · 2025-04-09T15:13:14Z

playwright/src/fixtures/toc.ts

@@ -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"]')


isn't this no longer true? if its true i would be worried about putting lists inside an anchor, but i'm pretty sure you updated that already

TomWoodward · 2025-04-09T15:14:56Z

src/app/components/Details.tsx

+
+// Other components than ToC use Details, so we need to style them separately
+// tslint:disable-next-line:variable-name
+export const DetailsTree = styled.a`


you've left these elements named the way they were before but they no longer really make sense with those names. this component no longer acts like a details component because the tree that it expands is not inside of it, its now more of a CollapseToggle or something like that.

TomWoodward · 2025-04-09T15:19:01Z

src/app/content/components/ContentLink.tsx

+        onClick();
+      }
+
+      navigate(navigationMatch, options);


onclick has the navigate inside of it, you don't have to call it again

Removing navigate(navigationMatch, options) produce that page doesn't update the content. It means maybe this navigation is important. The onClick() call doesn't bring an invoke of navigation.

TomWoodward · 2025-04-09T15:19:33Z

src/app/content/components/ContentLink.tsx

+      */
+      e.preventDefault();
+
+      if (hasUnsavedHighlight && !await showConfirmation(services)) {


this is duplicated from the onclick, i would extract a handler used in both callbacks

TomWoodward · 2025-04-09T15:20:14Z

src/app/content/components/ContentLink.tsx

@@ -95,14 +96,30 @@ export const ContentLink = (props: React.PropsWithChildren<Props>) => {
        navigate(navigationMatch, options);
      }
    }}
+    onKeyDown={(e) => onKeyDown?.(e, async() => {
+      /*
+        All this logic has to be inside onKeyDown === Enter || Space


should you be checking the key pressed here? i don't see that filtering happening anywhere

also i'm like 90% sure you don't need to specially handle pressing a link with the enter key, pretty sure it triggers the onClick

Yeah I was sure that will work but that was not the case. I have to preventDefault behavior of keys because Tree navigation is very special. Removing preventDefault and 'enter' as one of my cases of keyboardSupport just do nothing. I'll explore more about it

TomWoodward · 2025-04-09T15:43:03Z

src/app/content/components/TableOfContents/keyboardSupport.hook.tsx

+  return { filteredTreeItems, currentItemIndex };
+}
+
+export const onKeyDownNavGroupSupport = ({


Suggested change

export const onKeyDownNavGroupSupport = ({

export const treeNavSubtreeOnKeyDown = ({

might make it more clear what this function is for

TomWoodward · 2025-04-09T15:43:52Z

src/app/content/components/TableOfContents/keyboardSupport.hook.tsx

+  }
+};
+
+export const onKeyDownNavItemSupport = ({


Suggested change

export const onKeyDownNavItemSupport = ({

export const treeNavItemOnKeyDown = ({

TomWoodward · 2025-04-09T15:46:40Z

src/app/content/components/TableOfContents/keyboardSupport.hook.tsx

+  );
+  let index = pageElements.indexOf(document?.activeElement as HTMLElement);
+  if (shiftKey) {
+    focusFirstTreeItem(filteredTreeItems);


why are you changing the treeitem focus and then focusing something else right after?

TomWoodward · 2025-04-09T15:47:27Z

src/app/content/components/TableOfContents/keyboardSupport.hook.tsx

+  document?.querySelector<HTMLElement>(`[id="${filteredTreeItems[filteredTreeItems.length - 1]?.id}"]`)?.focus();
+};
+
+const focusOnTab = (filteredTreeItems: Element[], shiftKey: boolean) => {


some comments on these functions might be necessary, what is this doing that the regular browser tab doesn't?

TomWoodward · 2025-04-09T15:47:57Z

src/app/content/components/TableOfContents/keyboardSupport.hook.tsx

+
+const focusOnTab = (filteredTreeItems: Element[], shiftKey: boolean) => {
+  const pageElements = Array.from(
+    document?.querySelectorAll('input, button, select, textarea, a[href]') as ArrayLike<HTMLElement>


i think there is a bigger list of tabbable elements in the repo somewhere

TomWoodward

I went back and reviewed the w3c page on the tree pattern, and in particular the markup structure and event handlers don't seem to be lining up with what I see in the pr. Notably I didn't think our toggles are supposed to be links because they didn't go anywhere. It also seems like the JavaScript example they have there might solve some of the issues I've been noting in your event handlers.

We might consider using the react aria library that we use in other places, they have a tree view component and I think their library is generally pretty extensible in terms of display styling

jomcarvajal · 2025-04-10T14:44:10Z

I went back and reviewed the w3c page on the tree pattern, and in particular the markup structure and event handlers don't seem to be lining up with what I see in the pr. Notably I didn't think our toggles are supposed to be links because they didn't go anywhere. It also seems like the JavaScript example they have there might solve some of the issues I've been noting in your event handlers.

We might consider using the react aria library that we use in other places, they have a tree view component and I think their library is generally pretty extensible in terms of display styling

Hmmm I see... it is weird because when I was reviewing w3c page, the structure of using anchors as toggles is right there and that is why I kept using it. Probably I'll start a new PR from zero then.

jomcarvajal · 2025-04-10T19:25:22Z

@TomWoodward It is true that we are using links that don't go anywhere, but this was done this way following the W3C example, which clearly shows that role=treeitem elements should be links in this pattern.

When you mention that "the markup structure and event handlers don't seem to be lining up with what I see in the PR," could you clarify what you mean by that? I'm asking because the context of TableOfContents differs from the example provided. We have a dynamic tree in a context that involves handling a specific structure for reading data, such as book, page, section, among others.

Regarding your comment about using react-aria-components, I get the impression that you are suggesting I should remove my code entirely and use the library instead. Is that correct? Also, I understand that this change would come from ui-components, which implies a more general implementation.

On another note, when using the Tree from react-aria-components, I’ve noticed that its keyboard interaction implementation is somewhat limited. Additionally, the way styles are managed is different from what we currently have, which could lead to a greater effort to avoid breaking any existing functionality.

Given that there are many doubts and concerns, and since I don’t fully understand your vision on this, I would prefer if we could meet and review the code together. This way, we can clear up any misunderstandings.

TomWoodward · 2025-04-25T17:42:55Z

@jomcarvajal

It is true that we are using links that don't go anywhere, but this was done this way following the W3C example, which clearly shows that role=treeitem elements should be links in this pattern.

look again, those links go places, in the example they provide there are index pages for each subtree that we don't have. in the react-aria implementation it looks like they actually don't have role=treeitem on the folder nodes that can't be selected

When you mention that "the markup structure and event handlers don't seem to be lining up with what I see in the PR," could you clarify what you mean by that? I'm asking because the context of TableOfContents differs from the example provided. We have a dynamic tree in a context that involves handling a specific structure for reading data, such as book, page, section, among others.

they have example javascript for handling keyboard events which looks simpler than yours, handles fewer events, overrides less default functionality (the tab key for example)

Regarding your comment about using react-aria-components, I get the impression that you are suggesting I should remove my code entirely and use the library instead. Is that correct? Also, I understand that this change would come from ui-components, which implies a more general implementation.

my suggestion to use react-aria-components is because we seem to be struggling to get this right, and that library is designed to solve this problem for us. i'm not sure if the treeview component that i've been referencing is a perfect fit, maybe they have something else, but it seemed like it might be worth looking into. i like using react-aria-components because the whole point of it is to follow the w3c standards by default so we don't have to worry about it. not saying we have to go that route, but i think its worth considering.

i'm not suggesting that you do it through the ui-components library though (unless that makes sense for some other reason), you could also just implement the react-aria dependency right in rex.

jomcarvajal added 2 commits February 19, 2025 15:51

refactor toc with navigation tree

5ee7441

remove redundant role

93b6e82

jomcarvajal requested review from a team as code owners February 19, 2025 22:29

jomcarvajal requested review from gregfitch and TomWoodward February 19, 2025 22:29

TomWoodward temporarily deployed to rex-web-core-750-proper-gtsn7c February 19, 2025 22:29 Inactive

Merge branch 'main' of github.com:openstax/rex-web into CORE-750-prop…

7a8dbf7

…er-custom-controls

TomWoodward temporarily deployed to rex-web-core-750-proper-gtsn7c February 19, 2025 22:37 Inactive

replace quotemark

9408066

TomWoodward temporarily deployed to rex-web-core-750-proper-gtsn7c February 19, 2025 22:42 Inactive

update snapshots

c15001d

TomWoodward temporarily deployed to rex-web-core-750-proper-gtsn7c February 19, 2025 23:01 Inactive

Merge branch 'main' into CORE-750-proper-custom-controls

3fed124

TomWoodward temporarily deployed to rex-web-core-750-proper-gtsn7c February 19, 2025 23:37 Inactive

jomcarvajal added 2 commits February 20, 2025 10:00

fix coverage

c0df3fe

Merge branch 'CORE-750-proper-custom-controls' of github.com:openstax…

e2c2369

…/rex-web into CORE-750-proper-custom-controls

TomWoodward temporarily deployed to rex-web-core-750-proper-gtsn7c February 20, 2025 16:02 Inactive

Merge branch 'main' into CORE-750-proper-custom-controls

055e657

TomWoodward temporarily deployed to rex-web-core-750-proper-gtsn7c February 20, 2025 17:00 Inactive

Merge branch 'main' into CORE-750-proper-custom-controls

2c93f3a

TomWoodward temporarily deployed to rex-web-core-750-proper-gtsn7c February 21, 2025 16:27 Inactive

Merge branch 'main' into CORE-750-proper-custom-controls

a0ff154

TomWoodward temporarily deployed to rex-web-core-750-proper-gtsn7c February 21, 2025 17:27 Inactive

Merge branch 'main' into CORE-750-proper-custom-controls

f8dd35b

TomWoodward temporarily deployed to rex-web-core-750-proper-gtsn7c February 21, 2025 17:36 Inactive

Merge branch 'main' into CORE-750-proper-custom-controls

5042a1a

TomWoodward temporarily deployed to rex-web-core-750-proper-gtsn7c February 21, 2025 17:45 Inactive

Merge branch 'main' into CORE-750-proper-custom-controls

b6c8c27

TomWoodward temporarily deployed to rex-web-core-750-proper-gtsn7c February 21, 2025 18:00 Inactive

RoyEJohnson reviewed Apr 1, 2025

View reviewed changes

src/app/content/components/TableOfContents/index.tsx Outdated Show resolved Hide resolved

improve conditional

95365be

RoyEJohnson reviewed Apr 1, 2025

View reviewed changes

refactor keyboard support methods

1bb6ffe

jomcarvajal requested a review from RoyEJohnson April 1, 2025 23:03

RoyEJohnson approved these changes Apr 2, 2025

View reviewed changes

staxly bot added 2 commits April 3, 2025 15:58

Merge branch 'main' into CORE-750-proper-custom-controls

f3d977d

Merge branch 'main' into CORE-750-proper-custom-controls

1ef9eea

Merge branch 'main' into CORE-750-proper-custom-controls

97ca660

TomWoodward removed the request for review from gregfitch April 9, 2025 15:06

TomWoodward requested changes Apr 9, 2025

View reviewed changes

resolve comments

73030dc

jomcarvajal requested a review from TomWoodward April 9, 2025 22:10

TomWoodward requested changes Apr 10, 2025

View reviewed changes

jomcarvajal and others added 6 commits April 10, 2025 15:14

new querySelector to get all visible treeitems

0df16b1

run linter

b349bdc

Merge branch 'main' into CORE-750-proper-custom-controls

1cbf5b5

Merge branch 'main' into CORE-750-proper-custom-controls

7b446ee

Merge branch 'main' into CORE-750-proper-custom-controls

2249123

Merge branch 'main' into CORE-750-proper-custom-controls

6caf6c9

staxly bot added 6 commits April 28, 2025 21:18

Merge branch 'main' into CORE-750-proper-custom-controls

99e9458

Merge branch 'main' into CORE-750-proper-custom-controls

55757f2

Merge branch 'main' into CORE-750-proper-custom-controls

94294d6

Merge branch 'main' into CORE-750-proper-custom-controls

17d6048

Merge branch 'main' into CORE-750-proper-custom-controls

ec99cff

Merge branch 'main' into CORE-750-proper-custom-controls

3f2105c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core 750: proper custom controls #2433

Core 750: proper custom controls #2433

jomcarvajal commented Feb 19, 2025

RoyEJohnson left a comment

RoyEJohnson left a comment

jomcarvajal commented Apr 8, 2025

TomWoodward Apr 9, 2025

TomWoodward Apr 9, 2025

TomWoodward Apr 9, 2025

jomcarvajal Apr 9, 2025

TomWoodward Apr 9, 2025

TomWoodward Apr 9, 2025

TomWoodward Apr 9, 2025

jomcarvajal Apr 9, 2025

TomWoodward Apr 9, 2025

TomWoodward Apr 9, 2025

TomWoodward Apr 9, 2025

TomWoodward Apr 9, 2025

TomWoodward Apr 9, 2025

TomWoodward left a comment

jomcarvajal commented Apr 10, 2025 •

edited

Loading

jomcarvajal commented Apr 10, 2025

TomWoodward commented Apr 25, 2025

	export const onKeyDownNavGroupSupport = ({
	export const treeNavSubtreeOnKeyDown = ({

	export const onKeyDownNavItemSupport = ({
	export const treeNavItemOnKeyDown = ({

Core 750: proper custom controls #2433

Are you sure you want to change the base?

Core 750: proper custom controls #2433

Conversation

jomcarvajal commented Feb 19, 2025

RoyEJohnson left a comment

Choose a reason for hiding this comment

RoyEJohnson left a comment

Choose a reason for hiding this comment

jomcarvajal commented Apr 8, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomWoodward left a comment

Choose a reason for hiding this comment

jomcarvajal commented Apr 10, 2025 • edited Loading

jomcarvajal commented Apr 10, 2025

TomWoodward commented Apr 25, 2025

jomcarvajal commented Apr 10, 2025 •

edited

Loading