Centered Themes that make use of the following trick to turn on the browser scrollbars by default have troubles with panels modal popup. Everytime the modal popup is shown the scrollbar goes away and if the modal popup is closed the scrollbar came back.

html {
  height:100%;
  margin-bottom:1px;
}

Not sure what this is caused by. Are you altering one of the above values anywhere?

CommentFileSizeAuthor
#24 D5_panels_mc_upgrade3.patch10.77 KBhass
#16 D5_panels_mc_011.patch10.77 KBhass
#15 D5_panels_mc.patch6.77 KBhass
#13 mc.js_.txt6.99 KBhass
#10 blankspace1.png223.66 KBsdboyer
#10 blankspace2.png79.66 KBsdboyer
#5 panels_D5_2x_scrollbars_bug.patch789 byteshass

Comments

hass’s picture

hass commented

I've found it in panels\js\lib\mc.js

  // hide the scrollbars
  $('body').css('overflow', 'hidden');

No idea why this stupid thing is done. Removing this line fixes the jumping.

merlinofchaos’s picture

merlinofchaos commented

It's modal. It's supposed to be hiding the background anyway, so why does it matter if the scrollbars go away?

hass’s picture

hass commented

The complete page is jumping 10-15px to the right and when the popup is closed back to the left. It's flickering and I don't like flicker effects like this... this is an "accessibility" issue for sure. The theme is Section 508 and WAI priority 1, 2, 3 compliant... with panels 2.x and this issue this will break :-)

sdboyer’s picture

sdboyer commented

@hass: my feeling is that this is one of those things where it'd go a _lot_ faster if you submitted a patch that either took care of this, or at least pointed us in the right direction. Given that an 'admin theme' can be enabled, and that would take care of this problem under most normal circumstances (albeit _very_ inelegantly), I can't justify spending time on tracking this down right now.

hass’s picture

hass commented
Status: Active » Needs review
StatusFileSize
new panels_D5_2x_scrollbars_bug.patch789 bytes

Here is the patch.

sdboyer’s picture

sdboyer commented
Status: Needs review » Closed (won't fix)

So I'm waffling on this. The patch works as described, but because of the way the modal itself works you tend to get a whole bunch of extra blank vertical space at the bottom, and even some blank horizontal space on the right. hass, I get that you don't like seeing things fidget back and forth by a few px. In principle neither do I, although tbh I never noticed it until you pointed it out. However, I don't like superfluous scrollbars that take you way off into empty space far more than I dislike the jittery switch of making the scrollbars appear/disappear.

So, sorry, but I'm going to won't fix this. If you can point me at some accessibility/508/WAI documentation that specifically indicates how we're breaking some guidelines by doing this, I'll happily reconsider. However, I suspect that the big blank space would also be an accessibility concern (if the jitteriness is), so this patch would have to grow in order to take care of that problem as well before I'd commit it.

sdboyer’s picture

sdboyer commented

So I'm waffling on this. The patch works as described, but because of the way the modal itself works you tend to get a whole bunch of extra blank vertical space at the bottom, and even some blank horizontal space on the right. hass, I get that you don't like seeing things fidget back and forth by a few px. In principle neither do I, although tbh I never noticed it until you pointed it out. However, I don't like superfluous scrollbars that take you way off into empty space far more than I dislike the jittery switch of making the scrollbars appear/disappear.

So, sorry, but I'm going to won't fix this. If you can point me at some accessibility/508/WAI documentation that specifically indicates how we're breaking some guidelines by doing this, I'll happily reconsider. However, I suspect that the big blank space would also be an accessibility concern (if the jitteriness is), so this patch would have to grow in order to take care of that problem as well before I'd commit it.

sdboyer’s picture

sdboyer commented

gah, sorry for the double post

hass’s picture

hass commented
Status: Closed (won't fix) » Needs review

If you have theme that centers the page layout the *whole* page is jumping ~10px left to right if you turn off and on the scrollbars. This is very annoying and makes my eyes jittering and the page "flickers". Flicker effects are not WAI friendly. It's like "blink" "marque" and so on... and I don't like to provide a panels fork to my users only because of a buggy javascript line where someone missed something that MANY themes and sites are doing. You might not be aware about this... but if such a scrollbar fix/workaround wouldn't be in place you will understand how annoying this is. It is good user experience if a site is not jumping and usability is an important issue. Here is the docs part of "Jumping centered layouts in Firefox & Safari" at http://www.yaml.de/en/documentation/css-components/base-stylesheet.html and why we have the scrollbars. Not sure if we have a demo page for this bad behaviour... but we have for sure good reasons nevertheless you may think that turning on scrollbars is superfluous.

I will also try to contact the maintainer of this buggy JS script to fix this bug. I'm sure he/she is not aware about this scrollbar part.

Do you have an screen shot about this "extra blank vertical space at the bottom"? I'm not sure what you are talking about...

sdboyer’s picture

sdboyer commented
Status: Needs review » Needs work
StatusFileSize
new blankspace2.png79.66 KB
new blankspace1.png223.66 KB

Screenshots are attached.

I am aware of this - Garland is a centered theme, and it does do the jump. The jump simply doesn't bother me, to the point where I cannot even remember ever having noticed it until you brought it up. Being that that link isn't to an actual usability standard - just a best practice implemented by one school of thought - this is still really a matter of opinion. So I'm marking this CNW, and it'll stay that way until there's a patch that fixes both the jump and the extra blank space.

Also, this kind of talk:

It's like "blink" "marque" and so on... and I don't like to provide a panels fork to my users only because of a buggy javascript line where someone missed something that MANY themes and sites are doing.

really isn't necessary. Bringing out the word 'fork' just comes across like a threat, and threats just make me angry - and therefore, less able to give your request fair, level-headed consideration.

hass’s picture

hass commented

Looks like I need to repro this myself as I do not see where a "blankspace" is created by the modal window. No idea why this should ever happen except there is another bug in the JS code.

sdboyer’s picture

sdboyer commented

I don't know how those screenshots could be any clearer. I've scrolled the viewport down in that screenshot to show the extra vertical space that is created, but left plenty of the normal editing interface visible so it's clear just how much unnecessary extra vertical space had been added to the bottom of the page.

hass’s picture

hass commented
StatusFileSize
new mc.js_.txt6.99 KB

Looks like the code have changed. See http://www.nabble.com/modalContent-plugin-is-not-modal-td7898188s27240.h...

3) I removed the code that disables and reenables the scrollbars. This is just a suggestion really, realizing it could be one of those little things that causes fellow developers to become unnecessary foes ;=). Personally, I found it an annoying behaviour to remove a visible scrollbar causing the page elements to shift right upon modal popup, then back after closure. If the intent was to prevent scrolling it doesn't anyway since you can still page down with the keyboard. Maybe it should be a boolean option passed in the function signature?

I wasn't able to find the website for downloading the latest version... but here is the one from the forum linked above.

hass’s picture

hass commented

Today I received feedback from the developer of this mc.js. He wrote me that they stopped development of it quite a while ago. We should move over to jqModal and jQuery UI.

hass’s picture

hass commented
Version: 5.x-2.0-beta5 » 5.x-2.x-dev
Status: Needs work » Needs review
StatusFileSize
new D5_panels_mc.patch6.77 KB

Made a patch from attached code in #13.

hass’s picture

hass commented
StatusFileSize
new D5_panels_mc_011.patch10.77 KB

Here is the very latest version from http://jquery.glyphix.com/jquery-modalContent/jquery-modalContent.js. Not yet tested.

merlinofchaos’s picture

merlinofchaos commented

This patch reverts some updates we made to make the plugin compatible with later jquery plugins, where methods like top() and left() no longer exist.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Needs work
hass’s picture

hass commented

I wasn't aware about this. Another good reason to drop this buggy lib.

merlinofchaos’s picture

merlinofchaos commented
Status: Needs work » Closed (won't fix)

I don't care about this issue. Since you've chosen to approach it with combativeness, I've chosen to won't fix it. Please feel free to go fork panels if you like. It is not important enough of an issue for you to take the hardnose stance you're taking. If it were a crash bug, maybe, but it's a tiny visual error and you are making it out like it's the grand canyon. On my list of priorities it's somewhere right below "Visit Cambodia."

hass’s picture

hass commented
Status: Closed (won't fix) » Needs work

Merlin, we only need to use the latest version of the library and the issue I reported is solved. The maintainer fixed this bug 2 years ago. What is the problem with using the latest version?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Closed (won't fix)

@hass: Upgrading to a new library is not that easy. Remember that you are not the one who gets the support requests. If it works for you, then fine, upgrade. I agree with Earl that this very minor issue does not justify a library upgrade.

hass’s picture

hass commented
Status: Closed (won't fix) » Needs work

Sun: I only said to upgrade the mc.js library to a newer version and less buggier version. Earl said he don't like to upgrade to a different library like jqueryUI. This is ok, but the newer version of mc.js contains less bugs and should therefore upgraded. It is only a newer version. Nothing else. The bug found here seems not the only bug if I look inside the patch diff.

hass’s picture

hass commented
Status: Needs work » Needs review
StatusFileSize
new D5_panels_mc_upgrade3.patch10.77 KB

New patch that does not revert the top() and left() changes for later jquery versions as mentioned in #17.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Needs work

This patch just updates the library, but several function arguments in this library have changed. Did you ensure that all invocations in Panels' JavaScript are still valid?

hass’s picture

hass commented
Status: Needs work » Needs review

Are you talking about this?

- * @param animation (fade, slide, show)
+ * @param animation (fadeIn, slideDown, show)

- * @param animation (fade, slide, show)
+ * @param animation (fadeOut, slideUp, show)

This doesn't change anything... please provide an example of function arguments that have changed. I cannot see any changes in arguments. "animation" seems not used in panels.

esmerel’s picture

esmerel commented
Status: Needs review » Closed (won't fix)

No fixed to be committed to the 2.x line

hass’s picture

hass commented
Project: Panels » Chaos Tool Suite (ctools)
Version: 5.x-2.x-dev » 6.x-1.x-dev

This need to be moved to ctools and the issue should still be there as the JS library hasn't been replaced by a reliable one... moving now.