Remove the scrolling animation in collapsible.js

IMO we do not need to support beta browsers. FIle a bug at Mozilla, please. I am leaving this open for a few to let the JS mavens have their say.

Please try to reproduce the issue with the latest dev release of Drupal.

I Tested with Minefield/3.0b3pre on Windows Vista with the aforementioned fieldsets and I didn't notice any problems.

I checked out the latest Drupal head.

I tested with Vista, Firefox 3 Beta2, at /admin/content/types/page and /admin/content/types/story, the three, Submission form settings, Workflow settings and Comment settings collapse links work.

Also, /node/add/page and /node/add/story collapse links work as well for me.

Make sure you don't have any plugins installed with FF3 beta. Many plugins don't work correctly with FF3 as of yet.

Unless you can provide more info, I say this bug is closed.

I am having the same issue with another browser. I am using Drupal 6.0 RC 2 and the collapsing does not work at all in Epiphany (on Debian Etch amd64 gecko-1.8 backend). It does seem to work correctly in Iceweasel/Firefox 2, Safari 3 and IE 7.

I tested D6 Head in Vista IE7, Firefox 2.0.0.11, Safari 3.04 and Opera 9.5 Beta, I don't have any issues with the collapse links. If you are having issues with Firefox, please paste output from Firefox's Javascript terminal.

In Epiphany on Debian Etch amd64... upon loading the "Create Page" page I get the following error:

--- snip ---
Javascript error in http://www.simplifiedcomplexity.org/modules/system/defaults.css?m on line 43:
Error in parsing value for property 'display'. Declaration dropped.
--- snip ---

althought this doesn't seem relevant in the least.

When I try to drop down the "Menu settings" collapsible section its starts to open up... it slides down around 20-30 pixels and then stops half way so I can only see half the settings. After this I try to drop down the other sections and they don't work at all.

There are no other errors reported by Epiphany although I am going to poke at the JavaScript for a few minutes right now and will let you know if I find anything. I am grabbing FF 3 as well to if that tells me anything more.

The attached patch (diff) fixes the issue I had in Epiphany so that the collapsible sections work correctly. I retested in all the browsers I have right now (Mac OS X: Safari 3, FF 2, FF 3b2, Opera 9.25, Debian Etch amd64: Epiphany (gecko-1.8 backend), Iceweasel 2, Windows XP: IE 7, Safari 3, FF 2, FF 3b2) and they all work as expected and exactly the same.

The patch essentially removes a small bit of code that had no effect other than breaking collapsing in Epiphany.

Please let me know if this works for you as well.

The patch essentially removes a small bit of code that had no effect other than breaking collapsing in Epiphany.

Did that part of the code not scroll the fieldset into view (in any browser)?

Better title.

The code in question has been present since June 2007. It's odd that Firefox is now showing problems.

http://cvs.drupal.org/viewvc.py/drupal/drupal/misc/collapse.js?r1=1.12&r...

Crossed posts...

I agree that it is odd that the issue is showing up now after so long. Might it have anything to do with an updated jQuery perhaps? I don't know exaclty why it cropped up now but the patch certainly works better for me.

It removes what essentially seems to be an unneccesary duplicate call to:

Drupal.collapseScrollIntoView(this.parentNode);

which is used in a few other places as well. The duplicate call is what was causing the issue in some browsers. Once I removed the duplicate call collapsing worked again as it should (in all browsers that I have available).

title update... again

Note the label.. For "stepping" through the scroll animation when expanding. Without it, it jumps to the top of the fieldset in one step. So, it's there for a reason.

Anyone know which version of the gecko Epiphany uses? It should behave like FF but which version.?

The code in question has been present since June 2007. It's odd that Firefox is now showing problems.

FF 3? Do we care at this point?

It's Epiphany probably based on Gecko 1.7 or 1.8. FF 3b2 is based on Gecko 1.9 beta.

FF3 beta2 also breaks Yahoo mail for me, so I don't have a lot of trust in FF3 until it goes out of beta - probably in 2-3 months from now.

The end result of the animation appears to work exactly the same. Even with the "stepping" removed. It does not jump to the top. I am sure it is there for a reason but it does not work for all browsers. The stepping breaks the animation in my browser.

As I mentioned above Epiphany uses gecko 1.8, and I agree it should function the same as FF 2 but it apparently does not for this case (at least on my version 2.14.3 on Debian Etch amd64). Since this is a stable browser on a stable platform I think it is something that should be supported if possible.

As for FF 3, I understand it is still beta but it will not be for much longer. FF 3 will be stable soon, certainly during Drupal 6's life-cycle and I think that warrents attention for FF 3 issues. People will expect Drupal 6 to work properly in FF 3.

If the fix is this simple... remove three lines of code... without changing the end result I think it is a relevant fix to ensure compatibility among the broadest range of browsers.

Go into the module admin page. Collapse the first fieldset and expand. The scrolling animation only happens when the expansion spreads beyond the window boundaries (view port) to maximize the fields screen real estate.

I can do without it. I think it's superfluous. Animating the fieldset alone is good enough for me. It also gives the impression of it being slightly slower since it's doing two animations at once. Jumping to the top of the fieldset is useful and I have no issues with that.

[edit: Doesn't do it at once actually but I still think it's unnecessary. :) ]

I tested Firefox 3 Beta 2 on MacOS 10.5 Leopard. The collapse links worked for me against the latest D6 head. As mentioned in my earlier post, FF3b2 on Vista also worked correctly for me with D6 head.

Does removing that code fix the original poster's bug report - two links are missing altogether in FF3b2? Or does it fix mcgalvin's Epiphany scrolling collapse bug?

@dvessel: Ah, I see what you mean. I agree it does seem slower with the scrolling. Although my patch removes the scrolling to fix a bug for me it does, as you pointed out, make it faster as well (at least appear to be faster ;) ).

@momendo: It fixes my Epiphany collapse bug. The additional issue of the missing links does not happen to me. I am able to use everything on a content type page as expected.

I don't agree this is critical, but the additional slide effect to bring fieldsets into scope does seem like overkill, I'd never noticed it before, and jumping to it definitely *feels* faster. So if it also saves some bugs in FF3 beta (which people will increasingly use around the same time as D6 is released) and Epiphany, I don't think anyone will miss it.

What about the function to "scroll into view". It's left there. If everyone agrees we don't need the animated scrolling then that whole function should be removed. As is, the patch is incomplete.

I don't think it's critical either.

I don't like what we're proposing. We want to remove the scrolling animation from the collapse field that's been present since D5. That's eye-candy feature removal. So far we have 1 report of a bug from Epiphany, a browser we don't officially support, three reports of the animation/links working fine in FF3b2 (our target browser, albeit beta) and a patch that doesn't fix or have any relation to the original reporter's bug.

Chx was right, this bug should be closed.

Well, the scroll into view still works without the additional animation. But anyway I've just read back up and seen the positive FF3 beta reports. In which case setting this to minor, and active: needs more info since there's not much else can be done without people doing some debugging on the specific platforms/browsers this is an issue on.

momendo, the animations are still there as they exist in Drupal 5. In 6, the scrolling behavior has changed.

http://bitard.net/up/collapsible-scroll-behavior.mov

The first half is what we have now. Second is the change which works similar to Drupal 5. It's subtle and easy to miss.

I think that since the effect is so subtle that it is almost unnoticeable (i didn't even notice the difference when I first removed it) that removing it to fix a browser issue is worth it. In Epiphany, without the patch I cannot use the UI, plain and simple. It is a show stopper for me preventing me from using Epiphany.

While Epiphany may not be "officially" supported a lot of Linux users use Epiphany and as an open source project I think Drupal should support it if possible. This patch make Drupal's UI work in Epiphany without breaking any other already supported browsers.

I think that losing scrolling that is barely noticeable anyway is worth it to support a fairly well known browser.

It's entirely possible that the underlying issue is a jQuery bug, but this patch gets around a (possible) jQuery bug by not using a function that doesn't work in all browsers.

Just my two cents. If the patch does not get approved I will have to carry it around for myself until another solution is found. I don't want to have to do that but have no choice since it is the only way Drupal 6 works in Epiphany.

Neither critical or minor. Breaking in any browser should be prevented especially if the fix is simple but this patch isn't ready.

I'm interested to know if anyone else can confirm that this issue affects their installation of Epiphany.

Ugh, I started to look into this again because the transition to jQuery 1.2.3 was causing problems with Opera with these fieldsets.

It turns out that this behavior has been around for a long time. I tested an older install and I could have sworn it was jumping into view. Checked again on 5.6 and it does indeed scroll into view.

I've fixed this by replacing the following code

    content.slideDown(300, {
      complete: function() {
        // Make sure we open to height auto
        $(this).css('height', 'auto');
        Drupal.collapseScrollIntoView(this.parentNode);
        this.parentNode.animating = false;
      },
      step: function() {
         // Scroll the fieldset into view
        Drupal.collapseScrollIntoView(this.parentNode);
      }
    });

into:

    content.slideDown(300, function() {
        // Make sure we open to height auto
        $(this).css('height', 'auto');
        Drupal.collapseScrollIntoView(this.parentNode);
        this.parentNode.animating = false;
    });
 

Bumping to D7.

Rerolling patch for head. Someone needs to make a decision if this patch needs to go in or won't fix. I say won't fix because it seems to work on all browsers except for 1 unsupported browser. Who knows it may work on that browser now after these many months.

I have this problem occur on Firefox 3.0.3 with Drupal 6.4.
The patch in #32 seems to make me able expand one fieldset before it stops working again. Without the patch, I can't expand menus at all.

Drupal 6.4 works perfectly fine in Safari.

This is the output from the error console:
Error: doc.body is null
Source File: http://drupal6/misc/jquery.js?Z
Line: 13

I am having trouble opening a collapsed fieldset. My browser is Firefox 3.0.1 and my OS is Unbuntu 8.04. This seems like a major issue for the 6.4 release of Drupal. Those patches do nothing for me! I rolled back to older versions on:

collapse.js
/**
* Scroll a given fieldset into view as much as possible.
*/
Drupal.collapseScrollIntoView = function (node) {
var h = self.innerHeight || document.documentElement.clientHeight || $('body')[0].clientHeight || 0;
var offset = self.pageYOffset || document.documentElement.scrollTop || $('body')[0].scrollTop || 0;
var pos = Drupal.absolutePosition(node);
var fudge = 55;
if (pos.y + node.offsetHeight + fudge > h + offset) {
if (node.offsetHeight > h) {
window.scrollTo(0, pos.y);
} else {
window.scrollTo(0, pos.y + node.offsetHeight - h + fudge);
}
}
}

and added a deprecated method to drupal.js

/**
* Retrieves the absolute position of an element on the screen
* @deprecated
*/
Drupal.absolutePosition = function (el) {
var sLeft = 0, sTop = 0;
var isDiv = /^div$/i.test(el.tagName);
if (isDiv && el.scrollLeft) {
sLeft = el.scrollLeft;
}
if (isDiv && el.scrollTop) {
sTop = el.scrollTop;
}
var r = { x: el.offsetLeft - sLeft, y: el.offsetTop - sTop };
if (el.offsetParent) {
var tmp = Drupal.absolutePosition(el.offsetParent);
r.x += tmp.x;
r.y += tmp.y;
}
return r;
};

After some debugging, I've found that this seems the line FF3 can't handle:

var posY = $(node).offset().top;

@Moonray

I just tested 6.4 and FF3.0.3 on WinXP and it works fine. Can you list your OS and what modules you have enabled?

I'm on Mac OS X 10.5
When you mentioned modules, I decided to go with a clean core and found that all of a sudden things were working.

Looks like the offending code is in:
Theme developer 6.x-1.11

We should probably move this issue to the devel module cue, unless others are still having the issue due to other reasons.

Found this: http://drupal.org/node/312375

It doesn't seem to fix the problem, though, just advise not to use theme developer. Something this crippling to drupal should at least have huge big warning.

@mspagnut
Are you still having the issue? Could theme developer be the problem?

I switched off theme developer 6.x-1.11 and Lightbox 6.x-1.8 and collapsible fieldsets worked again.

#313393: input format and similar drop downs menus are not clicking under Firefox 3 !! marked as duplicate

For me it was with Firefox 3.0.3
Solved when i removed the devel module too.

Marking as won't fix as this is a devel issue. This has already been marked as won't fix in devel #306772: devel_themer conflicts with collapsible

i removed theme developer to solve this problem.

I don't know if this is the correct thread, but I was having this issue in ALL browsers and tried everything listed here. Eventually, I ended up just starting to disable modules until I was able to have my fieldsets expandable again. It turned out to be an issue with LightBox2. I still wanted to use this module, so I used the Web Developer toolbar in Firefox to uncollapse the fieldsets by editing the CSS and then set it to not show on Admin pages (admin/*). I didn't have this issue with Lightbox2 in Druapl 6.12 as far as I remember.

Thank you,

James

I experienced the same issue and it tuned out to be caused by the Pirobox tipster module. The collapsibles are working again once the module is disabled.

Having this issue with Mac firefox 3.5.8. Coworkers are reporting the problem on Windows as well. Did not have the problem until last time I upgraded Firefox.

The fieldset legend disappears when the fieldset is collapsed:

<fieldset class='collapsible collapsed'>
	  <legend><span>My Fieldset Title</span></legend>
	  <div class='fieldset-content'>
	  	<div>my fieldset content</div>
	  </div>
</fieldset>

If I remove the <span> tag, then the legend does not disappear when the fieldset is collapsed:

<fieldset class='collapsible collapsed'>
	  <legend>My Fieldset Title</legend>
	  <div class='fieldset-content'>
	  	<div>my fieldset content</div>
	  </div>
</fieldset>

Alternatively, if I replace "My Fieldset Title" with <a href="<script>document.write(window.location.pathname+'#fieldset'); </script>">My Fieldset Title</a>
then the legend does not disappear when the fieldset is collapsed:

<fieldset class="collapsible collapsed">
  <legend><span class="myclass"><a href="<script>document.write(window.location.pathname+'#fieldset');
</script>">My Fieldset Title</a></span></legend>
  <div class="fieldset-content">
    My fieldset content.
  </div> 
</fieldset>

Is this a bug in collapse.js?

Mindy

P.S. The problem I am having does not seem to be browser-dependent. I am experiencing the same behavior in both firefox 3.6 and in IE 8.0.

Reassigning for consideration in 8.x. The sliding animation is still there. I also agree it's not desirable (it's slows Firefox considerably).

@kobnim: no, there is no bug like this in collapsible.js. The bug is probably in another Javascript of your installation and/or some CSS.

I'd like to close this one and go with #1605960: Replace JS animations in core components with CSS transitions. Any objections?

Going the other way around. Let's go with this and contrib can add bells and whistles. That's not helping mobile either.

It looks like there is no animation going on, as it just uses window.scrollTo.

I don't really see any logic behind this behavior, if the user clicked the legend to open the fieldset, I would assume it's already in view, if the fieldset was opened by some other behavior (states?), then it's probably the responsibility of said behavior to do any additional scrolling or whatever. I could imagine that jumping to "the fieldset that just opened" can be rather problematic if something would, for instance, open up all fieldsets on a given page at once.

Personally, this behavior has annoyed me for quite a while now.

Attached patch removes the sliding part altogether.

Happy to be removing code, need UX opinion on that.

adding tags

Tested #53. I agree with @seutje: I can't myself see a good UX reason for this behaviour.

A good page to test in core is admin/modules (download a couple of contrib modules to create more fieldsets). The opening/closing behaviour feels more consistent with the animation removed, but the biggest win is that the link which activates the behaviour doesn't move. This is definitely better for peeping inside fieldsets to see what's there: if you've opened the wrong one you don't have to scroll to find the link to close it again.

Leaving this at Needs Review for more UX opinions.

#53 is good to go :)

This is indeed good to go, please note when a review is given - next time.

it's now a dup of #1685140: Refactor collapse.js