Hi guys,

I wanted to report a suspicious behaviour I encountered recently on the latest dev version [7.x-3.5+21-dev (2012-10-31)].

In any views with page display, with a required path configured, for example: node/%/custom_name

Under Page Settings > Menu:
(To display the Page: Menu item entry dialog form)

Select Menu tab for the menu item Type.
[This should set the menu item to be displayed as a menu local task]

In this particular form, it is possible to select to:
>Displays the link in contextual links

by marking as checked the Context checkbox (that is only displayed in this particular case).
See attached screenshot: views-page-display-menu-tab-overriden-contextual-link-0.jpg

Currently, it seems that if this setting is selected/checked, the menu local task/tab doesn't display anymore and the menu link is only displayed in contextual links (which prevents from displaying both: menu local task and contextual link).

This particular behavior comes from the code at:
/plugins/views_plugin_display_page.inc, line 156:
$items[$path]['context'] = MENU_CONTEXT_INLINE;

which seems to override the defaults from line 146:
$items[$path]['type'] = MENU_LOCAL_TASK;
since the context property is not set.

After checking what the node.module does, I was brought to:
node.module, lines 2027, 2028 [drupal-7.16]

    'type' => MENU_LOCAL_TASK,</strong>
    'context' => MENU_CONTEXT_PAGE | MENU_CONTEXT_INLINE,

which led me to think that we would perhaps also need to add a menu page context in order to display the Tab menu item (menu local task):
MENU_CONTEXT_PAGE

So at this point I would like to suggest the following options to try to approach this issue[feature request]:

1 - Allow user to select whether to display both: menu tab (local tasks) and contextual link.
Add another setting, such as a checkbox, to allow users to select whether to hide or display the menu tab, which would allow user to select any of the following behaviors:

  • menu local task overriden by contextual link: both checkbox checked
  • menu local task and contextual link: Hide menu tab unchecked

a. Addition of a new form setting:
/plugins/views_plugin_display_page.inc, line 391:
[setting variable name: contextpage]

        $form['menu']['contextpage'] = array(
          '#title' => t('Hide menu tab'),
          '#suffix' => '</div>',
          '#type' => 'checkbox',
          '#default_value' => !empty($menu['contextpage']),
          '#description' => t('Only display menu item entry in contextual links. Menu tab should not be displayed.'),
          '#dependency' => array(
            'radio:menu[type]' => array('tab'),
            'edit-menu-context' => array(1),
          ),
          '#dependency_count' => 2,
        );


b. Update of the logic:
/plugins/views_plugin_display_page.inc, line 156:

$items[$path]['context'] = (empty($menu['contextpage']) && MENU_CONTEXT_PAGE) | MENU_CONTEXT_INLINE;

when $menu['contextpage'] is not selected, the menu tab and the contextual link can be displayed at the same time.
If the checkbox is marked as checked (selected) then the menu tab is overriden by the contextual link (menu tab is hidden).


Please find attached a patch to sum up these changes: views-page-display-menu-tab-overriden-contextual-link-0.patch

2 - Modify current behavior to display both by default [menu tab (local tasks) and contextual link]
I haven't followed much of the discussions related with the current implementation for menu tab/contextual link($menu['context'], /plugins/views_plugin_display_page.inc, line 155), but perhaps allowing users to display both as default could be a better alternative than preventing menu tab from displaying by default:

/plugins/views_plugin_display_page.inc, line 156:
$items[$path]['context'] = MENU_CONTEXT_PAGE | MENU_CONTEXT_INLINE;

As a minor inconvenience, in this case, menu tab could be considered to be hidden with CSS.

I guess there would be other approaches to respond to this issue, so I would certainly be glad to hear your feedback on this.

Could you please let me know if I overlooked or missed anything in the current views module (and its API), in terms of whether this (displaying menu tab along with contextual link) would be doable already with existing hooks/settings?
But for this particular use case, I haven't been able to find any appropriate existing solution/answer.

Please let me know if you would have any questions on any points/code/aspects mentioned in this ticket, I would surely be glad to provide more information.

I would greatly appreciate if you could take a bit of time to take a look at the patch and give me your feedback on this feature request.

Feel free to let me know if you would have any questions about the code or any other aspect of the patch, I would be glad to explain in more details.

Thanks in advance.

CommentFileSizeAuthor
#10 drupal-1831094-10.patch2.13 KBdawehner
#10 interdiff.txt736 bytesdawehner
#9 views-page-display-menu-tab-overriden-contextual-link-1831094-9.patch2.2 KBDYdave
#5 views-page-display-menu-tab-overriden-contextual-link-1831094-5.patch2.18 KBDYdave
views-page-display-menu-tab-overriden-contextual-link-0.patch1.82 KBDYdave
views-page-display-menu-tab-overriden-contextual-link-0.jpg102.22 KBDYdave
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

DYdave’s picture

DYdave CreditAttribution: DYdave commented
Status: Active » Needs review
Issue tags: +Contextual links, +views displays, +menu tab

Changing status for review.

jonhattan’s picture

jonhattan
Primary language Spanish
Location Plasencia
CreditAttribution: jonhattan commented
Status: Needs review » Reviewed & tested by the community

The "context" checkbox was added in #1128088: Provide a way to create contextual links from local task menu items. It seems to me it was not intentional to hide the menu tab. Indeed hidding the menu tab is baffling since you're asking views to create a menu tab after all.

DYdave's patch works great!

DYdave’s picture

DYdave CreditAttribution: DYdave commented

Hi jonhattan,

Thanks you so much for getting back to me on this with a very positive feedback.

I had assumed this feature request fell in the abyss of the Views tracker and no one would actually notice this to give it some testing and a review.

I would greatly appreciate if this could get committed, as I would also agree with you that it would further refine the work that was done around contextual links at #1128088: Provide a way to create contextual links from local task menu items.

Feel free to let me know if you would have any further questions, comments, issues, concerns, suggestions, recommendations or objections on this feature request in general, I would be glad to provide more information or explain in further details.

Any further comments, feedback, testing, reviewing and reporting, would be highly appreciated.
Thanks again to all in advance for your support, comments, replies and feedback.
Cheers!

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Reviewed & tested by the community » Needs work 
+++ b/plugins/views_plugin_display_page.incundefined
@@ -383,12 +383,23 @@ class views_plugin_display_page extends views_plugin_display {
+        $form['menu']['contextpage'] = array(

Every option available should be documented as part of views_plugin_display_page::option_definition() which is missing here.

DYdave’s picture

DYdave CreditAttribution: DYdave commented
Status: Needs work » Needs review
FileSize
Revised patch based on initial version to add default value in views_plugin_display_page::option_definition().2.18 KB

Hi guys,

I'm so grateful I could get your attention on this and certainly wouldn't want to let this stall any longer.

Thanks a lot @dawehner for your feedback, review and clear answer.

Please find attached to this comment the updated patch based on the initial suggested version, against views-7.x-3.x at 74b42a9:
File named: views-page-display-menu-tab-overriden-contextual-link-1831094-5.patch

Added in views_plugin_display_page::option_definition() definition for 'contextpage' => array('default' => 0), to initially display both menu tab and contextual links (so Hide menu tab unchecked by default).

I would greatly appreciate if you could let me know if I missed or overlooked anything in this re-rolled patch, I would certainly be glad to work some more on the patch and take into account further reviews, comments or objections.

Feel free to let me know if you would have any further questions, comments, issues, concerns, suggestions, recommendations or objections on this feature request in general, I would be happy to provide more information or explain in further details.

Any further comments, feedback, testing, reviewing and reporting, would be highly appreciated.
Thanks again to all in advance for your support, comments, replies and feedback.
Cheers!

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented 
+++ b/plugins/views_plugin_display_page.incundefined
@@ -30,6 +30,7 @@ class views_plugin_display_page extends views_plugin_display {
+        'contextpage' => array('default' => 0),

In a perfect world you could specify it to be 'bool' => TRUE, and FALSE

+++ b/plugins/views_plugin_display_page.incundefined
@@ -386,12 +387,23 @@ class views_plugin_display_page extends views_plugin_display {
+        $form['menu']['contextpage'] = array(

I'm wondering whether it would be possible to come up with a better name for this variable? Just from 'contextpage' it seems hard to figure out what this is for.

DYdave’s picture

DYdave CreditAttribution: DYdave commented

@dawehner:
Thanks so much once again for the prompt reply.

No problem, I'll get the type changed in the form and in the option_definition().

As for the variable name, I'm not so sure either (because it could also easily get quite long). What about:

  • hide_menu_tab, menu_tab_hidden
  • contextual_links_only

Any other recommendations, comments, suggestions, would be highly appreciated.
As soon as I could have your approval on a variable name, I should be able to get the patched re-rolled.

Feel free to let me know if you would like to look for more variable name suggestions, I would be glad to help finding more possible options.

Thanks very much in advance for your reply.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Sorry for the delay.

+++ b/plugins/views_plugin_display_page.incundefined
@@ -153,7 +154,7 @@ class views_plugin_display_page extends views_plugin_display {
+        $items[$path]['context'] = (empty($menu['contextpage']) && MENU_CONTEXT_PAGE) | MENU_CONTEXT_INLINE;

Mh this logic seems to be hard to understand? MENU_CONTEXT_PAGE is just a 1, so cant' we do this simpler?

+++ b/plugins/views_plugin_display_page.incundefined
@@ -386,12 +387,23 @@ class views_plugin_display_page extends views_plugin_display {
+        $form['menu']['contextpage'] = array(

What about context_only_inline?

DYdave’s picture

DYdave CreditAttribution: DYdave commented
FileSize
Updated patch based on the comments and suggestion at #6 and #8.2.2 KB

Hi dawehner,

Thanks again so much for the prompt and clear feedback.

I have taken all your comments/suggestions into account and re-rolled the patch.

Please find attached to this comment the updated patch, against views-7.x-3.x at 74b42a9:
File named: views-page-display-menu-tab-overriden-contextual-link-1831094-9.patch

with the following changes:
1 - Changed variable name: contextpage to context_only_inline, as suggest in #8.
2 - Changed type of variable to bool with FALSE:
'context_only_inline' => array('default' => FALSE),
as suggest in #6.
3 - Changed the logic as suggested in #8:
$items[$path]['context'] = empty($menu['context_only_inline']) | MENU_CONTEXT_INLINE;

Feel free to let me know if you would have any further questions, comments, issues, concerns, suggestions, recommendations or objections on this updated patch, I would be happy to provide more information or work on the patch again if necessary.

Any further comments, feedback, testing, reviewing and reporting, would be highly appreciated.
Thanks again to all in advance for your support, comments, replies and feedback.
Cheers!

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
FileSize
interdiff.txt736 bytes
drupal-1831094-10.patch2.13 KB

That's looking great, for sure.

This is a small adaption to make the code more readable. Additional we might should ask the UX team in D8, how we gonna do that non-confusing, because currently you basically say: I want a tab, no I don't want one :)

DYdave’s picture

DYdave CreditAttribution: DYdave commented

Hi dawehner,

Thanks again very much for your help reviewing the patch and revising the code.

At this point, I'm not exactly sure how I could potentially help moving this forward.

Feel free to let me know if you would like me to discuss or suggest different options for this to be improved or integrated in Drupal 8, I would certainly be glad to help you work on this.

As a side note and I think you would understand, I would hope/wish that this could potentially be committed as soon as possible. In particular, I would assume the discussion for a D8 port of this feature could probably take a while..... so if possible, it would be great if we could perhaps go ahead with this first in D7, then continue discussing for D8.

Any further feedback, comments, ideas, issues, questions, reviews, testing, objections, recommendations or suggestions on any aspects of this ticket or patch would be highly appreciated.
Thanks very much to all for your great work, replies, comments, reviews and testing.
Cheers!

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Well one question is: Do you agree with the changes in made in #10? If yes I think this is ready to go in Views for Drupal 7.

Once this is committed we will move this issue to Drupal 8 and continue to work on that.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Project: Views (for Drupal 7) » Drupal core
Version: 7.x-3.x-dev » 8.x-dev
Component: Code » views.module
Status: Needs review » Patch (to be ported)

Thanks for your long work on that. Committed and pushed to 7.x-3.x

DYdave’s picture

DYdave CreditAttribution: DYdave commented

@dawehner :
Thank you so much for committing this, your great follow-up, all your suggestions and great reviews of the initially submitted patch.

Thank you also for suggesting to port this to D8 and I will certainly try to start working on that when I can.
Just wanted to track here a few useful hints you gave me on IRC to get me started (or in case anyone else would like to catch up):

  • the file is probably in views/lib/Drupal/views/Plugin/views/display/PathPluginBase.php
  • for Drupal 8 though we need a test to prove that your change actually works as expected

I will certainly get back to you as soon as I have further questions or more substantial information to discuss.

In the meantime, feel free to let me know if you would have any particular questions, comments, issues, suggestions, recommendations, ideas or concerns on any aspects discussed in this ticket, I would surely be glad to provide more information or explain in more details.

Thanks again to everyone for your interest, great feedback and reporting.
Cheers!

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

One request I would like to have: stop using < strong > in your posts :)

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Issue summary: View changes

style edit

almaudoh’s picture

almaudoh CreditAttribution: almaudoh commented

Is this still needed in D8?

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Patch (to be ported) » Needs work

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.