amperize icon indicating copy to clipboard operation
amperize copied to clipboard

img with unreachable src blocks all amp html

Open NNSTH opened this issue 7 years ago • 6 comments

Why do you fail all amp content if one image has wrong src? please leavi it for config option- what to do with wrong src: throw error, ignore and put it as amp-img and try using alt, etc.

I fail with trying to amperize my html like this: A marginal cost analysis graph with x axis Quantity and y axis Price. There are no units on the x or y axis, but assuming 0 to 10 for both axes. The marginal cost curve starts at 0, 4 and slopes down to 4, 2. It then rises from 4, 2 to 10, 10. There is a marginal cost line at extending from 0, 6 to 10, 6. The letters A to D are on the graph at the following locations: A 2, 2, B 6, 4, C 8, 6, D 10, 10.

NNSTH avatar Feb 11 '18 13:02 NNSTH

Hey @NNSTH

The parser will not stop after one broken image src. It will just return the original value, but continues with the other tags.

Using your provided HTML, I ended up having this result:

<img src="http://hillsborough.flvs.net/webdav/assessment_images/educator_econ_v13/06_06A_exam/0606_g8.jpg" alt="A marginal cost analysis graph with x axis Quantity and y axis Price. There are no units on the x or y axis, but assuming 0 to 10 for both axes. The marginal cost curve starts at 0, 4 and slopes down to 4, 2. It then rises from 4, 2 to 10, 10. There is a marginal cost line at extending from 0, 6 to 10, 6. The letters A to D are on the graph at the following locations: A 2, 2, B 6, 4, C 8, 6, D 10, 10.">
<amp-anim src="http://hillsborough.flvs.net/educator/images/spellcheck.gif" alt="" width="96" height="24" layout="fixed"></amp-anim>

There's a problem with accessing the first image (first it gets redirected, then the access is forbidden.). You can clearly see, that the second images (which is a .gif) gets transformed correctly into amp-anim.

I'm closing this issue, as it's not an issue with amperize.

aileen avatar Feb 12 '18 17:02 aileen

Hi @AileenCGN Sorry for the misunderstanding- I just ask: why do you leave the first image as it is? this breaks my page in AMP validator, and that's is not correct- if you can't get the image src- then it has to show alt value, but in any case I want the page to be valid AMP HTML. re-open it, thanks for your help

NNSTH avatar Feb 13 '18 06:02 NNSTH

Hey @NNSTH

I see. Yeah, you are right. A failure in the request should result in a converted img tag, but with the fallback default width and height values.

aileen avatar Feb 14 '18 10:02 aileen

You can see the fixes I've made to amperize.js in order to fix it the way I thought: If img src fails to load- I just made it as with defaults, if img has no src- if it has alt/title- I append a

{alt/title text}
, if it is blank- I just skip it

//skip invalid amp components function skip() { step(null, html); }

    //add div fallback to amp-image in case it has no correct src- but has alt/title
    function addImgFallback(element) {
        Object.assign(element, {
            name: `amp-${element.name}`,
            attribs: {
                width: 5,
                height: 5,
                src: 'blank'
            },
            children: [{
                type: 'tag',
                name: 'div',
                attribs: { fallback: '' },
                children: [{ data: `${element.attribs.alt || element.attribs.title}`, type: 'text' }],
                prev: null,
                next: null,
                parent: element
            }]
        });
        return enter();
    }

    function applyDefaults(element, name) {
        element.name = name;
        element.attribs.width = self.config[name].width;
        element.attribs.height = self.config[name].height;
    }

and then: function getImageSize(element) { var imageObj = url.parse(element.attribs.src), requestOptions, timeout = 3000;

        if (!validator.isURL(imageObj.href)) {

            if (element.attribs.alt || element.attribs.title)
                return addImgFallback(element);
            else
                return skip();
        }

        // We need the user-agent, otherwise some https request may fail (e. g. cloudfare)
        requestOptions = {
            headers: {
                'User-Agent': 'Mozilla/5.0 Safari/537.36'
            },
            timeout: timeout,
            encoding: null
        };

        return got(
            imageObj.href,
            requestOptions
        ).then(function (response) {
            try {
                // Using the Buffer rather than an URL requires to use sizeOf synchronously.
                // See https://github.com/image-size/image-size#asynchronous
                var dimensions = sizeOf(response.body);

                // CASE: `.ico` files might have multiple images and therefore multiple sizes.
                // We return the largest size found (image-size default is the first size found)
                if (dimensions.images) {
                    dimensions.width = _.maxBy(dimensions.images, function (w) { return w.width; }).width;
                    dimensions.height = _.maxBy(dimensions.images, function (h) { return h.height; }).height;
                }

                element.attribs.width = dimensions.width;
                element.attribs.height = dimensions.height;

                return getLayoutAttribute(element);
            } catch (err) {
                // fix amperize- apply defaults (name & dimensions) to this element
                applyDefaults(element, 'amp-img');
                return enter();
            }
        }).catch(function () {
            // fix amperize- apply defaults (name & dimensions) to this element
            applyDefaults(element, 'amp-img');
            return enter();
        });
    }

    if ((element.name === 'img' || element.name === 'iframe') && !element.attribs.src) {
        if (element.attribs.alt || element.attribs.title)
            return addImgFallback(element);
        else
            return skip();
    }

NNSTH avatar Feb 14 '18 10:02 NNSTH

@AileenCGN Is that important for Ghost?

kirrg001 avatar Feb 14 '18 17:02 kirrg001

@kirrg001 It shouldn't be bad. I need to check the behaviour if the image is not accessible and therefore can't be shown. In Ghost, we currently strip those tags out and they're just missing. But the expected behaviour should be that we transform the tag, give it default values and and show the alt. But I need to research, how Ghost behaves with this and Google.

aileen avatar Feb 15 '18 05:02 aileen