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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

aspilicious’s picture

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

theborg’s picture

Component: overlay.module » Seven theme
Assigned: Unassigned » theborg
Status: Active » Needs review
FileSize
533 bytes

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

Jacine’s picture

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.

Jacine’s picture

Issue tags: +Needs backport to D7

Needs to be backported too.

theborg’s picture

FileSize
16.53 KB
1.63 KB

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

jthorson’s picture

FileSize
1.63 KB

Re-test

Bojhan’s picture

Assigned: theborg » Unassigned

Looks good to me.

theborg’s picture

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

Reassinged. A bit old but still aplies.

xjm’s picture

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!

theborg’s picture

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.

theborg’s picture

Status: Needs work » Needs review
FileSize
1.63 KB

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.

xjm’s picture

Issue tags: +Novice, +Needs manual 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.)

theborg’s picture

Status: Needs work » Needs review
FileSize
1.69 KB

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

yoroy’s picture

FileSize
64.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

theborg’s picture

FileSize
2.56 KB

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.

yoroy’s picture

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?

Bojhan’s picture

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.

theborg’s picture

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.

theborg’s picture

FileSize
38.24 KB

For the sake of comparison, breadcrumbs below secondary tabs.

theborg’s picture

FileSize
50.99 KB

travelertt’s picture

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.

xjm’s picture

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.)

jerdavis’s picture

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-side

Tabs colliding with breadcrumbs

jerdavis’s picture

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

Assigned: theborg » jerdavis

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

jerdavis’s picture

Status: Needs work » Needs review
FileSize
2.96 KB

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

jerdavis’s picture

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; ?>
BrockBoland’s picture

Needs issue summary

roborn’s picture

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

I've tested manually and everything seems ok.

roborn’s picture

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

After
After patch

Boaah’s picture

Boaah’s picture

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.

jerdavis’s picture

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?

BrockBoland’s picture

Sounds like this still needs testing in IE9, yeah?

roborn’s picture

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.

roborn’s picture

Status: Needs work » Needs review
Issue tags: +Needs issue summary update, +Novice, +Needs backport to D7
klonos’s picture

Assigned: jerdavis » Unassigned

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

klonos’s picture

...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.

LewisNyman’s picture

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.

LewisNyman’s picture

Issue tags: +CSS, +CSS novice

tags

LewisNyman’s picture

Issue summary: View changes
Issue tags: +frontend
jamesquinton’s picture

Assigned: Unassigned » jamesquinton
jamesquinton’s picture

Attempting a backport to D7...

Unfortunately in D7 there is no $page['in_overlay'], or other similar variable - therefore it is not really possible to move the breadcrumb around. However, I've attached a patch that puts the secondary nav onto a new line, under the breadcrumb.

mgifford’s picture

Status: Needs review » Reviewed & tested by the community

Tested in SimplyTest.me. Looks much better.

I hadn't ever enabled all those tabs here -> admin/structure/types/manage/article/display

Status: Reviewed & tested by the community » Needs work

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

mgifford’s picture

mgifford’s picture

The patch just has CSS changes, so the bot must be wrong here.

mgifford’s picture

Status: Needs work » Reviewed & tested by the community

back to rtbc.

Status: Reviewed & tested by the community » Needs work

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

dcam’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

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

dcam’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

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

dcam’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

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

dcam’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

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

dcam’s picture

Status: Needs work » Reviewed & tested by the community
David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs review

Although this makes things look better in the specific case of a narrow screen with many tabs, doesn't it look pretty bad in all other cases (which is the most common case)? Seems to me like the gap it introduces there is pretty large and awkward-looking.

I think this needs more review. The earlier comments in this issue were looking into fixes that scaled better across different screen widths or that only took effect when there was overlap.

samiullah’s picture

Assigned: Unassigned » samiullah