jenkins-bot has submitted this change and it was merged.

Change subject: Get height and width from style to placeholder
......................................................................


Get height and width from style to placeholder

This reverses the mistake in T143768 and instead copies the dimensions
from the style attribute to the element so that a placeholder element
shouldn't be without dimensions.

It is possible that there are elements without width and height
attributes or styles but we ignore this case.

Other styles are not passed to the image as due to the different HTML
structure they could lead to unexpected side effects as we discovered
in I0a42887429fa64034a04bdb53eae9b8814cd1570.

Bug: T145222
Change-Id: If2b81135d52601614803c4834dd15bde7a77aea9
---
M includes/MobileFormatter.php
M tests/phpunit/MobileFormatterTest.php
2 files changed, 147 insertions(+), 4 deletions(-)

Approvals:
  Jhobs: Looks good to me, but someone else must approve
  Jdlrobson: Looks good to me, but someone else must approve
  jenkins-bot: Verified
  Phuedx: Looks good to me, approved



diff --git a/includes/MobileFormatter.php b/includes/MobileFormatter.php
index 67d0cc9..e9f1da6 100644
--- a/includes/MobileFormatter.php
+++ b/includes/MobileFormatter.php
@@ -245,6 +245,51 @@
        }
 
        /**
+        * @see MobileFormatter#getImageDimensions
+        *
+        * @param DOMElement $img
+        * @param string $dimension Either "width" or "height"
+        * @return string|null
+        */
+       private function getImageDimension( DOMElement $img, $dimension ) {
+               $style = $img->getAttribute( 'style' );
+               $numMatches = preg_match( "/.*?{$dimension} *\: *([^;]*)/", 
$style, $matches );
+
+               if ( !$numMatches && !$img->hasAttribute( $dimension ) ) {
+                       return null;
+               }
+
+               return $numMatches
+                       ? trim( $matches[1] )
+                       : $img->getAttribute( $dimension ) . 'px';
+       }
+
+       /**
+        * Determine the user perceived width and height of an image element 
based on `style`, `width`,
+        * and `height` attributes.
+        *
+        * As in the browser, the `style` attribute takes precedence over the 
`width` and `height`
+        * attributes. If the image has no `style`, `width` or `height` 
attributes, then the image is
+        * dimensionless.
+        *
+        * @param DOMElement $img
+        * @return array with width and height parameters if dimensions are 
found
+        */
+       public function getImageDimensions( DOMElement $img ) {
+               $result = [];
+
+               foreach ( [ 'width', 'height' ] as $dimensionName ) {
+                       $dimension = $this->getImageDimension( $img, 
$dimensionName );
+
+                       if ( $dimension ) {
+                               $result[$dimensionName] = $dimension;
+                       }
+               }
+
+               return $result;
+       }
+
+       /**
         * Enables images to be loaded asynchronously
         *
         * @param DOMElement|DOMDocument $el Element or document to rewrite 
images in.
@@ -254,10 +299,10 @@
 
                foreach ( $el->getElementsByTagName( 'img' ) as $img ) {
                        $parent = $img->parentNode;
-                       $width = $img->getAttribute( 'width' );
-                       $height = $img->getAttribute( 'height' );
-                       $dimensionsStyle = ( $width ? "width: {$width}px;" : '' 
) .
-                               ( $height ? "height: {$height}px;" : '' );
+                       $dimensions = $this->getImageDimensions( $img );
+
+                       $dimensionsStyle = ( isset( $dimensions['width'] ) ? 
"width: {$dimensions['width']};" : '' ) .
+                               ( isset( $dimensions['height'] ) ? "height: 
{$dimensions['height']};" : '' );
 
                        // HTML only clients
                        $noscript = $doc->createElement( 'noscript' );
@@ -274,6 +319,10 @@
                        // Assume data saving and remove srcset attribute from 
the non-js experience
                        $img->removeAttribute( 'srcset' );
 
+                       // T145222: Add a non-breaking space inside 
placeholders to ensure that they do not report
+                       // themselves as invisible when inline.
+                       $imgPlaceholder->appendChild( 
$doc->createEntityReference( 'nbsp' ) );
+
                        // Set the placeholder where the original image was
                        $parent->replaceChild( $imgPlaceholder, $img );
                        // Add the original image to the HTML only markup
diff --git a/tests/phpunit/MobileFormatterTest.php 
b/tests/phpunit/MobileFormatterTest.php
index e25881e..8401b90 100644
--- a/tests/phpunit/MobileFormatterTest.php
+++ b/tests/phpunit/MobileFormatterTest.php
@@ -45,12 +45,21 @@
                };
 
                $citeUrl = SpecialPage::getTitleFor( 'MobileCite', '0' 
)->getLocalUrl();
+               $imageStyles = '<img src="math.jpg" style="vertical-align: 
-3.505ex; '
+                       . 'width: 24.412ex; height:7.343ex; background:none;">';
+               $placeholderStyles = '<span class="lazy-image-placeholder" '
+                       . 'style="width: 24.412ex;height: 7.343ex;" '
+                       . 'data-src="math.jpg">'
+                       . ' '
+                       . '</span>';
+               $noscriptStyles = '<noscript>' . $imageStyles . '</noscript>';
                $originalImage = '<img alt="foo" src="foo.jpg" width="100" '
                        . 'height="100" srcset="foo-1.5x.jpg 1.5x, foo-2x.jpg 
2x">';
                $placeholder = '<span class="lazy-image-placeholder" '
                        . 'style="width: 100px;height: 100px;" '
                        . 'data-src="foo.jpg" data-alt="foo" data-width="100" 
data-height="100" '
                        . 'data-srcset="foo-1.5x.jpg 1.5x, foo-2x.jpg 2x">'
+                       . ' '
                        . '</span>';
                $noscript = '<noscript><img alt="foo" src="foo.jpg" width="100" 
height="100"></noscript>';
                $refText = '<p>They saved the world with one single unit test'
@@ -103,6 +112,22 @@
                                        . '<div 
class="mf-section-1"><p>text</p>'
                                        . $noscript
                                        . $placeholder
+                                       . '</div>'
+                                       . '<h2 class="section-heading">' . 
self::SECTION_INDICATOR
+                                       . 'heading 2</h2><div 
class="mf-section-2">abc</div>',
+                               $enableSections,
+                               false, false, true,
+                       ],
+                       // Test lazy loading of images with style attributes
+                       [
+                               '<p>text</p><h2>heading 1</h2><p>text</p>' . 
$imageStyles
+                                       . '<h2>heading 2</h2>abc',
+                               '<div class="mf-section-0"><p>text</p></div>'
+                                       . '<h2 class="section-heading">' . 
self::SECTION_INDICATOR
+                                       . 'heading 1</h2>'
+                                       . '<div 
class="mf-section-1"><p>text</p>'
+                                       . $noscriptStyles
+                                       . $placeholderStyles
                                        . '</div>'
                                        . '<h2 class="section-heading">' . 
self::SECTION_INDICATOR
                                        . 'heading 2</h2><div 
class="mf-section-2">abc</div>',
@@ -381,6 +406,75 @@
        }
 
        /**
+        * @dataProvider provideGetImageDimensions
+        *
+        * @param array $expected what we expect the dimensions to be.
+        * @param string $w value of width attribute (if any)
+        * @param stirng $h value of height attribute (if any)
+        * @param string $style value of style attribute (if any)
+        * @covers MobileFormatter::getImageDimensions
+        */
+       public function testGetImageDimensions( $expected, $w, $h, $style ) {
+               $mf = new MobileFormatter( '', Title::newFromText( 'Mobile' ) );
+               $doc = new DOMDocument();
+               $img = $doc->createElement( 'img' );
+               if ( $style ) {
+                       $img->setAttribute( 'style', $style );
+               }
+               if ( $w ) {
+                       $img->setAttribute( 'width', $w );
+               }
+               if ( $h ) {
+                       $img->setAttribute( 'height', $h );
+               }
+               $this->assertEquals( $expected, $mf->getImageDimensions( $img ) 
);
+       }
+
+       public function provideGetImageDimensions() {
+               return [
+                       [
+                               [ 'width' => '500px', 'height' => '500px' ],
+                               '500',
+                               '500',
+                               ''
+                       ],
+                       [
+                               [ 'width' => '200px', 'height' => 'auto' ],
+                               '500',
+                               '500',
+                               'width: 200px; height: auto;'
+                       ],
+                       [
+                               [ 'width' => '24.412ex', 'height' => '7.343ex' 
],
+                               '500',
+                               '500',
+                               'width: 24.412ex; height: 7.343ex'
+                       ],
+                       [
+                               [ 'width' => '24.412ex', 'height' => '7.343ex' 
],
+                               '500',
+                               '500',
+                               'height: 7.343ex; width: 24.412ex'
+                       ],
+                       [
+                               [ 'width' => '24.412ex', 'height' => '7.343ex' 
],
+                               '500',
+                               '500',
+                               'height: 7.343ex; background-image: 
url(foo.jpg); width:    24.412ex   ; '
+                                       . 'font-family: "Comic Sans";'
+                       ],
+
+                       // <img src="..." alt="..." />
+                       [
+                               [],
+                               '',
+                               '',
+                               ''
+                       ]
+               ];
+       }
+
+       /**
         * @covers MobileFormatter::insertTOCPlaceholder
         */
        public function testInsertTOCPlaceholder() {

-- 
To view, visit https://gerrit.wikimedia.org/r/309409
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: If2b81135d52601614803c4834dd15bde7a77aea9
Gerrit-PatchSet: 22
Gerrit-Project: mediawiki/extensions/MobileFrontend
Gerrit-Branch: master
Gerrit-Owner: Jdlrobson <jrob...@wikimedia.org>
Gerrit-Reviewer: Bmansurov <bmansu...@wikimedia.org>
Gerrit-Reviewer: Hashar <has...@free.fr>
Gerrit-Reviewer: Jdlrobson <jrob...@wikimedia.org>
Gerrit-Reviewer: Jforrester <jforres...@wikimedia.org>
Gerrit-Reviewer: Jhobs <jhob...@wikimedia.org>
Gerrit-Reviewer: Phuedx <g...@samsmith.io>
Gerrit-Reviewer: jenkins-bot <>

_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits

Reply via email to