Problem: A panel page created by two tabbed mini-panels using customized views of feedapi. The feed link goes to the right external website in mini-panel view. However in the combined panel view, the feeds went wild and pointed to non-existing internal links, i.e /template#ui-tabs-124. The html source code seems point to the right link path, but the feeds do not behave right.

Answer and Likely Solution Got From googletorp @ Stackoverflow.com:
This problem is purely a js problem. It seems highly likely that it's the javascript you use to generate the tab effect that caused this. With js disabled all the links work just fine. Looking at what the links are being recreated to, it looks very much like the work of jQuery UI tabs, look http://docs.jquery.com/UI/Tabs#option-idPrefix.

* It seems like too many of your links are being targeted by the script, so you either have to fix the script/markup so that only the right links are being made into "tabs".
* Another solution could be to let the js handle redirecting, however that seems like a more difficult solution.

Please help to debug this critical issue. Many thanks in advance

CommentFileSizeAuthor
#4 tabs_ui17.patch1.17 KBjcmarco

Comments

nedjo’s picture

Status: Active » Fixed

There was an incorrect call to .tabs() in the tabs.js file, fixed now.

jcmarco’s picture

Status: Fixed » Needs work

Now with UI Tabs 1.7.3 doesn't work.
It is not adding ui-tabs-panel classes to the div tabs-tabset's.
Still looking it

nedjo’s picture

I don't follow. What new error is raised? How is this caused by the changes I committed yesterday?

jcmarco’s picture

Status: Needs work » Needs review
StatusFileSize
new1.17 KB

Moving the changed js code from beginning to the end, you got to fix the tabs.js to the right ui.tab behavior. Now everything works as a UI Tab and we don't need to force anything as before.

But it moved as well a end() function that in UI 1.7.x stop the execution and don't show the tab.

Other fix is that inn UI Tabs the actions is against the div and not against the ul where was wrongly before.

I found that the active tab class added before, it is not needed as we don't need to force anything.

This patch works fine using the internal ui.tab 1.6 and ui.tab 1.7.3.

nedjo’s picture

Status: Needs review » Needs work

The processing must be of the > ul element, since otherwise all ul elements that contain li elements with links will be treated as tabs. That's exactly the error that was fixed in this issue.

We do need the active class added to retain the current functionality, that the tabs are styled like Drupal local task tabs.

Can you post a patch version that only solves the issue you've identified?

jcmarco’s picture

Status: Needs work » Needs review

Just checking the sample code from jQuery UI Tabs, http://jqueryui.com/demos/tabs/, what I see it is how to create an html structure and where add the tabs. If you see the jquery DOM in the sample there and some Tabs module installation you see that the css classes and DOM are not applied in the same way (I recommend use firebug+firequery to see in the html code the dom and jquery events/actions).

In fact the only way to have it working with 1.7.x is adding the tabs to the div that contain the ul/il and div tabs, and removing the end() function from the wrong place motivated by the code change in last patch.

I don't say that last patch it is wrong it's right and really needed as it brings to light some problems using multiple tabs in the same page, and now help to find this behavior.

I always try to check the sample code using the included jQuery Tabs 1.6 with the jquery_ui/jquery_update modules using jquery 1.3.2 and jQuery UI 1.7.3, and a complex testing site that I have using cck fieldgroup tabs.

Going step by step
Just installed, last dev version works fine with the included jq tabs but not when using jquery ui tabs 1.7.3.
So applying this patch everything works in my local version.

--- tabs.js.orig 2009-09-30 23:50:46.000000000 +0200
+++ tabs.js 2009-10-03 14:46:18.140625000 +0200
@@ -20,7 +20,6 @@ Drupal.behaviors.tabs = function (contex
// Process custom tabs.
var selected = null;
$('.drupal-tabs:not(.tabs-processed)', context)
- .find('> ul')
.tabs({
spinner: Drupal.t('Loading...'),
// Add the 'active' class when showing tabs and remove it from siblings.
@@ -36,7 +35,6 @@ Drupal.behaviors.tabs = function (contex
$(this).addClass(newClass);
})
.after('')
- .end()
.addClass('tabs-processed')
.each(function () {
if ($(this).is('.tabs-navigation')) {

Then I realized that if you are using tabs navigations there are problems as well, it scroll the screen up but doesn't do any action to change any tab, then removing the ul selection do it work as well

--- tabs.js.basic1 2009-10-03 14:58:57.515625000 +0200
+++ tabs.js 2009-10-03 14:59:32.531250000 +0200
@@ -47,7 +47,7 @@ Drupal.behaviors.tabs = function (contex
Drupal.tabs.tabsNavigation = function(elt) {
// Extract tabset name.
var tabsetName = $(elt).get(0).id.substring(5);
- var $tabs = $('> ul', elt);
+ var $tabs = $(elt);
var i = 1;
var $tabsContent = $('div.' + 'tabs-' + tabsetName, elt);
var count = $tabsContent.size();

Then I saw that there is incoherence between the:

$(this).addClass('error').addClass('ui-tabs-selected');

from the errors tab selection and the active class.

And chekcing jquery tabs then you see that the ui-tabs-selected is overlapped with the active added class.
If you want to leave this extra class for compatibility is nice but the really one needed to change tabs and detect status is ui-tabs-selected
In my patch I didn't removed the class tabs, but "tabs" and "active" probably are redundant classes if you can use the jquery ui tabs classes, but I understand that could need a new version as it affects to existing tab module theming. But if you plan to use jquery ui themes then the only one really needed and added by the jquery tabs by itself are ui-tabs and ui-tabs-selected, as you can see from the jquery ui basic theme css:

/* Tabs
----------------------------------*/
.ui-tabs { padding: .2em; zoom: 1; }
.ui-tabs .ui-tabs-nav { list-style: none; position: relative; padding: .2em .2em 0; }
.ui-tabs .ui-tabs-nav li { position: relative; float: left; border-bottom-width: 0 !important; margin: 0 .2em -1px 0; padding: 0; }
.ui-tabs .ui-tabs-nav li a { float: left; text-decoration: none; padding: .5em 1em; }
.ui-tabs .ui-tabs-nav li.ui-tabs-selected { padding-bottom: 1px; border-bottom-width: 0; }
.ui-tabs .ui-tabs-nav li.ui-tabs-selected a, .ui-tabs .ui-tabs-nav li.ui-state-disabled a, .ui-tabs .ui-tabs-nav li.ui-state-processing a { cursor: text; }
.ui-tabs .ui-tabs-nav li a, .ui-tabs.ui-tabs-collapsible .ui-tabs-nav li.ui-tabs-selected a { cursor: pointer; } /* first selector in group seems obsolete, but required to overcome bug in Opera applying cursor: text overall if defined elsewhere... */
.ui-tabs .ui-tabs-panel { padding: 1em 1.4em; display: block; border-width: 0; background: none; }
.ui-tabs .ui-tabs-hide { display: none !important; }

This is the explanation for this patch. It is possible that I couldn't test it with a plain basic clean garland installation but the test servers I use, have enough code already tested with other ones that helps me a lot to make different use cases for some modules and user interfaces.
Probably from here we could help to improve this great module in the actual path to evolution it to be fully compatible with jQuery UI, as this is the one coming in the D7.

nedjo’s picture

Priority: Critical » Normal

Thanks for the explanation. This is great preparation for the D7 upgrade.

For now, the module needs to work with jQuery UI 1.6 and without a jQuery UI theme. I'd also like it to be compatible with 1.7.

As I explained, the current patch you've uploaded creates two bugs for the 6.x version of tabs module, one of which will also be a bug even with jQuery UI 1.7, so it's not ready to apply. But I would like to get a patch applied that prepares us for 1.7 and works with the current module.

nedjo’s picture

Status: Needs review » Needs work