jbhannah / amperize

AMP up your plain HTML
MIT License
38 stars 14 forks source link

img with unreachable src blocks all amp html #112

Open NNSTH opened 6 years ago

NNSTH commented 6 years ago

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.
aileen commented 6 years ago

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.

NNSTH commented 6 years ago

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

aileen commented 6 years ago

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.

NNSTH commented 6 years ago

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();
    }
kirrg001 commented 6 years ago

@AileenCGN Is that important for Ghost?

aileen commented 6 years ago

@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.