Hide "Save and insert" and "Additional selected media" from users by default

How about:

1. Save and select → Save and continue
2. Save and insert → Save and use

Since these buttons are always together (?), we can also explore options where the of one button contexualizes the other. Choices that may not work for a single button but work well when paired. For example, the "Save and continue" suggestion in #4 appeals to me, I think that anything confusing about that wording can be mitigated by the wording of its companion button.

Anyway, I provided a list of several different places to store a bike. I have no presumptions about any of them being the ideal choice, this just seems like a problem that benefits from a little brainstorming.

I'm also wondering if the buttons should receive the same styling as one currently has primary styling and the other secondary, but that doesn't represent the actual relationship of the buttons.

Anyway, I provided a list of several different places to store a bike.

🤩😆

I realized it might help to know what buttons we're actually talking about. We're talking about the ones when uploading (adding) media:

Analysis

I tried to take a bigger picture perspective. I stepped through this in the UI.

I now think the problem isn't these button labels, but that we want the user to be able to "skip a step" and in doing so we complicate things.

• "Save and select" actually means "Save the media I just created and select it in the media library, adding to whatever selection I already have in the media library"
• "Save and insert" means "Save the media I just created and select it in the media library, adding to whatever selection I already have in the media library, and then take that entire selection and insert it into the field" (whether it's a reference field or a body field doesn't matter here)

Note that both actually select the media we just added. Which makes sense. Why would I be adding media that I don't intend to use?

Proposal

Imagine if we removed that ability to insert. Meaning, that once you're adding media, you're only adding media. You need to get back to the Media Library selection UI before you can insert. That'd mean one extra button click but zero confusion.

Because then we'd have only one button: "Save". For sighted users, it's going to be obvious the newly created media is selected. For visually impaired users, we could add a Drupal.announce() call.

The irony is that #3023797: Let users choose what to do after selecting and/or adding items in the media library (which triggered this issue) is where those buttons were added. But I think that thanks to improvements in other aspects of the Media Library, we should reconsider whether that solution was the right one.

That'd then look like this:

Consequence of the proposal

The whole "selection area" thing in a <details> element would then also go away. It's super confusing anyway.

I never noticed it even existed until today, until me trying to make sense of things. The selection area (a "collapsed details" element) only appears if you already have a selection and are adding more media. It looks super weird and confusing that the selection is still visually present in this weird way when adding more media. I wonder if this was done for accessibility reasons?

A walkthrough of the patch in #8:

1. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -201,7 +201,6 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   -      $form['selection'] = $this->buildCurrentSelectionArea($form, $form_state);
   
   @@ -365,90 +364,6 @@ protected function buildEntityFormElement(MediaInterface $media, array $form, Fo
   -  protected function buildCurrentSelectionArea(array $form, FormStateInterface $form_state) {
   ...
   -  protected function buildSelectedItemElement(MediaInterface $media, array $form, FormStateInterface $form_state) {
   

   This is the selection area being removed.

2. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -466,20 +381,12 @@ protected function buildActions(array $form, FormStateInterface $form_state) {
   -        '#value' => $this->t('Save and select'),
   +        '#value' => $this->t('Save'),
   

   This is the rename.

3. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -466,20 +381,12 @@ protected function buildActions(array $form, FormStateInterface $form_state) {
   -        '#value' => $this->t('Save and insert'),
   
   @@ -724,37 +635,6 @@ protected function buildMediaLibraryUi(FormStateInterface $form_state) {
   -  public function updateWidget(array &$form, FormStateInterface $form_state) {
   

   This button is now gone. Hence its callback can be removed too.

4. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   @@ -690,15 +597,19 @@ public function updateLibrary(array &$form, FormStateInterface $form_state) {
   +    $announcement = \Drupal::translation()->formatPlural(count($added_media_items), 'Added one media item to selection.', 'Added @count media items to selection.');
   +    $response->addCommand(new AnnounceCommand($announcement));
   

   This is the announcement for accessibility reasons, which informs a visually impaired user that triggering the "Save" button not only saved the newly created media, but also added it to the current selection.

5. +++ b/core/modules/media_library/src/Form/AddFormBase.php
   --- a/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   

   All test coverage changes are to accommodate for:

   1. the button rename
   2. the removal of the other button, which means the user must now trigger that "Insert selected" button afterwards manually, without a shortcut for it (I've laid out the reasoning why in my previous comment)
   3. the removal of the "selection area" which IMHO is a confusing/bad UX

We have had reports that’s it was not entirely clear what happened to previously uploaded files, so we tried to preserve those for UX and A11Y purposes in the second step of the add form. The extra save and insert button was added for users that want to skip a step and use the media library a lot.

I do see there is value in hiding options for users by default. We could make the extra button and selection overview optional by adding a checkbox to turn it on/off in the widget / WYSIWYG config? I would say the extra 'Save and insert' button should be disabled by default, and the selection area should be enabled by default.

On the other hand, since this is all a form, we could easily let this be handled by contrib as well. Not sure how many options core should support for this.

Ooof. Okaaayyy. @Wim Leers walked me through his patch and rationale a bit earlier today.

First, some backstory:

The reason that second button initially came about was two-fold:

1) There were a variety of interaction problems with Media Library at the time, which are documented in the original issue summary, and I think all of which are fixed now... yay!

2) With a mind toward the fact that the "80% case" for Media is to add new things, we wanted to add a "shortcut" mechanism that did not involve an extra frustrating click every time you did that. (This also brings parity with Image fields which let you just add the image and then you're done.) This is still relevant today.

However.

Having been away from these interactions for awhile, viewing it through a "new" user's (Wim's) eyes was pretty enlightening. There is definitely a cognitive load issue, for new and/or infrequent users. And I remember in UX reviews going back and forth on how to show the previously-selected things, how to do it in a way that was accessible but not drawing too much attention to it, etc. It was definitely not intuitive, and there were several rounds of backflips getting it to the current state of (relative) sense-making.

Versus the new patch is pretty dead-simple. "Save." That's it. And you're always taken back to the same place, which you grow to expect. The main downside is it's a bit annoying that you have to click two times once you know what you're doing.

When we get into these "should we do A which benefits these kinds of users or B which benefits these kinds of users" conundrums, my preferred approach is to look at what other CMSes do, and WordPress is an easy go-to one because it makes up 25%+ of the internet. :P So when we're talking user expectations, that tends to be it.

They also use a "save, then insert" UX pattern. So we are conforming to broader user expectations as well if we go the route of Wim's patch.

---

SO. :P

+1 to Wim's patch in #8. @seanB also suggested making this behaviour configurable since he (and I, and a lot of other "power users") actually use the "Save and insert" button quite a lot. I think that could also be a good idea, but we maybe want to handle that separately, because what we've got here is a straight-forward fix, that bascially only removes code, and that brings our Media Library up to what most users will expect.

Terribly sorry for the 6+ month rigamarole. :P Ugh.

If we want to make it configurable, removing the code and adding it back seems like more complex changes than directly adding a checkbox and changing the buttons conditionally.

Besides that, I have no problem with a change in the default behavior. Not having the extra 'Save and insert' is fine as long as I can easily enable it again if needed.

We might also want to check the a11y implications of removing the selection overview, but I see we already have a tag for that.

I agree with +1 to simplifying it, and +1 to making it configurable.

I just read this research, so important to strive for a simple(r) UI.
The Distribution of Users’ Computer Skills: Worse Than You Think
Across 33 rich countries, only 5% of the population has high computer-related abilities, and only a third of people can complete medium-complexity tasks.

I love the new simpler button. Even I, who have used Media Library for a while, have had to think about which button to press. Now there's only one, so there's zero stopping to consider options. So +1 to dropping the "Select and insert". The dropped button saves time, but it also means you have to read and think about the two buttons nearly every time to make sure you are pressing the right button.

I do think this is complicated by this issue: #3059955: It is possible to overflow the number of items allowed in Media Library

Video:
https://www.drupal.org/files/issues/2019-10-06/surprising-selection-due-...

This actually happened to me when testing. I would expect, if cardinality is one, my latest selection would be the one inserted.

The dropped button saves time, but it also means you have to read and think about the two buttons nearly every time to make sure you are pressing the right button.

Yep, it allows you to save time in principle, but the user having to make a decision every single time probably means that for a large number of users, it actually is not faster. I know it isn't faster for me at least :)

I do think this is complicated by this issue: […]

That's a long pre-existing issue and does not block this change. That problem exists both before and after this patch.

It sounds like we have unanimous agreement:

• @webchick in #11
• Wordpress in #11 😅
• @seanB in #12
• @phenaproxima in #13
• @shaal in #14
• @oknate in #15

The only remaining decision is to A) make this configurable here, B) if so, where to put that configuration, C) how to explain that to end users.

I am personally not convinced this is worth making configurable. I personally think this is better always.

So: what is the rationale to keep this around and make this configurable?

Based on the confirmation of current confusion, bumping to Major.

Personally, however, I think the setting to show the extra button and/or the selection overview should be sitewide config, rather than per-instance. It's one thing to tweak "things you're allowed to see" (i.e, media types etc.), but another to vary the entire workflow of the actual UI per instance. To keep things semi-consistent, IMHO these options should be exposed in config. I think it would also be quite a bit simpler to implement.

Makes sense, +1 for sitewide config.

So: what is the rationale to keep this around and make this configurable?

I don't have any hard statistics on usage of the buttons, but I work with a lot of trained editors that like these kind of shortcuts. I know people are using it. Specially for media rich sites where users have to upload multiple media items on a daily basis. I think that is exactly why webchick asked us to build this in the first place.
The fact that we went through multiple demo's for the UX team confirms that it is not a crazy complex button, even though we might want to hide it by default.

Not really sure about the BC rules for these kinds of things, but we might want to think about that too..

With regards to BC (Backwards Compatibility): this module is experimental. This does not cause data loss. There is therefore zero BC impact.

FWIW (not much), some random thoughts:

1. I'm +0 on if the button should completely go away from core or not. I can see both sides of that. Yes, contrib could re-add it, and that might be better than having to support it in core. Then again, it's sort of a drag to have to install a contrib module for this, especially if we already basically have the code for it.
2. If we keep it, +1 to hiding it by default.
3. If we keep it, we're still going to have to rename things and #include "bikeshed.h". ;)
4. If it's going to be configurable, +1 to being site-wide, not per instance.
5. However, this reminds me of the 'Use advanced search' permission. If a site builder wants to turn it on, they probably do not want to turn it on for all media selectors for all users. They'd probably have something like a 'Media power users' role, and only enable the extra power / fancy / confusion for those power users, not everyone.

Thanks for #21.3 + #21.4 (which implies we need to also bikeshed where to put this setting, how to label it, how to document it, how to explain this super abstract thing at all) + #21.5.

Those are exactly the reasons that cause me to propose to simply remove it. So much complexity. So little gain. For the small number of sites that want this, it's much easier to install a contrib module because that contrib module can more clearly explain it than some obscure setting can.

Ok, so that I was not deciding this in a vacuum, also discussed this on today's UX meeting with @Gábor Hojtsy, @ckrina, @andrewmacpherson, @worldlinemine, @phenaproxima, and @seanb. (I'll link to the video once I have it.)

We demoed the current state of things, as well as the state after the patch, and presented the rationale for simplifying the interface (even brought up the dreaded W word ;)).

Despite this justification, there was still broad consensus among those on the UX team that this "higher cognitive load" workflow is necessary from a "power user" POV. Multiple users over multiple UX calls raised this concern. (Though granted, people attending Drupal UX meetings tend to be a bit more "power" of a user than most. ;)) There was also some back and forth about the "Additional selected media" stuff, but that once again came directly from user feedback.

The consensus among those in attendance was basically the same as the latest comments here: that making it configurable, as a global setting, but with the default configuration being to show the "simple" setting w/ just a single button, is something everyone agreed to. This way, we don't lose a valuable feature / functionality until/unless "someone" creates an improved design that helps address the cognitive load issue.

And, if it's a global setting, then regardless of the setting, it's one consistent experience for all users on the site. (Versus using something like permissions or what have you.)

Additionally, it would be really nice if we actually had an issue to discuss the naming of those labels. ;) I'm not sure if it makes more sense to spin off a separate issue for "Make this configurable to address cognitive load" or repurpose this issue for that reason, and spin off a "Bikeshed the labels some more" issue, but. :)

Assigning to myself to work on test coverage.

Here's test coverage that builds on top of the test improvements in #3055648-47: Frequent random fail in \Drupal\Tests\media_library\FunctionalJavascript\MediaLibraryTest.

I was going back and forth on whether to use that patch, but at least for now, I think this is the right thing to do. I'll see if I can come up with a patch that doesn't use those improvements.

So postponing on that issue, for now. If I can create a patch with similar changes without that patch, I'll unpostpone it. Ideally, that one could go in quickly, and this patch would only need a reroll.

Unassigning

Reroll of #34, sorry, cross posting.

I added a data provider and it runs through the file upload test and the oembed add form test with advanced mode and basic mode.
The one thing it’s missing is assertions that the other buttons don’t appear, but I don’t think it’s really necessary.
If you find the button “Save”, you know “Save and select” isn’t present.

I agree with the test plan in #37 FWIW.

+++ b/core/modules/media_library/src/Form/SettingsForm.php
@@ -0,0 +1,49 @@
+    $this->config('media_library.settings')
+      ->set('advanced_ui', (bool) $form_state->getValue('advanced_ui'));

doesn't this also need a call to ->save()?

and in which case, missing tests?

crediting jibran who I consulted on the update vs post-update for the new config (thanks jibran, you're my update-path spirit animal)

for #43

Speaking of test coverage...I think what might be easiest for us to land this, is to go for the sledgehammer approach. I copied all of MediaLibraryTest's testWidgetUpload() and testWidgetOEmbed() methods into new test methods which use the "advanced" UI. That way we get to keep all existing coverage, but also test that both workflows work correctly.

FWIW I was only talking about tests for the admin form

Here's a fix for the failure in #45. I'll add test coverage for the admin form.

Here's test coverage for the admin settings form that @larowlan asked for in #43.

The interdiff is off. See the next comment for the corrected interdiff.

Corrected interdiff.

Removing the duplicate test coverage for the settings form.

I still need to read the whole test by applying the test, the diff makes it hard to see the result. But here is some first feedback.

1. +++ b/core/modules/media_library/tests/src/Functional/SettingsFormTest.php
   @@ -0,0 +1,43 @@
   + * Tests the Media Library settings form.@after
   

   Does that @after belong here?

2. +++ b/core/modules/media_library/tests/src/Functional/SettingsFormTest.php
   @@ -0,0 +1,43 @@
   +  public function testSettingsForm() {
   

   This test looks a bit funny on its own. Can't we do this as part of the advanced UI test?

3. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/MediaLibraryTest.php
   @@ -1750,6 +2316,33 @@ protected function saveAnd($operation) {
    
   +
   +  /**
   

   It looks like we have a double line break here?

I can live with #55.2 being a part of #3087227: [META] Split up and refactor MediaLibraryTest. The tests slowly got out of hand with every added feature and good thing we are now taking a step back to improve it.

Ad far as the feature itself, it is a nice and relatively simple change, which I personally think will definitely make a difference for power users. So happy we made it configurable, but at the same time happy we simplified for people first installing Drupal. So overall +1 to adding this asap.

Also removing the `needs accessibility review` since andrewmacpherson was there during the last UX demo and didn't see any objections from an a11y standpoint.

We have the setting, the upgrade path, and pretty extensive tests (and a followup to improve the overall tests). It looks ready to me. RTBC. Thanks!

Awesome stuff. I think this is a really great usability enhancement without screwing advanced users.

As intended, here's how the default behaviour now looks:

(Side note: I LOLed because my mouse IMMEDIATELY jumped to where the "Save and insert" button... wasn't. :D Muscle memory is a powerful thing. ;))

The new setting is under the "Media" grouping:

The form looks like this:

(Side-note: that description text is kinda novella-ish, but OTOH there's a lot it needs to cover, so I think that is fine.)

Now, you get the original UI when using the media library:

So all good from a UX-POV!

---

Code review-wise, everything is fine and good and really straight-forward until you get to the tests which... YOWZA! :O

I talked to @phenaproxima and @oknate about that, and they explained that because Media Library is now bifurcated along two flows, they didn't want to inadvertently introduce bugs due to lack of test coverage on the "advanced" path, so opted to WAY OVERKILL the test coverage for now, but to clean it up in #3087227: [META] Split up and refactor MediaLibraryTest, which is not bound by alpha deadline, and is a good idea to do at the end anyway because once all the "feature-y" bits are done, the tests can be refactored with the whole picture in mind. Makes sense.

So. Without further ado...!

...I have a phone call now, and don't have bandwidth to figure out issue credit. :P But once that's done, this sucker is IN! Great work everyone!!

AWESOME! Committed and pushed to 8.8.x (and 8.9.x and 9.0.x :P).

YAYYYY!

Also I think to simplified UI is worth calling out as a highlight, and the fact that this is now triggered by a setting is worth calling out in the release notes. Tagging accordingly.

Updating the issue summary, which only had a before screenshot, to have screenshot of the new default UI.

🥳 I'm now looking forward to using Media on my Drupal site even more!

Hooray! :)

And one more credit adjustment!

Reworking the release note a little for clarity.