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.

CommentFileSizeAuthor
#10 Screenshot of the Overlay open in IE10 showing that the toolbar vertical tray does not overlap the overlay.61.46 KBjessebeach
#10 Screenshot of the Overlay open in IE9 showing that the toolbar vertical tray does not overlap the overlay.70 KBjessebeach
#9 Chrome 29.0.1547.3 dev before.png114.83 KBekl1773
#9 Chrome after.png180 KBekl1773
#9 Firefox 22.0 Before.png115.98 KBekl1773
#9 Firefox After.png228.7 KBekl1773
#9 IE 10 Before.png116.91 KBekl1773
#9 IE 10 After.png130.25 KBekl1773
#9 Opera 12.15 Before.png116.03 KBekl1773
#9 Opera After.png166.26 KBekl1773
#9 Safari 5.1.9 Before.png115.52 KBekl1773
#9 Safari After.png117.51 KBekl1773
#7 box-sizing-vendor-prefixes-2028965-7.patch396 bytesekl1773
#1 box-sizing-vendor-prefixes-2028965-1.patch947 bytesjessebeach
Screenshot of the overlay open in Firefox. The right edge of the overlay is outside the viewport.249.56 KBjessebeach

Comments

jessebeach’s picture

jessebeach commented
Status: Active » Needs review
Issue tags: +CSS novice
StatusFileSize
new box-sizing-vendor-prefixes-2028965-1.patch947 bytes
yesct’s picture

yesct commented

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

Snugug commented 
+++ 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

jessebeach commented

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

Snugug commented

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

jessebeach commented

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

ekl1773
she/her
Primary language English
commented
StatusFileSize
new box-sizing-vendor-prefixes-2028965-7.patch396 bytes

Rerolled to reorder vendor prefixes.

jessebeach’s picture

jessebeach commented

It applies, thanks @ekl1773!

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

ekl1773’s picture

ekl1773
she/her
Primary language English
commented
StatusFileSize
new Safari After.png117.51 KB
new Safari 5.1.9 Before.png115.52 KB
new Opera After.png166.26 KB
new Opera 12.15 Before.png116.03 KB
new IE 10 After.png130.25 KB
new IE 10 Before.png116.91 KB
new Firefox After.png228.7 KB
new Firefox 22.0 Before.png115.98 KB
new Chrome after.png180 KB
new Chrome 29.0.1547.3 dev before.png114.83 KB

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

jessebeach commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new Screenshot of the Overlay open in IE9 showing that the toolbar vertical tray does not overlap the overlay.70 KB
new Screenshot of the Overlay open in IE10 showing that the toolbar vertical tray does not overlap the overlay.61.46 KB

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

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
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.