Re-implement chooser modals with new design #9246

stevedya · 2022-09-22T14:59:20Z

This draft PR is for converting the current chooser functionality to use the new accessible choosers.

Still left TODO as part of this work:

Maybe:

Convert page/link choosers to use a dropdown next to the search as seen in the figma file

In Progress Screenshots

Image chooser

Image chooser with duplicated image

#Image chooser with open filters

Snippet chooser with search

Image chooser update form

Page chooser

Choose image format for Draftail images

squash-labs · 2022-09-22T14:59:25Z

Manage this branch in Squash

Test this branch here: https://stevedyafeaturea11y-choosers-2jmh3.squash.io

zerolab · 2022-09-27T08:14:39Z

What is the impact of these changes for third party packages?
If none, amazing! If any, can you document the upgrade path with specific examples?

gasman · 2022-09-29T16:03:59Z

Initially I wasn't keen on having a {% dialog %} stub embedded into every single instance of a chooser widget - which could add up to a lot of extra boilerplate on a complex "streamfield from hell" type page... but I'm coming round to the idea that it is OK (or at least, something to be dealt with outside the scope of this PR):

The {% dialog %} tag essentially moves the modal dialog page furniture into a Django template where it was previously embedded in modal-workflow.js, and from a maintainability POV I can't really complain about that...
Rendering this furniture once globally, or once per chooser type, would be preferable to once per chooser instance, but we do want the Chooser widget class to behave as a normal standalone Django form widget, and not depend on additional external conditions like "if you're going to use this widget in your form, the template must also output this HTML declaration with id foo". Essentially, to make that approach play well with Django forms, we'd need something that works like form media (i.e. generates a single declaration shared by all widget instances), but with arbitrary HTML fragments rather than just JS/CSS filenames. That something doesn't exist in Django forms.
Consequently: short of totally overhauling the Django forms framework, we can't escape the fact that any rendered form widget has to provide its own copy of any HTML fragments it intends to use.
Long term, though, we do have a plan to overhaul the Django forms framework - namely, Telepath. If we extend the approach we currently use for StreamField to work with the edit form as a whole, we'll decouple the form data from the once-per-field JS/HTML declarations, and reconcile them in client-side code - which means we avoid serving a separate copy of this HTML for each element.

stevedya · 2022-09-30T06:30:51Z

@gasman I've made quite a few updates since we last chatted about these changes and I do have some good news 🙂

After tinkering with the {% dialog %} inside of every chooser element approach, I ultimately ended up revising this to keep a similar pattern to what we have currently. This now only renders one global dialog template for choosers inside of dialog.js . And modal-workflow.js uses the functions of the new Dialog. So that should help address those concerns 🎉

gasman · 2022-09-30T18:26:04Z

client/src/entrypoints/admin/modal-workflow.js

-        self.body.html(response.html);
-        self.container.modal('show');
+        self.container.show();
+        self.body.innerHTML = response.html;


If I recall correctly, jquery's html(...) function has the hidden feature of executing any <script> tags in the HTML content, which innerHTML doesn't do. If that is the case, we should check whether any of our modals are relying on inline <script>s (I suspect they are...) and ensure we reinstate that behaviour.

Yeah jQuery does do that. It’s not too hard to replicate with vanilla JS but I think we could just stick to jQuery’s .html for the time being (and add a comment that this is for good reason).

gasman · 2022-09-30T18:50:41Z

client/src/includes/chooserModal.js

+import {initTabs} from './tabs';
+import {initTooltips} from './initTooltips';
+import initChooserCreationForm from './initChooserCreationForm';
+import initChooserFilters from "./chooserFilters";


Wouldn't it be neater to make initChooserCreationForm and initChooserFilters into methods on ChooserModalOnloadHandlerFactory, so that subclasses of ChooserModalOnloadHandlerFactory can customise them where appropriate?

(ah, now I see that initChooserCreationForm is called on page load too. Does that imply that "chooser creation forms" exist outside of chooser modals?)

Looks like that would also avoid having to expose ajaxify-chooser-links as a custom event, which I'm not entirely keen on - 'ajaxifying' feels like too much of an internal implementation detail of choosers to be appropriate to expose to external code.

Thanks for reviewing this 😃 Thats a great idea. It definitely would be a lot neater to have them inside of the ChooserModalOnloadHandlerFactory Chooser creation forms/toggling don't exist outside of the choosers so we definitely wouldn't need them on page load then either yep.

I agree on removing the ajaxify-chooser-links event because we won't need it then either. I'll add this to my TODO list

gasman · 2022-09-30T19:37:21Z

client/src/includes/chooserModal.js

@@ -290,9 +303,11 @@ class ChooserModalOnloadHandlerFactory {
  }

  onLoadReshowCreationFormStep(modal, jsonData) {
-    $(this.creationFormTabSelector, modal.body).replaceWith(
+    $(this.creationFormTabSelector, modal.body).html(


html() sets the inner content of the creationFormTabSelector element, while replaceWith replaces it. Given that the data-chooser-creation-form-wrapper element that creationFormTabSelector refers to is the top-level element of creation_form.html - and will thus appear in the htmlFragment - won't this change mean that we end up with two nested data-chooser-creation-form-wrapper elements?

Hmm yes there is a chance for duplicated nested elements if a creation_form is returned for sure. I may have to revisit this then.

I had originally made this change to avoid having to re-add the same classes / data attributes to all the potential nested templates. For example confirm_duplicate_upload.html when using replaceWith() will also need to have these attributes/classes for styling/events.

class="w-chooser-creation-form" data-chooser-creation-form-wrapper

I will also have to re-update event listeners when the element is replaced if we went with replaceWIth() (not a problem)

I would be interested in knowing all the possible templates that onLoadReshowCreationFormStep could potentially send back.

stevedya · 2022-10-03T23:33:38Z

What is the impact of these changes for third party packages? If none, amazing! If any, can you document the upgrade path with specific examples?

@zerolab Sorry for the delayed reply. This is a good idea! I did try to keep as much similar as I could but i'm sure some notes on the upgrade path will still be needed because of the vast amount of changes. I've put this as a TODO on this PR now. Thanks!

lb- · 2022-10-04T06:21:22Z

client/scss/components/_button.scss

@@ -330,6 +330,16 @@
    }
  }

+  // For buttons we want to look like inline links
+  &.plain {


Should we add these to the pattern library and styleguide template?

Note: We actually recently removed something similar unbutton as it was not used, not sure if we should revisit this class that was already present in the code instead of adding a new name?

Alternatively, should we name this to be a bit clearer - .as-link or w-button--as-link (for longer term adoption of BEM classes). Or does this get close enough if we add .link to the button?

see Removed legacy unbutton styles #9044 / 1d5824d

done as part of UX Unification - button component with variations #8790

This seems to me like something that might not need to live in this file as a generic button style to start with, since (at least currently) it’s very specific to choosers?

stevedya · 2022-10-04T21:59:33Z

wagtail/admin/templates/wagtailadmin/chooser/browse.html

+        <header class="w-chooser-header">
+            <div class="w-chooser-header__wrapper">
+                <div class="w-chooser-header__search-field">
+                    <form class="search-form search-form" action="{% url "wagtailadmin_choose_page_search" %}?page_type={{ page_type_string }}" method="get" novalidate role="search">


Gotta remove this duplicated class oops

thibaudcolas · 2022-10-04T21:59:44Z

client/scss/components/_button.scss

@@ -330,6 +330,16 @@
    }
  }

+  // For buttons we want to look like inline links
+  &.plain {


This seems to me like something that might not need to live in this file as a generic button style to start with, since (at least currently) it’s very specific to choosers?

thibaudcolas · 2022-10-04T22:00:13Z

client/scss/components/_dialog.scss

+  }
+}
+
+.w-chooser-listing {


This could probably use its own file.

thibaudcolas · 2022-10-04T22:00:28Z

client/scss/components/_dialog.scss

+
+}
+
+.w-chooser {


Seems like this could be worth its own file.

thibaudcolas · 2022-10-04T22:02:39Z

client/src/entrypoints/admin/modal-workflow.js

-        self.body.html(response.html);
-        self.container.modal('show');
+        self.container.show();
+        self.body.innerHTML = response.html;


Yeah jQuery does do that. It’s not too hard to replicate with vanilla JS but I think we could just stick to jQuery’s .html for the time being (and add a comment that this is for good reason).

thibaudcolas · 2022-10-04T22:06:28Z

client/src/includes/dialog.js

-};
+
+    const template = `
+        <div aria-hidden="true" id="${id}" aria-labelledby="title-${id}" class="w-dialog ${isChooser && 'w-dialog--chooser'}">


I think this would need a comment to explain why we have this HTML in JS.

thibaudcolas · 2022-10-04T22:22:31Z

wagtail/images/templates/wagtailimages/chooser/chooser.html

+                                {% for tag in popular_tags %}
+                                    <option value="{{ tag.name }}">{{ tag.name }}</option>
+                                {% endfor %}
+                            </select>


thibaudcolas · 2022-10-04T22:22:44Z

wagtail/images/templates/wagtailimages/chooser/confirm_duplicate_upload.html

+    <div class="w-dialog__message w-dialog__message--warning">
+        {% icon name='warning' class_name="w-dialog__message-icon" %}
+        <div class="w-dialog__message-header">
+            <p class="w-dialog__message-description ">


Suggested change

<p class="w-dialog__message-description ">

<p class="w-dialog__message-description">

thibaudcolas · 2022-10-04T22:24:17Z

client/scss/components/_listing.scss

-      font-size: 3rem;
+    .icon {
+      width: 100%;
+      max-height: theme('spacing.9');
    }


Wouldn’t we have to keep all styles here as-is for tables listing?

thibaudcolas · 2022-10-04T22:30:48Z

client/scss/components/_dialog.scss

+  }
+
+  .w-chooser__body[aria-hidden="true"] {
+    transform: translateX(-100%);


Does this work in RTL mode?

thibaudcolas · 2022-11-23T09:47:46Z

We’re currently intending to pick this up in either the February or May 2023 releases. Here is a record of (potentially) related chooser issues and pull requests which I expect we will want to make updates to once this is merged.

I’ve classified them by chooser though there’s a high likelihood quite a few of those apply to more choosers than reported.

All choosers

Page/link chooser

Document chooser

Document chooser’s "Search" tab broken after a failed upload #6126

Image chooser

Generic chooser

PRs

lb- · 2022-11-23T09:55:24Z

Will there be any plans to revisit the JS approach in the context of Stimulus adoption? I've kind of kept that out of scope for the Outreachy project for now but have a few ideas about chooser implementations that would leverage Stimulus.

Let me know if it's worth a POC or some notes

thibaudcolas · 2022-11-23T10:08:51Z

@lb- we’re planning to revisit the JS but haven’t discussed further how exactly to go about that.

dkirkham · 2023-02-04T00:32:53Z

I'm not sure where this request lands in the 30-odd issues listed above - quite a few places possibly - but there are two things I'd find really useful in redeveloped choosers:

a unified link chooser dialog that could be called from inside a RichTextField for inline links, and which could also be called from a streamfield Block for rendering, for instance, as a button. A features field could be used to control the types of links that can be selected, (ie. pages, documents, email, phone, anchor) in a similar way to the features within a RichTextField itself.
the page chooser function (both alone and as part of a unified link dialog) supporting links to anchors in Wagtail managed pages.

Also I think worth considering for the chooser dialogs:

support for displaying per-model icons, as envisaged in Add the ability to define custom icons for Pages #9652. I have a PR for snippets at Custom snippet icons #9804 but got a bit stuck doing the same for pages.
support for displaying per-instance model images as thumbnails - eg. an Author model which includes an image of said author.

lb- · 2024-02-18T04:41:09Z

@stevedya @thibaudcolas - what's the status of this work? Is this something that still aligns with the frontend / UI strategy?

A lot has changed since this PR was opened so not sure on the best way forward.

Multiple parts of the JS impacted here has changed with Stimulus adoption, formalising support for RTL layouts, larger reworking of choosers that have since happened, and an extensive amount of rework of base listings & generic chooser viewsets.

Is there a way we can split out some parts of the goals here into smaller issues that can be picked up by a wider range of contributors?

Maybe there's parts that make sense as part of further inline script removal / Stimulus migration of choosers also.

I have a call lined up with @thibaudcolas and @laymonage for a fortnight's time, maybe this is a good discussion item if we get this happening.

@dkirkham you probably already know but there are lots of improvements to the way generic choosers can be build with Chooser Viewsets since this PR was raised. https://docs.wagtail.org/en/stable/reference/viewsets.html#chooserviewset

However, other items are still outstanding from what I can see.

dkirkham · 2024-02-18T06:48:33Z

@lb-, I don't believe any of my wish list have been implemented. Do you think it is worth writing them up as feature requests so they are not lost?

lb- · 2024-02-18T07:06:05Z

Thanks @dkirkham - I must not have read your comments correctly then.

Anyway, yes I think think an issue or two-ish to clearly lay out the enhancements you'd like to see would be helpful. Doesn't mean it will be built but it can help guide future directions, external packages or APIs even if not solved directly as requested.

It's also a good way to have a discussion with others that may want to see similar features outside of a PR.

Thanks.

gasman reviewed Sep 30, 2022

View reviewed changes

lb- reviewed Oct 4, 2022

View reviewed changes

stevedya commented Oct 4, 2022

View reviewed changes

thibaudcolas reviewed Oct 4, 2022

View reviewed changes

thibaudcolas added this to the 4.1 milestone Oct 7, 2022

gasman modified the milestones: 4.1, 4.2 Oct 17, 2022

thibaudcolas changed the title ~~Draft: A11y choosers setup and styling~~ Draft: Re-implement chooser modals with new design Nov 17, 2022

thibaudcolas changed the title ~~Draft: Re-implement chooser modals with new design~~ Re-implement chooser modals with new design Nov 23, 2022

thibaudcolas added type:Bug type:Enhancement type:Cleanup/Optimisation status:Needs Work status:Needs Tests Accessibility component:Choosers Modal choosers for Page, Snippet and other models labels Nov 23, 2022

Start plugging in new chooser dialog

873a0ff

thibaudcolas force-pushed the feature/a11y-choosers branch from 25127c8 to 873a0ff Compare November 23, 2022 10:06

gasman modified the milestones: 4.2, 4.3 Jan 18, 2023

lb- mentioned this pull request Feb 23, 2023

[WIP] Unify link choosers #1645

Closed

thibaudcolas removed this from the 5.0 milestone Apr 19, 2023

laymonage mentioned this pull request Oct 2, 2023

Add workflow actions to the Wagtail userbar #9106

Open

thibaudcolas mentioned this pull request Mar 14, 2024

MultipleChooserPanel like ListBlock #11318

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-implement chooser modals with new design #9246

Re-implement chooser modals with new design #9246

stevedya commented Sep 22, 2022 •

edited by thibaudcolas

squash-labs bot commented Sep 22, 2022

zerolab commented Sep 27, 2022

gasman commented Sep 29, 2022

stevedya commented Sep 30, 2022 •

edited

gasman Sep 30, 2022

thibaudcolas Oct 4, 2022

gasman Sep 30, 2022 •

edited

stevedya Oct 3, 2022

gasman Sep 30, 2022

stevedya Oct 3, 2022

stevedya commented Oct 3, 2022

lb- Oct 4, 2022

thibaudcolas Oct 4, 2022

stevedya Oct 4, 2022

thibaudcolas Oct 4, 2022

thibaudcolas Oct 4, 2022

thibaudcolas Oct 4, 2022

thibaudcolas Oct 4, 2022

thibaudcolas Oct 4, 2022

thibaudcolas Oct 4, 2022

thibaudcolas Oct 4, 2022

stevedya Oct 4, 2022

thibaudcolas Oct 4, 2022

thibaudcolas Oct 4, 2022

thibaudcolas commented Nov 23, 2022

lb- commented Nov 23, 2022

thibaudcolas commented Nov 23, 2022

dkirkham commented Feb 4, 2023

lb- commented Feb 18, 2024

dkirkham commented Feb 18, 2024

lb- commented Feb 18, 2024

	<p class="w-dialog__message-description ">
	<p class="w-dialog__message-description">


		}

		.w-chooser {

Re-implement chooser modals with new design #9246

Are you sure you want to change the base?

Re-implement chooser modals with new design #9246

Conversation

stevedya commented Sep 22, 2022 • edited by thibaudcolas

In Progress Screenshots

Image chooser

Image chooser with duplicated image

#Image chooser with open filters

Snippet chooser with search

Image chooser update form

Page chooser

Choose image format for Draftail images

squash-labs bot commented Sep 22, 2022

Manage this branch in Squash

zerolab commented Sep 27, 2022

gasman commented Sep 29, 2022

stevedya commented Sep 30, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gasman Sep 30, 2022 • 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

stevedya commented Oct 3, 2022

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

thibaudcolas commented Nov 23, 2022

All choosers

Page/link chooser

Document chooser

Image chooser

Generic chooser

PRs

lb- commented Nov 23, 2022

thibaudcolas commented Nov 23, 2022

dkirkham commented Feb 4, 2023

lb- commented Feb 18, 2024

dkirkham commented Feb 18, 2024

lb- commented Feb 18, 2024

stevedya commented Sep 22, 2022 •

edited by thibaudcolas

stevedya commented Sep 30, 2022 •

edited

gasman Sep 30, 2022 •

edited