[Meta] Improve UI of Reference field settings form

Thanks for trying! However, that patch is not correct. For one thing, the word "Title" would need to be translated... for another, it seems kind of like a "hack" -- not really addressing why the title field label is incorrect, just sort of putting a band-aid on it after it is first done incorrectly.

Sorry, I didn't understand "Jugada". :) Anyway, we should not have patches like hacks...

Here is what I think the problem is. In DefaultSelection::buildConfigurationForm()
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21...

it does essentially this:

$fields = [];
foreach (array_keys($bundles) as $bundle) {
      $bundle_fields = $this->entityManager->getFieldDefinitions($entity_type_id, $bundle);
      foreach ($bundle_fields as $field_name => $field_definition) {
        $fields[$field_name] = $this->t('@label', ['@label' => $field_definition->getLabel(),]);
      }
}
$form['sort']['field'] = [
      '#type' => 'select',
      '#title' => $this
        ->t('Sort by'),
      '#options' => [
        '_none' => $this
          ->t('- None -'),
      ] + $fields,
      '#ajax' => TRUE,
      '#limit_validation_errors' => [],
      '#default_value' => $configuration['sort']['field'],
    ];

So. If you have two bundles foo and bar for Node, and their title fields are labeled "Foo name" and "Bar name", and you go through this code, your reference field settings form in the "Sort by" setting will either show "Foo name" or "Bar name" as the sort by option. And this will be confusing, hence this Usability bug.

This will probably also be a problem for other fields that may have different labels in different bundles.

The question is what to do about it... Probably what we need to do is show all of the options instead of just the last one we encountered in the loop. Maybe something like this patch?

This patch isn't quite right -- it needs to remove duplicates in the labels before concatenating. How about this?

Updating the issue summary with more straightforward steps to reproduce. I am making screen shots and will update shortly.

Meanwhile here's a new patch with (hopefully) the coding standards problems fixed. Not making interdiffs right now since I think no one is looking at this patch and it's pretty small anyway.

Adding before/after screenshots to issue summary.

Better issue title.

1 more coding standards fix hopefully -- interdiff is only whitespace.

Discussed this on the weekly Usability call today. Points:

a) There's an additional related problem: two fields on two different bundles may have the same label. In this case, the same label would appear in the select list twice, and it would not be obvious which field you want to sort by. You might even choose one that doesn't appear on the bundles you've chosen.

b) With the current patch, if one field had a bunch of different labels across bundles (could often happen with the Title field, for instance), the label shown in the Select list would get really long. That's not good.

c) Additional problem: there's no sorting on the list of options.

d) Additional desirable feature: if you check boxes for which bundles to allow, it should filter the list of sort-by fields down to those that occur on the chosen bundles [this should be a separate issue].

So, we probably need a new patch that would:
1) Display the machine name as well as the label. Probably in format "field_foo_bar (Foo Bar)".
2) Limit the display of labels if there are multiple labels. If we're displaying machine name, probably only 1 option will fit, plus a "..." if there is more than 1. So it might look like: "title (Recipe name|...)"
3) Sort the list of options.

I'll work on it in the next day or few days, unless someone else has the inclination.

You know, what we really need here is the pop-up field picker you get in Views UI when you are adding a field, filter, or sort, rather than trying to stuff all the relevant information into a select list.

Looking at what Views UI does:

The code to build the Add dialogs is in \Drupal\views_ui\Form\Ajax\AddHandler
https://api.drupal.org/api/drupal/core%21modules%21views_ui%21src%21Form...

That method is building all types of "Add xyz" forms for Views UI. What that method does:

a) Gets a list of "options" to display in the form from Views::viewsDataHelper()->fetchFields(), passing in the "xyz" as the "type" of thing being added.

b) Builds them into a tableselect with filtering in that form method.

and (c) that form is popped up using Ajax (or a direct link if there is no JS) when you click on one of the Add links when editing a view.

This would be complex to do. Views UI has all this infrastructure to maintain the state when popping out to the links etc.; Field UI doesn't. So ... probably not going to happen, although I think it would be way better than trying to cram information into a select list here.

Another thing to think about: on the Field UI's "Manage Fields" page, there is a field select list for when you are adding an existing field to a content type. What does that do? Let's see... That is in FieldStorageAddForm::getExistingFieldStorageOptions.
https://api.drupal.org/api/drupal/core%21modules%21field_ui%21src%21Form...

That has a limited number of options -- only includes configurable fields that are used on bundles of this entity type, but not already on this bundle. And the options are formatted like this:

        $options[$field_name] = $this->t('@type: @field', [
          '@type' => $field_types[$field_type]['label'],
          '@field' => $field_name,
        ]);
  

So, that has set a precedent of using "Type: field_machine_name" in a field select list, when you add an existing field to a content type.

However, all of the field machine names here were presumably created in Field UI. It doesn't include built-in fields created by Core or Contrib developers with who knows what kind of coder-intuitive machine names... so not exactly the same situation.

OK, here's a new patch. Uploading so I can try it out on simplytest.me (and so tests will run). Will upload screenshots soon. I really should get a local test site going... :(

Ah, this isn't quite right -- the asort() doesn't work because the array elements are $this->t() objects and not strings that can be sorted.

But anyway, any comments on the output format, which looks like this screenshot:

Besides the sorting problem, the other thing I don't like about that latest patch is that () is used both for column names (for things like Image fields that have the alt text for example), and for machine name. It's confusing.

Not sure what to do. Any ideas?

Here's a new patch. I put the field machine names in [] instead of having two () in each label. And I cast the t() calls to string so the list can be sorted. And a screenshot of what it looks like:

Thanks for the usability review! Adding tag for the next time around.

So, I agree it would be good to mimic other UI patterns.

Here's what we have available.

a) In Field UI, when you go to add an existing field to a content type, what you have in the select list is "Field Type Name: field_machine_name". For example, it says "Entity reference: field_test" for one test field I set up. It does not show the label at all.

EDITED b) In Views, it just seems to pick one label. So for that field, it says "Test Test", although in another content type it is called "Vendor Test". So this would suffer from the same problem. However, the Title field says "Title" -- it hasn't picked up the customized title names. I am not sure why. Also in Views it is a multiselect, multi-column table and it doesn't show the machine name at all. So, that is actually I think not a great pattern.

Edited version: In views you have a multiple-column multiselect table. It is alphabetized by one of the labels. It does show you "Also known as Other Label". But it has a lot of space (multiple lines etc. in that area for descriptions). It doesn't show the machine name.

Here is a screenshot of what Views does. We just cannot do it in a select list.

Note that title is always called "Title" and the alternate forms are not shown. And machine names are not shown at all.

(In this screenshot, I made a field_test with labels "Test test" in the Test content type, and "Vendor test" in the Vendor content type.)

OK. @benjifisher and I were discussing this in Slack.

It looks like we agree on the following:
1. We want the machine name to be displayed.
2. We want the label to be displayed. But there could be a lot of labels. So, we need to convey that somehow.
3. We want the label(s) first, and alphabetical by label.

So, here are some options for formatting. Let's consider each option for the following cases:
C1: Title field, where one content type has it as "Recipe name" and another has it as "Vendor name".
C2: Image field field_image, label "Image", which has subfields of alt, target ID, etc. [see comments #21/22]
C3: Field added in Field UI, with machine name field_foo, with label "Foo" on all content types.

Formatting options

F1: Current patch

- C1:
Recipe name|... [title]
- C2:
Image (alt) [field_image.alt]
Image (target_id) [field_image.target_id]
- C3:
Foo [field_foo]

F2: Use () for the fields
This is not great -- see also comments #21/22.

- C1:
Recipe name|... (title)
- C2:
Image (alt) (field_image.alt)
Image (target_id) (field_image.target_id)
- C3:
Foo (field_foo)

F3: Separate by :

- C1:
Recipe name|...: title
- C2:
Image (alt): field_image.alt
Image (target_id): field_image.target_id
- C3:
Foo: field_foo

Another option would be to change it up and have the machine name first, alphabetical by machine name, so it would look like this.

F4: Machine name first with :

- C1:
title: Recipe name|...
- C2:
field_image.alt: Image (alt)
field_image.target_id: Image (target_id)
- C3:
field:foo: Foo

Given that the UI for adding existing fields to a content type has only the machine name and not the label, and that the machine name is shown on the other Field UI pages, I think I actually would vote for option F4 of the options listed in comments #27-28. Any other thoughts?

Adding the 4 formatting options to the issue summary, as well as information about the existing similar UI for Views and Field UI in Core.

Forgot some information.

Good point... Sounds like maybe the current patch, with formatting option 1, is probably the best we can do, unless someone thinks of a better option?

Regarding the question of "is this the best way"... To resolve this issue, we came up with a lot of possible solutions, looked at existing UI patterns in Core for other field-related things, discussed all the possibilities at several Usability group meetings, went over all the cases (including fields like Image that have columns), and decided this was the clearest presentation of all the solutions we could think of. I think it's been usability-reviewed enough, and the approach was signed off on by the Usability team.

But I'll address the code and ... character issues in a new patch soon.

Also regarding formatPlural, I don't think it would work well here. Some languages have many different "plurals" forms of nouns/verbs; for instance, I think Russian has different forms for @count being 0, 1, (2-4), or 5+, while English just uses singular for 1 and plural for 0 and 2+. So a translator is presented with the two English strings, which are normally things like ["1 thing", "@count things"], and asked to make them into their array of N "plural" forms. It makes pretty good sense if "@count" appears in the string, but there's no @count here at all. We really want to have one result for 1 and a different result for 2+, no matter what language is involved, and we're not talking about inflected forms for nouns/verbs. So to me, it doesn't feel like formatPlural is the right fit, where a Russian translator would see strings that don't involve @count at all and maybe not know how to translate it into a plural set?

Another note on formatPlural() is that there are some languages, like Chinese, that have only 1 plural form. For those languages, formatPlural would definitely fail us here, as they would not have 2 strings to submit. Anyway, here's a patch, and I added a note about not using formatPlural(), since it seems like a good question that could come up again in future code maintenance.

Anyway, here's a new patch that hopefully addresses #36:
- Use the elipses character instead of ...
- Move the calculation of t() arguments outside the if() statement (which I think/hope is what was meant in the third item).
- Add a comment about why we don't want to use formatPlural(), for future maintainers.

Thanks for the review!

I have adopted some of what you suggested. What I didn't adopt:

a) I think we do want two translatable strings for the label: '@label|… [@machine_name]' and '@label [@machine_name]'. I am just not sure that all languages use ... to mean "and then some". We should leave that open to translation.

b) I see what you're saying about the machine name, but I think it's hard to scan things that are sorted by something other than the left-most part of a string. And sometime in one of the comments here or UX meetings, I am pretty sure we were told to emphasize the labels (put them first) and not the machine names. Plus the other UI comparisons (like Views) sort by label. So... I think we're stuck with that?

Anyway, here is a new patch, and interdiff.

OK... that looks messy but I see your point. Adopting your code... I added the function as a static in the class here -- I thought it made it a bit easier to understand the code.

whoops. Should have probably tested it before uploading patch. I don't have a working environment now though (PHP version, ugh)... need to fix that sometime SOON. Meanwhile...

Needs namespace to pass in a callable...

Looking into after_build and process does seem like a better alternative. I was also bothered by how inefficient this seemed to be.

Looking at API documentation for RenderElement, maybe we could also use the "lazy builder" functionality? Also, as a note, post_render and pre_render still exist (they are documented on RenderElement).

Anyway... It looks like maybe a post_render callback that would create the sorted options would be good, but I'm not really sure how that would work.

Hah, actually how about if we added something to the Select form element, like a #sorted property, that would force the options form element to sort its options in its own post_render callback? That seems like an even better solution. Then we could just leave these things as TranslatableMarkup and everyone would have a solution. :)

Would that work?

Discussed the idea in #53 on Slack with alexpott and benjifisher. We all seem to think it's a good idea. benjifisher suggested for some use cases, it might be useful to specify "Sort starting at element N and leave the first N-1 elements unsorted". So, to do that, probably we would want to add 2 properties to the Select form element:
- #sort (boolean, default to FALSE for BC) -- if TRUE, sort the options by their labels after rendering/translating
- #sort_start (int, default to 0) -- if sorting, start the sort at this element, leaving the previous ones as they were.

This seems feasible and better than the last patch...

Hm... Well, I tried this idea out, and I don't think it is working out very well. I tried using the following callbacks on the Select form element class, with the following results:

- #pre_render, #after_build, #process -- the options at this point still have TranslatableMarkup objects in them
- #post_render -- the whole thing at that point is an HTML string

So, I looked into the theming. The theme hook is 'select' (not surprising). There is a preprocess function in includes/form.inc called template_preprocess_select(), which calls form_select_options() to do the work (also in form.inc).

But even here... The machine names for the options are cast to strings in this function (that I didn't modify), even if they are TranslatableMarkup. But the labels are still TranslatableMarkup objects when we get here. So, apparently they aren't actually turned into strings until you get to the Twig template, or rather I guess whatever gets passed into the Twig template is magically cast to a string somewhere else.

So... I am not sure what to do. I made this patch, which works (it does sort the options, and documents these new properties on the Select), but since it is still effectively casting the TranslatableMarkup to string too early (I guess?), maybe it is still wrong. I guess I don't really understand when it is and when it isn't OK to cast TranslatableMarkup to string. Maybe @alexpott can answer that question? If we can't cast to string in this theme preprocess function, then I guess we have to stick with cloning the objects still?

Meanwhile, here's a patch and an interdiff to the patch in #44... It does work, at least!

As a note, there doesn't seem to be any testing for the Select element to verify how it renders...at least, if there is, I couldn't find it. Do we need to write one? If so, is that out of scope for this issue? It seems like it would be good to unit test the sorting behavior (with translation), but given that there isn't even, as far as I can tell, any test for Select (or for many other elements)... I guess implicitly most Functional tests will be testing that select elements are working... but still!

Um.... I think #sort_start is an int in the patch? But I should have set it to 1 to skip the -none- option in the Reference field sort select in this patch. The default on the Select element itself is zero.

Anyway, the patch failed tests with a bunch of undefined index #sort_options errors, I think due to the recursion in form_select_options() that happens if there are option groups (which is why there were 82 fails and not thousands). This should fix it.

Chatted with @alexpott about this in Slack:

a) What we need to do is split off the modifications to Select into a separate, blocking child issue. So, doing that.
#3065903: Add label sort ability to Select element

b) In addition, there is one modification needed to the patch: when sorting the labels, we are implicitly casting the TranslatableMarkup objects to string. What we want to do is leave the 'label' array element as TranslatableMarkup, while sorting by another variable that is that cast to string. Subtle.

c) Also there is a test for the Select element in \Drupal\Tests\system\Functional\Form\FormTest::testSelect() that can be added to in this child issue.

So, setting this issue to Postponed until we do the child issue, in which case this patch will be smaller.

Here is the most recent patch, without the bits that are now on #3065903: Add label sort ability to Select element.

I agree that this patch should be using the empty options. I will just point out that this particular select wasn't using them before (no idea why not, but we should be).

Oh yeah, definitely! I was not aware of that function. Would totally be better to use it. It even counts to find the most commonly used label, which is better than what the code in this patch is doing.

Would we put it onto a class somewhere, or should it just be put into some other include file?

I guess we should spin that off into a separate issue:
#3069442: Deprecate views_entity_field_label() in favor of EntityFieldManager->getFieldLabels

Also updating the issue summary.

I explored the views_entity_field_label() function on that other issue, and concluded that we can't really call it here (whether or not it is inside Views or on the EntityFieldManager class... Reproducing what I wrote on #3069442-4: Deprecate views_entity_field_label() in favor of EntityFieldManager->getFieldLabels here (slightly edited to make sense here):

The problem is that for the reference field sort UI, we need to go through the field definitions in more detail, because we need to know:
- the most commonly-used label (the views function does that)
- whether there was more than one label or not (the views function does that)
- whether the field is considered to be Computed (the views function does not do that)
- what the columns are in the storage definition (the views function does not do that)

So we could call the new method to get the label, but since we already need to go through the field information in loops to gather the other information, I don't think it makes a lot of sense to do that.

So, this issue I think only needs to be postponed on #3065903: Add label sort ability to Select element. Thoughts?

Here's a new patch, which again requires #3065903: Add label sort ability to Select element.

I did three things here:
a. Used the #empty_value option in the select to add the "None" entry, instead of doing it manually like it was before.
b. As in views_entity_field_label(), use the most popular label as the one users will see if there are multiple choices.
c. Moved the calculation of the options list to a separate method, because it was getting complicated and broke the flow of the form builder method.

I think that the main patch will be actually quite a bit easier to read than the interdiff, but here are both. Leaving this as Postponed as this depends on that other issue's patch. Anyway, it works in my local test site...

Whoops. I don't need sort_start. Same patch with this one line taken out. Still works, because the sort_start default is now sensible on the other issue. :)

+        '#sort_start' => 1,

Apparently the patch in #70 fails tests, so setting to Needs Work rather than Active.

I like the new patch! Definitely cleaner and easier to understand than my last one.

However, there is a problem: It looks like the logic of whether to use "|..." or not is reversed. Screenshot:

As you can see, the Title field has no ... for "Vendor name", and the generic fields like ID and Comments all have the ... even though they have the same label across content types. Oops. Seems to be the correct logic for fields with columns, such as Image (alt).

Also as a note: when I tested it on simplytest.me with 8.8.x and the patch applied, the options were not sorted... Maybe their 8.8.x did not have the latest patch applied from the other issue? I guess I'd need to test it on my own machine to see. I may do that later.

I think this also needs a test.

Not quite! As noted in #77, the with-columns case was correct before. Now it is reversed!

We definitely need a test!

Still rushing. :) This is what is in the latest patch, for the column case:

+        foreach ($info['columns'] as $column_name) {
+          $arguments['@column'] = $column_name;
+          $options["$field_name.$column_name"] = $single_label
+            ? $this->t('@label (@column) [@field.@column]', $arguments)
+            : $this->t('@label (@column)|… [@field]', $arguments);

That last line should have .@column in it, I think? The only difference should be whether the |... is there or not.

Regarding the format of the labels, I don't feel strongly about it, but I think it's good as it is.

The new patch looks good.

It seems to me that defining node types should be done along with defining the fields (both in setUp() perhaps?)? But I think it is OK as it is. The test seems fairly thorough...

Maybe we could add a bit to it though, to verify it works with built-in fields? For instance, change the label of the Title field on one of the bundles to "Alternate title", and verify that the title entry works (has Title... in it), and also verify the entry for something like the node ID built-in field.

And also should we verify that the sort order is correct in the test? Because one of my earlier screenshots (#77) didn't have the sort order right (although the patch was fine, that was just a simplytest.me glitch). But maybe we don't need that test, I don't know.

Hm, let's see. In the UI, you go to Edit on the content type, and update a field in the form. Let's see what happens when you save the form... Some snippets from NodeTypeForm:

$type = $this->entity; // so $type is the NodeType config entity for the bundle being edited
...
$fields = $this->entityFieldManager->getFieldDefinitions('node', $type->id());
...
// Then in the submit() method, this is how it is saved:
$title_field = $fields['title'];
$title_label = 'something'; // set this to the string you want as the title label.
$title_field->getConfig($type->id())->setLabel($title_label)->save();

That should help?

EDIT: I guess we don't need $type though, because we know what the bundle IDs are ($type->id() here, but known strings in the test).

OK... Would have been nice to have that suggestion back in February (comment #16) when we first brought this to a Usability meeting. But that's OK.

So... if we're bringing back to the table how to format this...

Personally, since this form is only for site builders, when I'm building a site I would rather just see the machine names -- and yes, I do realize that I am not the average site builder, although I spent a number of years employed doing mostly that. The advantage of the machine names is that they are unambiguous -- which is why behind the scenes, that is what the values are for this dialog. And as another note, in the dialog from Field UI for adding an existing field to a new bundle, all you see is the bundle label and field machine name (NOT the field label). The issue summary has a list of other UIs where you are choosing fields... some use only the labels, and some use only the machine names, and I agree, there aren't any others that use both.

Another note: using "Bundle: Label" or something like that -- will not work unless we change what the values are for the dialog -- we can only have 1 entry for a given field machine name. So we cannot have both "Vendor: Vendor name" [one alias for the title field] and "Recipe: Recipe name" [another alias for the title field] in the select list, because both of them have the same field machine name [title]. I mean, we *can*, but we would have to change more than just the labels for this select list.

So... Not sure what to do.

If you have the machine name "title" instead of any of the labels, you will avoid all problems with ambiguity, and then you don't need permutations per bundle because there is only one machine name for each field.

Anyway... What you are asking for (permutations) will not work unless we do a lot more changes.

Right now, all we are storing in the field settings is the machine name of the field. You're asking for permutations, so they would select something like "Vendor: Vendor name" for the title field, and also have the choice of "Recipe: Recipe name" and "Page: Title", all for the title field.

So even if we have some kind of widget that allows us to have multiple labels for the same machine name (which this simple select would *not* allow, since they array key is the same, namely "title", for all 3 of these permutations), the next time someone came into this dialog to change the sort, we would have no way of remembering which permutation they chose.

Unless you're advocating saving the bundle they chose along with the field settings? Which seems silly, because it's only relevant to this particular, horrible, UI and not to the field setting at all.

So... I think we have a few options:
- Go with something similar to this patch, but fix it somehow so that it's clearer.
- Only show the machine name
- Keep the UI as it is in Core now
- Use something other than a select, like a tableselect, which has more room for columns and there we can list a column for the machine name, and a column that shows all the labels in use for that field and which bundles it's in and whatever else we want

Thoughts?

An optgroup for the multiple-label fields sounds like a reasonable idea, to group them together and highlight that they're different. I think it should just be called "Fields with multiple labels".

I'd be fine with saying "Various labels" instead of "One Example|..." too. That is an excellent idea [I like putting "labels" on there as it makes it clearer what "various" means -- we're trying to go for clarity!). That would also group them together (all the "Various" would alphabetize the same). Does an EM tag work inside of select lists?

So... Let's try out a few of these and make some screenshots (easier to critique and compare than hypotheticals, IMO). I will probably have some time tomorrow or Friday to do that. Not today.

OK, how about if we do both? We can put in the opt group called "Fields with multiple labels", which will indent the "various" ones, and also use the label "Various labels [machine_name]" within that opt group, so that the format matches the other items in the list.

I personally think that having both the label and the machine name is helpful, at least for some site builders. You see both on the Manage Fields page... If it is in [] it at least indicates to people that it is additional information, rather than the primary thing they should look at. Also, then all the fields shown have the same format. Anyway... I can make a few patches trying these options out, and we can see what we think.

OK, I've made a new patch.

With the option group, there was a choice:
a) I could let the opt group be alphabetized along with the fields, so it appeared in the middle of the list (under F for "Fields with multiple labels"). I didn't like that.
b) I could put it at the top. I opted for that.
c) We don't have the option of putting it at the end yet -- see #3069844: Provide additional options for sorting the options in Select form elements... but could postpone (again!) on that.

Anyway, here is the patch, interdiff, and a screenshot. Test in the patch passes locally...

I like that idea! I wonder how many characters we should allow?

Actually, on thinking about #104 more, I realized it wouldn't work, because of translations (the field labels can be translated).

What we could do though is make it look like:
Fields with multiple labels:
- Most common label, ... [title]

This avoids the confusing "|..." replacing it with ",...", and then because it's underneath the "Fields with multiple labels" header, I think it is more understandable what is going on?

That's kind of a merge of parts of the patch in #87 and parts of the latest patch... From the patch in #87, we would add in the option group stuff (with sort start of 2), and change the | to a comma (the latest patch gets rid of the counting, so it's less of a change to start from #87 I think).

Also, we need to make sure that the items in the option group are sorted. I am not sure if sorting is inherited from the outer option list or not.

I'll work on a patch later today.

OK, here's another patch. Interdiff is from patch in #87.

This patch:
- Groups fields with multiple labels together in an optgroup at the top
- For those, uses "Most common label, ... [machine_name]" format

See what you think... here's a screenshot:

We are trying to avoid translating too early... see several comments early in this issue about that. That is why we did #3065903: Add label sort ability to Select element so the option labels could be sorted as late as possible in the process, and then came back here to use its result.

I don't think it's too complicated to limit the options to the selected bundles -- just requires a little Ajax, so that when those checkboxes are checked or unchecked, we recalculate the options?

Oh thanks! My original "patch with the optgroup" #101 did remove it when it was empty, but then that was lost when I did #107. Glad you restored that functionality!

Anyway, the interdiff looks good. I did have one question: If someone checks no content types (bundles) on the form, does that mean nothing can be referenced, or does that mean everything can be referenced? Because we definitely have some UIs where checking nothing means everything.

I'll give this a manual test sometime soon and make screenshots.

The patch above doesn't apply... Something wrong there. It seems to be more of an interdiff? Way smaller than previous patches.

I guess I'll wait to review/test until the automated tests pass.

Looks like the tests are passing now! Also no coding standards violations. So that's good.

I've reviewed the patch itself (not interdiffs) in detail. It's looking very good; haven't done a manual test this time. I found a few things to address (some are definitely nitpicks):

a) core/lib/Drupal/Core/Entity/Plugin/EntityReferenceSelection/DefaultSelection.php

+  protected function calculateSortOptions($entity_type, array $bundles) {
+    // A single field may have different labels for different bundles.  Count

In this comment line, there should only be 1 space after the .

b) Up a bit from there in the main form method, I think we could use a comment to explain why #sort_start is 2 (which is to make the optgroup with the fields with multiple labels stay at the top):

+        '#options' => $this->calculateSortOptions($entity_type_id, array_keys($options)),
+        '#empty_value' => '_none',
+        '#sort_options' => TRUE,
+        '#sort_start' => 2,

Hm.... Actually, that is not always a good idea, because there might not be an optgroup at the top. So, I think we need to make #sort_start 1 if the optgroup is not there, and #sort_start 2 if the optgroup is there. I'm not sure how to detect it, but maybe the return value of calculateSortOptions needs to be ['#options' => (the options), '#sort_options'] => (1 or 2)], and the return value would be added using += to $form['sort']['field'] instead of being put directly into its #options element? Hope that makes sense...

c) in calculateSortOptions:

+    $options[$group_key] = ['#sort_options' => TRUE];
...
+    // $options[$group_key] has at least the key '#sort_options'.
+    if (count($options[$group_key]) == 1) {
+      unset($options[$group_key]);
+    }

Would this be clearer as:

$options[$group_key] = [];
...
if (count($options[$group_key]) {
  $options[$group_key]['#sort_options'] = TRUE;
}
else {
   unset($options[$group_key]);
}

Not sure which is better?

d) I think there maybe should be a space between the comma and the ... in the labels: "Most Common Label, ... [machine_name]" ?

e) The changes to FunctionalJavascript/EntityReference/EntityReferenceAdminTest.php make sense to me (and the test passes, so good!).

f) The new test core/tests/Drupal/KernelTests/Core/Entity/EntityReferenceSelection/EntityReferenceSelectionLabelTest.php also looks good. But I think we need to also test the case of selecting a subset of the bundles, and verifying that the options are correct there too?

The bot likes this latest patch, and there are no coding standards violations.

I've reviewed the interdiffs since my last thorough review (#125). It looks great!

Nitpick:

 +   *   Otherwise, '#sort_start' will be set to 1,
    */
   protected function calculateSortOptions($entity_type, array $bundles) {

Comment should end in . not ,

I would have just fixed that, but there is more to do:

#125 (f) The new test core/tests/Drupal/KernelTests/Core/Entity/EntityReferenceSelection/EntityReferenceSelectionLabelTest.php also looks good. But I think we need to also test the case of selecting a subset of the bundles, and verifying that the options are correct there too?

#126: (f) Still to-do, along with other updates to the tests. The attached patch only changes the form.

I'll give it a manual test now and make a screenshot.

Manual testing results: Looks good to me!
- When I uncheck all bundles, the Sort options select list goes away.
- When I check just one bundle, the title label from that bundle is shown for [title], and the list is correctly alphabetical.
- If I uncheck that one and check the other, the label from that bundle is shown for [title].
- When I check two bundles with different title labels, the "Fields with multiple labels" optgroup appears at the top. Here's a screenshot of that:

I'll add that to the issue summary and clean it up a bit. Seems like the only To Dos are in #129.... but I think the Usability group will review this issue later today at the weekly UX meeting so we'll see what they say.

OK, we can do that. I will take a stab at separating it out tomorrow if no one beats me to it.

I have separated this into several child issues:

• #3089521: Sort options for entity reference field are unsorted
• #3089523: Use progressive disclosure to hide Sort options on entity reference fields until bundles are selected
• #3089525: Sort options should correspond to bundles selected for entity reference field settings
• #3089528: Sort options for entity reference fields can be ambiguous if a field has different labels across bundles
• #3089529: Sort options for entity reference fields may be confusing if several fields share labels

I had a brief discussion with @benjifisher in Slack today. As a result, we closed 2 of the child issues as Won't Fix. The other 3 have already been Fixed. So... this Meta issue is done, as best we can, and we have a much more usable Reference Field UI for the most common use cases.