Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Found while trying to test #1807692: Introduce a column synchronization capability and use it to translate alt and titles through the image field widget
Reproduced in clean new 8.x.
Problem/Motivation
Nice browse and upload widget is there when 1 image allowed.
Widget goes away when multiple images are allowed.
Proposed resolution
(description of the proposed solution, the rationale behind it, and workarounds for people who cannot use the patch)
Remaining tasks
- locate cause
- add test
- propose fix
Steps to reproduce
- clean install
- create an article, see the image upload widget
- structure: article
- manage fields: edit image
- (global) field settings: change allowed values to 2 (or 4 or whatever). save.
- create an article, see the image widget is gone.
User interface changes
No. Just a fix.
API changes
No API change expected.
| Comment | File | Size | Author |
|---|---|---|---|
| #17 | 1884148-17.patch | 853 bytes | swentel |
| #7 | drupal8.file-multiple-details.7.patch | 1.47 KB | sun |
| #5 | cardinality-chrome-s01-013-01-09_0906.png | 57.9 KB | YesCT |
| #3 | Screen Shot 2013-01-09 at 13.51.26.png | 71.37 KB | swentel |
| #1 | multiplevalueimage-2013-01-09_0332.png | 69.13 KB | YesCT |
Comments
Comment #1
YesCT CreditAttribution: YesCT commentedI might have the component wrong...
Here is a screenshot.
Comment #2
YesCT CreditAttribution: YesCT commentednext steps (in any order):
Comment #3
swentel CreditAttribution: swentel commentedDon't see any problems here ..
Comment #4
swentel CreditAttribution: swentel commentedComment #5
YesCT CreditAttribution: YesCT commentedI double checked again. did a sudo rm -r sites; git checkout sites; git checkout 8.x; git pull --rebase; drush -y si ...
the screenshot in was in Firefox 17.0.1
Can still reproduce that
In Chrome
I get something different, almost the same, but with a arrow showing the field is collapsed.
Here is a screenshot of Chrome (Version 23.0.1271.101)
It expands ok, but after adding each image it "jumps" back to collapsed.
Comment #6
swentel CreditAttribution: swentel commentedSo that's a DETAILS problem (the new tag instead of fieldset - see #1168246: Freedom For Fieldsets! Long Live The DETAILS.), not a field problem. Could be that this version of firefox has a bug maybe ..
Comment #7
sunThere are two bugs here:
1) Details are always collapsible. #collapsible is obsolete. The rendering should no longer account for a potential FALSE value. (The remaining clean-up will happen in #1852104: #type details: Remove #collapsible property)
2) File module implements a custom multiple field widget behavior and manually implants theme_details() as a #theme_wrappers — that's completely missing all other element type properties of #type 'details', which means that collapse.js doesn't get loaded at all.
Comment #9
swentel CreditAttribution: swentel commented#7: drupal8.file-multiple-details.7.patch queued for re-testing.
Comment #10
swentel CreditAttribution: swentel commentedThat indeed fixes it, and I was wondering about that #collapsible thing, good to know about the other issue :)
If this comes green and YesCT confirms, this is RTBC from me.
Comment #11
YesCT CreditAttribution: YesCT commentedI've delayed, but I can verify that it fixes it in both firefox and chrome.
it also still applies.
Comment #12
YesCT CreditAttribution: YesCT commented#7: drupal8.file-multiple-details.7.patch queued for re-testing.
Comment #14
YesCT CreditAttribution: YesCT commented#7: drupal8.file-multiple-details.7.patch queued for re-testing.
Comment #16
YesCT CreditAttribution: YesCT commentedtagging for reroll (reroll doc: http://drupal.org/patch/reroll)
Comment #17
swentel CreditAttribution: swentel commentedRerolled after the #collapsible cleanup happened.
Comment #18
YesCT CreditAttribution: YesCT commentedThis might not need tests since it was browser dependent.
If someone knows how tests should be done, please provide some details to help direct that.
(untagging needs tests and also needs reroll since that was done. thanks. :) )
Comment #19
catchWas this really browser dependent? I'd expect the missing #type => details to have an impact on the HTML so presumably we could test for the HTML with it and that'd fail without?
Comment #20
sunYes, this bug depends on the browser being used, due to native browser support differences for details, which circles back into #1839158: Replace collapse.js with a proper polyfill for <details> — that is, because the render element manually specifies a #theme_wrappers of 'details', which means that it goes through the appropriate theme function, but alas, just without any element type defaults + pre_render/preprocessing.
This is simply a brain-dead oversight in form declaration. It's like forgetting to fill out the x in 1 + x = 2.
Due to that, I disagree with the need for tests here. Stuff like this just simply happens during refactoring.
Just simply consider the phpDoc for such a test:
Oh, wow. ;)
Comment #21
catchHmm OK if we had browser acceptance tests we could actually test that nicely but agreed that makes no sense with simpletest. Committed/pushed to 8.x.
Comment #23
Gaelan CreditAttribution: Gaelan commented