Fix UI regression in the menu link form

#2407913: Discuss/define the minimal UX for adding menu links to entities got resolved.

I'm taking this on now.

Isn't this blocked on #1959806: Provide a generic 'entity_autocomplete' Form API element?

#4: it isn't, because the initial version of this will not have the autocomplete yet. Or, at least, the initial patch won't use it, it will only implement the desired validation logic and will require the user-path: and entity: schemes to be used. In a follow-up issue, we can then just add the autocomplete support on top of that. This allows us to keep moving forward :)

Oh, nice plan :) Only now I saw the "basic UI" part in the issue title...

#2416955: Convert MenuLinkContent to use a link widget landed, should help with this.

@Wim Leers switched to another issue, I'm going to take a stab at this.

we can hack at making a new widget, but using it is blocked on #2417793: Allow entity: URIs to be entered in link fields (via conversation with @Wim Leers)

copied LinkWidget,
made a MenuLinkWidget (can change name later) (can move the location of the class later to whereever later, menu link content?)
Changed the name of the path input field to Link
And removed the fieldset.

MenuLinkContent
BaseFieldDefinition for link
sets title to disabled,

        'title' => DRUPAL_DISABLED,

but we will never want to use the link title since we have a Menu link title.

made a MenuLinkWidget (can change name later) (can move the location of the class later to whereever later, menu link content?)

I thought the whole point of writing a generic link widget was to use it everywhere (link field, shortcuts, menu links)?

Yup, a bit sad to duplicate the widget just to get rid of the fieldset.

Couldn't the widget determine that if there's only a 'link' input to display, the fieldset is not needed ?

+1 to #14.

yep. it is weird. but I'm just playing for now.

also, here is the diff between the two widgets, more useful than the first patch which is a copy and changes.

Removing the fieldset removes the
The location this menu link points to.
help text.

Also, I know why the description is at the end, because it is the description for the whole link thing
not the description for the uri form element (input field), that is: This can be an internal Drupal path such as node/add or an external URL such as http://drupal.org. Enter to link to the front page..

Related #2396145: Option #description_display for form element fieldset is not changing anything

--

I'll try next changing the LinkWidget.

this is a hack. just taking notes.

If we get rid of the fieldset description (and use a better label)
then we dont need to worry about how to have the fieldset description helptext but no fieldset decoration.

no interdiff cause this is a different approach.

some code from tim plunkett. They had the idea of making it more general than trying to see if was a menu link and ... but I have a concern.

will explain in next. comment.

1. +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
   @@ -121,9 +122,15 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   -    // If cardinality is 1, ensure a label is output for the field by wrapping it
   -    // in a details element.
   -    if ($this->fieldDefinition->getFieldStorageDefinition()->getCardinality() == 1) {
   

   this comment came from issue #1957670: Link field labels don't show in entity forms but it was not clear why it was only when cardinality == 1. I made the comment more accurate when I changed it anyway to explain the new additional condition on adding the fieldset.

2. +++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
   @@ -282,8 +282,7 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   -      ->setLabel(t('Link'))
   -      ->setDescription(t('The location this menu link points to.'))
   +      ->setLabel(t('Menu link location'))
   

   (I know, ironic given the issue we just did before which took it out and then put it back in)

   we can get rid of the description, if we make the label better.

This allows use to do a trick with not having to show the description even when we do not want the fieldset. (the description goes away when we take the fieldset off).

BUT the problem is, when we take the fieldset off, we get the label from the uri form element (not the link field).....
and I made a link field on page content type, diasabled the title ... and I lost my field label and the description I carefully crafted...

so this has to be more specific to menu... or not from a field UI something. :(
and if we change the uri field element label... it changes that everwhere, not just for menu links.

[edit: shortcut uses the same, so needs to work for that also. note that on the shortcut form, ( admin/config/user-interface/shortcut/manage/default/add-link ) Note shortcut doesn't have the uri form element help text because it is 'link_type' => LinkItemInterface::LINK_INTERNAL, ]

moving to needs review to let the tests run on the patches.

Also... this is ready for someone to look at. I'm done just "playing", and think this is worth considering.

If testing manually, keep in mind you will want to try all the things that use link, since this changes link widget (not just menu links).

Screenshots coming.

I am following this, but why are we reviewing a UI where the main interaction is still missing? I was assuming this was a in between step?

Menu link

admin/structure/menu/manage/main/add

Shortcut

admin/config/user-interface/shortcut/manage/default/add-link

link in content type with cardinality > 1

title optional

title disabled

cardinality > 1 title disabled

:(
field help text, is both on the whole filed and on each uri form element

I'm going to dinner.

just a reroll. automatic 3 way merge

this needs a review.

we can do the autocomplete in #2418017: Implement autocomplete UI for the link widget

fix for #25

oops. this is the patch

I agree.

+++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
@@ -121,9 +144,15 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
+    if ($this->fieldDefinition->getFieldStorageDefinition()->getCardinality() == 1 && $title_is_enabled) {

We should file a bug for this issue. When a widget has more then one field we have to make a special rule for cardinality 1. See #2331331: Widget for cardinality 1 is missing the field label

I'm also not totally sure what to do here.

Note, that for menu links, and shortcuts, the title is NOT part of the link field. (but we can make the UI without worrying about that implementation detail)

in #36
for the generic link widget,
it switches the order of the uri and the link title, so that it looks like the order for menulinks and shortcuts. (could be a good idea, but different from d7 (and d8 so far) for anywhere the link field is used. can we go back and see if the order was chosen for a reason originally?)

There is help text (or there could be) for the field and also for each form element... and that is not in the sketches in #36
And #36 is missing the case where the title form element is disabled. ... maybe we want to not design for that?

In the sketch in #36, for single value link, looks like there is some styling to group the link title and link... so that would be fieldset with a box.

oh, and I just noticed in #25 that the field label was used for each link element label ... need to try patch 29 and look closer to see what that looked like for that case.

hmm. (also, hm... tests for the UI maybe need to be added for whatever we decide it should be like, sounds like a lot of simple test and xpath)

I'll try some of the suggestions in #36. (feedback welcome still on variations, so leaving at needs review)

... I do agree for menu links and shortcuts title first totally makes sense, its required ...
for a normal link field I disagree ... why should a optional title comes first? Semantically a link is primarily a URI and then also has a title.

Would it perhaps make sense to create a "URL-only link widget", for use on entities where a link field is used as a base field, but only to store URIs?

That'd allow us to easily optimize for that use case, of which we now have 2 instances (Shortcut and MenuLinkContent), but still reuse most logic.

@Wim Leers Yes it would be good to see what that solution looks like.

For now, just fixing something in the previous approach. (title when cardinality != 1 was replacing the url form element label with the field label, so it was repeated a bunch)
did a $cardinality_is_one since call to getFieldStorageDefinition() was needed in a bunch of places

now:

something a little strange,
if I fill out something in the form element for the link text, but do not fill out anything for the url form element, then the whole form saves ok without an error or warning... and does not save the words I put in the link text form element. (url is not conditionally required when the link text is filled out, but the whole field is optional).

this interdiff does 2 things,
adds keeps URL when sucking in the parent field label as the label for the uri form element. It does this by using (URL) at the end of the label.

the other thing it does is make the title/name and page title more consistent between menu link and shortcut

noticed something wrong, no help text when either internal only or external only. :( I'll add that in.
(if we go this direction, the issue title probably becomes: make UI consistant (which fixes regression for menu links) )

I will try a widget that doesn't mess with the generic link widget though... in a bit.

ah, when internal only, the field is prepended with the url of the site. so no helptext is needed.
wait, you can use ... maybe we should say that. no. people wont want to use link to link to the front page. nvm.

this adds the help text, rearranges a little code to flow better.

interdiff.43a.txt is not in the patch, (but would apply on top of patch 43). it makes less code, but I wonder if it is less clear, or more prone to break later.

something not really related, but I noticed, is that we should probably use http://example.com as the example of an external url, not drupal.org (or make it configurable). <- separate issue to search for user facing strings and replace them. (workaround is form alter)
-------
screenshots as of 43.

-----
I'm going to take a break from this.

Me, or someone next should try, alternative approach, see #39.

The problems outlined in the issue summary seem to be quite easily solvable.

The change is basically this: when the field cardinality is 1 and the link text is disabled, don't display any fieldset and use the field label as a title for the 'uri' field property.

Here's how it looks like after I changed the label of the 'link' field on MenuLinkContet to link path:

1. +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
   @@ -169,12 +169,19 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +      // Otherwise wrap everything in a details element.
   +      else {
   +        $element += array(
   +          '#type' => 'fieldset',
   +        );
   +      }
   

   Oh, right, this doesn't set #tree => TRUE, hence this doesn't break any of the form handling. Awesome! :)

2. +++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
   @@ -280,7 +280,7 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   -      ->setLabel(t('Link'))
   +      ->setLabel(t('Link path'))
   

   IMO "link path" is wrong; it isn't always a path, especially not when it contains a entity: URI (or an external URL). Technically, it should be "URI", but that's not very user-friendly, so probably keeping "Link" is best. IMHO.

I noticed that in shortcut and menu the link text is always above the url box, but for Link fields it is below. Is that an inconsistency that needs to be addressed?

IMO "link path" is wrong; it isn't always a path, especially not when it contains a entity: URI (or an external URL). Technically, it should be "URI", but that's not very user-friendly, so probably keeping "Link" is best. IMHO.

This will get even more complicated in #2418017: Implement autocomplete UI for the link widget, so I just used "Link path" for now to have 1:1 parity with the old UI. But I'm not opposed to changing it if others feel the same as you.

I'm confused by this issue.

From the title it looks like it's discussing the form_alter on nodes that allows you to create a menu link from there.

But looks like it's actually about the link field widget on entity forms?

#51 is right, I was also confused by the title after reading the issue summary.

Sloppy me.

If it's fine functionally, latest patch seems right to me.

Nitpicks:

1. +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
   @@ -169,12 +169,19 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +    // If cardinality is 1, ensure a proper label is output for the field
   

   Missing trailing point

2. +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
   @@ -169,12 +169,19 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +      // If the link title is disabled, use the field definition label as the
   +      // title of the 'uri' element.
   +      if ($this->getFieldSetting('title') == DRUPAL_DISABLED) {
   +        $element['uri']['#title'] = $this->fieldDefinition->getLabel();
   

   $element['#title'] comes prepopulated with the correct title (field label), we could also re-use it from there. No biggie.

$element['#title'] comes prepopulated with the correct title (field label), we could also re-use it from there. No biggie.

I need to learn more of this Field API thingie, it's kinda awesome.

Edit: And the trailing dot was fixed in the previous patch :)

Works perfectly.

(First file is before, second is after.)

I dont think this solution was tried with all the permutations and combinations.

For a Link field on a content type, with cardinality 1, and the link text disabled, the helptext for the field is gone.

So now this is a regression in the link field UI.

@YesCT, that's correct, I only tested the link field on the menu link form and one added to nodes through field UI, neither of which had any description :/

We have two options here, either add it to $element['uri'] as a #suffix or append it directly to #description. Here's a patch for the first option which should be more clear.

As per #60

We also need test for For a Link field on a content type, with cardinality 1, and the link text disabled, the helptext for the field is gone..

Please open a new issue for the patch from #61 and the test coverage. Multiple patch issues get confusing in git history.

re #61 : I might be wrong, but aren't there some tricky subtleties about auto-escape with special cases on #description ? Not sure if putting the description text in #suffix would trigger the expected escaping behavior

I change my mind.

Let's revert this.

Then, add test coverage and fix it without adding a worse regression.

This regression fixed style on the menu link form. but caused missing help text on other forms. That seems worse.

patch is to see if it still passes with the revert.
I did
git revert bef6c3beee9080585f75ed1ea89f6650f4494a51
on a new branch and then diffed against 8.0.x to get the patch

then we can put it back to normal.

Let's do what @catch suggested in #61.

this was a normal when it was committed.

#2421001: Fix regression in the link widget where help text does not show

#2421011: Make shortcut add and edit form titles better
is for the shortcut consistancy changes.

#2421021: Missing help text for external url only for link widget
is for another bit that was in #43

This was also a task.