Problem/Motivation
#314385: Make position of #description configurable via the API introduced the #description_display
attribute, which controls placement of the form element #description
. The options for #description_display
are "before", "after" or "invisible".
But when using #description_display
with the fieldset
form element ('#type' => 'fieldset'
), the Form API ignored the option and the description was always placed after any elements inside the fieldset.
How to reproduce it
- Install Drupal 8 with the Normal profile.
- Create and enable a small custom module that alter the Node form. On this form add a simple fieldset. Here is a small sandbox module to test with the 3 options (before, after, invisible) https://www.drupal.org/sandbox/tplcom/2575395
- Go to node/add/page to see the new fieldset. All the fieldset descriptions are displayed after the elements.
Proposed resolution
- Fix the
core/modules/system/templates/fieldset.html.twig
template to properly handle this attribute. - Update all the core themes (all of which are already overriding this template, including Stable and Classy) to handle this attribute.
- Add test coverage.
Remaining tasks
Frontend Framework Manager reviewUpdate screenshot links in IS to latest screenshotsReviewRTBC- Commit
- Update version info in the CR and publish it
User interface changes
Before (description is always displayed after the elements)
After the patch (what we want!)
Other core themes
Screenshots for Bartik, Claro and Umami are available in comment #117:
https://www.drupal.org/files/issues/2020-05-29/2396145-117.bartik-before...
https://www.drupal.org/files/issues/2020-05-29/2396145-117.bartik-after.png
https://www.drupal.org/files/issues/2020-05-29/2396145-117.claro-before.png
https://www.drupal.org/files/issues/2020-05-29/2396145-117.claro-after.png
https://www.drupal.org/files/issues/2020-05-29/2396145-117.umami-before.png
https://www.drupal.org/files/issues/2020-05-29/2396145-117.umami-after.png
Screenshots of before patch and after patch using https://www.drupal.org/sandbox/tplcom/2575395 module for Seven, Bartik, Claro, and Umami are available in comment #132:
https://www.drupal.org/files/issues/2021-05-14/claro%20before%20patch.png
https://www.drupal.org/files/issues/2021-05-14/claro%20after%20patch.png
https://www.drupal.org/files/issues/2021-05-14/bartik%20after%20patch.png
https://www.drupal.org/files/issues/2021-05-14/seven%20after%20patch.png
https://www.drupal.org/files/issues/2021-05-14/umami-after%20patch.png
API changes
None.
Data model changes
None.
Release notes snippet
The #description_display
attribute on fieldset
form elements was ignored. Now, this attribute works properly, but only for themes provided by Drupal core or themes that update their copy of the fieldset.html.twig
template. See the change record for details on if your site is effected and how to update this template.
Comment | File | Size | Author |
---|---|---|---|
#157 | 2396145-157-8.9.x.patch | 20.37 KB | piggito |
#152 | 2396145-152.patch | 23.79 KB | dww |
Comments
Comment #1
jibranIt is not a minor issue and not major as well so normal and I think it is a novice task.
Comment #2
lucastockmann CreditAttribution: lucastockmann commentedComment #3
lucastockmann CreditAttribution: lucastockmann commentedHad to add
descripton_display
to template via preprocess to get this working.Comment #4
ArlaNice. Have not tested this yet, but anyway could we add a web test for it? It could probably follow the pattern of the tests in the parent issue.
Btw found a few other issues about missing #description_display support in other elements. Also adding tags.
Comment #5
Dom. CreditAttribution: Dom. commentedAdded patch containing tests only. Those are three tests that are WebTestBase. It's purpose it to validate fieldset rendering according to specifications here :
#314385: Make position of #description configurable via the API
- test description 'before' renders description in first of the inner fieldset fieldset-wrapper
- test description 'after' renders description in last of the inner fieldset fieldset-wrapper
- test description 'invisible' renders description in last of the inner fieldset fieldset-wrapper plus add the class "visually-hidden".
According to those tests, the patch #3 fails.
Dom.
Comment #6
Dom. CreditAttribution: Dom. commentedAdded a patch to fulfill with the tests defined #5. Compared to #3, this new patch introduces :
- add the class "visually-hidden" to hidden description, such as made for elements in general in parent issue.
- change the fieldset generic template (system/templates) but also the fieldset template in 'classy' theme currently used for tests.
- add description for the 'description_display' variable and it's possible value in the templates.
- remove 'aria-describedby' redefinition in template_preprocess_fieldset() since it is already done by heritance of element attributes.
- reorder ligns regarding description in template_preprocess_fieldset() to fit what was defined for elements in parent issue.
- finally thus have the #5 tests validated.
Changin tag to "need review".
Comment #7
Dom. CreditAttribution: Dom. commentedRe-Add tests-only patch for simpletesting.
Comment #9
Dom. CreditAttribution: Dom. commentedNeeds review since tests-only patch #7 cannot validate if solution patch #6 if not also used (obviously). So it would take manual testing here.
Testing procedure :
- apply patch only test and run the WebBaseTes called "ElementsFieldsetTest" : it should fail for invisible and before description cases.
- apply patch with issue solution
- rerun the ElementsFieldsetTest : it should now validate.
Comment #13
mgiffordComment #14
Dom. CreditAttribution: Dom. commentedHi,
I did not changed the tests-only patch.
However regarding "standard" patch:
- changed it to reflect #2407725: Remove classes from system templates d*.html.twig otherwise don't apply
- add tests inside, other don't pass validation
Comment #16
Dom. CreditAttribution: Dom. commentedOups, I was not expecting that with previous patch ! Language defines a Tour which rely on the detail description target with the wrong ID (compared to what introduced in this patch to match fieldsets.)
Correction is done in new patch.
Comment #17
mgiffordDoes this still need tests, or can that tag be removed?
What is the best way to test this patch manually?
Comment #18
Dom. CreditAttribution: Dom. commentedTests are included in patch. It tests that the description position can be set using form API. Therefore, there is not much to be done manually but reading through the patch to check how it's done.
Comment #22
Dom. CreditAttribution: Dom. commentedSame patch as #16, just making it apply.
Comment #23
jcnventura CreditAttribution: jcnventura at Wunder commentedMentoring work on this issue in Barcelona
Comment #24
jcnventura CreditAttribution: jcnventura at Wunder commentedWe're keeping at this. https://www.drupal.org/contributor-tasks/review
Comment #25
valthebald@jcnventura: there is no need in custom module, necessary additions are made to a test module
Comment #26
specky_rum CreditAttribution: specky_rum as a volunteer commentedCurrently testing this. Added a fieldset and field in a form_alter using the following:
Without the patch there is no difference in behaviour between "before" and "after".
Patch applies cleanly.
Now testing...
Comment #27
specky_rum CreditAttribution: specky_rum as a volunteer commentedWith patch in #22 applied can confirm that "after", "before" and "invisible" all work as expected.
Comment #28
specky_rum CreditAttribution: specky_rum as a volunteer commentedNew patch attached which changes the arrays to use the short array syntax as per standards. interdiff included. Hoping to get someone else here at Drupalcon Barcelona to review it shortly!
Comment #29
TheodorosPloumisPatch from #2396145: Option #description_display for form element fieldset is not changing anything solves the issue but it needs some work. Mainly needs to follow better coding standards.
1) If we have to add an empty array shouldn't it be initialized as []? Eg
$variables['description'] = [];
But why do we need to initialize the empty array? Similar patches on almost the same kind of problem seem to use NULL though.2) Probably this is irrelevant with this issue (tour module) and should be moved to a follow up issue. Or could we put it on the same patch?
3) We need to add an empty last line on this file. Simple.
4) We need to add the new option on the
template_preprocess_fieldset
function documentation oncore/includes/form.inc
file.5) We need to add a test case with an empty #description_display.
Comment #30
TheodorosPloumisComment #32
TheodorosPloumisComment #33
TheodorosPloumisComment #35
TheodorosPloumisComment #36
TheodorosPloumisComment #37
cwoky CreditAttribution: cwoky as a volunteer and at Smile commentedThe patch does not apply anymore.
It does not apply because of a change of a comment.
I rerolled it (just an auto merge).
Comment #38
TheodorosPloumisAll things that need to be done are listed on #29 (with the new one about the comment).
Comment #39
mgiffordGo bots!
Comment #42
mgiffordTo address @TheodorosPloumis points in #29 I've rerolled this patch.
I took out the edit-direction--description for the tour module. It was introduced by @Dom in #16. It was added there to make it consistent with the rest. I'm not sure if the tour module is broken without this or not. I've removed it in this patch.
Comment #43
cwoky CreditAttribution: cwoky as a volunteer and at Smile commented@mgifford, @TheodorosPloumis : You removed "$variables['description'] = NULL;", maybe we need to remove this initialisation in this patch too : https://www.drupal.org/node/2443815#comment-10411487 ?
Comment #46
mgiffordAdding back in the Tour module changes from #37, the bot wants them.... This makes it more consistent, so should be fine.
Comment #47
DuaelFrHi Mike, here is a quick review.
Out of scope. We should open a follow-up to remove that dead code.
Out of scope. If you need to change something here to make your tests pass, you have a problem somewhere else.
And, I think you may also add a test case with an empty #description_display. Doing that we ensure that this default value is not going to be changed by error someday.
Comment #48
mgiffordI'll re-roll the patch to address 1 & 2 from #47. It will need work though to include the tests.
Thanks @DuaelFr
Comment #49
mgiffordI want to see what the bots do. Still needs tests.
Comment #51
vdanielpop CreditAttribution: vdanielpop commentedComment #52
vdanielpop CreditAttribution: vdanielpop at PitechPlus for PitechPlus commentedComment #53
vdanielpop CreditAttribution: vdanielpop at PitechPlus for PitechPlus commentedComment #54
vdanielpop CreditAttribution: vdanielpop at PitechPlus for PitechPlus commentedComment #56
vdanielpop CreditAttribution: vdanielpop at PitechPlus for PitechPlus commentedComment #57
valthebaldBumping and tagging
Comment #58
mgiffordComment #59
vprocessor CreditAttribution: vprocessor at Skilld commentedComment #62
vprocessor CreditAttribution: vprocessor at Skilld commentedHi guys,
reroll is ready
Comment #63
vprocessor CreditAttribution: vprocessor at Skilld commentedComment #64
DuaelFrI just rerolled the patch and replicated the changes in the original fieldset.html.twig template to the stable theme one.
Let's see what the testbot thinks.
Comment #66
Dom. CreditAttribution: Dom. commentedWhat is currently holding this one ? How can we help this to land ? Seems almost ok right ?
Comment #67
Dom. CreditAttribution: Dom. commentedWe would add then consistency with other form-item description styling.
Comment #69
mgiffordUploading link from #64 to run against 8.4.
@Dom. in 67, does this still need to be added or are you commenting that this is what the code already does?
Comment #70
Dom. CreditAttribution: Dom. commented@mgrifford: I am suggesting to add this instead of line
+ $description_attributes = [];
in the patch. So that at least attribute class "description" is added, and so the fieldset description takes the same styling as other form item descriptions.
Comment #72
mgiffordOk, I've added that class here.
Comment #73
Munavijayalakshmi CreditAttribution: Munavijayalakshmi at Valuebound commentedRerolled the patch.
Comment #76
ricovandevin CreditAttribution: ricovandevin at Finlet for One Shoe commentedPatch in #73 is working for us with 8.4.5.
Comment #77
joachim CreditAttribution: joachim as a volunteer commentedCleaning up tags. Will trigger a retest too.
Comment #78
jhedstromWhile the patch in #73 is adding elements to a test form, it is not updating the corresponding test to ensure the position of these elements is as expected.
It needs something similar to this from
\Drupal\Tests\system\Functional\Form\ElementsLabelsTests
:Comment #80
dwwThis would be helpful at #2492171: Provide options to sanitize filenames (transliterate, lowercase, replace whitespace, etc), too.
Comment #81
PanchoWe already had a test that sadly got lost between #48 and #53. (interdiffs really help avoiding that! :)
I restored it here and converted it to a BrowserTest. But let's see first if it still tests green.
Comment #82
dwwHonest question: why does 'invisible' imply 'after'? Are there not fieldsets that would want to say something via a screen-reader before the elements, too? Shouldn't visible vs. invisible be a separate thing than before vs. after? Or at least, if it's all via #description_display, there should be 4 choices for all combinations? I guess that should have been done at #314385: Make position of #description configurable via the API and this is simply implementing what we already have in other places to work with fieldsets.
Comment #83
PanchoGood question, but certainly beyond scope here. D8 accessibility maintainer @mgifford was pretty much involved in that other issue, so I guess the placement might be negligible to screen readers. Otherwise feel free to file a followup issue.
Comment #84
dwwMostly looks good to me. A few minor nits, then this is RTBC:
Isn't this inserting a duplicate 'description' class in this case? We already set that into description.attributions.class in
template_preprocess_fieldset()
, right?This could be:
and then it only has to take 1 line.
and here...
Same concern as point 1 again here.
again
I'd re-roll myself, but then I can't RTBC this. @Pancho, are you willing to address these? If so, I'll RTBC as soon as the bot is green. I'll let the committers decide on keeping or ditching our policy of intentionally leaving "stable" templates broken in bug fix cases like this.
Regardless, I think we at least need a CR for the changes to the classy template.
I vote for fixing the stable templates, too (as we already have in here), and warn about it in the same CR.
Thanks!
-Derek
Comment #85
PanchoActually a bug, so against 8.6
Comment #86
Panchore 1/4:
Think so, but this patch doesn't introduce that, and it's not related to this bug, so would be a followup code cleanup task, possibly with different implications to BC, possibly not.
re 2/3/5:
fixed.
Comment #87
dwwThanks! Almost there. ;)
This is why the duplication is introduced in this patch, and why I'm asking about it.
This looks good.
This seems wrong, both because we already have it in the template, and because the actual content here should be the same in the before vs. after case - the only thing that should vary is the position, not the content.
Even after applying the patch, I don't see where
description_classes
is getting set. What's up with this? I see it in form-element.html.twig, but nowhere near fieldset. Is this a copy-paste error? Do I misunderstand?Again. This should look exactly like the "before" case, only the position should change.
Meanwhile, I'm still trying to get clarity from the committers about the policy on fixing stable (and apparently, classy). I might open a meta issue for this and some semi-related form element template bugs. I might also open a "policy; no patch" issue to clarify and document the policy somewhere. Stay tuned.
Cheers,
-Derek
p.s. Off topic, but sorry for your frustrations at the file sanitization issue. Please see my latest comment: #2492171-272: Provide options to sanitize filenames (transliterate, lowercase, replace whitespace, etc). *hugs*
Comment #88
dwwp.s. for now, the policy for getting decisions on if/how to fix stable seems to be "ping lauriii about it" via the "Needs frontend framework manager review" tag. ;)
@lauriii via Slack:
Comment #89
dwwProbably not worth trying to fix this until we get some clarity at #3031198: [META] Add classy_dev and minor-branch theme snapshots to allow fixing markup bugs within minor releases without front-end disruption.
I don't want to lose #314385: Make position of #description configurable via the API as the parent to this, so marking #3031198 related, instead.
Comment #90
dwwSorry for the noise, but #3031198: [META] Add classy_dev and minor-branch theme snapshots to allow fixing markup bugs within minor releases without front-end disruption has evolved, and now I see we have #2352949: Deprecate using Classy as the default theme for the 'testing' profile as the immediate solution to our bugfix deadlock.
Comment #91
PanchoWhile this is a bug and needs to be fixed either way, pragmatically we might also want to settle with a consistent naming scheme for the options first, see #3032325: Rename FormAPI #title_display option 'invisible' to what it is: 'visually-hidden' .
Comment #92
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedfixing accessibility tag
Comment #93
lauriiiI don't think this is a disruptive change. I think we should be able to make this change in Drupal 8 in a non-BC breaking way. I'll review this in more detail later, but for now, I wanted to let you know that I don't think this should be postponed.
Comment #94
lauriiiComment #96
dutchyodaI rerolled the patch based on the latest changes in core.
It didn't apply since 8.8.3.
Comment #97
eelkeblokI think we are effectively now back at #87, right? (I.e. review comments by @dww). Not sure why tests are not queueing...
Edit: no tests were enable, enabled PHP 7.1 + MySQL 5.5
Comment #98
dwwThanks for moving this forward again!
In addition to the previous review points, here are a few more:
Doesn't look like the tests care what the content of any of these are, but it seems very fragile to re-use exactly the same string for both the #title and #description. Any test trying to use this form that checks for the existence of a string on the page might hit either the #title or the #description. Since this is all new plumbing, I'd say we should start from a more firm foundation, and make sure we have unique strings in all of these places so we don't get weird false positives or negatives if we're not very specific with our assertions in some future test.
E.g. "Title: Fieldset test for description before element"
vs. "Description: Fieldset test for description before element."
(or whatever).
Same with these 3... I'd be more comfortable if they were all unique, too.
E.g. "Nested text field inside meta_before container"
...
This is missing
$defaultTheme
(per the test failures). Should be fine with this:Should be protected.
These might be more clear as
assertCount(1, $elements)
Also, assertion messages shouldn't be translatable and wrapped in
t()
like this.Thanks again!
-Derek
Comment #100
idebr CreditAttribution: idebr at iO commented#87.1 / #87.3 Removed the duplicate
description
class.#87.4 description_classes was not set anywhere, I updated this line accordingly
#87.5 The content is now identical in the 'before' and 'after' version
#98.1 / #98.2 Added unique titles and description to the test form
#98.3 Added
$defaultTheme
property to the test file#98.4
$modules
property is now protected#98.5 Updated the assertions to use assertCount, removed t() calls from the assertion message
#98.6 Fixed drupal coding standards in the patch
Comment #102
dwwThanks again for moving this forward! Sorry I missed some things on the last reviews. Our policy on markup changes has also evolved, as has my understanding of it. ;)
This is duplicating
{{ description.content }}</div>
Perhaps that explains the ElementsFieldsetTest::testFieldsetDescriptions failure? Haven't looked too closely.Sorry I didn't notice / suggest this before, but this could be a Kernel test where the tested form is the test class itself. See https://www.previousnext.com.au/blog/drupal-8-ftw-it-test-or-it-form-act... for details or core/modules/system/tests/src/Kernel/Form for some examples in core.
Doing this would make the test run *much* faster, and avoids having to expand the form_test module's form definition, keeping the form structure/code in the same class as the test that's testing it.
Note that this approach also allows you to render the form and do assertions on the generated markup. See #3020676: Support the #date_increment property in the 'duration' form element. for a recent example of such a test I wrote for a contrib module.
Not sure core committers are going to go for the pseudo code in these comments. They'll probably want you to spell them out. More like:
// Check #description placement with #description_display set to 'before'.
We're on a trip to remove custom messages from assertions unless they're inside a loop/helper where they provide crucial context for what's being tested. In all these cases, we should probably remove them, since it would be obvious from the line number what's wrong if any of them fail.
We shouldn't touch classy at all. That's why Drupal\KernelTests\Core\Theme\ConfirmClassyCopiesTest is failing. We should leave it broken.
We also shouldn't touch stable. We need to leave these hunks out, too.
Each core theme needs to be fixed directly (see below)
However, when we do this, we're going to start getting a test failure at
core/tests/Drupal/KernelTests/Core/Theme/StableDecoupledTest.php
in the 9.0.x branch. So, we'll also need a fix to that test for that 1 branch. See #3138739: Improve StableDecoupledTest in the 9.0.x branch for more.So, for example, Seven has:
core/themes/seven/templates/classy/form/fieldset.html.twig
I'm pretty sure that needs to move to:
core/themes/seven/templates/form/fieldset.html.twig
and be fixed there.core/themes/claro/templates/fieldset.html.twig
which needs to be fixed directly.core/profiles/demo_umami/themes/umami/templates/classy/form/fieldset.html.twig
which needs to move tocore/profiles/demo_umami/themes/umami/templates/form/fieldset.html.twig
and be fixed.Thanks/sorry!
-Derek
p.s. Edit: I moved the version back to 8.8.x since I believe that's what we're targeting for backport based on @lauriii in #93.
Comment #103
dwwPasting (with permission) a Slack exchange with @xjm about the above:
Phew. ;)
Thanks,
-Derek
Comment #104
narendra.rajwar27Comment #105
narendra.rajwar27Comment #106
idebr CreditAttribution: idebr at iO commented#102.1 Fixed the test failure
#102.2 Converting the Functional test to a Kernel test resulted in an error
'is_file(): Unable to find the wrapper "vfs" - did you forget to enable it when you configured PHP?'
on my local environment. This appears to be related to the Fieldset render element, but I could not find a source for this call. I left the Functional test as is.#102.3 Updated the comments
#102.4 Removed assertion messages
#102.5 Removed changes to classy
#102.6 Removed changes to stable
#102.7 Moved the template field and added the fix for Seven
#102.8 Moved the template field and added the fix for Bartik
#102.9 Added the fix for Claro
#102.10 Moved the template field and added the fix for Umami
#102.11 Added a draft change record at https://www.drupal.org/node/3143489
The patch was created against 9.1.x HEAD so I suppose this should be the target branch.
Signoff by laurii in #93 so removing 'Needs frontend framework manager review' tag
Comment #107
dwwThanks, @idebr! Almost there. ;)
Re: #106.#102.2: Hrm, it seems to work fine for me. See attached version as a Kernel test. Not sure what you did differently that resulted in that error. /shrug
I think we should keep the good docs on the
description_display
variable in all the templates. Since I'm uploading a new patch, I included that fix here. Sadly, the Claro template didn't document any variables, so that's a bigger hunk than the other templates. I hope it's considered in scope to fix that. If we have to back it out and do it in a follow-up, so be it.Also fixing
core/tests/Drupal/KernelTests/Core/Theme/ConfirmClassyCopiesTest.php
to resolve the reported failures.The 9.y.x patch applies to both 9.1.x and 9.0.x branches. Much to my great surprise,
core/tests/Drupal/KernelTests/Core/Theme/StableDecoupledTest.php
passes on the 9.0.x branch when I run it locally with this patch applied. I can't explain that. I thought that test would fail. Weird. Anyway, I'll queue this patch to test that branch, too, to see what happens on the d.o bot.I also did an 8.9.x branch backport. This does *not* apply to the 8.8.x branch, but I'm not sure this fix will be backported that far. We can probably wait to do the effort to backport this to 8.8.x once we get confirmation from core committers that it's eligible for that branch.
Thanks for the initial draft of the CR! However, that needs some help. It has to explain to theme builders a) that if they've got a
fieldset.html.twig
template override in their theme, they need to update it for this fix to work and b) how to do that update if needed. E.g. before/after versions of the template (or at least the relevant parts). Tagging for updating that.Finally, since the test only tests the raw (system) template, we should probably manually test this in the core themes that we're changing (all of them). I'll bet $20 that @xjm will want to see those screenshots before committing this). 😉
Cheers,
-Derek
Comment #108
dwwForgot to upload a test-only of the new Kernel test. This will fail and the bot will be confused, but let's see if there are any other fixes needed to the patch before we re-upload #107 as the legit full patch for RTBC.
Fails locally like so:
Line 105 is the
assertCount()
here:So yay, fails as expected: can't find the description as a preceding-sibling (because even though it's configured to use
#description_display => 'before'
the unpatched system template doesn't honor that and the description is at the end of the element.Comment #110
dwwI started skimming the full comment thread. @DuaelFr pointed out in #47:
Seems like a truly excellent idea. ;) Also gives me an excuse to upload test-only and full patches in the right order so the bot isn't confused. Finally, I realized there was some wonkiness with some of the inline comments in the test, which this patch also fixes.
Note: 2396145.107_110.interdiff.txt is the same interdiff between the test-only here and in #108.
Comment #112
dwwNew test-only is failing exactly as expected:
Line 128 is now the
assertCount()
I mentioned in #108. The new assertion added to test the default case (now lines 117 to 122) all passed (as expected -- the default behavior still works, even without the patch). Huzzah. ;)Comment #113
dwwI updated the CR:
https://www.drupal.org/node/3143489/revisions/view/11908309/11909678
Removing that tag.
Based on a quick Slack exchange with @xjm, it's not clear what branches this fix is eligible for:
So, tagging for @lauriii to review this again, now that it's basically ready to go.
Finally, updating the summary to be more complete, added a release note snippet, etc.
Thanks,
-Derek
Comment #114
dwwFixing headers inside User interface changes and adding correct alt tags.
Comment #115
xjm@lauriii putting this issue against 8.8.x in May of 2019 does not mean that it is eligible for backport to the same branch in May of 2020. So let's assume this is still 9.1.x only. Thanks!
Comment #116
lauriiiI don't think the change is BC breaking change because it's making the description work consistently across different types of form elements. That said, I think we should make this change only for the next minor release because it does contain a behavior change for some users. People might have accidentally configured the label display to be invisible or before. They wouldn't realize that the lable display is misconfigured because it didn't have any effect on how the label was rendered.
The change looks good so I'm removing the needs FEFM review tag. I think this is ready for RTBC except it would be good if someone could manually test this with Claro and Bartik, and provide screenshots.
Comment #117
dwwUsing the following form:
Here's before/after for all 4 core themes.
Comment #118
wrd CreditAttribution: wrd commentedIt's applying successfully for me and working well on 9.0.2.
Comment #119
dwwI'm not eligible to move this any further forward. I'll see if anyone in #bug-smash is available to RTBC this.
Thanks,
-Derek
Comment #120
Kristen PolThanks to everyone for all the work. Marking RTBC based on the following:
1) Skimmed the comments from before 2019 and looked more closely at the comments in 2019 & 2020.
2) Reviewed all the screenshots from #117 and they look fine.
3) Patch still applies cleanly to 9.1.x.
4) Lightly reviewed the code since it was already approved in #116. The only thing that caught my eye was the use of
===
inif ($element['#description_display'] === 'invisible') {
vs==
in{% if description_display == 'before' and description.content %}
but it seems to be consistent with other core code.5) Read the change record and it seems clear.
6) Tests pass.
7) Test-only code failed as expected.
8) @lauriii already has approved this work per #116.
Comment #121
lauriiiLooking at this again I'm asking what do people think about fixing this in Stable and Classy too? This feels rather low risk change given that you need to have
#description_display
configured for this to have any impact.Comment #122
lauriiiMoving back to needs review to get some thoughts on #121
Comment #125
ikit-claw CreditAttribution: ikit-claw commentedI think it would make sense to make sure Stable and Classy are fixed as well.
Comment #126
quietone CreditAttribution: quietone as a volunteer commentedAsked in #bugsmash for someone to reply to #121.
Comment #127
quietone CreditAttribution: quietone as a volunteer commentedSetting to NW for fixing this in Stable and Classy too.
Comment #128
dwwThanks for moving this forward. This patch also fixes stable and classy.
Comment #129
dwwWhoops, thanks bot!
Comment #131
dwwOh right,
ConfirmClassyCopiesTest
. How quickly I forget. ;) Thanks again, bot...Comment #132
Gauravvvv CreditAttribution: Gauravvvv at OpenSense Labs commentedPatch #131, fixes the issue for stable and classy too. Adding before and after patch screenshots by using https://www.drupal.org/sandbox/tplcom/2575395 module.
Moving to RTBC.
Comment #133
quietone CreditAttribution: quietone as a volunteer commented@Gauravmahlawat, thanks for the screenshots. The issue summary should be updated with those screenshots, it makes it easier for reviewers and committers. Looking at the issue summary, the screenshots are being linked to in the 'other core themes' section in the IS. Those links now point to screenshots from before that latest patch so need to be updated. From you comment I see that you have created the screenshot but there isn't a comment regarding a code review. If you have done that add a comment explaining what you did to review the code changes. There is information available about setting issues to https://www.drupal.org/docs/develop/issues/fields-and-other-parts-of-an-issue/issue-status-field#rtbc.
Adding tag for IS update, just for the screenshot links, and back to NR for a code review. I have updated the remaining tasks.
Comment #134
Gauravvvv CreditAttribution: Gauravvvv at OpenSense Labs commentedIssue summary updated.
Comment #135
Kristen PolI reviewed the interdiffs from #128, #129, and #131 and they address stable and classy as noted in #127 but I'm not seeing screenshots for those. @Gauravmahlawat said stable and classy were tested. Do we need screenshots for those to move this forward?
Comment #136
Gauravvvv CreditAttribution: Gauravvvv at OpenSense Labs commentedHi @Kristen Pol, here I have added, classy and stable screenshots. Please review.
Comment #137
Kristen PolThanks @Gauravmahlawat. Marking RTBC based on:
Comment #138
Kristen PolComment #139
quietone CreditAttribution: quietone as a volunteer commented@Gauravmahlawat and @Kristen Pol thanks for getting this to RTBC!
@Kristen Pol, both you and benjifisher teach me a lot about writing high quality comments. One thing I like is that you number each point, it makes it really easy to see what has been done.
Comment #140
larowlanIn reviewing this I had three questions, one of which remains unanswered:
\Drupal\Core\Form\FormBuilder::doBuildForm
providerTestClassyCopies
because at that point we weren't duplicating this change into stable and classy. But subsequently in #128 we decided to - but we didn't undo the changes toproviderTestClassyCopies
made in #107Moving back to needs-review pending an answer to question three. Other than that, this looks good to me, updating issue credits while I'm here
Comment #141
dww@larowlan - Excellent point! As much headache as that test has given me, it's saving the day right now. I reverted those changes to the provider and the test was confusingly failing locally. Turns out I botched things from #128 to #131. Now that we *are* fixing classy, we need to keep a bunch of templates in the 'classy' subdir in the child themes. So this patch undoes some of the file renames and leaves things where they should be.
The interdiff is kinda confusing, based on the previous patch trying to rename things it shouldn't have. But once you apply it, and add everything new, you should see this from your
git status
output:Both
Drupal\KernelTests\Core\Theme\ConfirmClassyCopiesTest
andDrupal\Tests\system\Kernel\Form\ElementsFieldsetTest
are now passing locally. Let's see if the bot agrees. 🤞Comment #142
kim.pepperThis change looks great. Reviewed it and didn't find anything glaringly obvious. I'm reluctant to rtbc as I just came across it, but I'll add my +1 to rtbc if that's helpful for the next reviewer.
Comment #143
paulocsI think this is RTBC. I did a CR in patch #141 and it looks good.
I also tested the patch with all themes available in Drupal Core and all of them are working as supposed to work.
Moving to RTBC and attaching images with the themes that I tested.
Comment #144
dwwTee hee. While editing https://www.drupal.org/node/3143489 (the draft change record) for this, I realized "fixing classy and stable" should now be "fixing classy, stable, and stable9". 🤦♂️ The only diff for
fieldset.html.twig
between stable vs. stable9 was this fix, so I copied the stable copy to stable9 and added it to the patch.@paulocs thanks so much! Very thorough and much appreciated. Hate to undo your work to RTBC, but uploading a new patch means I really should reset the status. Maybe when the bot is back green, you'd be willing to re-RTBC?
Thanks again,
-Derek
Comment #145
dwwNeeds change record updates: https://www.drupal.org/node/3143489/revisions/view/11909678/12316153Oh yeah, and upload the new patch, @dww! ;)
Comment #146
paulocsSetting to RTBC!
Comment #147
larowlanSorry to do this folks, but we now have one more fieldset.html.twig in core
core/themes/olivero/templates/form/fieldset.html.twig
Other than that, I can't fault this
Comment #148
larowlanComment #149
dwwGood catch! Sorry we missed that.
This is the only fieldset.html.twig with support for prefix and suffix. I figured if description_display is set to before it should come before the prefix. I'll try to get someone Olivero-enabled to confirm. ;)
Meanwhile, I'll update the CR accordingly.
Comment #150
dwwNeeds change record updates: https://www.drupal.org/node/3143489/revisions/view/12316153/12316567Comment #151
dwwWell drat. I changed my local dev site to use olivero as the admin theme (I know, what a hack), hacked core/modules/system/src/Plugin/Block/SystemBrandingBlock.php to set
#description_display => 'before'
, and configured the "Site branding" block to see a fieldset with a description. The styles are slightly different if the description is before vs. after. Looks like it's missing the fieldset__description class in the before case. I think I know why. Stay tuned.Comment #152
dwwHere we go. Simple fix. Copy/paste error on my part. Now the 'before' case looks the same as the 'after' case from the previous comment.
Comment #153
mherchelGood catch on the Olivero copy/paste error.
Just checked the Olivero fieldset template change, and it looks perfect.
This change is awesome. Thanks for pushing it through!
Setting this to RTBC since @larowlan's comment in #148 said this is the last remaining item (but note that I didn't extensively test the other configurations, or extensively review the non-Olivero code).
Comment #154
larowlanCommitted 0cd1d38 and pushed to 9.3.x. Thanks!
Thanks everyone for the mammoth effort here.
Thanks @mherchel for validating the olivero changes.
Published the change record.
Not backporting this to 9.2.x because there may be people relying on the broken behaviour, we try not to change markup outside releases.
Comment #157
piggito CreditAttribution: piggito as a volunteer and at Skilld commentedI ported last patch for 8.9.x in case someone else needs this in that version.
Comment #158
marco.b CreditAttribution: marco.b as a volunteer commented