Auto-generated URLs by URL filter are repeating the URL in the "title" attribute of the anchor tag.

This simply makes no sense.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, drupal.filter-url-title.0.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
5.51 KB

Glad to see that this is covered by tests :)

hingo’s picture

"Read-only review:"

This was discussed/discovered in #340776: URL filter does not urldecode() the URL for link text. Everyone seems to agree title attribute should be removed.

The change is simple and risk free, it only affects html output.

By reading, the patch looks ok.

Note: I'm running for work and did not have time to actually apply the patch and test it "with my own hands". Leaving status unchanged so someone else can do that. Except for this detail, I support swiftly committing the patch.

Everett Zufelt’s picture

Didn't have time to test the patch. But +1 conceptually on removing the title attribute from an accessibility perspective. It really serves no purpose to repeat the URL in the title attribute.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! So let's go.

hingo’s picture

Status: Reviewed & tested by the community » Needs review

Call me conservative, but shouldn't at least one person (apart from yourself) actually apply the patch first?

I've seen enough of silly reviews where the patch even had PHP syntax errors, I wouldn't want to become one of those reviewers myself. That's why I said, I support this, but didn't really test it for real myself.

mgifford’s picture

Status: Needs review » Reviewed & tested by the community

I refreshed my sandbox & applied this patch here - http://drupal7.dev.openconcept.ca/

It applied with no difficulty. I'd have liked to have had a simple way to test, but in looking over the code but it looks very straight forward.

Putting this back to RTBC.

Dries’s picture

#2: drupal.filter-url-title.2.patch queued for re-testing.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)
Issue tags: -Accessibility

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