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

Fix broken image links on the img page #6001

Closed
wants to merge 1 commit into from

Conversation

renodubois
Copy link

@renodubois renodubois commented Jun 14, 2021

What was wrong/why is this fix needed? (quick summary only)

Images used as examples in the img page were pointing to broken URLs

MDN URL of the main page changed

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/img

@renodubois renodubois requested a review from a team as a code owner June 14, 2021 17:24
@renodubois renodubois requested review from rachelandrew and removed request for a team June 14, 2021 17:24
@sideshowbarker
Copy link
Member

Thanks for raising this. While the replacement URLs work fine time, the existing URLs also seem to work fine for me:

And for me, those images are showing up as expected in https://developer.mozilla.org/en-US/docs/Web/HTML/Element/img

@github-actions
Copy link
Contributor

Preview URLs

Flaws

None! 🎉

External URLs

URL: /en-US/docs/Web/HTML/Element/img
Title: <img>: The Image Embed element
on GitHub

No new external URLs

@wbamberg
Copy link
Collaborator

Thanks for raising this. While the replacement URLs work fine time, the existing URLs also seem to work fine for me:

* https://developer.mozilla.org/static/img/favicon144.png

* https://developer.mozilla.org/static/img/favicon72.png

And for me, those images are showing up as expected in https://developer.mozilla.org/en-US/docs/Web/HTML/Element/img

That's interesting! They are broken for me :).

@hamishwillee
Copy link
Collaborator

Broken for me too.

@sideshowbarker
Copy link
Member

That's interesting! They are broken for me :).

Broken for me too.

Very odd. They show up for me in Firefox — and in Firefox devtools I see this:

08:20:26.820 GET https://developer.mozilla.org/static/img/favicon144.png [HTTP/2 200 OK 37ms]

image

…but trying for the command line:

$ curl -i https://developer.mozilla.org/static/img/favicon144.png
HTTP/2 404

So my only guess it that I might be hitting the Cloudfront cache while y’all aren’t. Otherwise I have no clue. Maybe @mdn/core-dev has some idea.

@hamishwillee
Copy link
Collaborator

@sideshowbarker You could be right. However if the link proposed here works for everyone shouldn't we just merge this? [i.e. is this the intended file to display]?

@sideshowbarker
Copy link
Member

sideshowbarker commented Jun 15, 2021

@sideshowbarker You could be right. However if the link proposed here works for everyone shouldn't we just merge this? [i.e. is this the intended file to display]?

I have no objection to merging this — but we have other articles that use the same image URLs that this PR is changing:

$ rg -l "https://developer.mozilla.org/static/img/favicon(144)|(72).png"
files/en-us/tools/page_inspector/how_to/examine_and_edit_css/index.html
files/en-us/learn/html/introduction_to_html/the_head_metadata_in_html/index.html
files/en-us/web/html/element/link/index.html
files/en-us/web/html/element/img/index.html
files/en-us/web/html/element/menuitem/index.html
files/en-us/web/html/element/figure/index.html
files/en-us/web/manifest/index.html
files/en-us/web/svg/tutorial/other_content_in_svg/index.html

…and we also have articles with other https://developer.mozilla.org/static URLs:

$ rg  "https://developer.mozilla.org/static/" | rg -v "https://developer.mozilla.org/static/img/favicon(144)|(72).png"
files/en-us/web/api/performance/clearresourcetimings/index.html:  image.src = "https://developer.mozilla.org/static/img/opengraph-logo.png";
files/en-us/web/api/resource_timing_api/using_the_resource_timing_api/index.html:  image1.src = "https://developer.mozilla.org/static/img/opengraph-logo.png";
files/en-us/web/css/background-repeat/index.html:                     url(https://developer.mozilla.org/static/img/favicon32.png);
files/en-us/learn/html/introduction_to_html/the_head_metadata_in_html/index.html:<pre class="brush: html">&lt;meta property="og:image" content="https://developer.mozilla.org/static/img/opengraph-logo.png"&gt;
files/en-us/learn/html/introduction_to_html/the_head_metadata_in_html/index.html:&lt;link rel="apple-touch-icon-precomposed" sizes="114x114" href="https://developer.mozilla.org/static/img/favicon114.png"&gt;
files/en-us/learn/html/introduction_to_html/the_head_metadata_in_html/index.html:&lt;link rel="apple-touch-icon-precomposed" href="https://developer.mozilla.org/static/img/favicon57.png"&gt;
files/en-us/learn/html/introduction_to_html/the_head_metadata_in_html/index.html:&lt;link rel="shortcut icon" href="https://developer.mozilla.org/static/img/favicon32.png"&gt;</pre>

So that’s why it’d seem like all of those must have used to work at some time before, and so if they’re not now, it could be good to know why — and to know if in general, we shouldn’t use https://developer.mozilla.org/static/img URLs any longer, and we should replace them all with https://developer.mozilla.org/img URLs (as this PR does for this one article).

And that’s why it seems to me it’d be good to hear from @mdn/core-dev before we start changing any of them.

@peterbe
Copy link
Contributor

peterbe commented Jun 15, 2021

What I think we should do is download them all and stop referring to images with an absolute URL.
I.e.

<a href="https://developer.mozilla.org">
-  <img src="https://developer.mozilla.org/static/img/favicon144.png"
+  <img src="favicon144.png"
       alt="Visit the MDN site">
</a>

we then download that favicon image and put it right next to the index.html file in the tree.
That's what I've been manually doing in lots of trees.
The advantages are a'plenty:

  • Content will be more "offline'able".
  • The live samples are deployment agnostic (e.g. developer.allizom.org or localhost:5000 etc. )
  • The image is part of the content in that it gets checked in PRs
  • If you ever decide to delete this document page, you'll delete the folder containing the index.(html|md) and no risk of an orphan image that nobody can figure out what to do with
  • When mentioned as a local file, the content and the image become somewhat interlinked. If you rename the image but fail to update the content, it will fail in CI and you notice early.
  • You can do things with the image. E.g. draw on it, resize it, reformat it, compress it, etc. without worrying about breaking some other page depending on that image.

Drawbacks of local images:

  • Multiple pages might actually be referring to the same image so you might download a logo.svg 3 times as 3 different copies. One for offset-margin, one for offset-padding, and one for offset-flex-gap (silly example). This adds bytes to the git repo.
  • There might be examples where you actually want to demonstrate a fully absolute remote URL as part of the actual code example. E.g. <meta property="og:image" content="https://developer.mozilla.org/example.png">
  • The pages KS macro might struggle with relative paths if it includes a portion of text from another page that has a relative image reference.

@peterbe
Copy link
Contributor

peterbe commented Jun 15, 2021

I have a Python script that isn't perfect but I'm slowly going through pages with it.
If someone wouldn't mind helping to review https://github.com/mdn/content/pulls?q=is%3Apr+mdn.mozillademos.org+is%3Aopen I can point my script to the tree of files that @sideshowbarker mentioned above.

@sideshowbarker
Copy link
Member

I have a Python script that isn't perfect but I'm slowly going through pages with it.
If someone wouldn't mind helping to review https://github.com/mdn/content/pulls?q=is%3Apr+mdn.mozillademos.org+is%3Aopen I can point my script to the tree of files that @sideshowbarker mentioned above.

I think #5906 was the only remaining open PR there — so I went ahead and reviewed it and merged it just now

@peterbe
Copy link
Contributor

peterbe commented Jun 16, 2021

I propose we close this and review #6052
But we're super appreciative of your great start and stir on this @renodubois !

@sideshowbarker
Copy link
Member

I propose we close this and review #6052

No merged, and commit message marked that as fixing this, so this is now closed too

But we're super appreciative of your great start and stir on this @renodubois !

👍

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants