fu: Close-up of Fu, bringing a scoop of water to her mouth (Default)
fu ([personal profile] fu) wrote in [site community profile] changelog2011-08-25 12:06 pm
  • Add Memory
  • Share This Entry

[dw-free] soundcloud.com embeds not embedded correctly

[commit: http://hg.dwscoalition.org/dw-free/rev/42d9e8a2718f]

http://bugs.dwscoalition.org/show_bug.cgi?id=3814

Allow width/height of embedded objects to be set in % rather than just in
px.

Patch by [personal profile] fu.

Files modified:
  • cgi-bin/LJ/EmbedModule.pm
  • cgi-bin/weblib.pl
  • t/clean-embed.t
--------------------------------------------------------------------------------
diff -r 8ab23737dca8 -r 42d9e8a2718f cgi-bin/LJ/EmbedModule.pm
--- a/cgi-bin/LJ/EmbedModule.pm	Thu Aug 25 19:00:22 2011 +0800
+++ b/cgi-bin/LJ/EmbedModule.pm	Thu Aug 25 20:05:33 2011 +0800
@@ -29,6 +29,8 @@
     # maximum embed width and height
     MAX_WIDTH => 800,
     MAX_HEIGHT => 800,
+    MAX_WIDTH_PERCENT => 100,
+    MAX_HEIGHT_PERCENT => 100,
 };
 
 my %embeddable_tags = map { $_ => 1 } qw( object embed iframe );
@@ -247,6 +249,14 @@
     $$postref = $newtxt;
 }
 
+# only allow percentage as a unit
+sub _extract_num_unit {
+    my $num = $_[0];
+
+    return ( $num, "" ) unless $num =~ s/%//;
+    return ( $num, "%" );
+}
+
 sub module_iframe_tag {
     my ($class, $u, $moduleid, %opts) = @_;
 
@@ -260,6 +270,8 @@
     my $content = $class->module_content( moduleid => $moduleid, journalid => $journalid, preview => $preview );
     my $width = 0;
     my $height = 0;
+    my $width_unit = "";
+    my $height_unit = "";
     my $p = HTML::TokeParser->new(\$content);
     my $embedcodes;
 
@@ -276,15 +288,23 @@
             my $attr = $token->[2];  # hashref
 
             if ($type eq "S") {
-                my ($elewidth, $eleheight);
+                my ($elewidth, $eleheight, $elewidth_unit, $eleheight_unit);
 
                 if ($attr->{width}) {
-                    $elewidth = $attr->{width}+0;
-                    $width = $elewidth if $elewidth > $width;
+                    ( $elewidth, $elewidth_unit ) = _extract_num_unit( $attr->{width} );
+                    $elewidth += 0;
+                    if ( $elewidth > $width ) {
+                        $width = $elewidth;
+                        $width_unit = $elewidth_unit;
+                    }
                 }
                 if ($attr->{height}) {
-                    $eleheight = $attr->{height}+0;
-                    $height = $eleheight if $eleheight > $height;
+                    ( $eleheight, $eleheight_unit ) = _extract_num_unit( $attr->{height} );
+                    $eleheight += 0;
+                    if ( $eleheight > $height ) {
+                        $height = $eleheight;
+                        $height_unit = $eleheight_unit;
+                    }
                 }
 
                 my $flashvars = $attr->{flashvars};
@@ -318,8 +338,8 @@
         }
 
         # add padding
-        $width += 50 if $width;
-        $height += 50 if $height;
+        $width += 50 if $width && ! $width_unit;
+        $height += 50 if $height && ! $height_unit;
     }
 
     # use explicit values if we have them
@@ -331,9 +351,19 @@
 
     # some dimension min/maxing
     $width = 50 if $width < 50;
-    $width = MAX_WIDTH if $width > MAX_WIDTH;
     $height = 50 if $height < 50;
-    $height = MAX_HEIGHT if $height > MAX_HEIGHT;
+
+    if ( $width_unit eq "%" ) {
+        $width = MAX_WIDTH_PERCENT if $width > MAX_WIDTH_PERCENT;
+    } else {
+        $width = MAX_WIDTH if $width > MAX_WIDTH;
+    }
+
+    if ( $height_unit eq "%" ) {
+        $height = MAX_HEIGHT_PERCENT if $height > MAX_HEIGHT_PERCENT;
+    } else {
+        $height = MAX_HEIGHT if $height > MAX_HEIGHT;
+    }
 
     # safari caches state of sub-resources aggressively, so give
     # each iframe a unique 'name' and 'id' attribute
@@ -344,7 +374,7 @@
     my $auth_token = LJ::eurl(LJ::Auth->sessionless_auth_token('embedcontent', moduleid => $moduleid, journalid => $journalid, preview => $preview,));
     my $iframe_link = qq{http://$LJ::EMBED_MODULE_DOMAIN/?journalid=$journalid&moduleid=$moduleid&preview=$preview&auth_token=$auth_token};
     my $iframe_tag = qq {<iframe src="$iframe_link" } .
-        qq{width="$width" height="$height" allowtransparency="true" frameborder="0" class="lj_embedcontent" id="$id" name="$name"></iframe>};
+        qq{width="$width$width_unit" height="$height$height_unit" allowtransparency="true" frameborder="0" class="lj_embedcontent" id="$id" name="$name"></iframe>};
 
     my $remote = LJ::get_remote();
     return $iframe_tag unless $remote;
@@ -371,7 +401,9 @@
                                 placeholder_html => $iframe_tag,
                                 link             => $iframe_link,
                                 width            => $width,
+                                width_unit       => $width_unit,
                                 height           => $height,
+                                height           => $height_unit,
                                 img              => "$LJ::IMGPREFIX/videoplaceholder.png",
                                 );
 }
diff -r 8ab23737dca8 -r 42d9e8a2718f cgi-bin/weblib.pl
--- a/cgi-bin/weblib.pl	Thu Aug 25 19:00:22 2011 +0800
+++ b/cgi-bin/weblib.pl	Thu Aug 25 20:05:33 2011 +0800
@@ -3748,11 +3748,13 @@
     my $placeholder_html = LJ::ejs_all(delete $opts{placeholder_html} || '');
     my $width  = delete $opts{width}  || 100;
     my $height = delete $opts{height} || 100;
+    my $width_unit  = delete $opts{width_unit}  || "px";
+    my $height_unit = delete $opts{height_unit} || "px";
     my $link   = delete $opts{link}   || '';
     my $img    = delete $opts{img}    || "$LJ::IMGPREFIX/videoplaceholder.png";
 
     return qq {
-            <div class="LJ_Placeholder_Container" style="width: ${width}px; height: ${height}px;">
+            <div class="LJ_Placeholder_Container" style="width: ${width}${width_unit}; height: ${height}${height_unit};">
                 <div class="LJ_Placeholder_HTML" style="display: none;">$placeholder_html</div>
                 <div class="LJ_Container"></div>
                 <a href="$link">
diff -r 8ab23737dca8 -r 42d9e8a2718f t/clean-embed.t
--- a/t/clean-embed.t	Thu Aug 25 19:00:22 2011 +0800
+++ b/t/clean-embed.t	Thu Aug 25 20:05:33 2011 +0800
@@ -1,7 +1,7 @@
 # -*-perl-*-
 use strict;
 
-use Test::More tests => 151;
+use Test::More tests => 175;
 use lib "$ENV{LJHOME}/cgi-bin";
 require 'ljlib.pl';
 
@@ -54,6 +54,7 @@
     $clean->();
     is( $orig_post, $clean_post, "Drop the data attribute" );
 
+
     note("script tag");
     $orig_post = qq{<object><script>bar</script></object>};
     $clean_post = qq{<object></object>};
@@ -228,6 +229,30 @@
             qr{\s*},
         ],
 
+        [   "dimensions: object tag with dimensions in percent",
+            qq{<site-embed id="1"><object width="100%" height="100%"></object></site-embed>},
+            qq{<site-embed id="1"/>},
+            qq{<site-embed id="1"><object width="100%" height="100%"></object></site-embed>},
+            qr{width="100%" height="100%"},
+            qq{<object width="100%" height="100%"></object>},
+        ],
+
+        [   "dimensions: object tag with mixed units for dimensions",
+            qq{<site-embed id="1"><object width="80%" height="200"></object></site-embed>},
+            qq{<site-embed id="1"/>},
+            qq{<site-embed id="1"><object width="80%" height="200"></object></site-embed>},
+            qr{width="80%" height="250"},
+            qq{<object width="80%" height="200"></object>},
+        ],
+
+        [   "dimensions: object tag with dimensions in percent -- too big",
+            qq{<site-embed id="1"><object width="1000%" height="101%"></object></site-embed>},
+            qq{<site-embed id="1"/>},
+            qq{<site-embed id="1"><object width="1000%" height="101%"></object></site-embed>},
+            qr{width="100%" height="100%"},
+            qq{<object width="1000%" height="101%"></object>},
+        ],
+
 
         [   "object tag; no site-embed",
             qq{foo <object>bar</object>baz},
@@ -250,16 +275,16 @@
 
         [   "embed tag; no site-embed",
             qq{foo <embed>bar</embed>baz},
-    
+
             qq{foo <site-embed id="1"/>baz},
             qq{foo <site-embed id="1"><embed>bar</embed></site-embed>baz},
             qr{foo ${iframe}baz},
             qq{<embed>bar</embed>},
         ],
-    
+
         [   "embed tag with site-embed",
             qq{foo <site-embed><embed></embed></site-embed> baz},
-    
+
             qq{foo <site-embed id="1"/> baz},
             qq{foo <site-embed id="1"><embed></embed></site-embed> baz},
             qr{foo $iframe baz},
@@ -385,7 +410,7 @@
         # TODO: DANGER: EATS EVERYTHING PAST THE OPEN TAG
         [   "iframe tag left open in site-embed (trusted)",
             qq{foo <site-embed><iframe src="http://www.youtube.com/embed/ABC123abc_-"></site-embed> baz},
-    
+
             qq{foo },
             qq{foo },
             qr{foo },
--------------------------------------------------------------------------------
Flat | Top-Level Comments Only
ninetydegrees: Art & Text: heart with aroace colors, "you are loved" (Default)

[personal profile] ninetydegrees 2011-08-25 12:12 pm (UTC)(link)
Yay! Thank you!
fu: Close-up of Fu, bringing a scoop of water to her mouth (Default)

[personal profile] fu 2011-08-25 04:34 pm (UTC)(link)
Thank you for pointing it out in the first place!
Flat | Top-Level Comments Only