Toolbar interferes with anchor links

Here's a screenshot showing this in action; "Title" was clicked.

We obviously can't ship Drupal 7 with this not working, so marking critical.

I do like how this works, but there are some problems with it for sure. I've added some other screenshots of this here http://drupal.org/node/447816#comment-1980664

Agreed that it's critical. Will just be causing confusion

Thanks for filing this issue. There are other places in the admin interface where anchors are used--I'll try to post examples here. One solution might be a Javascript enhancement that would turn anchors into Javascript that would scrollTo() with an appropriate fudge factor for the Seven toolbar.

I agree with #3 that this should be a fix that lives with the seven toolbar JS, so that all anchors work as expected.

See the related issue, #567754: Wrong anchor adjustment in tableheader.js. The patch there fixes this for anchors inside a table with a sticky header (though a general solution would of course be better).

Cool. My drupal.org unleashed user style has the same problem. I hope we find a neat CSS-only solution here. ;)

(And, btw, admin_menu doesn't have this issue, because it doesn't consume 15-20% of the visible viewport.)

@sun ... I would love to see a css-only solution here as well but I've been wracking my brain and the way I see it there are only a few options to make this happn w/o js:

1. frames - ohhhh the humanity
2. stop using position:fixed and just let the toolbar be visible when you're scrolled all the way to the top of the brwser window - I don't mind this option but I imagine a lot of people would
3. make the toolbar much much smaller (a la admin menu) but in reality that doesn't fix the prob either ... Just makes it less prevelent

if you have some other ideas I'm happy to do any of the heavy lifting to get them working but I'm stuck on his one ... I think it has to be a js solution ... Thoughts?

misc/jquery.ba-bbq.js implements adds cross-browser support for the window.onhashchange event. Upon this event we could increment the current scroll position by 30 pixels.

After a little playing around, it seems like we might be able to add a use negative margins to pull anchor tags up by N pixels. We would need to add an class to these tags, since obviously we don't want this to apply to regular links - i.e.:

<a id="comment-2290628" style="" class="anchor-correct"/>

And then CSS (inheriting off a body class indicating if the toolbar is present) along the lines of:

body.toolbar .anchor-correct {
  display:block;
  height:30px;
  margin-top:-30px;
}

Obviously the CSS would need to live in the theme, since different admin themes may have different sized toolbars. Of course, I haven't tested this in IE or Safari - does anyone have time to try that out and see if this is a viable approach?

That sounds quite interesting, but unfortunately, limiting this to anchor tags doesn't work.

<h2 id="jumpme">My jump target</h2>

...is a valid jump target as well.

You can jump to this #target, if you want to try.

errrrrrr. This is _really_ interesting. Played a bit with that. 8)

*[id] {
  /* display: inline-block; ** At least FF3 gets this right even without it!? */
  margin-top: -90px;
  padding-top: 90px;
}

Try this in action. (Don't try this at home. ;)

.

errr. Undo this.

...

Good point that you can use id with other tags. I think it is going to be pretty impossible to come up with a pure CSS negative-margin that works properly with h tags, because those will normally have theme styles that will clobber whatever we add. Perhaps what we could do is write a little JS behaviour that:

(a) finds *[id] attributes (i.e. on any tag)
(b) removes the original id (might be unnecessary, but I think duplicate IDs aren't allowed)
(c) adds an anchor tag with the same id directly in front, and adds a style or class that uses negative margins to pull it up as needed

@Owen, interesting idea. Removing the original ID might break contrib code, such as code that manipulates the page by ID in jQuery or using getElementById().value='new'. So as a slight variation of your proposal, we could instead keep the original ID in place, but add a new id in front (eg, 'origid-anchor-correct'). Then we would find all the href attributes and modify their targets from #origid to #origid-anchor-correct.

Note: This variation would only work for 'internal links' that come from within the same page, not for anchors that appear in links from other pages or other websites.

nah, a JS-based solution would of course calculate the real offset in the viewport and move the document accordingly. Altering the DOM would break literally every other JS and CSS on the page.

Update: the behavior has changed when jumping to the permissions page from the Modules page via the "permissions" operation: now it does not seem to accept any anchors and always jumps to the top of the permissions page.

@eigentor: this is issue is not specific to any one anchor link. In general, if i have a page with <div id="jumpto">...</div> and elsewhere on the page I have <a href="#jumpto">...</a> then the Toolbar will interfere.

subscribing

I agree that a CSS-only solution would be preferable, but I think if there was one we would have uncovered it by now ... this solves the issue via javascript.

Review away

Fixed a tiny whitespace problem ... otherwise the patch is unchanged from #18

+++ modules/toolbar/toolbar.js	12 Jan 2010 02:50:13 -0000
@@ -18,6 +18,17 @@ Drupal.behaviors.admin = {
+      if (location.hash != "") {

Single quotes.

+++ modules/toolbar/toolbar.js	12 Jan 2010 02:50:13 -0000
@@ -18,6 +18,17 @@ Drupal.behaviors.admin = {
+        $('body, html').scrollTop(scrollLocation);

1) Why both body and html? Either one is superfluous or this needs an inline comment.

2) The jQuery selector is missing the context to search in.

Powered by Dreditor.

@sun:

• single quotes = done
• body & html = in truth I was reviewing #567754: Wrong anchor adjustment in tableheader.js and I assumed that the code in there used both html & body for some good reason that I was missing. Perhapse not. For now, I altered the selector to use 'body' only
• "The jQuery selector is missing the context to search in." - I'm not sure what you mean here.

I would use $(window) or $(document.body) as it's faster.

Drupal behaviors get a "context" variable passed to them, which can be a window, document, or an arbitrary DOM element. This JS needs to account for that context, because we don't want to jump around in IFRAMEs, f.e. WYSIWYG editors on the page.

Additionally, this JS needs to account for the fact that Drupal behaviors can be attached multiple times at arbitrary times. Doesn't look this is covered already.

@casey: done

@sun: Still getting used to jquery in Drupal. I think all I needed to do was add that context var, yes? As for preventing the behavior from being attached multiple times, I'm not sure the way I'm going about it is the optimal solution, but it works. Basically I'm just doing an unbind before I bind the function. Thoughts?

- Improved comments
- Really no need to use context on window; Drupal behaviors are always applied to same window.
- No need to unbind when using a named function; it won't get bound twice
- Added a check to make sure anchor does actually exist

Wow this is going to collide with overlay. Bug in overlay. I'll post a issue for that bug. That one should go in first.

Don't test latest patch...

SERIOUSLY DON'T :)

OK UPDATE:

#25 is working now if you apply #684474: Overlay is re-applying behaviors first

Yes the other issue is critical and should go in first.

- Added namespaced event handler and trigger only that handler.

Tested in IE, ff and webkit. Works fine, if you apply other patch first offcourse.

In IE I discovered some realy realy bad issues but they are not related with this patch (I tested it on a fresh install).
Going to open topics for them.

OOOPS, it is related.

In IE compatibilty mode or IE6, this patch makes a page scroll down to the end see screenshot.

#32 is not related to this issue. Please open a new issue for that.

// Edit: #32 is caused by jquery-ba-bbq.js: http://benalman.com/projects/jquery-bbq-plugin/#comment-152

Tagging.

But i have two installs, one with the patches one without...

Something must cause this....

Re-tagging. ;)

- Really no need to use context on window; Drupal behaviors are always applied to same window.

Sorry, but this is plain wrong. Drupal behaviors are initially applied on the current main window only, but all scripts are free to re-apply them at any point to the same window, to specific DOM elements on the page, or an embedded window, such as an iframe. Scripts are not only free to do so, they are actually doing this all over the time, if coded correctly.

I'll also follow-up on #684474: Overlay is re-applying behaviors

Anyway, does $(window, context) actually work? I didn't test it, but I don't think so...

Applying behaviors from one window to another (e.g. from parent window to an embedded iframe) fails currently. We could make it work by passing the context document/window.

Drupal.attachBehaviors = function (context, settings) {
  context = context || document;
  settings = settings || Drupal.settings;
  var document = context.ownerDocument || document;
  var window = document.defaultView || document.parentWindow;
  // Execute all of them.
  $.each(Drupal.behaviors, function () {
    if ($.isFunction(this.attach)) {
      this.attach(context, settings, document, window);
    }
  });
};

Behaviors can than use the extra arguments when necessary.

But I am not sure why we want this to be possible.

Not critical. Please read and understand Priority levels of Issues, thanks.

+++ modules/toolbar/toolbar.js	14 Jan 2010 12:56:15 -0000
@@ -3,6 +3,8 @@
+ *
+ * @link http://docs.jquery.com/Namespaced_Events

Please remove.

+++ modules/toolbar/toolbar.js	14 Jan 2010 12:56:15 -0000
@@ -18,6 +20,13 @@
+    $(window)
+    // Ensure the toolbar doesn't hide content when named anchor is clicked.
+    .bind('hashchange.drupal-toolbar', Drupal.admin.toolbar.hashchange)
+    // Trigger the hashchange event once, after the page is loaded, to handle
+    // hash in original page request.
+    .trigger('hashchange.drupal-toolbar');

Wrong indentation.

+++ modules/toolbar/toolbar.js	14 Jan 2010 12:56:15 -0000
@@ -99,6 +108,19 @@
+Drupal.admin.toolbar.hashchange = function (event) {
+  if (location.hash != '') {
+    $anchor = $(location.hash);
+    if ($anchor.length) {

Why isn't this using jqBBQ API functions? Or is this not covered?

Powered by Dreditor.

Reroll

Also fixed a bug in tableheader.js (I tested this issue on /admin/modules#edit-modules-views)

#787940: Generic approach for position:fixed elements like Toolbar, tableHeader landed. Now we can handle all displaced elements like the toolbar at once. We have however also to deal with other scripts that might want to adjust scrollTop (like tableheader.js: #567754: Wrong anchor adjustment in tableheader.js).

KipHaas7 and me worked on a cleanup of tableheader.js and also included some code to get this working: #787670: Clean up tableheader.js

I opened another issue containing a generic solution: #800270: Anchor/focus hidden beneath toolbar/tableheader (CKEditor full screen menu).

I've noticed this bug recently, and it's a pain, especially with the per-row permission links on the modules page.

Looks like we have some issue inflation here: we also have #787940: Generic approach for position:fixed elements like Toolbar, tableHeader (adds displace.js after being reverted), #800270: Anchor/focus hidden beneath toolbar/tableheader (CKEditor full screen menu) (seemingly improves displace.js, which no longer exists), #567754: Wrong anchor adjustment in tableheader.js (patches tableheader.js, but from 2009) AND this issue. Gah! I'd like to see the effort consolidated, if we're going to squash this bug.

See also #841184-57: "Skip to main content" link doesn't work correctly in the overlay. Marking Major as this is not critical, but can really make things difficult to find / use, like the UI control to disable the Overlay.

Tagging

#41: toolbaranchor.patch queued for re-testing.

So it seems this issue is a straight duplicate of #800270: Anchor/focus hidden beneath toolbar/tableheader (CKEditor full screen menu)

Lots of brainpower on both issues, so pretty please review both issues in detail before marking one of both as duplicate. The other one sounds more generic and thus, more correct.

I just wanted to update the patch of #41. It doesn't seem to break anything, but I'm not sure if of it works either. The whole section for misc/tableheader.js was substantially rewritten so this section no longer is findable.

I only saw this issue on friday but the problem has annoyed me before then.

This patch implements a css-only solution inspired by alistapart.com article. It might need temporarily switching off js to ensure no interference – I’m not sure how much of this js is in head.

A couple of screen-shots as well – note position of scrollbar

• Anchor in middle of page
• node/add/page#edit-title (target is the text input element itself – label is hidden)

I’ve added a page-wrapper to seven’s page.tpl.php, à la bartik — Is there a better way to inject this <div> from toolbar?

Overlay will need more work.

Wrapping all page content like patch in #51 could be a solution, but then either themes are required to include a wrapper in page.tpl.php that or at least support that (we could wrap content in template_(pre)process_html() or using Javascript; css selectors and other scripts have to account for the possibility).

Anyways it could be much easier to implement/maintain than the bloated fix-fixed-positioning-script in #787940: Generic approach for position:fixed elements like Toolbar, tableHeader.

Use a table, let them in table box fix forever maybe

Restoring tags.

Don't think we can get away with adding the wrapper markup for D7, either in Seven or requiring other themes to do it.

Can anyone tell me what is stopping us from getting this fixed? It looks like #787940: Generic approach for position:fixed elements like Toolbar, tableHeader provides a generic approach, this issue needs to implement?

I'm marking this as a duplicate for now. It certainly seems to be in my review. It's also referenced from 787940 which is good.