When using the toolbar with a page that puts the HTML class 'contextual-region' the element, the toolbar renders incorrectly.
It places de toolbar DIV on top of the content area ,making it impossible to click anything underneath.

It's because 'contextual-region' triggers CSS that makes the body 'position:relative'. The 'position:absolute' of the toolbar is now relative to the body.

- How to reproduce:
Upload a file and go to the files overview page (admin/content/files). You won't be able to click the uploaded file.

- Possible solutions
1. add CSS: body.contextual-region{position:static}
2. Don't add the 'contextual-region' class to the body. (it's not a contextual region in the end).
3. Don't use {position:absolute} in #toolbar-administration

Files: 
CommentFileSizeAuthor
#22 fix-contextual-region-class-being-added-to-body-2494235-22.patch1.38 KBnuez
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,709 pass(es). View
#17 fix_contextual_region_class_in_body-1319154-17.patch1.36 KBnuez
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 59,261 pass(es), 21,308 fail(s), and 1,632 exception(s). View
#15 fix_contextual_region_class_in_body-1319154-15.patch935 bytesnuez
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,389 pass(es). View
#13 toolbar.png161.05 KBManjit.Singh
#4 fix-relative-body-for-toolbar-2494235-4.patch485 bytesnuez
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed attempting to get list of tests from run-tests.sh. View
toolbar.jpg405.41 KBnuez

Comments

Wim Leers’s picture

Issue tags: +CSS
nuez’s picture

There are more CSS issues, it doesnt make sense to treat them separately. We could possibly make this a duplicate of:

https://www.drupal.org/node/1663198

Wim Leers’s picture

@nuez: it is easier to get many incremental patches in because they're easier to review, than one enormous rewrite patch.

nuez’s picture

FileSize
485 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed attempting to get list of tests from run-tests.sh. View

@Wim Leers, ok.

In the end I've chosen the first approach. The patch is a simple fix: add body.contextual-region{position:static} to contextual.module.css.
The patch applies to the contextual module, not to the toolbar.

That is because, IMHO, the body tag should never get 'position:relative', even though it has the class 'contextual-region', as it complicates absolute positioning to the browser window. The fact that toolbar is not behaving as expected, is therefore an issue caused by contextual.

But I'm not confident enough, to change the 'component' of this issue, nor to say if this is the right approach, but I'll submit the patch for you or someone else to review.

Wim Leers’s picture

Status: Active » Needs review

I'll ping Lewis Nyman for a review.

Thanks! :)

Status: Needs review » Needs work

The last submitted patch, 4: fix-relative-body-for-toolbar-2494235-4.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 4: fix-relative-body-for-toolbar-2494235-4.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 4: fix-relative-body-for-toolbar-2494235-4.patch, failed testing.

LewisNyman’s picture

Version: 8.1.x-dev » 8.0.x-dev
Issue tags: +frontend

hmm, I'm not really a fan of directly styling the body tag either. It could cause all kinds of potential problems with custom themes that we don't anticipate. Is there a way of preventing that class being applied altogether?

If not, then I have one suggestion to make to the current solution:

+++ b/core/modules/contextual/css/contextual.module.css
@@ -6,6 +6,9 @@
 .contextual-region {
   position: relative;
 }
+body.contextual-region {
+  position: static;

As static is the default value for position, we can simplify this but just changing the .contextual-region to .contextual-region:not(body)

nuez’s picture

Will try and figure out where the class comes from, today or tomorrow.

About the :not selector: I considered that, but the not selector doesn't have support for older IE. But now I see that D8 won't support IE 8 anyway so your solution would indeed be better.

Manjit.Singh’s picture

FileSize
161.05 KB
body.contextual-region {
  position: static;
}

If we can add css as according to this then i guess problem can be solved :)

toolbar

Wim Leers’s picture

Title: Toolbar renders incorrectly with 'contextual-region' as HTML class in <body> » Toolbar renders incorrectly due to 'contextual-region' class on <body> added by views-contextual.js
Component: toolbar.module » views.module
Issue tags: -CSS +JavaScript, +Novice, +js-novice

Good point, it'd be even better to not ever apply the contextual-region class to the body. I didn't realize you were encountering this with just D8 core!

Then this is definitely a bug in core/modules/views/js/views-contextual.js, which makes this a JS bug.

nuez’s picture

FileSize
935 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,389 pass(es). View

The issue has indeed to do with views-contextual.js.

The JS file looks for contextual links (rendered by $title_suffix), and then traverses through it's ancestors until it finds the element that holds both the contextual links ($title_suffix) and the view to which they apply. When the ancestor in common has 'position:relative', the contextual links will be positioned relative to the view.

Seven renders the $title_suffix in the header, completely separated from the main content DIV, so the closest shared ancestor it finds is <body>. The class applied to body breaks the toolbar and makes body {position:relative} which we don't want.

Bartik renders the $title_suffix in the same region as the content, so it finds an ancestor that is not <body>, but a child DIV of body, so there is no problem.

The following solutions occur to me:

1. exclude <body> in the selector in views-contextual.js. The contextual links of views (with display:page) won't work if <body> is the ancestor, so they won't work in Seven.
2. Put page.html.twig from Seven in a wrapper DIV, so that div becomes the ancestor and the contextual links work again.
3. Add a wrapper to html.html.twig {{page}} - this would avoid this problem for any custom theme made (unless html.html.twig is overridden)
4. Allow body to have contextual-region and fix the toolbar in another way.

Patch submitted is a combination of 1 and 3: avoid body being the shared element, both in JS and in html.html.twig.

The JS fix is necessary: In case someone wants to override html.html.twig in a custom theme it won't break the toolbar.
The html.html.twig fix is necessary to correctly render the contextual links for Views that have contextual links enabled, such as admin/content/files.

nuez’s picture

Status: Needs work » Needs review
nuez’s picture

FileSize
1.36 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 59,261 pass(es), 21,308 fail(s), and 1,632 exception(s). View

Classy will need to be changed too...

Status: Needs review » Needs work

The last submitted patch, 17: fix_contextual_region_class_in_body-1319154-17.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 17: fix_contextual_region_class_in_body-1319154-17.patch, failed testing.

lauriii’s picture

+++ b/core/themes/classy/templates/layout/html.html.twig
@@ -48,7 +48,7 @@
-    {{ page }}

I guess the reason for the high amount of fails is missing {{ page }}

nuez’s picture

FileSize
1.38 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,709 pass(es). View

Stupid typo, fixed patch

Manjit.Singh’s picture

Status: Needs work » Needs review
Manjit.Singh’s picture

Great... Thanks nuez :)

Now we just need a before/after screenshots :)

Manjit.Singh’s picture

Issue tags: +SrijanSprintNight
prabhurajn654’s picture

Issue tags: -Needs screenshots
FileSize
241.92 KB
239.81 KB

step 1 : patch downloaded fix-contextual-region-class-being-added-to-body-2494235-22.patch

step 2 : following screenshot describes that

overlapped due to which unable to click on items

step 3 : now we can see in below image after applying patch,

is not overlapping now

Before
before.png

After
after.png

lauriii’s picture

I'm not sure about adding page-container div only for this :/

nuez’s picture

Me neither...

We can just apply the javascript fix, but that means that contextual links on views with display 'page' won't show in Classy and Seven.

We can change the approach in views_contextual.js but who knows what kind of other issues we might cause.

Happy to hear suggestions.

davidhernandez’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/templates/html.html.twig
@@ -42,7 +42,7 @@
+    <div class="page-container">{{ page }}</div>

Definitely not a fan of this.

As solution #2 points out, it is not a contextual region, so it shouldn't be getting the class. However, I don't think adding wrapper divs in the html template is a good way to do that. If that is necessary, it may be that the JS logic isn't as flexible as it should be.

LewisNyman’s picture

Status: Needs work » Needs review
Issue tags: -SrijanSprintNight +Needs screenshots
+++ b/core/modules/system/templates/html.html.twig
@@ -42,7 +42,7 @@
-    {{ page }}
+    <div class="page-container">{{ page }}</div>

Should we really be adding this div into Stark? Is it required for functionality?

idebr’s picture

This issue was reported earlier in #2446663: Toolbar "covers" admin/content/files table in Seven. I have contributed a patch with a different approach in that issue.

mgifford’s picture

Assigned: nuez » Unassigned

dawehner’s picture

Status: Needs review » Needs work

#29 seems to be still true, so back to needs work

prabhurajn654’s picture

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.