[drupal-devel] [feature] Form groups should be collapsible
tangent
drupal-devel at drupal.org
Mon Feb 7 03:49:55 UTC 2005
Project: Drupal
Version: 4.5.2
Component: base system
Category: feature requests
Priority: normal
Assigned to: Anonymous
Reported by: tangent
Updated by: tangent
Status: patch
That seems like a a reasonable expectation but testing in Firefox and IE
6 indicates that it isn't the case. If the first command in the external
javascript file assigns a class to document.documentElement (and that
class is added to the selector for fieldsets) there is still a delay
before the style is applied.
tangent
Previous comments:
------------------------------------------------------------------------
January 25, 2005 - 22:42 : tangent
Attachment: http://drupal.org/files/issues/collapsible_formgroups.patch (2.8 KB)
With all the talk of form reorganization and with the latest attempts at
coding a solution, I thought I'd step up and contribute my solution. It
is a quick development that I just threw together and it will probably
need to be expanded to support more options, but it works and it has
zero impact on existing code other than a small change to
form_groups(). I've added an optional boolean parameter called
"collapsed" so that modules can specify if the group should be
initially collapsed or not.
Known issues:
1. Styling the collapsed forms to look the same or even just good in
every browser may prove troublesome. It looks fine in Mozilla/Firefox
but the legend jumps around a bit in IE. I'm not terribly concerned
about that at the moment though.
2. Required fields are not handled so a collapsed fieldset *could*
contain a required field. This will need to be dealt with before this
is a real solution, in my opinion. The difficulty lies in the fact that
required fields are not labeled (in the markup) as such, other than by
adding an extran span. Maybe we should think about adding a "required"
attribute to required fields.
I'm attaching the patch (against HEAD) now and will attach the
javascript file shortly as well as a test page so you can see the
functionality.
As an aside, I'm setting this issue to patch status since that seems to
be the process used around here. However, I think that generally an
issue should ONLY be set to patch status if the attached file is worthy
of being commited. Please correct me if I have misunderstood.
------------------------------------------------------------------------
January 25, 2005 - 22:44 : tangent
Attachment: http://drupal.org/files/issues/drupal.js (1.68 KB)
Here's the (new) drupal.js file. Note that don't necessarily think the
functions included in it need to be loaded for every request. If more
javascript is added, it should probably be broken up into separate
pieces that can be sourced as need.
------------------------------------------------------------------------
January 25, 2005 - 22:46 : tangent
Attachment: http://drupal.org/files/issues/fieldset_test.html (3.41 KB)
Here's a stand-alone test page so you can see what the functionality
would be like. Please report any issues with specific browsers by
attaching your user-agent string or a description of your browser and
the issue. Screenshots are welcome.
------------------------------------------------------------------------
January 26, 2005 - 03:22 : Junyor
FYI, window.addEventListener is not supported in Opera.
------------------------------------------------------------------------
January 26, 2005 - 03:27 : chx
A cross browser solution to add event listeners can be found at
http://www.scottandrew.com/weblog/articles/cbs-events
That one works with Opera.
------------------------------------------------------------------------
January 26, 2005 - 03:45 : tangent
Attachment: http://drupal.org/files/issues/fieldset_test2.html (3.84 KB)
Here's an update of the test page. You should see the toggle image now
and I've added extra event handling for all those substandard browsers
;-)
I also corrected an issue where, if javascript is disabled, the group
could be collapsed and not expandable.
------------------------------------------------------------------------
January 26, 2005 - 03:48 : tangent
For what it's worth, if this functionality doesn't work in some browsers
that's ok. As long as the code fails silently everything else will
degrade nicely and they'll simply see all the forms.
------------------------------------------------------------------------
January 26, 2005 - 04:08 : tangent
Attachment: http://drupal.org/files/issues/fieldset_test3.html (4.06 KB)
This one might work better. I missed some stuff if the last one. I don't
have Opera or Konqueror to test with though.
I'm working a method to deal with required fields next.
------------------------------------------------------------------------
January 26, 2005 - 04:37 : Dries
Looks good (though I haven't looked at the code).
The node-edit/submit form would be an obvious candidate for this kind
of stuff.
Interesting!
------------------------------------------------------------------------
January 26, 2005 - 07:34 : tangent
Should fieldset collapsing be optional? This implies that form_group()
would need 2 new arguments: $collapsible and $collapsed.
Someone has already voiced a desire for a configuration to
enable/disable collapsible groups but I think that we should simply let
the module decide. In my opinion it would be like offering a config
preference for allowing expanding menus or not.
------------------------------------------------------------------------
January 26, 2005 - 08:35 : Bèr Kessels
Attachment: http://drupal.org/files/issues/drupal_form_toolbox.png (19.23 KB)
Great work!
Some initial thoughts from me:
We need logic to uncollapse forms that have the required flag set.
(required forms should be visible by default)
We need logic to uncollapse forms that have errors.
The form group should, in this case get an extra class to show that,
when collapsed, it contains an error, or a required field.
We must think of a method to avoid colapses-in-collapses. I.E: you
collapse form_group, which is great, but what if (i think bigger) we
have three areas on a page, that can be collapsed: in that case, the
form_groupos inside such an area should not be collapsible.
We could decide to collapse the other forms, when opening one. This is
somethimes called "cards", sometimes they are called "toolboxes' see
attachement for an example.
------------------------------------------------------------------------
January 26, 2005 - 09:32 : mathias
One usability issue surrounding collapsible elements is controlling how
they would operate when present on the node editing form (which would
be a great location for this functionality). For example what happens
when someone clicks the preview button? I would want forms fields that
contain data to stay expanded and not re-collapse.
------------------------------------------------------------------------
January 26, 2005 - 13:32 : tangent
Attachment: http://drupal.org/files/issues/fieldset_test4.html (4.07 KB)
Here's an updated test page that handles required fields the easy way.
It simply borders the collapsed field with red. I think we should try
this one out and discuss further before deciding on how best to deal
with this issue. I am not immediately convinced that one solution is
better than another.
I've changed the addEvent function again to reduce it as far as
possible. I am not convinced that the other version added support for
any other browsers than this version so please correct me if I am
mistaken.
------------------------------------------------------------------------
January 27, 2005 - 03:54 : tangent
After giving further thought to some of your suggestions I have revised
my opinions.
1. A fieldset containing an input with a validation error MUST be
expanded. It would be a major usability issue not to. The only question
is whether to allow the fieldset to be collapsible or force it to not be
collapsible. I might imagine a scenario where the user, once she has
updated the input, would want to collapse the fieldset to look at other
areas of the form but perhaps this scenario is unlikely or undesirable.
2. A fieldset containing an input that is required MUST be expanded by
default. While it would be possible to style the collapsed fieldset to
convey that it contains a required field, it would require the user to
expand the fieldset before being able to input the required form data
and it could prove too confusing to users since it would be an unusual
UI method. Again, the only question is whether to allow a fieldset
containing a required input to be collapsible. I find the scenario
where a user would want to collapse a completed fieldset more likely in
this instance so I would lean toward allowing it.
3. Since this change uses client-side scripting which may cause
problems for or be negatively viewed by Drupal admins it may be
desirable to provide a configuration setting to disable (or enable as
the case may be) the functionality. Because the functionality is
contained solely in the client-side code implementing this option
should be as simple as checking for the preference and adding the
"script" tag to the output or not and should not require adding any
other checks elsewhere. Where would this configuration setting be
placed though? Adding this option to the main settings screen seems
like the logical place, for lack of a more suitable area.
4. The preview mode would, ideally, preserve the form state. If the
user collapses a fieldset which is not collapsed by default then it
should remain collapsed in the preview mode. While I haven't examined
the preview method to see if it would support "form state" without
modification I can't imagine that it does. I also imagine that adding
this support could be more trouble than it is worth. How necessary is
this feature?
I am not going to concern myself with other collapsible form areas
(e.g., tab groups) at this time. If this happens at some future time my
implementation should be easy to extend to live within the new
environment but it seems unnecessary to worry about it until it
happens.
------------------------------------------------------------------------
January 27, 2005 - 07:40 : moshe weitzman
Re: collapsing sections with required fields.
Keep in mind that a lot of fields are 'required' but they are hardly
ever changed. `If you are an admin, you see lots of fields on the
node/edit page such as Author which cannot be blank. This field is
always contains a value already. You might want this admin section
collapsed by default. Another example might be a taxonomy selector
called 'Departments'. Perhaps the user always posts to the 'Marketing'
department. Lets say that we re-instate an old feature whereby the
system remembers your last chosen taxonoy terms. So this user might
want to always collapse the taxonomy section and just accept default
values. I hope that is possible even if the section contains a required
field. The concern is not with required fields, but with *empty*
required fields.
Anyway, please consider this wrinkle in your design.
------------------------------------------------------------------------
January 27, 2005 - 10:00 : Steven
I like the clean implementation. Just some notes:
- Your method for changing the classname to ensure the CSS is not
applied without JS is a bit convoluted. You can do this more easily by
defining your rules as:html.javascript ... {}
and doing:
if (document.documentElement) {
addClass(document.documentElement, 'javascript');
}
function addClass(element, classname) {
if (element.className.length > 0) {
element.className += ' '+ classname;
}
else {
element.className = classname;
}
}
This will add the "javascript" class to the <html> element, and
automatically enable all rules that select on that.
- For matching classNames you should use:
element.className.match('\\bclass\\b')
to properly support multiple classes per element.
- If we are going to include JS in drupal, we need to figure out some
code style. While most of Drupal's PHP rules translate to JS, some
don't really make sense. For example, Javascript uses 'studlyCaps' for
variable and function naming. As we have to use that notation for
predefined functions and variables anyway, it would seem to make sense
to use that naming convention instead.
- "cursor: pointer;" does not work in IE... you have to use "cursor:
hand;" instead. The best solution is to use "cursor: hand; cursor:
pointer;". CSS ignoring rules make this work on all browsers.
------------------------------------------------------------------------
January 27, 2005 - 10:24 : tangent
Thanks for all the suggestions.
1. Your suggestion to apply a "javascript" class to html would result
in the the fieldsets being collapsed if their DOM supports
documentElement even though they may not support the addEvent
implementation. I hope to do the class assignment as late as possible
to eliminate the possibility of css being applied when the javascript
is not functioning.
2. I tried to use the drupal code style where applicable but I agree
that javascript should have its own code-style conventions.
3. As far as I know IE5 only supports cursor: hand while IE6 supports
cursor: pointer (at least my version does). I would prefer to avoid
adding non-standard CSS to drupal.css if possible.
------------------------------------------------------------------------
January 28, 2005 - 09:04 : Junyor
Attachment: http://drupal.org/files/issues/fieldset_test5.html (4.08 KB)
@tangent:
If you change the third parameter for addEventListener() to 'false',
it'll work in Opera, too. It's actually a bug in Mozilla that it works
with the value 'true' in your script[1]. I'm attaching a modified
version of fieldset_test4.html.
Tested and working in IE 6.0, Opera 7.54 and 8.0B1+, Firefox 1.0, and
Safari 1.2.4.
[1] https://bugzilla.mozilla.org/show_bug.cgi?id=235441
------------------------------------------------------------------------
February 5, 2005 - 02:38 : Steven
"1. Your suggestion to apply a "javascript" class to html would result
in the the fieldsets being collapsed if their DOM supports
documentElement even though they may not support the addEvent
implementation. I hope to do the class assignment as late as possible
to eliminate the possibility of css being applied when the javascript
is not functioning.
"
It doesn't matter when you do the class assignments, my point was that
it would be much shorter than having to muck around with the class of
/every/ fieldset. Surely you can check for the addEvent presence too?
------------------------------------------------------------------------
February 5, 2005 - 14:59 : tangent
Changing the class of each fieldset is necessary each time the user
clicks on the legend to expand or collapse it since I am using
definitions in drupal.css to change the style settings of the fieldset
(as opposed to restyling with js). This will allow a theme to change
the style of fieldsets without having to alter the javascript.
Further, I don't really like the idea of ".html .fieldset-expanded"
selectors in drupal.css because it looks rather hackish.
Finally, I have a dilemma over whether this feature is really wise.
When testing the feature with a small test page it works great but in a
live environment where pages take longer to load there is a delay (while
the page is loading) before the fieldsets collapse because the js does
not execute until the page is completely loaded. This flash is less
than desirable and may be unnacceptable in some cases.
The only workaround for this that I can think of is to integrate the
feature fully into Drupal such that if js is detected fieldsets would
be displayed already collapsed (unless they have a error field of
course). It doesn't really seem appealing to go this route right now
though. Thoughts? I am considering if the feature is more appriopriate
as a theme feature.
I have everything working and I am creating patches right now which
will be attached shortly.
Final functionality will be:
If javascript is available and all DOM requirements are met then all
fieldsets will be collapsible (not optional)
If a fieldset has
a class of "collapsed" AND
the fieldset contains no EMPTY form elements that have a class of
"required" AND
the fieldset contains no form elements that have a class of "error"
then the fieldset will be collapsed after the page is completely
loaded.
If a fieldset contains a form element that has a class of either
"required" or "error" then it will ONLY
be collapsible AFTER the user has entered a value in the form
element.
An additional note. Due to a bug in mozilla as of Firefox 1.0,
fieldsets may not be styled as a block element. Specifically, if the
"display" css property of a legend is altered it will render
incorrectly so it may not be changed from inline to block (or none).
This limits how collapsed fieldsets may be styled with respect to
Firefox. This bug has reportedly been fixed since FF 1.0 but due to the
widespread use of that version it is small consolation.
------------------------------------------------------------------------
February 6, 2005 - 05:14 : Dries
1. Make sure the patch applies against CVS. The version is currently
set to 4.5.2.
2. Can we use this to simplify the settings pages?
Right now, some settings live under 'admin/settings' while other
settings live under 'admin/settings/foo' (eg. 'admin/settings/search'
or 'admin/settings/throttle'). This is an artifact of our module
system, so I bet this organization looks pretty random for anyone that
is not familiar with Drupal's inner workings (we got used to it so take
a step back and try looking at it from a user's point of view). I
envision putting all 'simple settings' on one page. Combined with the
collapsible form groups this might be a workable solution. If all
forms are collapsed, you're presented an overview of all simple
settings.
Of course, that won't work for the 'complex settings' (eg.
'admin/settings/forum', 'admin/settings/profiles' and not to mention
the filter settings on 'admin/filters') but nonetheless it might be a
step towards a simpler model.
One showstopping issue is that some of the settings pages (eg.
'admin/settings/search' and 'admin/settings/statistics) use several
form_group() on their own.
(If you haven't already, take a look at Jetbox One (easy to use demo:
http://opensourcecms.com/index.php?option=content&task=view&id=169).)
------------------------------------------------------------------------
February 6, 2005 - 09:10 : tangent
1. Of course.
2. Of course. ;-)
I've only added options to collapse some content creation fieldsets so
far but I'll add options for the settings page as well. Is "General
settings" expanded and all others collapsed a reasonable approach?
------------------------------------------------------------------------
February 6, 2005 - 09:23 : tangent
Note that JetBox uses tables and divs to markup their form layout
(instead of fieldsets). If we go the generic markup route it would make
styling easier (fieldsets are not well supported elements for DHTML) but
would not change the fact that collapsible element support must move
into PHP to resolve the collapse delay issue.
------------------------------------------------------------------------
February 6, 2005 - 09:44 : Bèr Kessels
JetBox uses tables and divs to markup their form layout
(instead of fieldsets). If we go the generic markup route it would make
styling easier (fieldsets are not well supported elements for DHTML)
but
would not change ...
Yes, Taht was the exact reason, I wanted to revive the default Drupal
Boxing/Div-ing nmethod. See Improve theme_box to make it better
themeable and consistent with blocks [1] for more information.
[1] http://drupal.org/node/15332
------------------------------------------------------------------------
February 6, 2005 - 16:38 : Anonymous
Actually, the "html.javascript something { ... }" trick is there to
prevent the annoying flash of style. You set the HTML class immediately
rather than on load (making sure to check if everything is supported).
The CSS cascade then makes the elements collapse immediately.
--
View: http://drupal.org/node/16204
Edit: http://drupal.org/project/comments/add/16204
More information about the drupal-devel
mailing list