Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

img with unreachable src blocks all amp html #112

Open
NNSTH opened this issue Feb 11, 2018 · 6 comments
Open

img with unreachable src blocks all amp html #112

NNSTH opened this issue Feb 11, 2018 · 6 comments
Assignees

Comments

@NNSTH
Copy link

NNSTH commented Feb 11, 2018

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
Copy link
Collaborator

aileen commented Feb 12, 2018

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 aileen closed this as completed Feb 12, 2018
@NNSTH
Copy link
Author

NNSTH commented Feb 13, 2018

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
Copy link
Collaborator

aileen commented Feb 14, 2018

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 aileen reopened this Feb 14, 2018
@NNSTH
Copy link
Author

NNSTH commented Feb 14, 2018

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
Copy link
Collaborator

@AileenCGN Is that important for Ghost?

@aileen
Copy link
Collaborator

aileen commented Feb 15, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants