The breadcrumb in the overlay overlaps with the secondary tabs. This causes a usability issue when you have multiple themes enabled and visit a specific theme's settings page at: #overlay=admin/appearance/settings/bartik.

See attached screenshot.

Confirmed with FF 3.6 and IE 8.

Files: 
CommentFileSizeAuthor
#37 1058144-Overlay_breadcrumb_overlaps_secondary_tabs-5.patch2.89 KBroborn
PASSED: [[SimpleTest]]: [MySQL] 40,556 pass(es).
[ View ]
#32 1058144-4-before-patch.png28.8 KBroborn
#32 1058144-4-after-patch.png28.15 KBroborn
#31 1058144-Overlay_breadcrumb_overlaps_secondary_tabs-4.patch2.73 KBroborn
PASSED: [[SimpleTest]]: [MySQL] 40,565 pass(es).
[ View ]
#31 #1058144-4-before-patch.png28.8 KBroborn
#31 #1058144-4-after-patch.png28.15 KBroborn
#29 1058144-Overlay_breadcrumb_overlaps_secondary_tabs-3.patch2.9 KBjerdavis
PASSED: [[SimpleTest]]: [MySQL] 40,252 pass(es).
[ View ]
#28 1058144-Overlay_breadcrumb_overlaps_secondary_tabs-2.patch2.96 KBjerdavis
PASSED: [[SimpleTest]]: [MySQL] 40,251 pass(es).
[ View ]
#25 breadcrumbs-tabs-sidebyside.png53.91 KBjerdavis
#25 breadcrumbs-tabs-collision.png36.35 KBjerdavis
#25 1058144-Overlay_breadcrumb_overlaps_secondary_tabs.patch3.14 KBjerdavis
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1058144-Overlay_breadcrumb_overlaps_secondary_tabs.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#21 seven_secondary.png38.24 KBtheborg
#22 seven_secondary.png50.99 KBtheborg
#17 seven_secondary_3.patch2.56 KBtheborg
PASSED: [[SimpleTest]]: [MySQL] 34,110 pass(es).
[ View ]
#16 wraptabs.png64.82 KByoroy
#15 seven_secondary_2.patch1.69 KBtheborg
PASSED: [[SimpleTest]]: [MySQL] 34,085 pass(es).
[ View ]
#12 seven_secondary_1.patch1.63 KBtheborg
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch seven_secondary_1.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#6 seven_secondary.patch1.63 KBjthorson
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch seven_secondary_0.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#5 seven_secondary.patch1.63 KBtheborg
PASSED: [[SimpleTest]]: [MySQL] 33,292 pass(es).
[ View ]
#5 seven_secondary.png16.53 KBtheborg
#2 seven_local_tasks.patch533 bytestheborg
PASSED: [[SimpleTest]]: [MySQL] 29,936 pass(es).
[ View ]
overlay_css_breadcrumb.jpg91.52 KBNoDice

Comments

Version:7.0» 8.x-dev

I'm prety sure this is a duplicate, but I can't find one so just moving this to 8.x

Component:overlay.module» Seven theme
Assigned:Unassigned» theborg
Status:Active» Needs review
StatusFileSize
new533 bytes
PASSED: [[SimpleTest]]: [MySQL] 29,936 pass(es).
[ View ]

Is because seven theme is adding an unnecessary <h2> to secondary local tasks that is already suffixed in theme function.

Priority:Minor» Normal

I haven't tested this myself, but apparently we are printing 2 <ul> tags, because they're being added in #prefix/#suffix. Whoopsie... :P

Oh, and this isn't really minor... Could even be major, but I'll set to normal for now.

Issue tags:+needs backport to D7

Needs to be backported too.

StatusFileSize
new16.53 KB
new1.63 KB
PASSED: [[SimpleTest]]: [MySQL] 33,292 pass(es).
[ View ]

Added some wrapping to the secondary tabs to prevent overlapping breadcrumb trail with small screen sizes.

StatusFileSize
new1.63 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch seven_secondary_0.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

Re-test

Assigned:theborg» Unassigned

Looks good to me.

Assigned:Unassigned» theborg
Status:Needs review» Reviewed & tested by the community

Reassinged. A bit old but still aplies.

Status:Reviewed & tested by the community» Needs review

Please don't mark your own patch RTBC. :) Also, I'm not sure how this can still apply, because of the core/ change. At the least, it needs to be rerolled. Thanks for your work on this!

Issue tags:-needs backport to D7

#6: seven_secondary.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+needs backport to D7

The last submitted patch, seven_secondary.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.63 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch seven_secondary_1.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

Re-rolling, I tested it with a fresh install and thought Bojhan comment was enough... at least you notice it, thank you Jess!

Status:Needs review» Needs work

The last submitted patch, seven_secondary_1.patch, failed testing.

The reason it is not applying is that D7 and D8 now have different directory structures (see #22336: Move all core Drupal files under a /core folder to improve usability and upgrades). So, it's best to create the patch against D8 from git. Once it is rerolled, we can have one other person test the patch manually to confirm that the fix works and doesn't have any unintended side effects. Thanks!

Tagging novice for some patch review/testing. (When testing, see the screenshots in #5, which illustrate the change.)

Status:Needs work» Needs review
StatusFileSize
new1.69 KB
PASSED: [[SimpleTest]]: [MySQL] 34,085 pass(es).
[ View ]

Re-rolling with 8.x file structure. Thanks!

StatusFileSize
new64.82 KB

I would probably look into giving the breadcrumbs and tabs their own line the moment tabs start wrapping. Instead of keeping things floated next to eachother for as long as possible as is the case now.screenshot of wrapping tabs to below the breadcrumbs

StatusFileSize
new2.56 KB
PASSED: [[SimpleTest]]: [MySQL] 34,110 pass(es).
[ View ]

Looks like a good solution to me, re-uploading a new patch with your suggestion. Also note that I've moved secondary tabs after breadcrumbs inside branding div, that way negative margins are not needed.

Oh, that points out a weakness in my proposal, it's probably not a good idea to put breadcrumbs in between primary and secondary tabs in the source code. Nor visually, now that I look at it again. Wish we could just move or even remove the breadcrumbs all together.

So thanks for the patch @theborg, I'm just not sure my suggestion was an improvement. Thoughts?

Yup, I would avoid doing it like this. Apart from changing the visual hierachy it also causes a bit of strange positioning when you change between items that have many and those with few secondary navigation items.

Status:Needs review» Needs work

In any case the bug still persists so let's review that at http://drupal.org/node/1359500 and continue from here.

StatusFileSize
new38.24 KB

For the sake of comparison, breadcrumbs below secondary tabs.

StatusFileSize
new50.99 KB

Secondary Tabs over breadcrumbs looks visually better. Keeping the tabs close together shows that they are connected with the parent tabs. Separating them with the breadcrumbs would give the impression that the secondary tabs don't have any relevance with the Primary Tabs.

I agree with #23. The screenshot in #22 is what we want, and the source should be ordered in the same way. Can someone confirm if this is what the patch in #17 does that? (Provide before-and-after source markup and screenshots, both in the overlay and without it.)

Assigned:jerdavis» theborg
Status:Needs review» Needs work
StatusFileSize
new3.14 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1058144-Overlay_breadcrumb_overlaps_secondary_tabs.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new36.35 KB
new53.91 KB

The patch in #17 did not do source reordering for #22. I've re-rolled #17 to re-order source for the desired result in #22. This patch retains the tabs being floated to the side on the same line as breadcrumbs when they would not collide. When secondary task tabs and the breadcrumbs do collide the breadcrumbs drop below the tabs as displayed in #22.

The patch also "fixed" the fact that in style.css we were referencing breadcrumbs using div.breadcrumbs still. The element has been changed to nav as part of the html5 updates but this CSS had not been changed.

Tabs showing side-by-sideTabs colliding with breadcrumbs

Assigned:theborg» jerdavis
Status:Needs work» Needs review

Assigned:theborg» jerdavis

The last submitted patch, 1058144-Overlay_breadcrumb_overlaps_secondary_tabs.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.96 KB
PASSED: [[SimpleTest]]: [MySQL] 40,251 pass(es).
[ View ]

Odd, thought I was up to date. Let's try this.

StatusFileSize
new2.9 KB
PASSED: [[SimpleTest]]: [MySQL] 40,252 pass(es).
[ View ]

The source order changing is introducing a bug when used without overlay. This version fixes that, but the markup in page.tpl.php seems not super ideal.

  <div id="branding" class="clearfix">
    <?php if (!$page['in_overlay']): ?>
      <?php print $breadcrumb; ?>
    <?php endif; ?>
    <?php print render($title_prefix); ?>
    <?php if ($title): ?>
      <h1 class="page-title"><?php print $title; ?></h1>
    <?php endif; ?>
    <?php print render($title_suffix); ?>
    <?php print render($primary_local_tasks); ?>
    <?php if ($secondary_local_tasks && $page['in_overlay']): ?>
      <div class="tabs-secondary clearfix"><?php print render($secondary_local_tasks); ?></div>
      <?php print $breadcrumb; ?>
    <?php endif; ?>

Needs issue summary

StatusFileSize
new28.15 KB
new28.8 KB
new2.73 KB
PASSED: [[SimpleTest]]: [MySQL] 40,565 pass(es).
[ View ]

Currently, the patch on #29 doesn't apply cleanly.
I've updated the patch.

I've tested manually and everything seems ok.

StatusFileSize
new28.15 KB
new28.8 KB

Mmm... bad file name choice. Here are the screenshoots:
Before
Before patch

After
After patch

Status:Needs review» Needs work
Issue tags:-Needs manual testing

Without the Patch I saw the problem. I applied the patch on #31, cleared the drupal cache using drush cc all and in cleared the cache in Chrome. It worked by pushing the breadcumb below the tabs instead of overlaying the tabs.

I removed the tag 'Needs manual testing', because it worked on Chrome, Firefox, Safari on Mac OSX as well as Windows plus IE7 and IE8.

Status:Needs work» Needs review

Sorry, not clear the reason for the move to needs work? It sounds like the manual testing produced the desired results. Was there any issue?

Setting back to needs review in case further review is needed, otherwise RTBC?

Sounds like this still needs testing in IE9, yeah?

StatusFileSize
new2.89 KB
PASSED: [[SimpleTest]]: [MySQL] 40,556 pass(es).
[ View ]

I had a 2nd thought about what jerdavis said on #29.
Actually, we don't need to move the secondary tabs to the branding section, and can simplify the markup.
We can move overlay's breadcrumb to the page section, and apply the required overlay css properties, making use on .overlay class. An the float on overlay's breadcrumb gets always cleared by the content region.

Here's the new patch with these changes.

Status:Needs review» Needs work
Issue tags:-Needs issue summary update, -Novice, -needs backport to D7

The last submitted patch, 1058144-Overlay_breadcrumb_overlaps_secondary_tabs-5.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+Needs issue summary update, +Novice, +needs backport to D7

Assigned:jerdavis» Unassigned

...I think you need to unassign yourself so that others can then review.

...perhaps this needs a reroll (lots changed in the past 6+ months). I think we're shifting to removing the breadcrumbs from the overlay (not settled on that yet - more here: #1889150: Simplify overlay look, *only use for contextual operations*), since I guess we're aiming to use the overlay only for "quick" tasks (through modal dialogs) in D8 and forth.

So, practically yeah, I don't know if this issue here is relevant any more. Perhaps postpone till we see what the outcome of #1889150: Simplify overlay look, *only use for contextual operations* is.

PS: Perhaps still is a valid bug for D7 though.

Version:8.x-dev» 7.x-dev

Tabs are being completely redesigned in 8.x under #1490402: Redesign tabs and the content header. We should still fix this for D7.

Issue tags:+CSS, +css-novice

tags