Problem/Motivation

The overlay child element is set to width 100% with left/right padding.

#overlay {
  box-sizing: border-box;
  padding: 2em 40px;
  width: 100%;
}

In a content-box box model (default) this will cause the box to appear larger than 100% of its container, as seen in Firefox.

Screenshot of the overlay open in Firefox. The right edge of the overlay is outside the viewport.

We're missing vendor prefixes for -moz and -webkit.

Proposed resolution

Add vendor-prefixed version of the box-sizing property.

Remaining tasks

Review the patch.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jessebeach’s picture

Status: Active » Needs review
Issue tags: +CSS novice
FileSize
947 bytes
YesCT’s picture

To review, this might help: https://drupal.org/contributor-tasks/manual-testing
and I think this needs before and after screenshots in different browsers,
as that contributor task document describes:

If the patch for the issue includes changes to CSS or JavaScript, it will need to be tested in all supported web browsers for that version of Drupal. (Link here.) Try to reproduce the issue in as many supported browsers as are available to you.

Snugug’s picture

+++ b/core/modules/overlay/css/overlay-child.cssundefined
@@ -15,6 +15,8 @@
+  -moz-box-sizing: border-box;
+  -webkit-box-sizing: border-box;

These two lines should be switched, the -webkit property should come before the -moz property.

Additionally, if we are not supporting Android < 4 in Drupal 8 Core, we can drop the -webkit prefix as per http://caniuse.com/css3-boxsizing

jessebeach’s picture

These two lines should be switched, the -webkit property should come before the -moz property.

Is there really an ordering concern? I just put them in alphabetical order. Do we really need to reroll this to swap the order? Is there a case you know of where the order matters? Truly, you've got my interest piqued now :)

Android versions 2+ are still fairly popular and one extra line doesn't cost us much. Not that one would be using the overlay on a phone. Maybe a tablet. In any case, it's a byte of prevention.

Snugug’s picture

I agree that Android 2.3 is still important, just was curious as to if we were still targeting it. We should be, so I'm OK with keeping it.

As for the ordering, it's not a matter of CSS override ordering for this feature, but when it comes to standards, when writing vendor prefixes, the correct order is -webkit, -moz, -ms, -o, standard. This is important for vendor prefixed properties that change when they become standards, but just so happens to not be the case here. That being said, I'd prefer to see us follow best practices when dealing with vendor prefixes even if it's swapping two lines of code.

jessebeach’s picture

Incidence of -moz before -webkit in core modules.

/^\s+(\-moz).*\n.*\n?\s+(\-webkit).*$/g

and the reverse.

/^\s+(\-webkit).*\n.*\n?\s+(\-moz).*$/g

-webkit then -moz is slightly more prevalent as a pattern. elk1773, said she wanted to review this. Want to reroll and reverse the order of the properties?

ekl1773’s picture

Rerolled to reorder vendor prefixes.

jessebeach’s picture

It applies, thanks @ekl1773!

We still need a manual testing review to get this RTBC'd.

ekl1773’s picture

Alright, I did what I could testing in all the browsers, but I didn't do any mobile testing. It looks like this solves the original problem, but when I tested in IE 10, the overlay slides under the menu to the left. Second opinion on the IE situation?

Chrome 29.0.1547.3 dev before.png Chrome after.png

Firefox 22.0 Before.pngFirefox After.png

IE 10 Before.pngIE 10 After.png

Opera 12.15 Before.pngOpera After.png

Safari 5.1.9 Before.pngSafari After.png

jessebeach’s picture

We ran into this issue in a bad way during usability tests. The fix is super simple and makes a big difference for end users. Let's please get it in.

I've checked IE9 and IE10 for the overlap issue mentioned by ekl1773 in #9. Changes to the Toolbar CSS since this comment was posted have resolved the overlap issue:

Screenshot of the Overlay open in IE10 showing that the toolbar vertical tray does not overlap the overlay.

Screenshot of the Overlay open in IE9 showing that the toolbar vertical tray does not overlap the overlay.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 075b602 and pushed to 8.x. Thanks!

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