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)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Jeff Burnz’s picture

Version: 7.0-beta1 » 7.x-dev
Component: Seven theme » theme system

I 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.

tkoleary’s picture

I 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.

bleen’s picture

... which is a Drupal core issue and not one I would be hugely in favor of at this stage

Gotta agree with Jeff on this one. I'm a mac user and I've never thought to click that arrow on website ...

Jeff Burnz’s picture

I'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...

sirtet’s picture

Status: Active » Closed (duplicate)

duplicate of
http://drupal.org/comment/reply/1305574

PS:
it DID matter to the DO designers, see the collapsible fieldsets on this page here...

DamienMcKenna’s picture

Status: Closed (duplicate) » Active
DamienMcKenna’s picture

Instead of fiddling with the margins so the link overlaps the image, could the image itself be made a link?

Bojhan’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Usability

Let's fix this.

Jeff Burnz’s picture

#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)

html.js fieldset span.fieldset-legend {
  background-image: none !important;
  padding-left: 0 !important;
}
html.js fieldset.collapsible .fieldset-legend a {
  background: url('images/menu-expanded.png') no-repeat scroll 5px 65% transparent;
  padding-left: 15px;
}
html.js fieldset.collapsed .fieldset-legend a {
  background-image: url('images/menu-collapsed.png');
  background-position: 5px 50%;
}
sirtet’s picture

Status: Active » Needs review
FileSize
762 bytes

just adding an "a" tag to the corresponding css instructions in system.theme.css seems to do the trick:

.js fieldset.collapsible > legend .fieldset-legend a{
.js fieldset.collapsed > legend .fieldset-legend a{

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.

Status: Needs review » Needs work

The last submitted patch, Move arrows to anchor-948516-10.patch, failed testing.

bleen’s picture

@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

sirtet’s picture

Status: Needs work » Needs review
FileSize
762 bytes

Patch renamed

Bojhan’s picture

This needs before/after shot.

sirtet’s picture

FileSize
15.78 KB
15.65 KB

original:
original
patched:
patched

Bojhan’s picture

Status: Needs review » Reviewed & tested by the community

RTBC!

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks!

Dries’s picture

Committed to 8.x. Thanks!

DamienMcKenna’s picture

Would it be OK to backport this?

DamienMcKenna’s picture

FileSize
679 bytes

D7 version of the patch from #13.

DamienMcKenna’s picture

Status: Fixed » Needs review

Status: Needs review » Needs work

The last submitted patch, drupal-n948516-21-d7.patch, failed testing.

jerdavis’s picture

Version: 8.x-dev » 7.x-dev

Setting to 7.x

jerdavis’s picture

Status: Needs work » Needs review

#21: drupal-n948516-21-d7.patch queued for re-testing.

DamienMcKenna’s picture

#facepalm. Thanks Jer.

sirtet’s picture

Status: Needs review » Reviewed & tested by the community

as 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)

webchick’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +Needs backport to D7

I 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!

sirtet’s picture

newbie 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?

bleen’s picture

sirtet: 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...

David_Rothstein’s picture

Title: Usability bug: down arrow of fieldsets in D7 overlays should be part of the link... » (RTL stying broken) Usability bug: down arrow of fieldsets in D7 overlays should be part of the link...
Version: 7.x-dev » 8.x-dev
Status: Fixed » Needs work
FileSize
6.91 KB
7.89 KB

I 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.

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
676 bytes

Hopefully 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.

webchick’s picture

Wow, thanks for catching that, David! So sorry. :(

sirtet’s picture

FileSize
19.55 KB

yes, the patch from #32 does work in D8:
D8 test

jenlampton’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Looks like that's a review! :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 32: drupal-948516-32.patch, failed testing.

bill richardson’s picture

Status: Needs work » Closed (cannot reproduce)

This does not seem to be an issue in latest Drupal 8 beta ---- closing as cannot reproduce

David_Rothstein’s picture

Title: (RTL stying broken) Usability bug: down arrow of fieldsets in D7 overlays should be part of the link... » Usability bug: down arrow of fieldsets in D7 overlays should be part of the link...
Version: 8.0.x-dev » 7.x-dev
Status: Closed (cannot reproduce) » Needs work

Looks 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.

  • Dries committed 85f1c85 on 8.3.x
    - Patch #948516 by sirtet: usability bug: down arrow of fieldsets in D7...

  • Dries committed 85f1c85 on 8.3.x
    - Patch #948516 by sirtet: usability bug: down arrow of fieldsets in D7...

  • Dries committed 85f1c85 on 8.4.x
    - Patch #948516 by sirtet: usability bug: down arrow of fieldsets in D7...

  • Dries committed 85f1c85 on 8.4.x
    - Patch #948516 by sirtet: usability bug: down arrow of fieldsets in D7...
aubjr_drupal’s picture

I 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?

aubjr_drupal’s picture

Status: Needs work » Needs review

  • Dries committed 85f1c85 on 9.1.x
    - Patch #948516 by sirtet: usability bug: down arrow of fieldsets in D7...