Problem/Motivation

There is a transparent div over some forms when the admin toolbar is oriented vertically. This is a problem because sometimes the fields/buttons that are placed under are unreachable.

Steps to Reproduce:
1.- go to /admin/structure/block/block-content
2.- try to edit block description.

Proposed resolution

move the div to the left or set a lower z index for the div.

Remaining tasks

create patch, Review and commit.

User interface changes

Screeenshot Error

Data model changes

Original report by [juanse254]

Comments

juanse254 created an issue. See original summary.

juanse254’s picture

Issue summary: View changes
juanse254’s picture

Status: Active » Needs review
StatusFileSize
new548 bytes

Deleting the this sets the div to upper.

giancarlosotelo’s picture

Component: menu system » toolbar.module
Status: Needs review » Needs work

I think is not the best solution. This div cannot disappear, is the wrapper of the elements in the menu. We just have to move it.
The problem is that the toolbar is taking the position of the body which have some margin left.
So I suggest to change the position from absolute to fixed in.

body.toolbar-fixed .toolbar-oriented,
.toolbar-oriented .toolbar-bar,
.toolbar-oriented .toolbar-tray {
  left: 0;
  position: absolute;
  right: 0;
  top: 0;
}
juanse254’s picture

Status: Needs work » Needs review
StatusFileSize
new439 bytes

Maybe you are right, this also works. Im adding the patch here, some feedback needed.

andread’s picture

StatusFileSize
new66.68 KB

This solution solves the original problem, but gives problems, when scrolling on a longer page.

The buttons on the page gets positioned on top of the navigation toolbar.

andread’s picture

Status: Needs review » Needs work
lucastockmann’s picture

Status: Needs work » Needs review

Nevermind, didn't saw what Andrea was mentioning...

lucastockmann’s picture

Status: Needs review » Needs work
StatusFileSize
new136.02 KB

So back to "Needs work".

AndreaD:

This solution solves the original problem, but gives problems, when scrolling on a longer page.

The buttons on the page gets positioned on top of the navigation toolbar.

screenshot

giancarlosotelo’s picture

The toolbar is working well in other pages. So basically the problem is that in contextual pages(?) a class that has a position relative is added to the body, and this is moving the toolbar inside the body, just in those pages.

.contextual-region {
    position: relative;
}

So the possible solution is to remove the added class or change the style there.

andread’s picture

Status: Needs work » Needs review
StatusFileSize
new467 bytes

If we are keeping the position, but changing z-index, it solves the problem, without causing more.

Needs review.

juanse254’s picture

StatusFileSize
new1.24 KB

This still needs to be tested completely but seems to work.

Edit: You cant just fix the Z-Index, because despite it does the job the div remains in the same position which is not where its meant to be(Left).

juanse254’s picture

StatusFileSize
new430 bytes

Im sorry that patch 12 included unrelated stuff :).

The last submitted patch, 12: Admin_toolbar_div-2570681-8.patch, failed testing.

The last submitted patch, 12: Admin_toolbar_div-2570681-8.patch, failed testing.

giancarlosotelo’s picture

Status: Needs review » Reviewed & tested by the community

Ok, I think last patch solve the issue without problems.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs manual testing

I think we need to confirm that this does not break the toolbar on multiple browsers and multiple os's

edurenye’s picture

Status: Needs work » Needs review

Tested in linux with chrome, chromium, firefox, firefox developer edition (check diferent screen sizes), opera and Browser. And it works fine in all them.
@juanse254 Tried in Mac, so we just need somone else that try it in Windows.
I think this tests are kind of a review, so it needs review.

juanse254’s picture

Tested for 3 days now in Chrome with OSX, also tested with safari and firefox in OSX. Everything works fine.

edurenye’s picture

Status: Needs review » Reviewed & tested by the community

I tested it in Windows 10 with Edge, Chrome, Firefox and Opera.
Works perfect everywhere, so goes to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 2acfed8 and pushed to 8.0.x. Thanks!

@edurenye in future any chance you can post screenshots - it makes it easier to confirm the testing - thanks.

  • alexpott committed 2acfed8 on 8.0.x
    Issue #2570681 by juanse254, AndreaD, giancarlosotelo, lucastockmann,...
edurenye’s picture

Status: Fixed » Needs review
StatusFileSize
new404 bytes

We realise that this breaks another thing, a lot of things use contextual-region, so it breaks the edit button in the nodes.
I think that maybe it should be set in toolbar-vertical class as it is ther just in that case.

Sorry to not be able to see that before, I just tested in the forms all the time, and I didn't get that detail.

juanse254’s picture

Status: Needs review » Needs work

im not sure if that is supposed to go there..

edurenye’s picture

Status: Needs work » Needs review
StatusFileSize
new1011 bytes
new867 bytes
new987 bytes
new843 bytes

Maybe better here.
I'll try to test as much as I can.

edurenye’s picture

juanse254’s picture

StatusFileSize
new482 bytes

Okay, i think this is more acurate. After disscusion with edurenye we realized that the commited patch was breaking the edit buttons because the contextual-region is too widely used, changing the position just when the toolbar is vertical and active makes more sense.

Edit: Please use patch 24, as thats the one that fixes the issue. As tested 25 and 27 fail sometimes, please review the patch 24 and assert that nothing is wrong.

juanse254’s picture

Status: Needs review » Needs work

The last submitted patch, 27: Admin_toolbar_div-2570681-27.patch, failed testing.

juanse254’s picture

Status: Needs work » Needs review
edurenye’s picture

Status: Needs review » Fixed
alexpott’s picture

Status: Fixed » Needs work

As this as caused a regression - I've reverted this.

  • alexpott committed f3f0555 on 8.0.x
    Revert "Issue #2570681 by juanse254, AndreaD, giancarlosotelo,...
edurenye’s picture

Status: Needs work » Needs review
StatusFileSize
new581 bytes

I think this should work, I tested in Windows and Linux in all my browsers, but seeing the last time, always is posible to miss somthing, so help me to test a bit please.

codonet’s picture

Tested manually on Windows with
Opera 32.0
Firefox 41.0
Google Chrome 45.0
IE 11.0

Works ok.
I am new contributor. This issue is not assigned to anyone? Stays this way?

codonet’s picture

Assigned: Unassigned » codonet
Status: Needs review » Reviewed & tested by the community
codonet’s picture

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

Sorry. I am new contributor (learning). Now I tested it with Simply TestMe so that I am sure that I am testing the right thing. Works ok on the browsers I mentioned in the previous comment. Since I am new somebody else can change the status.

edurenye’s picture

Did you test that it works or that it didn't break anything else? We need to test that last thing more I think to not have again the same problem, but I don't know how to tell what is tested and what not, maybe we should try to identify the cases when this css is active.
Also maybe we need to test in tablets?
We don't need to assign it to anybody as it needs to be tested.

juanse254’s picture

Status: Needs review » Closed (cannot reproduce)

cant reproduce anymore.