Search page tabs not highlighting

The primary menu class="active" seems to have been dropped between 5.x and 6.x. Since this seems to be a change from 5.x to 6.x, I'm leaving it as a 6.x issue.

I haven't debugged what's going on. I don't know if this is a problem in menu or in search's use of menu.

I'm experiencing this issue, as well. My initial guess is that it's a bug in the menu module, bug I can't be sure.

Actually - looks like it is an issue with search module's implementation of the menu. It defines paths for /search/[module]/[search string] but not for just /search/[module]. The attached patch, which is likely larger than it needs to be, fixes this issue. I don't quite understand the menu implementation well enough to cut it down beyond what the patch contains.

This patch is confirmed to work with 6.10

@jrbeeman: good catch! This one bugs me a lot ;)

Bumping to D7. We fix bugs first in the most current development version, then backport in other affected versions. That allows us to avoid regressions.

I believe the patch works partially on 6.12: "Content" and "Users" tabs are correctly highlighted after clicking on them, but none is highlighted, as per default, when first entering the search page.

OK. Just to clarify, after you do a search, the tabs are highlighted. The problem is that when you initially go to the Search page, no tab is highlighted, even though if you actually do the search, it will be searching Content.

It is still broken that way in Drupal 7 as well as Drupal 6.

Changing the status back to "active" since we normally use "to be ported" when we are porting back to previous version, and because the patch above apparently didn't fix the issue.

There are a couple of things going on here...

- The search module is currently defining path 'search', as well as tabs for paths 'search/module_path/%menu_tail' for each module that implements hook_search_info().
- Path 'search/module_path' without the menu tail is not being defined.
- None of the local tabs is designated as the default local task.

These factors are all working together to create the bug.
- If you go to path 'search', the search tabs are shown, but none is recognized as a default task.
- In the current D7, if you click on either Content or Users without entering search terms, that tab isn't highlighted, because the path doesn't have the %menu_tail on it.

I tried taking %menu_tail off the paths in search_menu(), but that screwed up other behavior (the search terms weren't kept when you switched tabs).

So I am not exactly sure how to resolve this issue...

Why wouldnt we create a new tabl, something like "Global" or "All" and that is the tab when you land on the search page without clicking any tab.

Because there is no way in the current search module to search "all". You can only search an individual tab. See
#252211: Override creation of separate tab for other modules' invocation of hook_search
for feature request.
[EDIT: Removed editorial comment about the feature request]

merlinofchaos thinks maybe this can be solved via menu_set_active_trail() or menu_set_active() or something like that. I'll give it a try...

jhodgdon: i didn't really mean that the search, searches all. It's more for the user. Maybe this will be more clear, just creat a tab with the term "Search" in it when you get on the search page.

What would that tab do?

Hmmm...

Regarding #11, I tried the following, in function search_view() in search.pages.inc:

a)

  $trail_item = menu_get_item('search/' . $type . '/%menu_tail');
  menu_get_item(NULL, $trail_item);

This did highlight the tab when you first went to search, but then when you clicked around on tabs, it put in %menu_tail as the search term, so that was no good.

b) So then I tried this hack, which I hoped would set the active trail and then set the menu item back:

  $real_item = menu_get_item();
  $trail_item = menu_get_item('search/' . $type . '/%menu_tail');
  menu_get_item(NULL, $trail_item);
  drupal_static_reset('menu_set_active_trail');
  $trail = menu_set_active_trail();
  menu_get_item(NULL, $real_item);
  menu_set_active_trail($trail);

But this didn't end up highlighting the tabs at all.

I also tried adding in a line

  $trail[] = $real_item;

before doing menu_set_active_trail(), but that didn't seem to be any good either (probably not the right thing to add).

I think I'll punt on this for the moment anyway...

jhodgdon: that tab would just be active on the first visit on the search page. when you click the tab you get send to /search

I don't think that's a good idea. When you first go to Search, you are implicitly doing search/node (i.e. the Content tab), assuming the node module's search is turned on. So we don't want to have another tab there that says "Search" that is the same as "Content". That would be utterly confusing.

I agree that it would be confusing, but if nothing else works and you still want a active tab...

I believe I've found a way to fix this. In CTools, I'm using hook_menu_alter to take over the search page, and one of the effects was that I had to add multiple entries for search: search/$name and search/$name/%menu_tail -- the reason for this is that CTools can take over each search page individually, but they are all actually controlled by 'search' if they have no keywords.

What I found was that tabs were not consistently showing up if I had keywords or if I didn't. I finally realized that using tab_parent and tab_root, I could make two sets of tabs that would consistently work.

My proposal is that we do the same thing CTools is doing. We *add* search/$name as a menu entry, alongside search/$name/$menu_tail and that we properly use menu system arguments while doing so. We use tab_parent and/or tab_root (this is the confusing part; I'm not sure if I should be just using one or the other. I used both to be absolutely certain the tabs would show up in the right place).

All entries for search/$name set their tab_root to 'search' and these are what will show up when you go to just 'search'. We also transform search/node to MENU_DEFAULT_LOCAL_TASK. We set all entries for search/$name/%menu_tail to tab_root to search/%node/%menu_tail. This should properly preserve all tab highlighting, keywords will still be preserved (so long as %menu_tail is still working more or less as it did in D6) and the two sets of tabs continue to work properly.

The menu_alter I'm using in CTools D6 can be found in this file: http://drupalcode.org/viewvc/drupal/contributions/modules/ctools/page_ma... -- it's a little complex so that it can handle the overrides as well. For the purposes of understanding what it needs to do for this, assume that variable_get('page_manager_search_disabled_' . $name, TRUE) always counts as TRUE, and the 'continue' in that if clause skips the rest of the loop. Those are the bits that matter here.

I tried an approach like this in D7 in the search module. I couldn't get it to work...

Here's what I did:
a) Set up menu items for search/module as well as search/module/%menu_tail
b) Made them all MENU_LOCAL_TASK except the default module (normally 'node'), which is MENU_DEFAULT_LOCAL_TASK
c) Gave the search/module ones a tab_root and tab_parent of 'search'.
d) Gave the search/module/%menu_tail ones a tab root/parent of 'search/' . $default_module . '/%menu_tail'

This didn't work. I got errors in function menu_local_tasks() saying that $tasks didn't have an element for 'search/node/%menu_tail'. When I looked into it, I found that the keys (paths) it retrieved had %menu_tail changed to just % in the router item database.

So I changed (d) to just be 'search/' . $default_module . '/%' . This didn't work either -- I ran into an infinite loop in menu_local_tasks() where it does

  for ($p = $item['tab_parent']; $tasks[$p]['type'] == MENU_DEFAULT_LOCAL_TASK; $p = $tasks[$p]['tab_parent']);

because search/node/% is its own parent item.

I also tried setting the tab_parent of search/node/% to be search or search/node (without changing the tab_root) but that didn't work because only items with the same tab_root are retrieved in the query in menu_local_tasks(), so it couldn't find the item for search/node that it needed in that for loop. And you can't change the tab root, because the query only gets items with the same tab root, so all tabs at the same tab level must have the same tab root.

So I'm confused about what to do...

Here's some code from the latest failed attempt (in search_menu()):

  // Make menu entries for each module.
  foreach($modules as $module) {
    if (isset($search_hooks[$module])) {
      $path = $search_hooks[$module]['path'];
      $items['search/' . $path] = array(
        'title' => $search_hooks[$module]['title'],
        'page callback' => 'search_view',
        'page arguments' => array($module),
        'access callback' => '_search_menu_access',
        'access arguments' => array($module),
        'type' => MENU_LOCAL_TASK,
        'file' => 'search.pages.inc',
        'tab_parent' => 'search',
        'tab_root' => 'search',
      );

      if ($module == $default_module) {
        $items['search/' . $path]['type'] = MENU_DEFAULT_LOCAL_TASK;
        $items['search/' . $path]['weight'] = -10;
      }
        
      $items['search/' . $path . '/%menu_tail'] = $items['search/' . $path];
      $items['search/' . $path . '/%menu_tail']['tab_parent'] = 'search/' . $default_module . '/%';
      $items['search/' . $path . '/%menu_tail']['tab_root'] = 'search/' . $default_module . '/%';

      if ($module == $default_module) {
        $items['search/' . $path . '/%menu_tail']['tab_parent'] = 'search/' . $default_module;

      }

Try this patch, adapted from jhodgdon's code. It certainly needs some comments to explain itself, but my initial testing shows that this works.

Works for me! I will do a bit of cleanup (for instance you can't assume the node module is the default, because it might not be turned on).

Here's a new patch...

It also fixes #567100: Search module is assuming node is default search even when it's disabled, because I needed the logic in that issue's patch to determine the default module (in case node is turned off), and some of the other stuff in that patch was affected by this one.

uh oh. That patch fails in one case: if you don't enter keywords and click "submit", you get two rows of tabs, at least with the Node module turned off and another search module turned on in the search settings page. See screen shot.

Sigh.

Actually, it does the same with the default config of just having node and user modules turned on for searching.

And you can do the same thing by just navigating to search/node

I am not sure why it is showing up that way.

And by the way, here's a patch that slightly simplifies the above patch, in that it removes some gotos that are not necessary. But it still suffers from this problem. Double sigh.

I also verified that merlinofchaos's patch from #21 suffers from the same problem: If you navigate to search/node, you get an extra row of tabs similar to the screen shot in #24

I can find a combination that fixes that problem. We could sidestep the issue by adding a drupal_goto() in search_vew() if 1) there are no keys and 2) the current search is the default search. This patch does this, though it feels cheesy.

er, can't find a combination that will fix that problem in the menu directly.

As near as I can tell the issue is that because its the DEFAULT local task, menu is making all kinds of exciting assumptions that aren't true in our little Strange case.

There was a goto in search_view() before, so putting one back in this patch will not be any cheesier than before. :)

I'll test this patch out a bit later...

This fixes all the issues I know about. Let's get this in. Thanks merlinofchaos for figuring this out!

This looks a bit more convoluted than preferred but from what I can tell, it is the proper thing to do. I'm going to wait for a '+1' from a menu system maintainer just to make sure.

chx is working on a new patch, which will clean this patch up, and NOT fix #567100: Search module is assuming node is default search even when it's disabled at the same time - he wants these to be kept separate.

Rerolled, added comments, a bit cleaner.

Sigh. The room was closing down and i was in a hurry.

last patch has a php error, that I fixed, plus I tried to make the logic in search_menu a little tighter

Previous patch ended in an infinite loop, and that's why tests failed, hopefully this fixes it.

@jhodgdon @chx this is basically chx's last past, with a few tweaks by me. I'm ready to mark RTBC, but would like to get a +1 from one of you two first.

Works for me!

#40: 245103.patch queued for re-testing.

Bump... If the test bot agrees, I'd really like to get this in, as it's blocking work on another issue:
#567100: Search module is assuming node is default search even when it's disabled

Actually, I just noticed that the comment above the redirect in search.pages.inc is incorrect now. search/node *is* the default tab.

Here's the above patch with a better comment.

I applied the patch and functionally tested the patch by confirming that tabs are highlighted on search. The patch did have an offset, but applied correctly.

Should there be a comment in the first hunk explaining the change/process? Or is that not necessary?
Also is it okay to not have a default tab if node is disabled?

If not, then I think it's ready to go back to RTBC.

We have a separate issue on what to do when node module search is disabled. As soon as this patch goes in, I'll get back to that one. See #45 above and other comments about not mixing this with that other issue.

Agreed on the need for a comment in hook_menu().

Here's the patch above, rerolled and with a comment.

Comment is really informative. :-) I'm going to set to RTBC since it's been through the approval process above.

That workaround strikes me as "ugh" but I can't really see any other way around it. We talked about this in the hacker lounge as well, and I know at least chx was in agreement, as well as sun on IRC just now.

The comments look good for people who come across this code and go "WTF?" and it's nice to have this fixed cos it's a SUPER annoying UI glitch.

Committed to HEAD! Great work, all!

I'm guessing this will need some massaging to apply to D6.

I wanted this fixed in v6, attached is a somewhat quick and dirty patch against 6.15. I only commented out the replaced lines which makes it a bit ugly to read, sorry about that. This patch seems to work, only thing it doesn't do is retain your search string when you flip tabs.

Just to prove what a nub I am at this, that patch was backwards. Attached is a right-way-round patch.

Thanks... that's a good start. Patch will need to be cleaned up and tested...

This actually needs to be reopened for Drupal 7.

No one noticed that the page title wasn't working when you search. Someone just reported this:
#812990: Search page title changes to Home

I've closed that issue as a duplicate and am reopening this one for Drupal 7 for a follow-up patch. I think we just need to do a drupal_set_title() in the page callback function.

subscribe.

Here's a follow-up patch to fix the issue mentioned in #54 and #812990: Search page title changes to Home. I also added a few lines to the search text test to test the page titles, so this will not come back to haunt us again.

Thanks to Jeff Burnz for identifying where the problem was occurring - saved a huge amount of debugging time.

Pardon and thank you. can someone direct me to the solution for D6 - Search page title changes to home? My version of search.pages.inc doesn't match to patch. And while I've been working with Drupal for a few years now patches always feel like bandaids. Will there be a new a search module that I can just upload and update?

$Id: search.pages.inc,v 1.4 2007/12/06 09:51:01 goba Exp $

Ummm... I don't think this happens in Drupal 6, at least without a port of the patches above. Which version of Drupal 6 are you seeing this happening in?

As far as patches vs. a new search module, patches are how we work around here... You can also grab the latest version of a module file from CVS, or wait until a formal release comes out.

hello. I'm not knocking patches. I'm just tired .. hoping anything anything would be easy. I'm arms deep in views and civicrm headaches so these "little" things are thorns in my you know what.

I'm running 6.16 - it's happening for me:
http://www.ctndev.com/search/node/computer

I see there is a new recommended upgrade so I'll run that -- but I didn't see any note of the bug in the release notes.

Well, I will have to test that. I didn't think that issue was present in Drupal 6.... If it is a Drupal 6 bug, it isn't likely it's fixed in Drupal 6.17, since it was just reported against Drupal 7 a day or two ago, and no one was aware of it in Drupal 6.

But just to make sure... are you running any search-related contributed modules on your Drupal 6 site?

And it is unlikely the above patches will apply directly to Drupal 6, sorry. It will be a while until someone makes a patch for D6, because first we have to get D7 fixed. Which means someone needs to review the patch on #57, and if it's OK then it can be committed, and *then* we can work on the D6 patch. So if you can get someone motivated to review that patch (hint hint), it will go faster.

OK. I just updated to the latest Drupal 6.x-dev (which is probably almost exactly the same as Drupal 6.17 that just came out), and I cannot see this problem of the page title being wrong.

thanks jhodgdon! Yes, Faceted Search. Was looking for a snazzy way to have pseudo ajax taxonomy sort.
I'll try the upgrade and let you now if that works for me. msjones

Well, if you had added the patch in #52 to your Drupal 6 installation, I can see why it might have screwed up the page title in the same way that the Drupal 7 patch above did to Drupal 7. I don't think Faceted Search would have any affect on your core search page (URL .../search/node). Anyway. So I expect that if you upgrade to Drupal 6.17, you'll have the original problem mentioned here (search tabs sometimes not highlighting). And I wouldn't advise trying to patch it unless you are willing to live with the consequences (page title screwed up).

We'll eventually (hopefully) get this fixed in Drupal 6...

Just a note:
http://drupal.org/node/812990#comment-3048236
"moreorless" noted that they had this problem in Drupal 6, but it was a Chaos Tools Page Manager bug, not Drupal core (see comment there for details).

#57: 245103-fixactivetrail.patch queued for re-testing.

#57: 245103-fixactivetrail.patch queued for re-testing.

Looks like this patch needs a re-roll. Probably should wait for #567100: Search module is assuming node is default search even when it's disabled

This is too confusing. I am going to reopen #812990: Search page title changes to Home, which describes what the current issue is, and mark this one back to fixed.