Migrate breadcrumbs to Stimulus (`RevealController` / `w-breadcrumb`) #10793

lb- · 2023-08-17T10:00:19Z

Overview

Adds a new generic RevealController that handles the ability for content to be opened/closed while updating the relevant parts of the DOM such as aria-expanded on the toggle.
Register this controller as a generic w-reveal and also the w-breadcrumbs (plural) identifiers so that we can differentiate usage via different DOM attributes and events.
Updates the root class on the breadcrumbs to align with the plural form w-breadcrumbs not w-breadcrumb.
This code paves the way to replace the other two JS implementations of open/close panel widgets; Collapsible Panels and the Collapsible sidebar but aims to not implement too much for that upcoming work. e.g. 🎛️ Migrate panels to a Stimulus controller w-panel #10168
Closes 🎛️ Migrate Breadcrumbs to a Stimulus Controller w-breadcrumbs #10118
Replaces PR Migrated breadcrumbs to stimulus controller #10160

Testing

If manually testing this feature; be sure to check the 'hover to expand' behaviour, this is a bit nuanced but basically allows the 'peek' of the breadcrumbs being shown as open when you hover and they should only close if you move away from the header as a whole. Additionally, clicking the toggle when in this 'peek' mode should mean they stay open.
Breadcrumb usage (JS enabled); Editing a Snippet, Editing a Page.
Breadcrumb usage (no JS should be enabled); Page chooser panel, same UI but not the hover effect and no toggle.

Checklist

Do the tests still pass?
Does the code comply with the style guide?
For Python changes: Have you added tests to cover the new/fixed behaviour?
For front-end changes: Did you test on all of Wagtail’s supported environments?[^2]
- Safari 16.1, Firefox 115, Chrome 117
For new features: Has the documentation been updated accordingly? (added note to upgrade considerations)

squash-labs · 2023-08-17T10:15:44Z

Manage this branch in Squash

Test this branch here: https://lb-feature10118-migrate-breadc-dd2fd.squash.io

lb- · 2023-08-17T22:28:15Z

Just cleaned up a few things upon re-reading the PR; some duplicate HTML in the unit tests, a better solution for the initial toggle target access that I had to implement a workaround for hotwired/stimulus#711 and additional classes handling.

Finally, the 'ready' event now fires after a setTimeout, this should allow any other non-Stimulus JS on initial load to listen to these events a bit easier. Less chance of a race condition between Stimulus & non-Stimulus code (e.g. minimap listeners / tooltip listeners).

docs/releases/5.2.md

laymonage

Thanks for this PR! In terms of functionality, it seems to work as expected (I didn't see any change in user-facing behaviour). I can't wait to use it to work on #8767!

However, I find it hard to review because the refactoring is done in a single commit that simultaneously handles other cases that aren't actually observed in Wagtail's breadcrumb templates.

I appreciate that you're trying to pave the way for migrating the panels to Stimulus, which I think would make sense to use the same controller if possible. That said, it's much easier to review if we try to build the minimum Stimulus-equivalent to breadcrumb.js first, and then try to handle other cases in subsequent commits/PRs. It helps the reviewer be much more confident in understanding what exactly the changes are trying to accomplish, which makes it more likely to be merged faster 😃

laymonage · 2023-08-31T11:25:18Z

client/src/controllers/RevealController.ts

+    if (isClassNameClosed && (!ariaExpanded || ariaExpanded === 'false')) {
+      this.closedValue = true;
+    } else if (ariaExpanded === 'false') {
+      this.closedValue = true;
+    }


I wonder if the following is equivalent and easier to read?

Suggested change

if (isClassNameClosed && (!ariaExpanded || ariaExpanded === 'false')) {

this.closedValue = true;

} else if (ariaExpanded === 'false') {

this.closedValue = true;

}

if (ariaExpanded === 'false') {

this.closedValue = true;

} else if (isClassNameClosed && !ariaExpanded) {

this.closedValue = true;

}

or maybe even

Suggested change

if (isClassNameClosed && (!ariaExpanded || ariaExpanded === 'false')) {

this.closedValue = true;

} else if (ariaExpanded === 'false') {

this.closedValue = true;

}

// if aria-expanded is set to false,

// or the closed class is used but aria-expanded is unset,

// ensure it's closed

if (ariaExpanded === 'false' || (isClassNameClosed && !ariaExpanded)) {

this.closedValue = true;

}

That said, I'm not sure what kind of use case we're trying to handle here and why we need to be smart about it. Is it in case the initial values of aria-expanded, the closedValue, and isClassNameClosed are inconsistent? If so, that sounds like a developer error due to misconfiguration in the templates to me and not something we need to explicitly handle case-by-case. Like, in here, what happens if ariaExpanded is true but closedValue is also true?

Can we just rely on the initial value of closedValue as the single source of truth and enforce it by calling closedValueChanged or some way? I'd much prefer us have one thing that acts as the initial value of the expand/collapsed state, and the other attributes (aria-expanded, class) would follow.

This is mostly to set up for the panels use case where you can add the collapse class and it will load in the DOM as collapsed.

https://github.com/wagtail/wagtail/blob/main/client/src/includes/panels.ts#L67

It's confusing I know but it will be required for the Panels usage alongside dynamic usage in StreamFields.

I can remove this for now and we can just discuss the implementation when ready for the Panels work.

This has been removed.

Ahh yes, sorry, I completely forgot about the fact that we support the collapse class for panels. But yep, while it's necessary for panels, we can focus on that later.

Maybe we can look into alternatives such as providing the appropriate Stimulus attribute from the Python side if the collapse class is used. Or a different approach entirely e.g. a collapsed flag on the Panel instance itself, with the Stimulus attributes handled in the base class. A breaking change, but I think would be a welcome one. In any case, it's easier to focus on exploring those approaches later than trying to fit it in here. Thanks for the update!

laymonage · 2023-08-31T13:39:23Z

client/src/controllers/RevealController.ts

+    const iconClass =
+      icon &&
+      (!hasCloseIconClass || !hasOpenIconClass) &&
+      [...icon.classList].find((className) => className.startsWith('icon-'));
+
+    if (iconClass) {
+      if (!hasCloseIconClass) {
+        if (iconClass) {
+          this.element.setAttribute(
+            `data-${this.identifier}-close-icon-class`,
+            iconClass,
+          );
+        }
+      } else if (!hasOpenIconClass) {


Again, I don't know why we need to be "smart" about this. I think that instead of trying to figure out and resolve values/classes that are unset in the HTML, it's better if we agree on what the expected attributes for this controller are, and make it so that the HTML templates are consistently configured. At least for now, it's much easier to understand that way, as the code doesn't have to deal with cases like "what if attribute X isn't set?".

There are cases where optional attributes make sense and it's pretty clear to give sensible defaults, but at this point most of these cases still feel very hypothetical and make the code harder to review.

In other words: if you're configuring the HTML in a way that we don't currently use within Wagtail, it's going to result in an error (like the missing content target) or undefined behaviour, and that's OK. If we do find use cases where it's possible to reuse the code with a slightly different attribute configuration, then we start handling such variations and making the controller code more robust.

If we do it too early, it's premature abstraction. We don't yet know what the other use cases would look like, so the abstraction/handling we put in place may or may not be necessary in the future, and if it does, it may not retain its shape as-is. Until then, it's hard to read the code without imagining what exactly the case is here.

Agreed, this can be removed and added back later when needed for the panels or maybe we just change the approach.

This has been removed.

laymonage · 2023-08-31T13:42:46Z

client/src/controllers/RevealController.test.js

+      );
+    });
+
+    it('should allow for a class to set the initial state of closed', async () => {


Ah, yep, I just read the test after reviewing the implementation. This and the following two cases are what I meant, I think we should just consistently use the closedValue's attribute as the single source of truth for the initial state, ignoring (and resolving) the others (class and aria-expanded) based on it.

Yeah, I agree this is a bit convoluted, removing for now and IF we need it built this way for the panels implementation, we can revisit then.

Thanks for the feedback here.

laymonage · 2023-08-31T13:52:10Z

client/src/controllers/RevealController.ts

+ * Adds the ability to make the controlled element be used as an
+ * opening/closing (aka collapsing) element.
+ * Supports, and relies on, correct usage of aria-* attributes.


With this description I think it's more fitting if we name it something like ExpandController or CollapsibleController. The former seems short and expressive enough, but not sure if that would cause any confusion with aria-expand. I don't think so, and it makes sense (at least to me) that this would control aria-expand just because it's clearly controlling an element that can be expanded/collapsed.

I started this way when first looking into this but ran into a few issues.

Common typos - Collapsible vs Collapsable - https://thecontentauthority.com/blog/collapsable-vs-collapsible

Longer name makes all attributes longer

It makes it tricky to NOT use the 'state' of expanded or collapsed and the 'actions' of expand or collapse. Using hide/show is not the most suitable here and is better for things that fully hide/show (e.g. tooltips, dialog use hide/show).

Additionally, based on feedback on the Panel issue #10168 (comment) we found that there is already a nice precedent in the community for naming this https://www.stimulus-components.com/docs/stimulus-reveal-controller/

Our existing implementations have the following

https://github.com/wagtail/wagtail/blob/main/client/src/includes/breadcrumbs.js (open/closed)

https://github.com/wagtail/wagtail/blob/main/client/src/entrypoints/admin/preview-panel.js (show/hide)

https://github.com/wagtail/wagtail/blob/main/client/src/includes/panels.ts (expand/collapse)

If it's critical I am more than happy to re-write this using the Expand/Collapse naming but I did go down this way already and it felt more verbose.

Good points, yeah with ExpandController I kind of wondered if it's going to be confusing with actions/states, not just aria-expanded. Thanks for the link to the precedent, I'm happy to keep the naming 👍

laymonage · 2023-08-31T13:57:27Z

client/src/controllers/index.ts

@@ -34,6 +35,8 @@ export const coreControllerDefinitions: Definition[] = [
  { controllerConstructor: DropdownController, identifier: 'w-dropdown' },
  { controllerConstructor: MessagesController, identifier: 'w-messages' },
  { controllerConstructor: ProgressController, identifier: 'w-progress' },
+  { controllerConstructor: RevealController, identifier: 'w-breadcrumb' },


Any particular reason why the singular breadcrumb?

haha, it's always been like that since the initial version @thibaudcolas built data-breadcrumb-next not data-breadcrumbs-next.

I am not sure, but have kept with the convention that exists, especially since the root class is w-breadcrumb not w-breadcrumbs.

If you think it's worth changing, I would 100% prefer the plural form here but would want to get the class changed also.

Don't really mind either way, but I also would prefer the plural form. If it were only data-breadcrumb-next that we're dealing with, I think it's worth renaming, but as we also have w-breadcrumb in the CSS, might be better to stick with the existing singular form. If Thibaud weighs in, I'm happy to go with whatever he says 👍

I see that our JS events were in the plural form so I think it is reasonable to update this across the board to be plural.

I have done this, will force push and I have included a note in the release notes about the classes change from w-breadcrumb to w-breadcrumbs.

lb- · 2023-08-31T21:41:30Z

Thanks for this PR! In terms of functionality, it seems to work as expected (I didn't see any change in user-facing behaviour). I can't wait to use it to work on #8767!

Brilliant, thank you for testing.

However, I find it hard to review because the refactoring is done in a single commit that simultaneously handles other cases that aren't actually observed in Wagtail's breadcrumb templates.
I appreciate that you're trying to pave the way for migrating the panels to Stimulus, which I think would make sense to use the same controller if possible....

Agreed, I have removed the ones that are clearly isolated from the core open/close behaviour and class setting/resetting. I can remove the initial class if needed also.

@laymonage - really appreciate the thorough review, let me know if you really disagree with the RevealController/Open/Closed naming setup and I can rebuild this accordingly but this seemed to be the best direction to go in based on lengthy discussions in #10168 and my review of all other implementations of similar functionality.

laymonage · 2023-08-31T22:36:40Z

Thanks for the updates LB! I'm off tomorrow but I'll re-review when I'm back on Monday. Really appreciate the fast response 😃

laymonage

Thank you, I think this is easier to understand now.

I would've much preferred it if we hadn't handled the "fall back to aria-controls if the toggle target is not specified" just yet, or at least add it in a separate commit.

It seems the code would be much easier to understand if we had one commit that did the initial RevealController implementation just by sticking to how the breadcrumbs.html template uses the controller, i.e. the icon classes, toggle target, and content target are all well-defined in the template.

Then, a separate commit can be added that does all the "smart" tricks like falling back to aria-controls, allowing the toggle target to be outside of the controller's scope, etc. Or, we can even leave that in a separate PR, with the accompanying concrete use case (e.g. panels migration).

At this point those changes are completely irrelevant to the breadcrumbs code, so I found them too distracting when reviewing. That said, it's already done and I've already reviewed it though, so I suppose it's fine to leave them in.

Anyway, for the breadcrumbs, I've tested in the following places:

Page explorer
Page edit
Page chooser
Snippets

And I didn't notice change in behaviour, other than the hidden="until-found" I mentioned, so hopefully we're good after that.

laymonage · 2023-09-04T16:12:49Z

client/src/controllers/RevealController.ts

+        /**
+         * Use experimental `until-found` value, so users can search inside the content.
+         * Browsers without support for `until-found` will not have this value set
+         */
+        if ('onbeforematch' in document.body) {
+          content.setAttribute('hidden', 'until-found');


TIL!

But this doesn't seem to work as expected?

Screen.Recording.2023-09-04.at.17.10.54.mov

Or do you need to listen to the beforematch event and toggle the expand?

The beforematch listener needs to be in the html actions. Good find, I'll revisit this.

Actually sorry, beforematch won't make sense for breadcrumbs as they are set to hidden by default.
I will remove this functionality completely, thanks for raising.

laymonage · 2023-09-04T17:53:36Z

client/src/controllers/RevealController.ts

+  initialize() {
+    const toggleTarget = this.toggleTarget;
+
+    if (!this.hasContentTarget) {


As I understand, this aria-controls handling is still unused in our usage of the breadcrumbs, and only here to support potential future RevealController usages. Is that correct?

We probably should use it in the breadcrumbs but I didn't want to add too much to the html. We would need to add an ID to each Breadcrumbs item and then also add the set of the IDs to the toggle.

But yes, it's not used by the breadcrumbs, but to add that in another PR seemed quite noisy. So I opted to keep it in.

ok, I have cleaned this up further, sorry for the runaround, new update coming.

Unfortunately, adding an ID to each breacrumb item is not practical without a larger change to the breadcrumbs include/shared template usage as we would probably need a base ID to build on.

Thank you!

We probably should use it in the breadcrumbs but I didn't want to add too much to the html. We would need to add an ID to each Breadcrumbs item and then also add the set of the IDs to the toggle.

Unfortunately, adding an ID to each breacrumb item is not practical without a larger change to the breadcrumbs include/shared template usage as we would probably need a base ID to build on.

That is exactly why I much preferred not having the code to handle aria-controls just yet. The breadcrumbs HTML code in this PR works without changing much of the HTML (just sprinkling some Stimulus attributes is enough), so I didn't see why we need to add the aria-controls assumption for the toggles in the controller if it isn't used.

And I wasn't expecting you to add the aria-controls to the HTML so that the code is used, but rather: we stick to Stimulus attributes for now, and see if we can reintroduce the aria-controls handling later when there's a more appropriate use case, e.g. the panels where they already have aria-controls and ids and it's much simpler to use those there instead of adding Stimulus attributes.

Sorry if I wasn't clear, glad you didn't proceed with it 😅

- Adds a new generic `RevealController` that handles the ability for content to be opened/closed while updating the relevant parts of the DOM such as aria-expanded on the toggle. - Register this controller as a generic `w-reveal` and also the `w-breadcrumb` identifiers so that we can differentiate usage via different DOM attributes and events. - Closes wagtail#10118

- This way we align with the controller name

lb- · 2023-09-04T20:41:52Z

Thanks @laymonage - I hope this is in a better shape for the initial implementation.

There is still some code that relates to aria-controls in place but this is stuff that intertwines with other code and hopefully it's accepted as is.

Should be good for another review now, all the initalize and the onbeforematch code has been completely removed.

laymonage

Thank you! This is much, much easier to understand as the controller code is now closer to the most minimum implementation required to migrate the breadcrumbs to Stimulus.

It could be even more simplified if we just assume that there's only one toggle and that's only linked as a Stimulus target (via toggleTarget) instead of using toggleTargets and also gathering the elements that use aria-controls to the contentTargets. I believe that would be sufficient for the breadcrumbs without any other changes to the HTML.

Or, at least using toggleTargets directly without scanning for aria-controls, so we can keep all the handling of "multiple toggles" in other methods without necessarily handling the aria-controls case just yet. Later, we can add the logic in get toggles() that concats toggleTargets with the aria-controls for the contentTargets, when we actually do have the use case for it.

However, I understand that it could be useful later on e.g. with the panels, and it's simple enough to understand so I'm happy to keep that in.

Again, thank you very much! I'll leave this unmerged in case you want to make additional tweaks.

lb- · 2023-09-05T10:05:51Z

Thank you @laymonage - appreciate the feedback. I know it's a bit of a stretch to include some code for 'future', thanks for being OK with some of this being included.

- This is not used and was added as part of wagtail#10793 - Avoid this controller managing this behaviour and instead if needed we can use w-init

- This is not used and was added as part of #10793 - Avoid this controller managing this behaviour and instead if needed we can use w-init

lb- mentioned this pull request Aug 17, 2023

Migrated breadcrumbs to stimulus controller #10160

Closed

8 tasks

lb- force-pushed the feature/10118-migrate-breadcrumbs-to-stimulus branch from 6d1bf0a to ef8814c Compare August 17, 2023 22:25

lb- mentioned this pull request Aug 17, 2023

🎛️ Migrate Breadcrumbs to a Stimulus Controller w-breadcrumbs #10118

Closed

zerolab reviewed Aug 18, 2023

View reviewed changes

docs/releases/5.2.md Outdated Show resolved Hide resolved

lb- force-pushed the feature/10118-migrate-breadcrumbs-to-stimulus branch from ef8814c to 76b1550 Compare August 18, 2023 22:36

This was referenced Aug 18, 2023

🎛️ Migrate panels to a Stimulus controller w-panel #10168

Open

Migrate panel stimulus w panel #10238

Closed

lb- added status:Needs Review javascript Pull requests that update Javascript code component:Stimulus labels Aug 21, 2023

laymonage self-assigned this Aug 29, 2023

laymonage self-requested a review August 29, 2023 08:25

laymonage added this to the 5.2 milestone Aug 29, 2023

laymonage requested changes Aug 31, 2023

View reviewed changes

lb- force-pushed the feature/10118-migrate-breadcrumbs-to-stimulus branch from 76b1550 to b0b66fd Compare August 31, 2023 21:34

lb- requested review from zerolab and laymonage August 31, 2023 21:41

lb- force-pushed the feature/10118-migrate-breadcrumbs-to-stimulus branch from b0b66fd to 02b6766 Compare September 1, 2023 06:04

laymonage reviewed Sep 4, 2023

View reviewed changes

Rename breadcrumbs.js to RevealController

22fef6a

lb- force-pushed the feature/10118-migrate-breadcrumbs-to-stimulus branch from 02b6766 to 260575e Compare September 4, 2023 20:32

lb- added 2 commits September 5, 2023 06:38

Update w-breadcrumb root class to w-breadcrumbs (plural)

bad0faa

- This way we align with the controller name

lb- force-pushed the feature/10118-migrate-breadcrumbs-to-stimulus branch from 260575e to bad0faa Compare September 4, 2023 20:40

lb- requested a review from laymonage September 4, 2023 20:41

laymonage approved these changes Sep 5, 2023

View reviewed changes

lb- removed the status:Needs Review label Sep 5, 2023

lb- merged commit 8a52269 into wagtail:main Sep 5, 2023
18 of 19 checks passed

lb- deleted the feature/10118-migrate-breadcrumbs-to-stimulus branch September 5, 2023 11:23

lb- pushed a commit to Chiemezuo/wagtail that referenced this pull request Oct 24, 2023

Remove RevealController initial class

b01ddd1

- This is not used and was added as part of wagtail#10793 - Avoid this controller managing this behaviour and instead if needed we can use w-init

lb- pushed a commit to Chiemezuo/wagtail that referenced this pull request Oct 24, 2023

Remove RevealController initial class

0356438

- This is not used and was added as part of wagtail#10793 - Avoid this controller managing this behaviour and instead if needed we can use w-init

lb- pushed a commit that referenced this pull request Oct 24, 2023

Remove RevealController initial class

415b569

- This is not used and was added as part of #10793 - Avoid this controller managing this behaviour and instead if needed we can use w-init

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate breadcrumbs to Stimulus (`RevealController` / `w-breadcrumb`) #10793

Migrate breadcrumbs to Stimulus (`RevealController` / `w-breadcrumb`) #10793

lb- commented Aug 17, 2023 •

edited

squash-labs bot commented Aug 17, 2023

lb- commented Aug 17, 2023

laymonage left a comment

laymonage Aug 31, 2023

lb- Aug 31, 2023

lb- Aug 31, 2023

laymonage Aug 31, 2023 •

edited

laymonage Aug 31, 2023 •

edited

lb- Aug 31, 2023

lb- Aug 31, 2023

laymonage Aug 31, 2023

lb- Aug 31, 2023

laymonage Aug 31, 2023

lb- Aug 31, 2023

laymonage Aug 31, 2023

laymonage Aug 31, 2023

lb- Aug 31, 2023 •

edited

laymonage Aug 31, 2023

lb- Sep 1, 2023

lb- commented Aug 31, 2023

laymonage commented Aug 31, 2023

laymonage left a comment •

edited

laymonage Sep 4, 2023

lb- Sep 4, 2023

lb- Sep 4, 2023

laymonage Sep 4, 2023

lb- Sep 4, 2023

lb- Sep 4, 2023

lb- Sep 4, 2023

laymonage Sep 5, 2023

lb- commented Sep 4, 2023

laymonage left a comment •

edited

lb- commented Sep 5, 2023

Migrate breadcrumbs to Stimulus (RevealController / w-breadcrumb) #10793

Migrate breadcrumbs to Stimulus (RevealController / w-breadcrumb) #10793

Conversation

lb- commented Aug 17, 2023 • edited

Overview

Testing

Checklist

squash-labs bot commented Aug 17, 2023

Manage this branch in Squash

lb- commented Aug 17, 2023

laymonage left a 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

laymonage Aug 31, 2023 • edited

Choose a reason for hiding this comment

laymonage Aug 31, 2023 • edited

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

lb- Aug 31, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lb- commented Aug 31, 2023

laymonage commented Aug 31, 2023

laymonage left a comment • edited

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

lb- commented Sep 4, 2023

laymonage left a comment • edited

Choose a reason for hiding this comment

lb- commented Sep 5, 2023

Migrate breadcrumbs to Stimulus (`RevealController` / `w-breadcrumb`) #10793

Migrate breadcrumbs to Stimulus (`RevealController` / `w-breadcrumb`) #10793

lb- commented Aug 17, 2023 •

edited

laymonage Aug 31, 2023 •

edited

laymonage Aug 31, 2023 •

edited

lb- Aug 31, 2023 •

edited

laymonage left a comment •

edited

laymonage left a comment •

edited