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.
Comment | File | Size | Author |
---|---|---|---|
#46 | tabs-after-patch.png | 135.56 KB | jamesquinton |
#46 | tabs-before-patch.png | 210.93 KB | jamesquinton |
#46 | 1058144-Overlay_breadcrumb_overlaps_secondary_tabs-46.patch | 372 bytes | jamesquinton |
#37 | 1058144-Overlay_breadcrumb_overlaps_secondary_tabs-5.patch | 2.89 KB | roborn |
#32 | 1058144-4-before-patch.png | 28.8 KB | roborn |
Comments
Comment #1
aspilicious CreditAttribution: aspilicious commentedI'm prety sure this is a duplicate, but I can't find one so just moving this to 8.x
Comment #2
theborg CreditAttribution: theborg commentedIs because seven theme is adding an unnecessary
<h2>
to secondary local tasks that is already suffixed in theme function.Comment #3
JacineI haven't tested this myself, but apparently we are printing 2
<ul>
tags, because they're being added in #prefix/#suffix. Whoopsie... :POh, and this isn't really minor... Could even be major, but I'll set to normal for now.
Comment #4
JacineNeeds to be backported too.
Comment #5
theborg CreditAttribution: theborg commentedAdded some wrapping to the secondary tabs to prevent overlapping breadcrumb trail with small screen sizes.
Comment #6
jthorson CreditAttribution: jthorson commentedRe-test
Comment #7
Bojhan CreditAttribution: Bojhan commentedLooks good to me.
Comment #8
theborg CreditAttribution: theborg commentedReassinged. A bit old but still aplies.
Comment #9
xjmPlease 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!Comment #10
theborg CreditAttribution: theborg commented#6: seven_secondary.patch queued for re-testing.
Comment #12
theborg CreditAttribution: theborg commentedRe-rolling, I tested it with a fresh install and thought Bojhan comment was enough... at least you notice it, thank you Jess!
Comment #14
xjmThe 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.)
Comment #15
theborg CreditAttribution: theborg commentedRe-rolling with 8.x file structure. Thanks!
Comment #16
yoroy CreditAttribution: yoroy commentedI 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.
Comment #17
theborg CreditAttribution: theborg commentedLooks 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.
Comment #18
yoroy CreditAttribution: yoroy commentedOh, 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?
Comment #19
Bojhan CreditAttribution: Bojhan commentedYup, 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.
Comment #20
theborg CreditAttribution: theborg commentedIn any case the bug still persists so let's review that at http://drupal.org/node/1359500 and continue from here.
Comment #21
theborg CreditAttribution: theborg commentedFor the sake of comparison, breadcrumbs below secondary tabs.
Comment #22
theborg CreditAttribution: theborg commentedComment #23
travelerttSecondary 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.
Comment #24
xjmI 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.)
Comment #25
jerdavisThe 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.
Comment #26
jerdavisComment #28
jerdavisOdd, thought I was up to date. Let's try this.
Comment #29
jerdavisThe 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.
Comment #30
BrockBoland CreditAttribution: BrockBoland commentedNeeds issue summary
Comment #31
roborn CreditAttribution: roborn commentedCurrently, the patch on #29 doesn't apply cleanly.
I've updated the patch.
I've tested manually and everything seems ok.
Comment #32
roborn CreditAttribution: roborn commentedMmm... bad file name choice. Here are the screenshoots:
Before
After
Comment #33
Boaah CreditAttribution: Boaah commented#31: 1058144-Overlay_breadcrumb_overlaps_secondary_tabs-4.patch queued for re-testing.
Comment #34
Boaah CreditAttribution: Boaah commentedWithout 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.
Comment #35
jerdavisSorry, 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?
Comment #36
BrockBoland CreditAttribution: BrockBoland commentedSounds like this still needs testing in IE9, yeah?
Comment #37
roborn CreditAttribution: roborn commentedI 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.
Comment #39
roborn CreditAttribution: roborn commented#37: 1058144-Overlay_breadcrumb_overlaps_secondary_tabs-5.patch queued for re-testing.
Comment #40
klonos...I think you need to unassign yourself so that others can then review.
Comment #41
klonos...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.
Comment #42
LewisNyman CreditAttribution: LewisNyman commentedTabs are being completely redesigned in 8.x under #1490402: Redesign tabs and the content header. We should still fix this for D7.
Comment #43
LewisNyman CreditAttribution: LewisNyman commentedtags
Comment #44
LewisNyman CreditAttribution: LewisNyman commentedComment #45
jamesquinton CreditAttribution: jamesquinton commentedComment #46
jamesquinton CreditAttribution: jamesquinton commentedAttempting 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.
Comment #47
mgiffordTested in SimplyTest.me. Looks much better.
I hadn't ever enabled all those tabs here -> admin/structure/types/manage/article/display
Comment #49
mgifford46: 1058144-Overlay_breadcrumb_overlaps_secondary_tabs-46.patch queued for re-testing.
Comment #50
mgiffordThe patch just has CSS changes, so the bot must be wrong here.
Comment #51
mgiffordback to rtbc.
Comment #54
dcam CreditAttribution: dcam commentedComment #57
dcam CreditAttribution: dcam commentedComment #60
dcam CreditAttribution: dcam commentedComment #63
dcam CreditAttribution: dcam commentedComment #66
dcam CreditAttribution: dcam commentedComment #67
David_Rothstein CreditAttribution: David_Rothstein commentedAlthough 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.
Comment #68
samiullah CreditAttribution: samiullah at Srijan | A Material+ Company commented