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.
The first thing the user will try to click on to open or close the fieldset is the arrow, not the title. Padding left should be added to all fieldset titles so that the background image arrow is included in the link.
See attached image of current link (firefox)
Comment | File | Size | Author |
---|---|---|---|
#43 | drupal-948516-fieldset-arrows-43.patch | 1.29 KB | aubjr_drupal |
#34 | D8-rtl-fieldset.jpg | 19.55 KB | sirtet |
#32 | drupal-948516-32.patch | 676 bytes | David_Rothstein |
#31 | rtl-before-patch.png | 7.89 KB | David_Rothstein |
#31 | rtl-after-patch.png | 6.91 KB | David_Rothstein |
Comments
Comment #1
Jeff Burnz CreditAttribution: Jeff Burnz commentedI personally have never seen a user do this in user testing, they always click the Legend anchor. Adding padding will not automatically include the arrow in the link - it has to be moved from the span wrapper to the actual link, which is a Drupal core issue and not one I would be hugely in favor of at this stage.
Comment #2
tkoleary CreditAttribution: tkoleary commentedI have seen this in testing (and done it myself!) Any user who owns a mac will do this because the use pattern in the mac os is that you click the arrow to expand the contents.
Comment #3
bleen CreditAttribution: bleen commentedGotta agree with Jeff on this one. I'm a mac user and I've never thought to click that arrow on website ...
Comment #4
Jeff Burnz CreditAttribution: Jeff Burnz commentedI'm windows and mac user, although these days I spend much more time on windows to be frank, maybe someone who is pure mac user might do this, but honestly its not something I have seen. I was a mac user for some 15 years or more before starting with windows just a few years ago, only sporadically did I use windows before that. FWIW and a bit of history my very first mac was an LCIII "pizza box" circa 1993, which replaced my Amiga 500... lol... ah the bad old days...
Comment #5
sirtetduplicate of
http://drupal.org/comment/reply/1305574
PS:
it DID matter to the DO designers, see the collapsible fieldsets on this page here...
Comment #6
DamienMcKennaI closed #1305574: Make arrows on collapsible fieldsets clickable too and am re-opening this one.
Comment #7
DamienMcKennaInstead of fiddling with the margins so the link overlaps the image, could the image itself be made a link?
Comment #8
Bojhan CreditAttribution: Bojhan commentedLet's fix this.
Comment #9
Jeff Burnz CreditAttribution: Jeff Burnz commented#7, yes, its very simple, just move the image background from the span to the anchor, my admin theme does this, its a two second fix:
This is what my theme does (I have to override core)
Comment #10
sirtetjust adding an "a" tag to the corresponding css instructions in system.theme.css seems to do the trick:
Jeff's solution is basically the same, only he used a slightly different selector.
PS:
I hope i got my first patch right.
Just "learned" how to contribute yesterday at the Swiss DUG Meeting in Zurich, where i also discussed this issue here with Lukas von Blarer.
Comment #12
bleen CreditAttribution: bleen commented@sirtet: Welcome! Glad your getting into the patching game :)
I dont hink patch names can have spaces in them. That might be why testbot seems to be unhappy. Give it another go without spaces and dont forget to change the issue back to "needs review" so testbot will know to come back and have another look at your new patch
Comment #13
sirtetPatch renamed
Comment #14
Bojhan CreditAttribution: Bojhan commentedThis needs before/after shot.
Comment #16
sirtetoriginal:
patched:
Comment #17
Bojhan CreditAttribution: Bojhan commentedRTBC!
Comment #18
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks!
Comment #19
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks!
Comment #20
DamienMcKennaWould it be OK to backport this?
Comment #21
DamienMcKennaD7 version of the patch from #13.
Comment #22
DamienMcKennaComment #24
jerdavisSetting to 7.x
Comment #25
jerdavis#21: drupal-n948516-21-d7.patch queued for re-testing.
Comment #26
DamienMcKenna#facepalm. Thanks Jer.
Comment #27
sirtetas the code change is identical to the D8 patch, i guess i can set this to RTBC?
(I did successfully download and apply the patch from #21)
Comment #28
webchickI think we can make an exception to backport this since it's such a minor markup adjustment. If David disagrees, he should feel free to revert this patch.
Committed and pushed to 7.x. Thanks!
Comment #29
sirtetnewbie question: what's the "exception" here?
I guess it's something like that this is not seen as a bug, but just a style question, where styles are normally not changed/backported?
Comment #30
bleen CreditAttribution: bleen commentedsirtet: we typically try to avoid changing the js behaviors (and markup, and translatable strings, and API functions, and a couple other things...) on a released version of Drupal. In this case webchick decided that the change was minor enough (and unlikely to be something that could break any existing sites out there) so she committed it.
Had this change been to remove the arrow completely or renamed the classes used by it I doubt she would have committed it...
Comment #31
David_Rothstein CreditAttribution: David_Rothstein commentedI just rolled this back from Drupal 7 not for the reasons discussed above, but rather because it looked to me like the patch forgot to deal with right-to-left languages.
I wasn't sure how bad that omission would be, so I tested it and (see the attached screenshots) the answer is "very bad" :) Hopefully this isn't too hard to fix, but it needs to go in Drupal 8 first.
Also, before backporting it to Drupal 7 again, we probably should discuss the backport a little more? The thing about fieldsets is that they are very commonly used not just on admin screens but also on public-facing pages of a website, and thus they sometimes can be heavily themed. So I think we should be especially careful backporting changes unless they're really necessary. Just from looking at the patch, it's hard for me to get a sense of how likely it is to break a theme which has heavily customized its fieldsets, but maybe someone else has a better idea?
Oh, also, another backport note: The patch here looks like it conflicts with #1361810: Non-collapsible fieldsets within collapsed fieldsets have "expanded" icon which might get committed to Drupal 7 soon.
Comment #32
David_Rothstein CreditAttribution: David_Rothstein commentedHopefully the Drupal 8 fix is as simple as this.
It's untested for Drupal 8, but the same change does seem to work correctly in Drupal 7 (figured I'd test it there quickly while I already had the RTL language set up in my development environment).
The only reason I'm not sure is that the CSS rules for LTR and RTL languages look a fair amount different in this part of the code, so perhaps a more complicated change is actually required.
Comment #33
webchickWow, thanks for catching that, David! So sorry. :(
Comment #34
sirtetyes, the patch from #32 does work in D8:
Comment #35
jenlamptonLooks like that's a review! :)
Comment #37
bill richardson CreditAttribution: bill richardson as a volunteer commentedThis does not seem to be an issue in latest Drupal 8 beta ---- closing as cannot reproduce
Comment #38
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedLooks like the regression is no longer present (probably was fixed when Drupal 8 changed the way it handled RTL styling to no longer use separate CSS files) but we still need to consider the original patch (plus fixes to prevent the regression) for possible Drupal 7 backport.
Comment #43
aubjr_drupal CreditAttribution: aubjr_drupal commentedI know this is a really old D7 ticket, but for record's sake: I had no trouble with RTL output when I applied the changes in #10 to the rules in both system.theme.css AND system.theme-rtl.css. Could my reformed patch get put into the next release of D7?
Comment #44
aubjr_drupal CreditAttribution: aubjr_drupal commented