Updated: #4

Problem/Motivation

While porting Wysiwyg Linebreaks to D8 (see #1917702: D8CX: I pledge that Wysiwyg Linebreaks will have a full Drupal 8 release on the day that Drupal 8 is released.), I noticed that there's a blank (empty) vertical tabs area appearing underneath the Toolbar Configuration section on a text format/editor configuration page:

Empty settings vertical tabs

Proposed resolution

Don't display the vertical tabs fieldset if there are no extra settings to be displayed.

Remaining tasks

Test patch in #4.

User interface changes

An ugly, empty vertical tabs area will not be displayed if there are no extra settings to be displayed.

API changes

None.

None.

CommentFileSizeAuthor
#12 don_t_show_empty-2067323-12.patch819 bytesValentine94
#10 don_t_show_empty-2067323-1.patch839 bytesValentine94
#5 empty-vertical-tabs-2067323-5.patch859 bytesjessebeach
#5 interdiff.txt873 bytesjessebeach
#4 2067323-4-empty-vertical-tabs-fix.patch859 bytesgeerlingguy
#1 2067323-1-empty-settings.png72.63 KBgeerlingguy
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

geerlingguy’s picture

geerlingguy CreditAttribution: geerlingguy commented
FileSize
2067323-1-empty-settings.png72.63 KB

Adding an image to illustrate the sadness.

geerlingguy’s picture

geerlingguy CreditAttribution: geerlingguy commented

Hmm... looking further into this, it looks like there is one plugin with settings loaded—the 'Styles dropdown' (stylescombo)—but the settings only show up if the button is dragged onto the toolbar.

Unfortunately, the default behavior with the Standard install profile makes it look like this admin form is broken, at least until an admin drags the Styles button onto the Wysiwyg toolbar and the settings show up (or if the site has something like Wysiwyg Linebreaks installed).

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Title: Don't show empty vertical tabs area if no extra CKEditor settings available for configuration » Don't show empty vertical tabs area if all vertical tabs are hidden
Component: ckeditor.module » base system
Issue tags: +Usability, +DrupalWTF

The same problem applies to the case of a text format with zero filters enabled, like the Full HTML text format. It's a bug in the vertical tabs system.

i.e. see admin/config/content/formats/manage/full_html's filter settings at the very bottom. This has been the case at least since January 2013, if not longer, in Drupal 8.

I totally agree we should fix this though :) If you can roll patches, I'll provide reviews!

geerlingguy’s picture

geerlingguy CreditAttribution: geerlingguy commented
Status: Active » Needs review
FileSize
2067323-4-empty-vertical-tabs-fix.patch859 bytes

So, the attached patch works... is this the right approach?

geerlingguy’s picture

geerlingguy CreditAttribution: geerlingguy commented
Issue summary: View changes

Added illustration.

jessebeach’s picture

jessebeach CreditAttribution: jessebeach commented
FileSize
interdiff.txt873 bytes
empty-vertical-tabs-2067323-5.patch859 bytes

Oy, I've never really looked at vertical-tabs.js. It could use some love eventually. geerlingguy, I think you've found a fine solution regardless that works within the spirit of the current code. The only change I suggest is to use closest() instead of parents(). Otherwise it gets the job done well.

geerlingguy’s picture

geerlingguy CreditAttribution: geerlingguy commented

Ah, right, I always seem to forget about closest(). Presuming it comes back green (how could it not?), I think this should be RTBC.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs backport to Drupal 7

Very simple, works great! Thanks, geerlingguy!

This probably needs to be backported too.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed dda8d4d and pushed to 8.x. Thanks!

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Issue tags: +JavaScript

tag

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Issue summary: View changes

Updated info about patch to test.

Valentine94’s picture

Valentine94
Location Kyiv, Ukraine
CreditAttribution: Valentine94 commented
Issue summary: View changes
Status: Patch (to be ported) » Needs review
Issue tags: -
FileSize
don_t_show_empty-2067323-1.patch839 bytes

Back-port to 7.x-dev

vlad.dancer’s picture

vlad.dancer
Primary language Ukrainian
Location Kyiv
CreditAttribution: vlad.dancer commented
Status: Needs review » Patch (to be ported)

@Valentine94.
There is no such selector ".form-type-vertical-tabs", use instead of it this one ".vertical-tabs". As soon as this patch will be updated fix should works as designed.

Also how to reproduce this error on Drupal 7:
Go to the admin/config/content/formats/filtered_html (for example) and disable all items to test it.

Valentine94’s picture

Valentine94
Location Kyiv, Ukraine
CreditAttribution: Valentine94 commented
Status: Patch (to be ported) » Needs review
FileSize
don_t_show_empty-2067323-12.patch819 bytes

Changed a class selector.

vlad.dancer’s picture

vlad.dancer
Primary language Ukrainian
Location Kyiv
CreditAttribution: vlad.dancer commented
Status: Needs review » Reviewed & tested by the community

Now it works as expected. Thanks @geerlingguy, @Valentine94!

geerlingguy’s picture

geerlingguy CreditAttribution: geerlingguy commented

+1

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 12: don_t_show_empty-2067323-12.patch, failed testing.

Status: Needs work » Needs review

geerlingguy queued 12: don_t_show_empty-2067323-12.patch for re-testing.

idebr’s picture

idebr CreditAttribution: idebr commented
Status: Needs review » Reviewed & tested by the community

Setting back to RTBC per #13/#14

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 12: don_t_show_empty-2067323-12.patch, failed testing.

Status: Needs work » Needs review

geerlingguy queued 12: don_t_show_empty-2067323-12.patch for re-testing.

Valentine94’s picture

Valentine94
Location Kyiv, Ukraine
CreditAttribution: Valentine94 commented
Status: Needs review » Reviewed & tested by the community
David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented
Status: Reviewed & tested by the community » Fixed

Committed to 7.x - thanks!

  • David_Rothstein committed 5b781af on 7.x 
    Issue #2067323 by Valentine94, geerlingguy, jessebeach, vlad.dancer: Don...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.