Firefox does not respect the width set on a fieldset when inner content is larger than it. So having max-width: 100% does not work on images and such.

image overflow of fieldset

Replacing the fieldset with a normal div makes everything work just fine.

CommentFileSizeAuthor
#17 panopoly_test-preview-fieldset-2467073-16.patch570 bytescboyden
#17 interdiff-15-16.txt593 bytescboyden
#16 interdiff-15-16.txt0 bytescboyden
#16 panopoly_test-preview-fieldset-2467073-16.patch601 bytescboyden
#15 panopoly_test-preview-fieldset-2467073.patch601 bytescboyden
#14 panopoly-reusable-multi-preview.png324.34 KBcboyden
#14 panopoly-reusable-single-preview.png186.8 KBcboyden
#14 panopoly-large-image-preview.png552.96 KBcboyden
#12 interdiff.txt884 bytesdsnopek
#12 panopoly_magic-preview-fieldset-2467073-12.patch8.35 KBdsnopek
#11 panopoly_magic-preview-fieldset-2467073-11.patch8.35 KBdsnopek
Screen Shot 2015-04-07.png163.6 KBsegovia94

Comments

segovia94’s picture

segovia94 commented

Here is a followup issue in Firefox.
https://bugzilla.mozilla.org/show_bug.cgi?id=504622

Here is a codepen illustrating things.
http://codepen.io/segovia94/pen/pvmOMN

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented

Thanks for reporting this and doing the research! Looks like there are a couple workarounds in the Firefox bug you link to, so that should provide a place to start when creating a patch to fix this.

segovia94’s picture

segovia94 commented

Trying to add display: table-cell; doesn't work with the float: right;. If you remove the float then everything is fine. Of course, that doesn't really help. Moving the float and 48% width to the wrapping div works, but I'm not sure how easy it would be to target it since it's just an empty div.

segovia94’s picture

segovia94 commented

This seems to be working initially.

#modal-content fieldset.widget-preview-single .fieldset-wrapper {
  box-sizing: border-box; 
  display: table;
  table-layout: fixed;
  width: 100%;
}
segovia94’s picture

segovia94 commented

#4 causes the max-height to be ignored which means the preview window could grow to be really tall.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented

What about setting a 'height' rather than 'max-height'?

segovia94’s picture

segovia94 commented

Height was ignored also.

segovia94’s picture

segovia94 commented

I've been trying to fix this lately with Flexbox, but the <fieldset> element still has the same issues. Is anyone against removing the use of the <fieldset> for the preview container and instead using a <div> or some other html5 element like a <section>. It wouldn't be difficult to style it the same, and <fieldset> is semantically incorrect anyway since it has nothing to do with form elements.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented

Is anyone against removing the use of the <fieldset> for the preview container and instead using a <div> or some other html5 element like a <section>. It wouldn't be difficult to style it the same, and <fieldset> is semantically incorrect anyway since it has nothing to do with form elements.

I'd be fine with that!

One thing to note, this will probably conflict with the patch here which is almost finished:

#2155377: Only show one widget preview at a time when adding content in panels

Ideally, it'd be good to keep in mind how the two sets of changes will merge..

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented

This issue will be much easier to implement once #2496027: Generate preview markup in a theme function/template is fixed!

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented
Status: Active » Needs review
StatusFileSize
new panopoly_magic-preview-fieldset-2467073-11.patch8.35 KB

Here's a first pass at this!

This has been trickier to get right than I thought it would be. The CSS still isn't quite right, and is doing some weird things when hovering over the "Add" button for example, so it not only needs more work from me, but could also use some extensive testing and review.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented
StatusFileSize
new panopoly_magic-preview-fieldset-2467073-12.patch8.35 KB
new interdiff.txt884 bytes

Here's a new version that fixes the issues with the "Add" button. It's a little strange because the previous CSS was a one-to-one conversion changing 'legend' to '.panopoly-magic-preview-title-wrapper', so you'd think it would work, but instead using '.panopoly-magic-preview-title-inner' gets the desired effect.

I haven't been able to find any other issues with the styling! More testing and review is welcome. :-)

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented

Here's an issue to make corresponding changes in Radix: #2688455: Don't use <fieldset> in Panopoly preview

cboyden’s picture

cboyden commented
StatusFileSize
new panopoly-large-image-preview.png552.96 KB
new panopoly-reusable-single-preview.png186.8 KB
new panopoly-reusable-multi-preview.png324.34 KB

This looks good on Responsive Bartik. Screenshots were taken on Firefox 45/Mac OSX 10.11.3.

Preview of Add Text widget with a 1024px wide image:
Screenshot showing preview of Add Text widget with a large image

Adding the reusable widget in Single Preview mode:
Screenshot showing the Add Reusable Content screen with the large image widget in single preview mode

Adding a reusable widget in Automatic Preview mode, with one 640px wide image and one 1024px wide image:
Screenshot showing the Add Reusable Content screen with TWO large image widgets in automatic preview mode

cboyden’s picture

cboyden commented
StatusFileSize
new panopoly_test-preview-fieldset-2467073.patch601 bytes

Behat tests relying on live preview will fail because the region definition needs to change. I've attached a patch for panopoly_test that updates behat.common.yml.

cboyden’s picture

cboyden commented
StatusFileSize
new panopoly_test-preview-fieldset-2467073-16.patch601 bytes
new interdiff-15-16.txt0 bytes

Hm, the previous patch will cause upgrade tests to fail. New patch has a less-specific Live Preview region definition.

cboyden’s picture

cboyden commented
StatusFileSize
new interdiff-15-16.txt593 bytes
new panopoly_test-preview-fieldset-2467073-16.patch570 bytes

Previous comment had incorrect patch and interdiff.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented

After fixing #2688644: Pin to Drush 8 so we can keep testing with PHP 5.4, this appears to be working on Travis-CI:

https://travis-ci.org/panopoly/panopoly/builds/116469778

Just waiting for the update build to finish and hopefully it'll pass too. :-)

  • dsnopek committed 5cceb87 on 7.x-1.x 
    Update Panopoly Magic and Test for Issue #2467073 by cboyden, dsnopek,...
dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
commented
Status: Needs review » Fixed

Tests passed! Committed. :-) Thanks @cboyden and @segovia94!

Status: Fixed » Closed (fixed)

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