Problem/Motivation

User menu is hiding behind node preview bar, check screenshots. When we remove background gradient then it appears.

Proposed resolution

Change node preview bar design so that user menu appears on preview page.

API changes

None

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Wim Leers’s picture

We probably want the preview bar to push everything down? Perhaps we even want it to sit above the toolbar? Not sure.

joelpittet’s picture

Component: CSS » entity system
Issue summary: View changes
Issue tags: -templating +CSS

Yes pushing it down would probably be ideal. A bit tricky with position fixed items in the theme (aka sticky headers) although that would be a problem regardless.

Not sure what pattern is being used for the navbar but maybe they can work the same and together?

LewisNyman’s picture

Berdir’s picture

Component: entity system » node system

This is node specific.

Berdir’s picture

Double post.

Wim Leers’s picture

#2 Yes, for those kind of changes we have Drupal.displace (in displace.js).

Manjit.Singh’s picture

Status: Active » Needs work
LewisNyman’s picture

I'm wondering if we should even fix this, if a user clicks on those links they loose their changes to the node. Is this a use case we want to support?

lauriii’s picture

I think its very annoying especially on a smaller screen if the admin menu doesn't push the content down. It might not be that big problem with bartik but with your custom theme it might be PITA.

LewisNyman’s picture

This is just the node preview bar though?

idebr’s picture

Status: Needs work » Active

There is no patch attached to this issue to work on, so I believe this should be on 'Active'.

mansspams’s picture

Marked #2550691: Node preview bar occludes page header, should use Drupal.displace() JS API instead of CSS as duplicate to this, but it has some ideas on Proposed resolution - use Drupal.displace(), which was designed for this.

bruvers’s picture

Assigned: Unassigned » bruvers
kylebrowning’s picture

FileSize
51.43 KB
952 bytes
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,582 pass(es). View

I attempted to fix this issue, but I believe I'm missing something.

Patch attached does displace the bar, but for some reason its not displacing the page. Using Drupal.displace wrong?
Displace

bruvers’s picture

I will look into this issue today. The problem seems to be that Drupal.displace does not deal correctly with multiple elements that have data-offset-top set.

bruvers’s picture

Setting data-offset-top on .node-preview-container does not solve this issue as you can see in the screenshot from comment #14.

The problem is that Drupal.displace assumes that every displacing element is positioned correctly while .node-preview-container has neither top or margin-top set. The toolbar module does it's own calculations and sets margin-top in ToolbarVisualView.js line 255.

Possible solution. The Node Preview toolbar could do it's calculations, check the orientation of the toolbar, get the height of the toolbar but this does not seem to be generic enough aproach. What if the user installs another toolbar module?

Another solution would be not to use Drupal.displace and instead set padding-top on the body element to whatever the value is now + the height of .node-preview-container.

LewisNyman’s picture

Yeah, it feels like part of the problem is that the node preview container is injected into the page as a separate component to the toolbar and then styling to look like it's part of the toolbar UI.

I think this would automatically work if we moved this node-preview-container to the toolbar module, rename it to something generic like toolbar-information-container and then allow the node module to inject into it somewhow. How does that sound?

bruvers’s picture

Assigned: bruvers » Unassigned

Not sure about the best solution. Giving a chance to others to take over this task.

kylebrowning’s picture

If we added it to toolbar, and someone turned toolbar off, then what?

LewisNyman’s picture

Issue summary: View changes
FileSize
399.98 KB

@kylebrowning Good point... what happens when you disable the toolbar now?

I tried with the patch applied and it still doesn't fix the issue:

Displace.js doesn't seem to be running. Is there something else we need to pass into Drupal.settings?

LewisNyman’s picture

Status: Active » Needs work
snap_x’s picture

FileSize
941 bytes
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,644 pass(es). View
141.56 KB
121.76 KB

I suggest to add class to body on preview page and to add padding-top that depends on that class. It doesn't depend on Toolbar module.

andypost’s picture

Status: Needs work » Needs review

and what's about "displace"?

Status: Needs review » Needs work

The last submitted patch, 22: user-menu-hidden-2384153-22.patch, failed testing.

bruvers’s picture

@andypost As mentioned in #16 Drupal.displace should work fine if the preview bar would be positioned using CSS top and margin-top values the same as the toolbar does. The preview bar currently has no top nor margin-top values set, this makes Drupal.displace push it down every time it runs.

This issue https://www.drupal.org/node/1847084 led to the creation of Drupal.displace. If you want to get an idea how the code works see bottom part of comment #69.

The problem is how do we get a correct top and margin-top values taking into account that the toolbar changes it's layout, that the toolbar might be disabled or there might be other elements that are displacing content. It seems Drupal.displace could be extended to provide some sort of global registry for displacing elements. A similar idea was proposed in the same aforementioned discussion in #66.

nod_’s picture

When the layout changes because of the toolbar or something else having to do with displace we have an event firing, use that: http://read.theodoreb.net/drupal-jsapi/global.html#event:drupalViewportO...

Drupal.displace already does too much (it shouldn't have to figure out element size automatically) so I'm not willing to expand on that, especially now.

bruvers’s picture

Ok, we cannot expand Drupal.displace to register all displacing elements. This means that every fixed element must determine it's own correct position. The preview bar therefore must know what elements are already position: fixed, determine their height, set it's own top and margin-top value and update those when layout changes. This sounds tricky.

A better way to go about this could be to have all displacing elements in one container. That container would be positioned with position: fixed; top: 0;. Then it's children would not have to be positioned fixed and no JS height calculations would have to be made.

Lewis already mentioned a similar idea in #17.

bruvers’s picture

Of course, we can't expect every module to place it's displacing elements markup into one template. Modules could mark elements as displacing the same way it is done now with the data-offset-top attribute and instead of passing an offset value in pixels they would pass a weight value for ordering. This could look something like this: data-offset-top="-20". Than a script would pick up all displacing elements, sort them by weight and place them into the one single fixed positioned container.

Drupal.displace was built to be a convenient tool for a single displacing element that can be aligned to any edge of the viewport. The real world use case is rather that we have multiple items. Drupal.displace is good at what it does but it just doesn't solve the problems we have with multiple items.

These changes would introduce a new feature and most likely will not cut it into 8.0. Possible solutions or rather workarounds for 8.0 could be:

  1. Leave it as it is. For a high percentage of users it shold not be critical that about 30px of the top of the page are hidden. It is a preview after all.
  2. Change the theming or layout of the preview bar. Make it semi-transparent, reduce height, position absolutely etc.
  3. Change the UI of the preview bar. For instance make it a button that expands on tap to show all options.

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.

Wim Leers’s picture

Title: User menu hiding behind node preview bar » Node preview bar should use Drupal.displace(), otherwise user menu (top of page) is obscured
Issue tags: +JavaScript
xjm’s picture

Priority: Major » Normal

@alexpott, @catch, @cilefen, @effulgentsia, and I discussed this issue this morning while triaging issues with the entity and field system maintainers. We agreed that this does not meet the criteria for a major bug. While there is definitely a visual bug, as @bruvers points out, the impact is not that serious for most sites and content creators.

Thanks for working on this issue!

droplet’s picture

Title: Node preview bar should use Drupal.displace(), otherwise user menu (top of page) is obscured » Node preview bar should re-calculate BODY padding-top, otherwise user menu (top of page) is obscured
Issue tags: +ux

1. the Preview bar is position: fixed;
2. the padding-top is tracking & applying via Toolbar.
3. Preview bar needs to detect the Toolbar status. If Toolbar is found, it should delay the BODY "padding-top' re-calculation after Toolbar.

There's an OutsideIn module, I think we should keep the visuals consistency of these top bars.

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.

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

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

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