Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dave Reid’s picture

sun’s picture

Status: Active » Needs work
FileSize
4.56 KB
5.26 KB

Hm. I'd rather prefer something like this - re-using existing functionality.

However, it's clear from the patches that both admin_menu and Shortcut/Toolbar are not fully prepared for integration yet.

klonos’s picture

You guys are talking about the Keyboard shortcut utility module. Right?

subscribing anyways...

klonos’s picture

...never mind. I just realized this is a D7 core module.

cindyr’s picture

Please work on this! I love admin menu, but that shortcut bar is so handy too, and I'd really like to see the ability to use both (seeing the shortcut toolbar under the admin menu like it already works with the Drupal toolbar). I'm afraid to patch the shortcut toolbar, as I doubt Drupal core would be willing to make your patches, wouldn't they? What can I do to help? I'm new at module development, but I'm willing to try...

callison’s picture

Priority: Normal » Major

@sun: Would you please explain what is wrong with Dave Reid's approach. I'm assuming what you mean by existing functionality is that in his solution he went through each link adding them to the array instead of using the pre_render approach as in your solution. While yours does seem better in the long-run, I wonder if it would be better to go ahead and use Dave Reid's approach so that we won't have to wait for the Shortcut & Toolbar modules to change first. Later, if they have changed, we could always redo it in a more efficient way. Thoughts?

BTW, I changed the priority to major because this is a pretty integral part of having shortcuts in the first place. If we're going to have shortcut functionality, it needs to work as the original, right?

jsenich’s picture

subscribing

stuwat’s picture

Integration of the shortcuts with the admin_menu would be an obvious step forward in the development of this module. I look forward to it.

callison’s picture

Status: Needs work » Needs review

I have re-rolled Dave Reid's patch from #1 against the latest 7.x dev version (see my note in #6). I also added the 'Edit Shortcuts' link on the right side as in the original toolbar drawer from the Toolbar module. Please review and advise.

callison’s picture

callison’s picture

Sorry about that - something weird happened. Hopefully this attachment will work correctly.

callison’s picture

I made some more changes to the above patch. First of all, it had some errors. The cache clearing was in the wrong place and was causing issues, so it now works properly.

I removed the styling for adding toolbar.png in the background of the shortcut links. I didn't see where that was being used in the shortcuts, and it was causing some weird issues.

I added functionality for the shortcut bar to stay open (at least until the user refreshes or clicks a link) when the toggle button is clicked. It even stays open while the other menu items are dropped down. I think a next step for this functionality would be to save the open state of the shortcut bar in a cookie or something (similar to how the toolbar module does it).

I look forward to your review and thoughts on how I did this.

stuwat’s picture

Looks great. New items that are added to shortcuts don't appear there until after coming out of the administrative interface, but I suspect that's an issue with the shortcut module. The open state also needs a bit of attention, as you said. On the whole, this is exactly what was needed. Thanks.

David_Rothstein’s picture

I think we need to do this the way @sun outlined above. The patch in #12 already requires more knowledge of shortcut module internals than is ideal, and will require more in order to work correctly (for example, currently it displays disabled shortcuts in addition to enabled ones, which is not correct).

So, I've written a patch (attached) that is based on @sun's approach, although also borrowing some of the toggling behavior from @callison's patch above.

To clarify, this approach does not require any changes to Drupal core. (I think above @sun was just saying that the core patch would be a good idea in its own right, i.e. to improve the integration more generally, that's all.)

The attached patch also fixes the caching issues described in #13 (where new shortcuts do not appear immediately when inside the overlay), which was definitely an admin menu bug; Drupal core does not have this problem. Actually it was a bug with the admin menu more generally, although it's more noticeable with the shortcuts since you are much more likely to change those than you are to change e.g. the items in the top menu. Doing the overlay integration properly was a bit tricky and involved fixing some related bugs as well, so that's responsible for the size of this patch. But it should all work correctly now.

I also added some code to make the vertical displacement work correctly when the shortcut bar is enabled (so the main page content is not covered up).

Remaining issues:

  1. The vertical displacement still isn't working entirely right inside the overlay (there's code in the Toolbar module that handles that, so someone might need to figure out how to copy it here).
  2. The admin menu drop shadow appears underneath the top bar only, and the shortcut bar covers it up. Ideally, the drop shadow should be underneath the shortcut bar when it's active (as happens in core).
  3. As described by @callison above, the shortcut bar open/closed state does not persist across page requests. (I don't think that necessarily needs to be resolved as part of this issue, though; it could be a followup.)
David_Rothstein’s picture

I also added a link to @sun's core patch from #2 to this issue: #724782: Clean up the shortcut module's CSS

scott.whittaker’s picture

Subscribing

acoustika’s picture

subscribing

Shadlington’s picture

Subbing

pjcdawkins’s picture

Subscribing

For those wanting to use both Shortcut and Admin Menu at the moment, you could extend the admin theme to provide a bar at the top for the Shortcut block. It would only work within that theme though.

kriskhaira’s picture

I ran the patch at #14 on admin_menu-7.x-3.x-dev and got the following error. Also didn't make a difference to my Toolbar and Shortcut despite clearing my cache. I only ran the #14 patch. Is there anything I missed?

$ patch < admin-menu-shortcuts-742184-14.patch 
patching file admin_menu.js
Hunk #1 succeeded at 36 (offset 1 line).
patching file admin_menu.module
Hunk #1 succeeded at 117 (offset 1 line).
Hunk #2 succeeded at 137 (offset 1 line).
Hunk #3 succeeded at 202 (offset 1 line).
Hunk #4 succeeded at 263 (offset 1 line).
Hunk #5 succeeded at 402 (offset 1 line).
Hunk #6 succeeded at 502 (offset 1 line).
Hunk #7 succeeded at 605 (offset 1 line).
Hunk #8 succeeded at 613 (offset 1 line).
patching file admin_menu_toolbar.css
Hunk #1 FAILED at 13.
Hunk #2 FAILED at 116.
Hunk #3 FAILED at 146.
3 out of 3 hunks FAILED -- saving rejects to file admin_menu_toolbar.css.rej
patching file admin_menu_toolbar.js
Hunk #1 FAILED at 4.
Hunk #2 FAILED at 20.
2 out of 2 hunks FAILED -- saving rejects to file admin_menu_toolbar.js.rej
patching file admin_menu_toolbar.module
Hunk #1 FAILED at 35.
Hunk #2 FAILED at 58.
2 out of 2 hunks FAILED -- saving rejects to file admin_menu_toolbar.module.rej
klonos’s picture

@kriskhaira: ...looks like some lines in the code might have been moved around. Try applying the patch manually Kris.

mstrelan’s picture

subscribe

joostvdl’s picture

subscribe

kriskhaira’s picture

@klonos: Doing it manually worked. Thanks.

klonos’s picture

Well it took you a couple of months Kris, but I am glad you finally got it working now ;)

kriskhaira’s picture

Well I did it a few months ago, but realised I didn't leave a note :)

David_Rothstein’s picture

@kriskhaira, if you were able to get the patch to manually apply, would you be able to reroll that as an updated patch file and post it back here? (That way this issue will have an up-to-date patch that people can continue working from.)

If you're already working from a Git checkout you could make the patch pretty easily using "git diff" as described at http://drupal.org/patch/create. If not, but if you're willing/able to spend time on this, we could help you get set up to do it anyway.

Tony Stratton’s picture

I've applied #14 patch manually from an updated Git checkout. While applying it manually, I noticed a change to this code where it was referenced in the patch:

file: admin_menu.module
patch:

-  // Performance: Defer execution.
-  drupal_add_js($path . '/admin_menu.js', array('defer' => TRUE));

file from checkout:

  // Previous versions used the 'defer' attribute to increase browser rendering
  // performance. At least starting with Firefox 3.6, deferred .js files are
  // loaded, but Drupal.behaviors are not contained in the DOM when drupal.js
  // executes Drupal.attachBehaviors().
  drupal_add_js($path . '/admin_menu.js');

The attached patch is just an updated #14, but I left the above code in. Should it be removed?

BTW, I'm new at this, so if I did anything wrong, please let me know. Thanks!

james.williams’s picture

The attached patch applies against the current master branch and works :-) It's an updated version of the one in comment #14.

james.williams’s picture

Tony - those lines have been replaced with a different fix, which is why the previous patches weren't applying I think. They are to do with a different issue, so probably shouldn't have crept into the original patch in #14. The patch I posted is against the current master branch, so goes alongside the fix for those lines.

james.williams’s picture

The attached patch is an attempt at addressing one problem mentioned in #14 - "The admin menu drop shadow appears underneath the top bar only, and the shortcut bar covers it up. Ideally, the drop shadow should be underneath the shortcut bar when it's active (as happens in core)."

Unforunately, this doesn't work in IE currently.

james.williams’s picture

Users without the permission to edit their shortcuts don't see the 'Edit shortcut' link - which breaks the styling on the shortcut bar currently (and with the patch I posted above). The attached patch includes the fix for this. Sorry for continually adding additional patches - there may well be a couple more over the next week as I look to address the remaining problems that David outlines with his patch (which my patches are based on) in comment #14.

sun’s picture

David_Rothstein’s picture

@james.williams: Thanks for picking up work on this patch!

Regarding some of the comments above, note that the reason my patch in #14 had this:

@@ -132,8 +136,6 @@ function admin_menu_init() {
   if ($user->uid == 1) {
     drupal_add_css($path . '/admin_menu.uid1.css');
   }
-  // Performance: Defer execution.
-  drupal_add_js($path . '/admin_menu.js', array('defer' => TRUE));

Was that it also had this:

- * Implements hook_page_alter().
+ * Implements hook_page_build().
  */
-function admin_menu_page_alter(&$page) {
+function admin_menu_page_build(&$page) {
   $page['page_bottom']['admin_menu'] = array(
     '#markup' => admin_menu_output(),
   );
+
+  // Attach the administration menu JavaScript to the same part of the $page
+  // array as the menu itself. This ensures that when this region of the page
+  // is not intended to be rendered (for example, inside the Overlay), the
+  // JavaScript won't be added either. (If the JavaScript were added, the
+  // client-side code would add the menu to the page even when it isn't
+  // supposed to be there.)
+  // @todo: Should all the JavaScript added in admin_menu_init() move here too,
+  //   rather than just this file?
+  if (user_access('access administration menu') && !admin_menu_suppress(FALSE)) {
+    $js_file = drupal_get_path('module', 'admin_menu') . '/admin_menu.js';
+    // Performance: Defer execution.
+    $page['page_bottom']['admin_menu']['#attached']['js'][$js_file] = array('defer' => TRUE);
+  }
+}

In other words, it was just moving that JavaScript from one part of the code to another. Therefore, I don't understand why the patch in #32 only has the second part of that but not the first... seems like we either need both parts removed from the patch or both parts kept in.

I haven't looked at the current code carefully, but I assume we still need both parts kept in, for the reasons mentioned in the above code comment. (And if the current module code isn't using the 'defer' stuff anymore, then we should remove it from the new code as well.)

sun’s picture

Note: the 'defer' attribute was removed in http://drupalcode.org/project/admin_menu.git/commit/a83b14a96bd78995e62a... - might have to revisit that at some point, but for now, it's out.

Thanks for moving this forward, @james.williams and @David_Rothstein! Will review this patch later.

mefisto75’s picture

sub

Wolfgang Reszel’s picture

subscribe

boreg’s picture

subscribe

franzkewd’s picture

Subscribe

luco’s picture

subs. :]

ralf.strobel’s picture

Any progress on this would be greatly appreciated. This is some really crucial functionality that's missing.
It should at least be committed to a dev package.

scelza’s picture

+1 for committing to dev package

chriz001’s picture

Subscribe

codesmith’s picture

Subscribe

Dave Reid’s picture

Assigned: Dave Reid » Unassigned
Yura Filimonov’s picture

I'm surprised this hasn't been fixed yet, as it's an important part of the website development process and the reason why the admin menu may be disabled or used simultaneously with the core toolbar module (bad, yes, but I need a faster and more convenient access to non-default shortcuts, such as "Create a page").

Thank you for your work.

petsagouris’s picture

Just for reference, when this is all done, close #1397872: Admin menu - how to add favourites links? too.

scottrigby’s picture

marked #1397872: Admin menu - how to add favourites links? as duplicate of this issue.

mstrelan’s picture

Patch in #32 no longer applies. I've recreated this against the latest dev snapshot.

Status: Needs review » Needs work

The last submitted patch, admin-menu-shortcuts-742184-49-no-prefix.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
11.45 KB

ok, this is quite a patch... attached one simplifies a couple of things and leverages Shortcut module's existing CSS by faking a div#toolbar into the output.

It still contains a couple of changes from the previous patch, which I'm not happy about, and which require some more in-depth review to understand them.

We're getting really close to a stable 7.x-3.0 release though, and this issue is one of the last remaining blockers.

cinseattle’s picture

I tried to apply patch #51 against the current 7.x-3.x-dev branch today, (shortly after it was posted), and am getting errors. Specifically git am results in "Patch format detection failed", and a patch review using tortioseGit results in errors saying that the patch lines do not match.

Am I applying it toward the right branch or do you have any suggestions on how to properly apply this specific patch?

Thanks!

scottrigby’s picture

@cinseattle #51 applied fine for me with git apply.

@sun & everyone, this patch is so kick ass!

I noticed two things:

  1. After visiting the /admin path, the toolbar is a little messed up after page refresh (see screenshot - clearing the admin_menu cache straightens it out). Reproduce by clicking 'find content', then back to the homepage.
  2. This is prob more minor, but when in the the /admin path, core toolbar is still visible below the admin_menu_toolbar one (only visible it if we click to collapse the admin_menu one, but it still seems a little funky - or is that by design for now?).
sun’s picture

I've merged and committed the cache flushing improvements from this patch in #282775: Hide disabled menu items

Additionally, #51 lives in a dedicated 7.x-3.x-shortcut topic branch already.

klavs’s picture

I have the same problem as #53. I just tried to install latest from 7.x-3.x-shortcut branch - but it has the same problem.

sun’s picture

Additionally, #502408: Move invocation/processing into hook_page_build() just landed.

Thus, re-rolled this patch (and rebased/recreated the 7.x-3.x-shortcut branch) against latest HEAD.

mstrelan’s picture

The padding on this seems unnecessary - at least in the Seven theme and in my custom theme.

/* Wrapper list excluding shortcuts themselves. */
#admin-menu li.admin-menu-shortcuts > ul {
  background-color: #666;
  padding: 5px 10px;
  right: 20px;
}

Also after enabling shortcut.module shortcut.css wasn't loaded in my aggregated css until a cache flush and therefore Edit Shortcuts overlapped my actual shortcuts, but I don't think this is admin_menu's problem.

Other than that it works well in Firefox 11 and Chrome 17.

Macronomicus’s picture

Status: Needs review » Needs work

Awesomeness! There was an error on applying the patch though

admin_menu.shortcut.56.patch:106: new blank line at EOF.
+
warning: 1 line adds whitespace errors.

Also when hovering over the little arrow that you click to show/hide the bar it disappears.
Other than those small issues it seems to work perfectly!

:) Cheers!

mlncn’s picture

Here's a re-roll of the patch so that it applies.

* Still loses the drawer arrow when the drawer (shortcut bar) opens.
* Feels like a little more vertical padding than necessary.
* Had to clear cache after adding a new shortcut menu item for them not to print on top of each other.

occupant’s picture

It seems that some css has been introduced that really deforms the usability. When the shortcut menu is expanded, the links appear, not on the right hand side of the screen below the arrow / trigger as it used to be, but all the way over on the left hand side of the window. The offending css is in admin_menu_toolbar.css

Line 123

#admin-menu li.admin-menu-shortcuts-active ul {
  display: block !important;
  left: 20px !important;
  right: 20px;
  width: auto;
}
#admin-menu li.admin-menu-shortcuts ul {
  background-color: #666;
  left: 20px !important;
  padding: 5px 10px;
  right: 20px;
  width: auto;
}

Removing the !important properties from both of the left declarations fixes the issue. I'm not sure what they were intended to correct, so I don't know if that will create problems or not.

sun’s picture

The shortcut bar is supposed to appear and behave like it does in combination with the Toolbar of Drupal core. The right-aligned drop-down-menu-alike expansion on hover was a bug, not the intended behavior.

sun’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, admin_menu.shortcut.59.patch, failed testing.

occupant’s picture

Status: Needs work » Needs review

@sun #61

Really? It's a huge step backward. If a hover-triggered right justified menu is supposed to display it's children full width and left justified, I'd seriously question the decision. It means I have to hover and then scroll from right to left the entire width of my window every time I want to access the items, hoping my cursor doesn't fall outside the hover area (all 33px or so).

If you really feel strongly it should be done this way, it would be nice if you didn't use the !important so it can be easily overridden in theme stylesheets.

sun’s picture

Status: Needs review » Fixed

Thanks for reporting, reviewing, and testing! Committed this patch, and a follow-up clean-up and simplification.

A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.

David_Rothstein’s picture

Status: Fixed » Needs review

@occupant, I think the idea here is that the hover behavior will be removed entirely. Once you open the shortcuts, they're supposed to stay open.

As far as I remember the earlier patches did this, although the open/closed state did not yet persist between page requests (see #12 and #14). I haven't looked at the newer patches, but if they're back to having the shortcuts only appear on hover then it's possible something got lost along the way.

David_Rothstein’s picture

Status: Needs review » Fixed

Oops, crosspost. Well, if there's something still wrong with the behavior, I'm sure we'll find out eventually :)

luco’s picture

I agree with @occupant. the way it is, even if it doesn't go away, it's a pain to use. either that arrow should be moved to the left, where it's closer to the links, or the links should be put back on the right, near the arrow that triggers them.

basically you can't have a trigger for a menu that's far from the menu itself. it's not a matter of opinion, it's usability. we're keeping the default look of shortcut.module in a context where it doesn't work just for the sake of looks. that's a bad decision.

scottrigby’s picture

@luco re 'bad decision', I disagree. this ticket is not to redesign the core toolbar layout, but to integrate with core shortcut functionality

David_Rothstein’s picture

The reason this isn't a usability problem for Drupal core is that the shortcuts stay open across page requests. The arrow is deliberately in an out-of-the-way place, because it's something that is only clicked very rarely.

As I mentioned above, the last time I looked at this patch it did not do that (so you had to specifically open the shortcuts on each page request you wanted to use them). If that's still the case, should a followup issue be filed to fix that?

sun’s picture

I'm happy to re-open this issue for "simple" follow-up fixes. Larger design/UI changes (that possibly diverge from the Toolbar/Shortcut experience in core) should be properly proposed in separate issues (using mockups, annotated screenshots, or even patches, you know... ;)).

However, most of the discussion in this issue is based on earlier patches and the committed code and behavior looks entirely different. I spent pretty much an entire day with cleaning up and manually testing the new code. It's in no way comparable to any patches or previous behavior you might have tested before.

Thus, before discussing any hypothetical UI/UX issues any further, please make sure that you're running and testing the latest 7.x-3.x-dev development snapshot first.

luco’s picture

you're right on both topics, Sun. sorry :)

luco’s picture

btw I've tested the newest DEV and at first I missed the shortcuts. but then I turned on shortcut.module and voilà!

JSCSJSCS’s picture

Downloaded the may 1st dev version and found two issues:

1. Selecting Home (house icon) the edit links moves to the left and ovelaps other links. This does not happen on other admin menu links, just the home icon.

2.) when using a reduced browser window, there is overlap on the left that covers the edit links link.

sun’s picture

Thanks for testing @JSCSJSCS!

Both issues require larger changes, for which I've created an issue + patch: #1564934: Separate toolbar and dropdown menu markup

klavs’s picture

FileSize
6.7 KB

I checked out the latest shortcut branch - and it still has one major problem for me :(

When I've visited the /admin path (f.ex. /admin/modules) - it removes almost all menu-items and also screws up the shortcut bar a bit.

The menu comes back to normal, if I clear the adminmenu cache.

See attached image.

If you have any ideas, as to why it does this for me (and seemingly not for you) I'd gladly try whatever :)

kitikonti’s picture

@klavs
I think this is the Same Problem like http://drupal.org/node/1546206?mode=2&sort=2

With the latest dev the problem should be solved

klavs’s picture

@kitikonti: but I just (yesterday) pulled the latest from shortcut branch from git. This fix seems to not be commited to shortcut branch then?

JSCSJSCS’s picture

This is an issue in process of resolution. Apply this patch:

http://drupal.org/node/1564934#comment-5962972

I have been testing it and it the issue of the edit shortcut link is fixed but there are still some other issues.

sun’s picture

Sorry, the 7.x-3.x-shortcut branch in the git repo was entirely outdated and obsolete. Forgot to delete that.

The most current code lives in 7.x-3.x now, and thus is also packaged in the latest dev snapshot. It even contains #1564934: Separate toolbar and dropdown menu markup already.

klavs’s picture

hehe - that explains it :)

Installed latest dev instead - much better.

Status: Fixed » Closed (fixed)
Issue tags: -D7 stable release blocker

Automatically closed -- issue fixed for 2 weeks with no activity.

The last submitted patch, 56: admin_menu.shortcut.56.patch, failed testing.