Link field labels don't show in entity forms

First a test to demonstrate the label is not output.
Then the fix.
I used a details element here, because container does not allow a title.
But perhaps we should refactor container to allow #title - although details is better for a11y.

This is a UX improvement too.
Before

After

Adding some tags to get some input from UX and a11y experts.

I think this needs extended test to change cardinality

Good call, extra-test-coverage++

Yes, that's enough

I didn't notice this issue before.

Maybe we should use a fieldset instead of a <details> to wrap the widget form fields.

@falcon03 Please take a look at #1168246: Freedom For Fieldsets! Long Live The DETAILS.

The scope of the issue to fix a bug, accessibility issue is #1801268: Change link field 'title' field to 'link text'

@andypost - I think that would be semantically better as a fieldset actually. I was quite involved in @sun's campaign for DETAILS.

The move to details was partly a shift for the visual element of having a collapsible fieldset hack that we used in everything prior to D8. There wasn't usually a semantic relationship between the fields.

However with dates, checkboxes, radios and other compound form elements are a group of input fields which are properly organized with a fieldset & legend in order to pass on their relationship to each other.

Related link #504962: Provide a compound form element with accessible labels

Here it is with fieldset instead

Screenshot

Mike - if this is better for a11y (and I like it not being collapsible) - please put back to RTBC.
My first attempt used a fieldset but I thought they were no-go except for radio groups - thanks for clarifying.

I'd like to see this collapsible by default. Because better to have ability to free the screen area for complex elements once they do not need changes (node edit for example)

I'm pushing it back to RTBC. I'll cycle back about the collapsible piece, but ultimately this is the kind of thing that fieldsets have always been used for.

I don't imagine there's any way to default to having both the URL & Title on on line is there? I'm not sure if that would break some UX pattern. It would of course have to be responsive.

But current implementation does not allow the fieldset to be collapsed

I don't think collapsibility is the goal here, adding a label to the field is.
The collapsibility was a by-product of using the details in the first patch - but only because I thought fieldsets were off-limits.
No other fields are collapsible - people can use a fieldgroup contrib module or form_alter if they want to maximise edit space.
Adding '#collapsible' => TRUE is mis-use of the fieldset element in my opinion.
Thoughts?

The collapsible part should be discussed in a followup I dont think its needed at this time.

Committed c44404a and pushed to 8.x. Thanks!

@alex you had this tab open waay to long :P tnx for commit

The collapsible part should be discussed in a followup...

...#1968050: Allow the Link fieldset to be collapsed.

+++ b/core/modules/link/link.module
@@ -176,6 +176,14 @@ function link_field_widget_form(&$form, &$form_state, $field, $instance, $langco
+  // If cardinality is 1, ensure a label is output for the field by wrapping it
+  // in a details element.
+  if ($field['cardinality'] == 1) {

I'm working on another issue, #2416987: Fix UI regression in the menu link form and I wonder why this is cardinality == 1.

I would have thought it would be > 1
or >= 1

why have it in an if at all?

oh, this is why you dont want a fieldset on each when there is more than one.

they are already grouped (so they can be dragged)

:)