Problem/Motivation

#2416987: Fix UI regression in the menu link form, in an effort to remove a fieldset style for menu links add/edit form (which uses the link widget always with link text (disabled),
broke using the link widget elsewhere.

Steps to reproduce

  1. Install Drupal
  2. Add a Link field - with an "Help text" and "Allow link text" set to "Disabled" - to any content type
  3. Create a node of this content type

Expected result: the "Help text" is shown under the link field
Current result: the "Help text" is not shown

Cases

  • menu link
  • shortcut
  • link widget used in content types (or other things that have fields):
    • cardinality 1, link text disabled, internal url only
    • cardinality 1, link text disabled, external url only
    • cardinality 1, link text disabled, internal and external url
    • cardinality 1, link text enabled, internal url only
    • cardinality 1, link text enabled, external url only
    • cardinality >1, link text enabled, internal and external url
    • cardinality >1, link text disabled, internal url only
    • cardinality >1, link text disabled, external url only
    • cardinality >1, link text disabled, internal and external url
    • cardinality >1, link text enabled, internal url only
    • cardinality >1, link text enabled, external url only
    • cardinality >1, link text enabled, internal and external url

Proposed resolution

Remaining tasks

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Create a patch Instructions Done
Reroll the patch if it no longer applies. Instructions Done
Update the issue summary Instructions Done
Add automated tests Instructions Done
Update the patch to incorporate feedback from reviews (include an interdiff) Instructions To Do. See:
Manually test the patch Novice Instructions Done
Add steps to reproduce the issue Novice Instructions Done
Embed before and after screenshots in the issue summary Novice Instructions Done
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards Instructions

User interface changes

Before

After

API changes

Only local images are allowed.

Files: 
CommentFileSizeAuthor
#87 2421001-87.patch11.89 KBgeertvd
#85 2421001-85.patch11.86 KBpfrenssen
#76 2421001-76.patch9.71 KBpfrenssen
#68 interdiff.2421001.65.67.txt960 bytesDuaelFr
#1 2416987.43.patch8.26 KBYesCT
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2416987.43.patch. Unable to apply patch. See the log in the details link for more information. View
#6 regression-link-widget-2416987.6.patch8.44 KBmgifford
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,011 pass(es). View
#12 2416987.12.patch10.37 KBYesCT
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,412 pass(es), 1 fail(s), and 0 exception(s). View
#14 2421001.14.patch20.87 KBYesCT
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,794 pass(es). View
#14 interdiff.2421001.12.14.txt14.49 KBYesCT
#14 2421001.14.tests-only.patch14.49 KBYesCT
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,791 pass(es), 5 fail(s), and 0 exception(s). View
#18 2421001-18.patch14.04 KBeiriksm
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,417 pass(es). View
#18 2421001-18-test-only.patch7.66 KBeiriksm
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,418 pass(es), 5 fail(s), and 0 exception(s). View
#18 interdiff.txt15.7 KBeiriksm
#22 link_widget_description-2421001-22.patch841 bytesDuaelFr
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,198 pass(es), 0 fail(s), and 115 exception(s). View
#24 link_widget_description-2421001-24.patch967 bytesDuaelFr
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,282 pass(es). View
#24 interdiff.2421001.22.24.txt1.12 KBDuaelFr
#27 test_only-fix_regression_in_the-2421001-27.patch7.86 KBStryKaizer
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 108,629 pass(es), 5 fail(s), and 0 exception(s). View
#27 fix_regression_in_the-2421001-27.patch8.99 KBStryKaizer
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,934 pass(es). View
#29 2421001-before.png77.44 KBDuaelFr
#29 2421001-after.png67.71 KBDuaelFr
#31 interdiff-2421001-27-31.txt6.84 KBeiriksm
#31 fix_regression_in_the-2421001-31.patch9.37 KBeiriksm
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,060 pass(es). View
#32 External-link-help-text.jpg14.89 KBthorandre
#37 fix_regression_in_the-2421001-37.patch9.51 KBeiriksm
#37 interdiff31-37.txt3.44 KBeiriksm
#39 link description text.png46.47 KByoroy
#39 link description shortcut.png34.15 KByoroy
#46 interdiff.txt5.49 KBeiriksm
#46 fix_regression_in_the-2421001-46.patch9.89 KBeiriksm
#48 fix_regression_in_the-2421001-48.patch9.89 KBeiriksm
#49 2421001-48-after.png64.17 KBmohit_aghera
#49 2421001-48-1-after.png42.9 KBmohit_aghera
#51 fix_regression_in_the-2421001-51.patch10.18 KBeiriksm
#51 interdiff2421001.txt1.12 KBeiriksm
#52 2421001-51.png56.36 KBmohit_aghera
#52 2421001-51-1.png42.5 KBmohit_aghera
#65 interdiff.2421001.51.65.txt1.4 KBDuaelFr
#65 fix_regression_in_the-2421001-65.patch10.11 KBDuaelFr

Comments

YesCT’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
8.26 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2416987.43.patch. Unable to apply patch. See the log in the details link for more information. View

this will not apply, but is the patch from #43 from the other issue.
it did the approach "use logic based on cardinality and link text being disabled or enabled" (the bit about fixing the short cut page title will have a separate issue).

Status: Needs review » Needs work

The last submitted patch, 1: 2416987.43.patch, failed testing.

YesCT’s picture

next step is to strip out all the unrelated stuff from that patch.

amateescu’s picture

What was wrong with the patch from #2416987-61: Fix UI regression in the menu link form?

YesCT’s picture

taking the fieldset off made the help text that used to show, not show.
(we traded a task that removed a boarder for a bug that made helptext not show)

mgifford’s picture

Status: Needs work » Needs review
FileSize
8.44 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 88,011 pass(es). View

Mostly testing out the credit link on a patch I've already contributed.

amateescu’s picture

@YesCT, in #4 I linked to a specific comment from [#9580553], which is comment #61 from that issue :)

YesCT’s picture

#2421021: Missing help text for external url only for link widget went in, now we can expand those tests for all the cases here.

kerby70’s picture

Issue tags: -Need tests +Needs tests

Correcting non standard tag 'Need tests' to 'Needs tests'

idebr’s picture

Status: Needs review » Needs work
  1. The patch from #6 includes changes that are out of scope for this issue. Per YesCT in #2:

    next step is to strip out all the unrelated stuff from that patch

  2. +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
    @@ -184,16 +192,38 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
    +      if ($this->supportsExternalLinks() && $this->supportsInternalLinks()) {
    +        $element['uri']['#description'] .= $this->t('This can be an internal Drupal path such as %add-node or an external URL such as %drupal. Enter %front to link to the front page.', array('%front' => '<front>', '%add-node' => 'node/add', '%drupal' => 'http://drupal.org'));
    +      }
    +      elseif ($this->supportsExternalLinks() && !$this->supportsInternalLinks()) {
    +        $element['uri']['#description'] .= $this->t('This must be an external URL such as %drupal.', array('%drupal' => 'http://drupal.org'));
    +      }
    

    User facing strings of an example url should no longer use drupal.org, see #2418209: Replace user facing strings that use drupal.org as example of an external url.

YesCT’s picture

I'm going to start on this.

YesCT’s picture

Status: Needs work » Needs review
FileSize
10.37 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,412 pass(es), 1 fail(s), and 0 exception(s). View

For menu link,
it used to be labeled "Link path"
#2416987: Fix UI regression in the menu link form changed the label to just be "Link".

So far, this changes it to be "Link (URL)"
since the premise is to use the label from the field, and also use the label from the widget.

--

I did not use a const for the optional/required/disabled link text. could not find a const on the interface.

--

used random machine name for the help text. there might be a better way.

--

I'm having trouble setting the description and the link text setting in the field form....
uploading this for now. If anyone has ideas, let me know. I'll continue to work on this some more now though also.

Status: Needs review » Needs work

The last submitted patch, 12: 2416987.12.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review
FileSize
20.87 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,794 pass(es). View
14.49 KB
14.49 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,791 pass(es), 5 fail(s), and 0 exception(s). View

there are many permutations...

might be nice to add in tests for the field label, and the url label. but I'm done (for today).

[edit: hm. made interdiff wrong as it is exactly the same size as patch)

Status: Needs review » Needs work

The last submitted patch, 14: 2421001.14.tests-only.patch, failed testing.

YesCT’s picture

The fails in #14
Are:

1.
Raw "q1aT6RQm" found Other LinkFieldUITest.php 275 Drupal\link\Tests\LinkFieldUITest->testFieldUI()
Raw "hw9dscwI" found Other LinkFieldUITest.php 306 Drupal\link\Tests\LinkFieldUITest->testFieldUI()
Raw "kKFeJtau" found Other LinkFieldUITest.php 338 Drupal\link\Tests\LinkFieldUITest->testFieldUI()

All three when the link text is disabled, the field help text (description) is missing.

2.
MenuLinkContentFormTest.php 46
ShortcutLinksTest.php 61

The field help text (description) from the base field definitions:

core/modules/menu_link_content/src/Entity/MenuLinkContent.php
298: ->setDescription(t('The location this menu link points to.'))

core/modules/shortcut/src/Entity/Shortcut.php
151: ->setDescription(t('The location this shortcut points to.'))

We should either show them (which this patch does). Or take them out (... throwing an error if they are try and set).

YesCT’s picture

1. I'm not sure if we want to test *where* the description is (with xpath selectors).

2. I'm also not sure how to organize the tests.

3. The combination of link text optional is not tested with cardinality greater than 1.

4. Some of the comments are cut and paste and need to be corrected.

5. In this issue, I think we might want to NOT append the (URL) to the label for the field.

6. In fact, I think we might want to change the label there to Link location and not URL.
Since the thing that is typed there is not always (not often) a url... for example... people might type part of a title of a node, or an internal path.
Changing the label on the default link widget would be a separate issue.
#2461361: Change link widget URL field label to Link location, since what people put in the field does not have to be (usually will not be) a URL

eiriksm’s picture

FileSize
14.04 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,417 pass(es). View
7.66 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,418 pass(es), 5 fail(s), and 0 exception(s). View
15.7 KB

I ran into this issue while building a Drupal 8 site. Had me wondering for a while :)

Tested the patch and it does not apply anymore. So here is a reroll, which also include these changes:

- Refactor all cases of testing for help texts into a loop (maybe address your concern #2). This also addresses concern #3 and #4.

Now, the code changes in this patch addresses the mentioned regression, but I was wondering about another thing (potentially another issue i guess):
- Why do we hide the description for internal links (and instead rely on the "helpful" prefix)? The help text that in the case of external/internal states that "Start typing the title of a piece of content" and this also applies when the setting is set to "only internal", only no-one gets this helpful hint. In fact, adding the prefix kind of makes you think that the input is required to be a internal URL, when it can in fact be the title of a node.

Thoughts?

eiriksm’s picture

Status: Needs work » Needs review

Setting to needs review.

Status: Needs review » Needs work

The last submitted patch, 18: 2421001-18-test-only.patch, failed testing.

eiriksm’s picture

Status: Needs work » Needs review

Yes, that was expected :)

DuaelFr’s picture

FileSize
841 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,198 pass(es), 0 fail(s), and 115 exception(s). View

I don't understand why this patch is so complicated.

The following cases seems to already be handled by default :
- cardinality > 1, all cases
- cardinality = 1, title enabled, all cases

We just need to handle
- cardinality = 1, title disabled, all cases

Please find attached a simple solution that fixes the issue (without tests, sorry).

The side effect of this fix is that, in the Menu UI, the MenuLinkContent 'link' base field description ('The location this menu link points to.') is appended to the field description. This leads to that help text displayed on the field : 'Start typing the title of a piece of content to select it. You can also enter an internal path such as /node/add or an external URL such as http://example.com. Enter to link to the front page.
The location this menu link points to.'

I think that the appended information improves the usability of the field so it might not be considered as a problem.

Let's see what the testbot have to say about that patch an discuss my tiny solution.

Status: Needs review » Needs work

The last submitted patch, 22: link_widget_description-2421001-22.patch, failed testing.

DuaelFr’s picture

Status: Needs work » Needs review
FileSize
967 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,282 pass(es). View
1.12 KB

Better handling of empty descriptions (fixes tests)

eiriksm’s picture

Status: Needs review » Needs work

Looks fine to me. Fixes the bug.

Can we add some quick tests, though? You can still base it on the "test-only" patch in #18 I guess.

dawehner’s picture

+++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
@@ -244,6 +244,14 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
+        if (!empty($element['#description'])) {
+          if (empty($element['uri']['#description'])) {
+            $element['uri']['#description'] = $element['#description'];
+          }
+          else {
+            $element['uri']['#description'] .= '<br />' . $element['#description'];

It would be great to have some quick comment why we have this logic here.

StryKaizer’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
7.86 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 108,629 pass(es), 5 fail(s), and 0 exception(s). View
8.99 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,934 pass(es). View

Attached you can find a test only, which should fail, and a test + patch.

Test is the code from #18, added 1 verbose command to see which variant is being tested.

Patch is taken from #24, added comment as asked in #26

The last submitted patch, 27: test_only-fix_regression_in_the-2421001-27.patch, failed testing.

DuaelFr’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
77.44 KB
67.71 KB

Thanks @StryKaizer for your work!

Tests pass.
Coding standards are OK.
And finally, labels are where they're supposed to be \o/

Before

After

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
@@ -244,6 +244,17 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
+          else {
+            $element['uri']['#description'] .= '<br />' . $element['#description'];
+          }

There does not seem to be any test for this. Since adding html together is one of losing safeness with need to confirm that we're not introducing an escaping bug here.

eiriksm’s picture

Status: Needs work » Needs review
FileSize
6.84 KB
9.37 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,060 pass(es). View

There does not seem to be any test for this. Since adding html together is one of losing safeness with need to confirm that we're not introducing an escaping bug here.

+++ b/core/modules/link/src/Tests/LinkFieldUITest.php
@@ -67,31 +71,84 @@ function testFieldUI() {
+          // Also assert that the description we made is here, no matter what
+          // the cardinality or link setting.
+          $this->assertRaw($description);
+        }

I might misunderstand your comment, but this should cover the case you are asking for, no?

Granted, we do not test _not_ adding a custom description, so here is a quick patch adding that as well. Since you mention " Since adding html together is one of losing safeness with need to confirm that we're not introducing an escaping bug here." do you want to add an assertion that we are not seeing the
tag as text also? (for the record, it does not introduce an escaping bug, i just tested manually)

thorandre’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
14.89 KB

Manually tested. Looking good.

thorandre’s picture

Issue tags: +dcoslo15
thorandre’s picture

Issue summary: View changes
thorandre’s picture

Issue summary: View changes
eiriksm’s picture

Assigned: Unassigned » eiriksm
Status: Reviewed & tested by the community » Needs work

Re-read the comment in #30 and I think I might have misunderstood the concern, so I'll just add a quick test for that. Also, want to replace the random data in there (see #2571183: Deprecate random() usage in tests to avoid random test failures for more info)

eiriksm’s picture

Status: Needs work » Needs review
FileSize
9.51 KB
3.44 KB

- Replaced some random data.
- Fixed some lines that were 81 characters :p
- Added html in the descriptions, to test this does not get escaped (hope that was what you were after, @alexpott?)

DuaelFr’s picture

I made a new round of manual testing and that still looks good!
Thank you @eiriksm.

yoroy’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
46.47 KB
34.15 KB

Tested on simplytest with a link field on the article content type and editing a shortcut link.

Link field:

Shortcut:

Whenn I compare that shortcut screen with a Drupal 8 install without the patch the description "The location this shortcut points to." is not shown in the latter.

I *think* this is rtbc.

Committers want to look at #30, #31 and #37

catch’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
@@ -243,6 +243,17 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
+            $element['uri']['#description'] .= '<br />' . $element['#description'];

Why the <br /> tag?

Also it's not clear to me whether the test covers the case where the uri description and field description are concatenated.

mgifford’s picture

Seems to have been added in #31.

eiriksm’s picture

To first comment on the <br>, it was first introduced in #22. I guess the reason was not explicitly defined, but I assume the main point is that we want to differentiate between "core help text" and "user entered help text". As an example, look at this image:

example

If the text "Here is my link help text" would be just after the "core help text", that would appear strange. In my opinion, anyway.

Would it be preferable if we added these in different render arrays, making them paragraphs instead of concatenating them with a <br>? Or was your point that you would prefer it to not be there, and instead be concatenated with no markup between?

Now, regarding test coverage for the above mentioned. It is not really tested that the concatenated text is on the page. It is, however, tested that both the user entered and the "core help text" is on the page. Although I guess an extra assertion would not hurt. :)

Let's just first decide how to proceed with the "<br> problem". Any thoughts from anyone there? Keep it? Remove it? Change it?

DuaelFr’s picture

@eiriksm You're totally right. I introduced that <br> because I thought it was less confusing than without.
I don't know at all if it's better than paragraphs but I'm afraid that paragraphs default style would make it bad looking.

mgifford’s picture

So do we style the <p>, add documentation to the <br> or just set it back to RTBC?

I'm for adding a comment in the PHP to explain why this was added.

DuaelFr’s picture

I'm not de the right guy to write comments.
I can put it in the code and make the patch file but I'm not confident enough in english to write it :)

eiriksm’s picture

OK, here is an updated patch.

- Added a comment in both the test and the widget why we add the BR tag.
- Fixed a typo.
- Simplified the test code a little.
- Added assertions for when the BR tag should be added.

swentel’s picture

+++ b/core/modules/link/src/Tests/LinkFieldUITest.php
@@ -126,32 +131,24 @@ function testFieldUI() {
+              // If the link type was not internal, and the cardinalyty was 1,

typo: cardinality

Looks good to me other than that.

eiriksm’s picture

oops. Oh well, typo fixed.

mohit_aghera’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
64.17 KB
42.9 KB

I tested #48 on simplytest.
Things are appearing fine. Screenshots are attached.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
@@ -243,6 +243,21 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
+            // If we have the description of the type of field together with
+            // the user provided description, we want to make a distinction
+            // between "core help text" and "user entered help text". To make
+            // this distinction more clear, we separate them with a line break.
+            $element['uri']['#description'] .= '<br />' . $element['#description'];

This is concatenating markup which will lose safe-ness. This works because #description is XSS admin filtered. Not sure what is the right thing to do here. Setting back to needs review to get more people than just me thinking about this.

eiriksm’s picture

How about this? An inline template for the description?

mohit_aghera’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
56.36 KB
42.5 KB

Looks good by testing on simplytest.me
Moving it to RTBC

DuaelFr’s picture

+1 RTBC for the online template, good idea @eiriksm

catch’s picture

Status: Reviewed & tested by the community » Needs review

Is the br tag necessary? Why not just a space?

mohit_aghera’s picture

Issue summary: View changes

@catch
As per me it will look good and add some more user friendliness.
For ex: See 2421001-51-1.png in #52

In Link 3 field, if we don't separate link description in new line it will be hard to identify link description and link help text. Same holds true for this snapshot as well.

catch’s picture

@mohit that's what I'm saying though. If the description and help text are actually semantically different, then they should not be within the same HTML tag. The <br > tag is not visible to screen readers for example.

mohit_aghera’s picture

@catch
How about moving both to separate div like

$element['uri']['#description'] = [
              '#type' => 'inline_template',
              '#template' => '<div class="uri-core-description">{{ core_description }}</div><div class="uri-user-description">{{ user_description }}</div>',
              '#context' => [
                'core_description' => $element['uri']['#description'],
                'user_description' => $element['#description'],
              ],
            ];

https://api.drupal.org/api/drupal/core%21modules%21block%21src%21Control...

DuaelFr’s picture

Assigned: eiriksm » Unassigned
Issue tags: +accessibility

<div> are not read by screen readers. Most of the time, they don't vocalize <p> either (and they would need an extra layer of theming to avoid too big margins at this place).
The best option would be to add visually hidden labels to these elements but I'm not sure we can find something really relevant.

I also think we should put the user defined description first because screen readers users often skip content they already know. So, if that starts with the exact same words as everywhere else on the website, they'll tend to skip it entirely and miss the user entered part.

joelpittet’s picture

Component: menu system » link.module
Issue tags: +Twig

Nice work on getting tests going for this and resolving the regression.

Toying with link.module component vs maybe form api as this doesn't really touch the menu system. Please correct this if it's wrong.

I'm reviewing this to see if I can't get this back to RTBC.

I'd prefer to move this markup to the template(as always).

  1. +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
    @@ -243,6 +243,28 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
    +        // By default the field description is added to the title field. Since
    +        // the title field is disabled, we add the description, if given, to the
    +        // uri element instead.
    

    Why do we add the description to the uri? Was it the only thing to add it to? Maybe there is a more appropriate place for this?

  2. +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
    @@ -243,6 +243,28 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
    +            // between "core help text" and "user entered help text". To make
    +            // this distinction more clear, we separate them with a line break.
    

    This may be harder but it would be nice to get this variable into the template to make it easier to theme. Even if a break tag is the solution, it will make it much easier to override.(I hope)

Ping me on irc #drupal-contribute if you have any questions.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

xjm’s picture

Version: 8.1.x-dev » 8.2.x-dev
Priority: Major » Normal

The core committers (@xjm, @alexpott, @catch, @effulgentsia, @Cottser) and theme maintainers (@lauriii, @joelpittet) discussed this issue today and agreed that this issue does not need to be major priority. However, we did accidentally remove the ability to add this text to these fields, and that can be considered a bug.

The screenshots in the summary explain the problem, but this issue would be easier to understand with specific steps to reproduce in the summary. (I see the issue has test coverage already in the patch, although I did not review the changes thoroughly.)

@joelpittet also noted that using an inline template and joining two strings with a <br /> tag are non-ideal (though the inline template is at least not introducing any escaping bugs, so it is an improvement over earlier patches for that reason.) If possible, we should use actual templates for markup, and/or more semantic markup. (For example, maybe it should be an unordered list?) It's worth considering whether this is the best way to ensure both description texts are available.

We should consider whether the issue might be better targeted for 8.2.x, because it is making a user-facing change and adding new UI text. (Some module could already be altering to add text back in addition, and this patch could result in that text being repeated twice, for example.) I've moved it to 8.2.x for now.

Finally, it looks like the feedback from @DuaelFr, @catch, and @joelpittet all still needs to be addressed as well.

Thanks everyone for your work so far on this bug!

xjm’s picture

Per #61, let's add the specific steps to reproduce to the summary.

DuaelFr’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: -Needs steps to reproduce +DevDaysMilan

Added steps to reproduce and improved remaining tasks.

DuaelFr’s picture

Issue summary: View changes
DuaelFr’s picture

Here is a new patch where I choosed to use an item_list to show both descriptions.

To anwser @joelpittet in #59.1: no, we sadly don't have any better option. In the case we are talking about, the only element we have is that 'uri' element. That's only when we get both uri and text that we have a surrounding container.

The last submitted patch, 65: fix_regression_in_the-2421001-65.patch, failed testing.

YesCT’s picture

Status: Needs review » Needs work

#65 uses a render array.
a template would be better.
but since this is in a field widget, which is already using a render array, the change to template would be out of scope and a separate issue.

I discussed test changed with @DuaelFr (at dev days)
and I think that asserting the strings are there is sufficient,
and we should NOT test if the html is br or ul, that would be a test of the list generator, or an accessibility testing layer, or in a theme test.

Since the order of the strings was agreed to be important though,
if there is a somewhat easy way of testing the order (maybe an existing assert method) it could be worth doing in this issue,
if it is difficult and complex xpath things are needed, maybe we can go on without a test of the order.

DuaelFr’s picture

Status: Needs work » Needs review
FileSize
960 bytes
9.64 KB

@YesCT thanks for the review :)
I wasn't able to find any usefull existing assertion to test strings order.
This new patch removes the part where we try to test the html structure of that description.

YesCT’s picture

uploading a testsonly patch so we can check the tests fail appropriately
(and the same whole patch, so that retests will also retest the whole patch and not just the testonly one, which will fail and change the status to needs work confusingly)

The last submitted patch, 69: fix_regression_in_the-2421001-67-testsonly.patch, failed testing.

DuaelFr’s picture

Green! \o/

YesCT’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
    @@ -238,6 +238,28 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
    +                // User description first to avoid screen reader users to skip.
    

    can improve the grammar here a bit. (and explain why it might be skipped...? or we could assume people would git blame and come here and read the comments on the issue. ;)

    If the usual system message is first, screen reader users might skip it and not notice the more likely custom help text. Put the user description first to avoid screen reader users skipping it.

    Or just

    Put the user description first to avoid screen reader users skipping it.

  2. +++ b/core/modules/link/src/Tests/LinkFieldUITest.php
    @@ -62,31 +66,82 @@ function testFieldUI() {
    +    foreach ($cardinalities as $cardinality) {
    +      foreach ($link_types as $link_type) {
    +        // Now, test this with both the title enabled and disabled.
    +        foreach ($title_settings as $title_setting) {
    +          // Both test empty descriptions and not empty descriptions.
    +          foreach ([TRUE, FALSE] as $description_enabled) {
    

    oh! I love these loops and this test set up!

In general this looks great!

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

pfrenssen’s picture

Assigned: Unassigned » pfrenssen
Issue tags: +Needs reroll

Patch doesn't apply any more.

Assigning to me. I'll first make a patch with a straight reroll to latest 8.4.x, and then a second one to address the remarks from #72.

pfrenssen’s picture

Status: Needs work » Needs review

Setting to needs review for the bot. The tests have been ported to BrowserTestBase in the meantime so there might be subtle changes causing failures now.

Leaving assigned to me to review + address remarks from #65.

Status: Needs review » Needs work

The last submitted patch, 76: 2421001-76-test_only.patch, failed testing.

pfrenssen’s picture

  1. +++ b/core/modules/link/src/Plugin/Field/FieldWidget/LinkWidget.php
    @@ -239,6 +239,28 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
    +        // By default the field description is added to the title field. Since
    
    +++ b/core/modules/link/tests/src/Functional/LinkFieldUITest.php
    @@ -52,9 +52,13 @@ function testFieldUI() {
    +    $this->fieldUIAddNewField($type_path, $field_name, $label, 'link', array(), $field_edit);
    

    We can use [] instead of array() to make this line more hipster.

  2. +++ b/core/modules/link/tests/src/Functional/LinkFieldUITest.php
    @@ -62,31 +66,82 @@ function testFieldUI() {
    +            // Output variation being tested for debugging purpose.
    +            $this->verbose("Now testing - cardinality: $cardinality - link_type:
    +            $link_type, title_setting: $title_setting, description_enabled:
    +            $description_enabled");
    

    Let's remove the debugging cruft.

  3. +++ b/core/modules/link/tests/src/Functional/LinkFieldUITest.php
    @@ -62,31 +66,82 @@ function testFieldUI() {
    +            // Log in an admin to set up a content type for this test.
    +            $this->drupalLogin($this->adminUser);
    +
    +            // Add a new content type.
    +            $type = $this->drupalCreateContentType();
    +            $type_path = 'admin/structure/types/manage/' . $type->id();
    +            $add_path = 'node/add/' . $type->id();
    ...
    +            // and link field settings.
    

    Relogging and creating content types are expensive operations. Since these actions are done in the inner loop of the test a lot of time is spent doing this. A bit later in the loop a new test user is also created and logged in for every iteration.

    I think we can save a lot of cycles if we only log in with the admin user once to create the test fields, and then relog once as the test user to verify the expected output.

    I also think creating one single content type with multiple fields would be sufficient. If we give the fields unique machine names we can check that the expected messages are associated with the expected fields.

Jo Fitzgerald’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
5.16 KB
9.36 KB
  1. Used the hipster array syntax. :)
  2. Removed debugging cruft.
  3. Refactored to only log in once as admin & once as user and only create one new content type. Had to remove the test for absence of certain text because, now that all the fields are on the same content type, the text is actually there (but I think the test is still valid).

(and removed Needs Re-roll tag)

pfrenssen’s picture

Great improvement in speed!

[pieter@thinkarch core ((b2c3a31...) $%)]$ SIMPLETEST_BASE_URL=http://drupal.dev/drupal ../vendor/bin/phpunit modules/link/tests/src/Functional/LinkFieldUITest.php 
PHPUnit 4.8.27 by Sebastian Bergmann and contributors.

.

Time: 1.59 minutes, Memory: 4.00Mb

OK (1 test, 176 assertions)
[pieter@thinkarch core (2421001 $%)]$ git checkout 2421001-80 
Switched to branch '2421001-80'
[pieter@thinkarch core (2421001-80 $%)]$ SIMPLETEST_BASE_URL=http://drupal.dev/drupal ../vendor/bin/phpunit modules/link/tests/src/Functional/LinkFieldUITest.php 
PHPUnit 4.8.27 by Sebastian Bergmann and contributors.

.

Time: 1.03 minutes, Memory: 4.00Mb

OK (1 test, 38 assertions)

From 1.59 minutes to 1.03 minutes, it's almost twice as fast!

We can probably shave off some more seconds if we create the fields using the field API instead of through the interface. Since we are not actually testing the field UI there's no need to use the UI for this.

Refactored to only log in once as admin & once as user and only create one new content type. Had to remove the test for absence of certain text because, now that all the fields are on the same content type, the text is actually there (but I think the test is still valid).

I think we can keep this part of the test if we only look inside the field container for checking the absence of the text, instead of checking the entire response page.

pfrenssen’s picture

Assigned: pfrenssen » Unassigned

Used the field API to create the test fields. I also restored the tests for the absence of non-configured help texts by using XPath to pinpoint exact fields in the document.

[pieter@thinkarch core (2421001 $%)]$ SIMPLETEST_BASE_URL=http://drupal.dev/drupal ../vendor/bin/phpunit modules/link/tests/src/Functional/LinkFieldUITest.php 
PHPUnit 4.8.27 by Sebastian Bergmann and contributors.

.

Time: 31.02 seconds, Memory: 4.00Mb

OK (1 test, 65 assertions)

The test runs again almost twice as fast, now down to 31 seconds from the original 1 minute 59 seconds!

pfrenssen’s picture

And here is the patch :D

Status: Needs review » Needs work

The last submitted patch, 83: 2421001-82.patch, failed testing.

pfrenssen’s picture

Status: Needs work » Needs review
FileSize
11.86 KB

Rerolled against latest HEAD.

peterhebert’s picture

Having the description appear after the link text field is a bit confusing IMHO. It is confusing because it appears like it's describing the link text as opposed to the URL itself. I would suggest that it appear after the URL field, before the drupal-supplied text about the requirements (internal/external link). Here is a mock-up of what I'm suggesting:

Only local images are allowed.

geertvd’s picture

Reroll

rogierbom’s picture

Issue tags: +sprint
John Cook’s picture

Assigned: Unassigned » John Cook
Issue summary: View changes

I've checked used the reproduction instructions to test geertvd's patch from comment #87.

Before:
Only local images are allowed.

After:
Only local images are allowed.

The help text has been added to the field's "help" area.

I'll go through the other variants in the summary before changing the status.

John Cook’s picture

OK. I've run through the the variants mentioned in the summary. Get ready for a load of screenshots.

Before After
Menu
Shortcut
File field
Cardinality: 1
Link text: disabled
Url type: internal
File field
Cardinality: 1
Link text: disabled
Url type: external
File field
Cardinality: 1
Link text: disabled
Url type: both
File field
Cardinality: 1
Link text: required
Url type: internal
File field
Cardinality: 1
Link text: required
Url type: external
File field
Cardinality: 1
Link text: required
Url type: both
File field
Cardinality: Unlimited
Link text: disabled
Url type: internal
File field
Cardinality: Unlimited
Link text: disabled
Url type: external
File field
Cardinality: Unlimited
Link text: disabled
Url type: both
File field
Cardinality: Unlimited
Link text: required
Url type: internal
File field
Cardinality: Unlimited
Link text: required
Url type: external
File field
Cardinality: Unlimited
Link text: required
Url type: both
John Cook’s picture

The help text is now displayed on menu and shortcut link fields as well as link fields where the cardinality is 1 and the link text option is disabled.

It appears that the problem doesn't exist on other setting combinations on link field.

I've also looked at the code. All appears to be OK.

I've now changed the status to RTBC.

idebr’s picture