Testing the patch on #2307533: Insufficient space at page bottom has turned up a bug where the node form container loses its clearfix while elements are still floated, causing the form submit buttons to sit too close to the bottom of the screen.
The problem reveals itself for me in Chrome browser on mac, on the node edit page at screen width <= 975px, with the admin menu "Manage" tab (sidebar) collapsed.
The problem boils down to an interaction between the Seven theme stylesheet, and the Node module stylesheet that has to manually be kept in sync which is challenging:
Problematic code:
<div class="layout-node-form clearfix">
//core/themes/seven/css/style.css
/**
* Widescreen
*
* Both of the following media queries must *exactly* match what is in
* node.module.css. This is rather crazy.
*
* @todo Figure out how to reduce media query duplication across files
* and modules. Layout styles only allowed in themes?
*/
@media
screen and (max-width: 1020px),
(orientation: landscape) and (max-device-height: 1020px) {
// ... other code here ...
.toolbar-vertical .layout-node-form:after {
display: none;
}
}
The above code essentially unsets the "clearfix" defined on the div=
// Core/modules/node/css/node.module.css
/**
* The vertical toolbar mode gets triggered for narrow screens, which throws off
* the intent of media queries written for the viewport width. When the vertical
* toolbar is on, we need to suppress layout for the original media width + the
* toolbar width (240px). In this case, 240px + 780px.
*/
@media
screen and (max-width: 1020px) {
.toolbar-vertical.toolbar-tray-open .layout-region-node-main,
.toolbar-vertical.toolbar-tray-open .layout-region-node-footer,
.toolbar-vertical.toolbar-tray-open .layout-region-node-secondary {
float: none;
width: auto;
padding-right: 0;
}
}
The media queries match, but the css rules do not. Because the toolbar is collapsed the node modules css doen't unfloat the items, but the seven stylesheet still does unset the clearfix.
| Comment | File | Size | Author |
|---|---|---|---|
| #20 | seven-nodeform-toolbar-collapsed-2330661-20.patch | 627 bytes | amitgoyal |
| #15 | 2014-09-27 15.06.46.png | 188.5 KB | risse |
| #9 | 2330661-after.png | 181.61 KB | jwilson3 |
| #9 | 2330661-before.png | 167.79 KB | jwilson3 |
| #8 | seven-nodeform-toolbar-collapsed-2330661-8.patch | 629 bytes | jwilson3 |
Comments
Comment #1
jwilson3Comment #2
jwilson3Comment #3
lewisnymanComment #4
lewisnymanI'm confused as to what I'm testing, can I see a before/after screenshot? Thanks!
Comment #7
jwilson3Needed a re-roll due to #2321505: Split Seven's style.css into SMACSS categories. Screenshots on the way assuming I can get this patch to apply on simplytest.me ;)
Comment #8
jwilson3When trying to take the screenshots, I found that the approach above doesn't even fix the issue -- but removing the entire media query seemed to work.
Comment #9
jwilson3Before/after screenshots of node/add/article at 1021 pixels wide screen width, with vertical toolbar expanded.
Comment #10
jwilson3It appears that this media query is a vestige of #1838156: Implement the two-column layout for the new create content page, most of which had already been gutted out of the seven/style.css file a while ago.
Also, I'm thinking maybe we should just clean up this entire node-add.css file of what will be left after this:
Wondering why this isnt just using
.page-node-add? And why this has to be inside a media query special case for this page only. Seems very odd.Comment #11
lewisnymanAh I see it now. This was before my time. Is all the layout CSS in the module now?
Either way, I love fixing bugs by deleting CSS. Great stuff.
Comment #12
alexpottThis looks to be iPad/tablet related code are we are we're not removing something we need? Also the issue summary seems out of date.
Comment #13
webchickComment #14
jwilson3Ok, so we need to test on ipad.
Also: #2028053: Add typographic styles, components, and utility classes has an alternate solution for the whitespace "leader-double" "trailer-double", which would let us remove the remaining stuff in node-add.css ( the
page-node-addandpage-node-editclasses) but needs a separate issue.Comment #15
risse commentedTook a screenshot with an iPad Air and it looks good. There's some whitespace beneath the initial screen.
Comment #16
Bojhan commented@Lewis could you take a look at this? Looks like this patch is correct.
Comment #17
lewisnymanOk, I reviewed this on an iPad with all the orientations and the different toolbar tray positions and I can't find any problems. I guess the problem that this CSS was fixing has been fixed by other means since then? The design of the node layout form has changed a little since initial implementation. I am happy.
Comment #19
lewisnymanComment #20
amitgoyal commentedRe-roll of #8.
Comment #21
lewisnymanThanks. Back to RTBC.
Comment #22
alexpottLess CSS. nice. Committed 1505993 and pushed to 8.0.x. Thanks!