Issue Summary

Need issue summary, related issues (there are a few here and there now).

Original Issue

I added a link/URL field between title and body and I did a double-take when creating a node. I don't think I would ever personally be confused at this, but it may very well be confusing for someone trying to figure things out or posting content to a site someone else configured (or as part of a distribution).

I could not make Voice Over to say the form labels so I wasn't able to figure out what this sounded like.

Should the label of the URL title element be changed to "URL Title" or "Link Title"?

Side note: I also tried adding help text to the widget to see what that did, but that uncovered a bug where it is not displayed.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

URL Description may be a better term than Title.

larowlan’s picture

Issue tags: +Novice, +Accessibility, +#pnx-sprint

Tagging

Risse’s picture

Status: Active » Needs review
FileSize
26.52 KB

Correct me if I'm wrong, but isn't the text also called an Anchor text?

On that note, here is a patch, renaming it to "Anchor text".

Status: Needs review » Needs work

The last submitted patch, link-1801268-3.patch, failed testing.

Anonymous’s picture

No, it is actually the title attribute.

Risse’s picture

Hmm, I checked the link-module, and I think the title-attribute is never really used, and that field is, in fact, used as an anchor text.

Risse’s picture

Status: Needs work » Needs review
Issue tags: -Novice, -Accessibility, -#d8ux, -#pnx-sprint

#3: link-1801268-3.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Novice, +Accessibility, +#d8ux, +#pnx-sprint

The last submitted patch, link-1801268-3.patch, failed testing.

falcon03’s picture

Title: Link/URL field widget might be confusing » accessibility Link/URL widget needs to be improved
Category: feature » task
Priority: Minor » Major

Hi all,

as a blind user, I think that this issue could be abstracted a lot; in fact:

  • As you said, the double "title" label is confusing;
  • Moving link to core, we lost the support for the "title" link attribute (that is different from the ancor text);

So I think that we should have the support for the link title attribute (which form label could be something similar to "link title", maybe with a description to clarify its purpose) and that we should change the "title" form label to something like "link label" or "anchor text" (I would prefer the first one).

Also, we have another issue: when the actual "title" form field (to clarify: the anchor text) is required, a screen reader user cannot know this anyway.

falcon03’s picture

Title: accessibility Link/URL widget needs to be improved » Link field accessibility needs to be improved

changing title

mradcliffe’s picture

Thank you for the feedback in #9.

falcon03’s picture

You're welcome...

After a more detailed test, though, I have to say that we have another issue: when a user must enter the "url", the "link label" and (when it will be implemented) the "link title", there should be semantics to let a blind user know that these information are related.
As you can suppose: yes, I am talking about grouping these form items together in a fieldset that should have the field label as its legend.

mgifford’s picture

Issue tags: +a11ySprint

Hopefully we can draft up a new patch to include some of @falcon03's direction.

mgifford’s picture

Status: Needs work » Needs review
Issue tags: -Novice, -Accessibility, -#d8ux, -a11ySprint, -#pnx-sprint

#3: link-1801268-3.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Novice, +Accessibility, +#d8ux, +a11ySprint, +#pnx-sprint

The last submitted patch, link-1801268-3.patch, failed testing.

mgifford’s picture

FileSize
26.66 KB

Re-roll.

mgifford’s picture

Status: Needs work » Needs review

go bot.

Status: Needs review » Needs work

The last submitted patch, link-1801268-16.patch, failed testing.

falcon03’s picture

Assigned: Unassigned » falcon03

ok, maybe I am having a big risk, but I am assigning this issue to myself! I'll try to write a patch after Christmas! :D

Eventually, it will be my first patch! :-)

BTW, merry Christmas everyone!

falcon03’s picture

Status: Needs work » Needs review

#3: link-1801268-3.patch queued for re-testing.

falcon03’s picture

Component: field system » link.module
Status: Needs review » Postponed

Since we are talking about a lot of changes, I decided to introduce link title support into another issue:
http://drupal.org/node/1874354
so that patches will be easier to review.

Once we will get that patch in, I'll re-open this issue to address the remaining problems.

This is my first time working on drupal issues myself, so correct me if I am doing any mistake! :D

sun’s picture

Cross-posting from #1874354-50: Rename current "link title" and add support for the true link title attribute:

"Anchor text" is very techy. The average Joe does not know what an "anchor" is. Please rename to "Link text".

Along the same lines, "Link title" is confusingly ambiguous. I'd suggest to use a wording like "Tooltip" or "Summary".

Ultimately though, I'm not sure whether an input for the 'title' HTML attribute should be supported by core.

The only precedent we have is Image module's Image field widget, which optionally supports 'alt' and 'title' field widget attributes.

klonos’s picture

I also think that "anchor" is too techy for the average less-computer-literate user. I support using something along the lines of Link text, Link path/URL/location and Link tooltip (with tooltip being optional). They make much more sense and are really close to the purpose of their respective component.

mgifford’s picture

Some ideas of alternatives in #1874354-51: Rename current "link title" and add support for the true link title attribute.

Ultimately though I don't think any of us are stuck on the text being "Anchor" and it seems like we can focus on the tooltip concept instead.

sun’s picture

Status: Postponed » Postponed (maintainer needs more info)

I don't really understand what remains here...? The title attribute support was won't fixed.

So all that would remain here would be to potentially rename the label from "Link title" to "Link text"?

We should adjust the issue title + issue summary accordingly.

falcon03’s picture

Status: Postponed (maintainer needs more info) » Needs work

@sun, we still have some issues:

  1. The link title should be renamed to "link text" (it should be also renamed in the theme layer and elsewhere in core);
  2. When the user must enter the URL and the link text (depending on the field instance settings), the widget form must be wrapped in a fieldset;
  3. The logic to require the link text if the URL field has been filled in and the link text is required should be reversed so that it works in the opposite way (the link text is required, but it becomes not required during the form validation if the url field is empty);

However, I am not sure if we can deal with all these problems in this issue or if we should use this as a meta-issue and open a follow-up for each problem.

andypost’s picture

Let's fix the widget for cardinality 1 in #1957670-3: Link field labels don't show in entity forms and close this one as duplicate

heddn’s picture

Issue tags: -Novice

Let's not close this duplicate. A lot of the work from this issue went in under #1957670: Link field labels don't show in entity forms but we should talk about collapsibility, more appropriate labeling of the fields and validation(#26) here.

heddn’s picture

Status: Needs work » Needs review
FileSize
18.31 KB

Let's see if it will pass tests before we start changing things. I had to move things around since the widgets and field formatters were moved into plugins since this was tested last time.

mgifford’s picture

Status: Needs review » Needs work
Issue tags: +Novice

Now that #1957670 is in core there's still more to do.

1) Title is still confusing as there is the title attribute. It makes more sense as "Link text"

2) There is now a fieldset when adding a single entity, but not if there are multiple entities. When multiple links are allowed they are all within a table so that they can be re-ordered. That's probably sufficient for organizing within a logical context.

3) I think the logic for required elements is working properly now.. Least in my testing of it.

@andypost, any concerns with changing the name from Title to "Link title"? Should that be addressed here or in the other post?

mgifford’s picture

Issue tags: -Novice
FileSize
197.19 KB

I didn't mean to add back in the "Novice" tag.

@heddn thanks for re-rolling a patch for this. It's useful to see an interdiff of the changes, particularly when you've made a bunch of changes to the last patch. I find using the linux interdiff tool to be the easiest way to generate them.

Unfortunately it didn't pass the bot & also seemed to possibly add some notice errors and drop the title in my testing:

Link Core vs patch

You up for rolling another one? Any concerns about the switch to Link text - Link titles often refer to the title attribute which isn't what we're getting at.

falcon03’s picture

According to various comments, the label should be "link text", not "anchor text".

Another +1 for renaming "link title" to "link text" is that in l() the variable containing the link text is already defined as $text. I can write the needed patch for renaming link title to link text anywhere the renaming is needed. I can also write the patch to fix the validation logic issue I mentioned in #26.3. I just need directions on how to deal with these issues: should we handle them all in this issue? Or should we open an issue for renaming title to text and another one for fixing the validation logic?

mgifford’s picture

FileSize
41.92 KB

I quickly created an interdiff to include it. A lot of it is changing

I agree that it's more common to to use "link text".

There are a lot of changes in the patch that I don't know are needed. (EDIT: A lot has changed with this code so maybe it is required.)

Looking forward to seeing the patch fixing the validation logic.

I think we can probably deal with #26.3 here.

It's always a judgement call about what we can get in in this stage with the time we have left in the D8 cycle.

falcon03’s picture

Status: Needs work » Needs review

Ok, working on this.

Attached is a first (completely untested) patch to handle #26.1. I'm posting it here to see how many things it breaks.

Disclaimer: Since the patch alters the link field schema, I think that it can be tested only on a fresh install...

falcon03’s picture

mmm... D.o. removed the attached patch from #34; here it is!

falcon03’s picture

Status: Needs review » Needs work

The last submitted patch, linkfield-a11y_improvements-34.patch, failed testing.

Bojhan’s picture

Lets make sure we don't add visisble border here.

falcon03’s picture

Status: Needs work » Needs review
FileSize
31.8 KB

New patch fixing tests, I hope.

At this point the issue summary needs some love, I think.

This patch fixes #26.1; if someone started having a look at the code it would be great! ;)

Status: Needs review » Needs work

The last submitted patch, link-a11y_improvements-39.patch, failed testing.

mgifford’s picture

For Bojhan's point, I think we should be relying on code from #1932074: Tags Includes Label which isn't Associated with an Input Form

+/* Fieldset variant */
+fieldset.fieldset-no-border {
+  border: none;
+  padding: 0;
+}
+fieldset.fieldset-no-border legend {
+  text-transform: none;
+}

However, that patch isn't in core yet and it may be that it needs to be moved up so that it is more generalized.

falcon03’s picture

I worked on this issue during the code sprints. Not sure if I am able to roll a patch (time goes really fast!), but I think that we'd better deal with the renaming from "link title" to "link text" just for the link field in this issue and then open another issue for renaming the "#title" to "#text" in renderable link arrays.

This means writing the whole patch again! :D

falcon03’s picture

Issue tags: +Needs issue summary update, +adopting D7 usability to D6

So, here's what we should accomplish in this issue:

  1. The "link title" should be renamed to "link title" in any place related to the link module;
  2. When the user must enter the URL and the link text (depending on the field instance settings), the widget form must be wrapped in a fieldset;
  3. The logic to require the link text if the URL field has been filled in and the link text is required should be reversed so that it works in the opposite way (the link text is required, but it becomes not required during the form validation if the url field is empty).

And You guess.. Now that we have a roadmap, this issue needs a summary!

falcon03’s picture

Issue tags: -adopting D7 usability to D6 +Usability

fixing tag.

falcon03’s picture

Assigned: falcon03 » Unassigned
Status: Needs work » Needs review
Issue tags: +Novice

Here's the patch that I started working on at drupalcon Portland. Let's see what tests are broken.

This patch still addresses only #43.1. I'm unassigning this issue from myself and adding the "novice" tag so that someone can work on the issue summary. I can keep working on this issue until next week; after that, I'll start studying for my high school final exams and I don't know how much time I could be able to spend on d.o.: not too much I guess, unfortunately...

falcon03’s picture

falcon03’s picture

I ate d.o when it alters tags without any reason. Hope that this will be fixed by the d.o upgrade to D7! :-)

Status: Needs review » Needs work

The last submitted patch, linkfield_a11yimprovements-45.patch.patch, failed testing.

falcon03’s picture

Issue tags: +Needs manual testing

Tests are failing, so we need manual testing to know whether we need to fix the patch or the tests.

Also, the "novice" tag is for the manual testing and the issue summary update.

Steps for manual testing:

  1. if the link module is already enabled before applying this patch, make sure to disable and uninstall it.
  2. Apply the patch and re-enable the link module.
  3. Test anything related to the link field: adding new fields and instances with different settings, sharing an existing link field, entering values into link fields, using different formatters with different settings, etc.
  4. Report any PHP warning/notice/fatal error reported by Drupal on this issue, describing the scenario and the operation you were trying to perform when it was shown. If you don't see any error or weird behavior please let us know
mgifford’s picture

I got this error 3X on an instance with the patch & without the patch, so that's not it.

Type	menu
Date	Sunday, June 2, 2013 - 21:59
User	Anonymous (not verified)
Location	http://s4f7e2b9a0cd57fd.s3.simplytest.me/core/install.php?langcode=en&profile=standard&id=1&op=do_nojs&op=do
Referrer	http://s4f7e2b9a0cd57fd.s3.simplytest.me/core/install.php?langcode=en&profile=standard&op=start&id=1
Message	Symfony\Component\Routing\Exception\RouteNotFoundException: Route "field_ui.display_overview.node.search_index" does not exist. in Drupal\Core\Routing\RouteProvider->getRouteByName() (line 146 of /home/s4f7e2b9a0cd57fd/www/core/lib/Drupal/Core/Routing/RouteProvider.php).

Otherwise didn't run into any unseen errors.

Edited content types with both single & unlimited links. Created content, edited it. Modified the content types....

All behaved as expected with the patch in #46.

falcon03’s picture

Issue tags: -Needs manual testing
falcon03’s picture

@mgifford: thanks. The error you got was not introduced in this patch. It's related to the routing system, and there is already an issue to fix it.

So basically we need to fix the tests. Any help is more than welcome! ;)

Do we need to get this patch in before code freeze?

larowlan’s picture

Title: Link field accessibility needs to be improved » Change link field 'title' field to 'link text'
Status: Needs work » Needs review
FileSize
13.38 KB

New title
Reverted much of the changes from the last patch, we're only interested in changing the UI here, ie the user facing elements. I've updated some of the comment text as well so it is clear what the code is doing. But thinks like $link_title can stay just that. The key difference here is that we don't change the field schema columns, which keeps the upgrade path from D7 intact.
So all in all, the patch is smaller but achieves the same user-facing change and doesn't change any of the existing API's/behaviour.

larowlan’s picture

pruning more tags

falcon03’s picture

@larowlan: thanks for working on this issue. I tried using this approach as well, but I thought that this scenario could be confusing for other developers. So I decided to rename the variables as well.

Also, we could want to rename "#title" to "#text" for renderable link arrays. Maybe not in D8 at this point, but this is something to think about during d9 development for sure...

In any case, I think that we should rename the link field variables (e.g. $title to $link_text) as well... Do you think that this will make fixing this issue harder?

larowlan’s picture

I think those changes will complicate/delay the issue getting in.
We are really only interested in the UI here.
I don't think we need to be changing theme functions etc and the field schema change is a no-go in my opinion, just for upgrade path reasons.

larowlan’s picture

Also smaller patch = less likely to need re-rolling and/or break other issues.

falcon03’s picture

@larowlan: for the upgrade path: the link field has been introduced in D8 and we're still in the development phase, so we don't have to worry about it. From what I know Drupal 8 should provide upgrade path between stable versions and, if possible, between candidate releases.

I still think that having "link text" assigned to a $title or $link_title variable is really confusing from a novice developer point of view.

Also, changing theme functions is not in the scope of this issue. It was originally, but I decided to deal with it in a (eventual) follow up.

klonos’s picture

Let's get this in and file a separate follow-up issue for the rest of the changes (rename variables, fix renderable link arrays, upgrade path etc). If that one doesn't make it for D8, we'll push it to D9.

larowlan’s picture

Link is a contrib module in D7, imo we need to respect the field schema columns to play nice with it.

mgifford’s picture

Status: Needs review » Reviewed & tested by the community

I'm marking this RTBC. I tested @larowlan's patch in a bilingual environment and it works as expected.

Follow-up issue here #2013296: Clean up variable names in Link field (followup)

falcon03’s picture

Ok, if this is the way to go, then let's get this in as soon as possible so that we can start working on the other issue.

I'm still convinced that the variables related to the link module should have been renamed in this patch and that we should have taken care of the theme system, renderable arrays & Co in a followup. In any case, what is important IMO is the final result: we must make the link module consistent before the d8 release, while the other stuff can be pushed to d9.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed e26a156 and pushed to 8.x. Thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Added a section for someone to fill in a summary and moved original issue body with note about related issue.