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.

Comments

jwilson3’s picture

Title: Media query bug between node.module.css and seven/styles.css » Node form layout bug when toolbar collapsed
jwilson3’s picture

Status: Active » Needs review
Issue tags: +Needs manual testing
StatusFileSize
new780 bytes
lewisnyman’s picture

Issue tags: +frontend, +CSS
lewisnyman’s picture

I'm confused as to what I'm testing, can I see a before/after screenshot? Thanks!

Status: Needs review » Needs work

The last submitted patch, 2: seven-nodeform-toolbar-collapsed-2330661-2.patch, failed testing.

jwilson3’s picture

Status: Needs work » Needs review
StatusFileSize
new787 bytes

Needed 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 ;)

jwilson3’s picture

StatusFileSize
new629 bytes

When 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.

jwilson3’s picture

StatusFileSize
new167.79 KB
new181.61 KB

Before/after screenshots of node/add/article at 1021 pixels wide screen width, with vertical toolbar expanded.

jwilson3’s picture

It 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:

@media
  screen and (min-width: 780px),
  (orientation: landscape) and (min-device-height: 780px) {

  [class*="page-node-add-"] .messages,
  .page-node-edit .messages {
    margin-top: 1em;
    margin-bottom: 1em;
  }
}

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.

lewisnyman’s picture

Status: Needs review » Reviewed & tested by the community

Ah 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.

alexpott’s picture

This 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.

webchick’s picture

Status: Reviewed & tested by the community » Needs review
jwilson3’s picture

Ok, 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-add and page-node-edit classes) but needs a separate issue.

risse’s picture

StatusFileSize
new188.5 KB

Took a screenshot with an iPad Air and it looks good. There's some whitespace beneath the initial screen.

Bojhan’s picture

Assigned: Unassigned » lewisnyman

@Lewis could you take a look at this? Looks like this patch is correct.

lewisnyman’s picture

Assigned: lewisnyman » Unassigned
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing, -Needs issue summary update

Ok, 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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: seven-nodeform-toolbar-collapsed-2330661-8.patch, failed testing.

lewisnyman’s picture

Issue tags: +Needs reroll
amitgoyal’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll +#dcdelhi
StatusFileSize
new627 bytes

Re-roll of #8.

lewisnyman’s picture

Status: Needs review » Reviewed & tested by the community

Thanks. Back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Less CSS. nice. Committed 1505993 and pushed to 8.0.x. Thanks!

  • alexpott committed 1505993 on 8.0.x
    Issue #2330661 by jwilson3, Risse, amitgoyal: Node form layout bug when...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.