Fixes https://github.com/primer/design/issues/141

[juliyvchirkov]

Fixes #141

This styled component imported on pageload by React engine and applied to the images included with tutorials on primer.style

The component includes directive max-width: 100%, which prevents images to stretch to their original full width thus avoiding possible overflow-x and forcing images to be responsive

In general this trick should be enough to achieve the goal, 'cause if image property height in css is not declared explicitly, by default it respects the state of property width and scales accordingly itself. This behaviour is taken as usual and almost like an axiom, since for a long time all common browsers have been treating this situation in the same way

But current actual builds of Safari v14.x.x (the latest one for this moment is 14.0.3 16610.4.3.1.4) break this idyll - due to some bug in Safari css interpreter, with this browser height property on images doesn't rely on width as usual

This miss like a snowball leads to serious abnormalities for the whole page render - since height is not deriving the proper scale from width, the css interpreter inspects the project's css rules for explicit declaration for height (or does not inspect indeed; this is only my guess, since this step is mandatory according to css specifications), and since there's no matching rule declared, Safari applies to height the default (initial) value, which for height property means the original image height in pixels

This results in total mess on pages which utilize large images to illustrate text content. Since directive max-width: 100% clearly and undoubtedly by css specifications prevents images to stretch in width over main content area, width is rendered according to the limits declared by layout. But at the same time height is scaled to the original image height

The consequences are ugly and disgusting, 'cause every large illustration on the page gets dreadfully stretched, and the page looks totally broken

In upcoming Safari 14.2 this bug is already fixed and now for this moment it renders the same pages on primer.style correctly (this beta is available for download at developer.apple.com as Safari Technology Preview Release 120 / v14.2, WebKit 16612.1.2.6)

The update in this commit overrides the destructive behaviour of actual builds of Safari, forcing image height property to follow width by explicit declaration of height value as 100%.The value declared thru directive height: 100% is relative, so the css interpreter is forced to utilize the exact relative value for final calculations. And since 100% for width property de facto is always bound by max-width: 100% to the limits of content area layout, height scales accordingly and the goal is achieved

Inequality with existing and applied directive (height vs max-width) is affected by concept of web design per se. The width of pages has always been limited by the screen width, so max-width directive is reasonable and acceptable, while the scrollable concept of web pages turns the height attribute into the abstraction. In common cases this always leads max-height: 100% directive to be equal to original (full) height of an object and thus makes it useless

[changeset-bot]

⚠️ No Changeset found

Latest commit: affc58b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

gatsby-theme-primer-example – ./

🔍 Inspect: https://vercel.com/primer/gatsby-theme-primer-example/9rXRpyKjmHvfhVPP4SLvndHgpuvU
✅ Preview: https://gatsby-theme-p-git-fork-juliyvchirkov-patch-1-89aad7.vercel.app

doctocat – ./

🔍 Inspect: https://vercel.com/primer/doctocat/5SiZaSrbJ6W1wg4uHN8jqCFP8grS
✅ Preview: https://doctocat-git-fork-juliyvchirkov-patch-1-primer.vercel.app

[juliyvchirkov]

the rule .markdown img on line #4 of @primer/css/src/markdown/images.scss can also be affected with this Safari bug

.markdown-body {
  // Images & Stuff
  img {
    max-width: 100%;
    // because we put padding on the images to hide header lines, and some people
    // specify the width of their images in their markdown.
    box-sizing: content-box;
    background-color: $bg-white;

[juliyvchirkov]

I'm new to changeset and despite I've thoughtfully explored the foregoing, I need an advice to make the changeset the right way

The case seems to be a bit complicated

For this moment there're three @primer repos plus one website involved in this pull: @primer/doctocat, @primer/design, @primer/css and https://primer.style

• The update proposed for current repo @primer/doctocat has been made in order to fix the issue #141 for another repo @primer/design
• @primer/design utilizes @primer/doctocat to compile the content templates into the live https://primer.style
• In addition to the code of styled React component in this repo, which turns on the live site https://primer.style into the css rule affected by #141, I've also localized the same pattern inside @primer/css repo in scss code at @primer/css/src/markdown/images.scss
• Since scss code turns on the live site into the css rule as well as styled React component in this repo, and js and scss source both end up wth the same css rule, which consists of 3 directives targeted to images, repo @primer/css and any project based on it can also be affected by #141
• My pull request offers the patch for this repo @primer/doctocat only. This patch resolves #141 and delivers the fix in sequence for the triplet @primer/doctocat -> @primer/design -> https://primer.style
• Of course, @primer/design repo needs to be recompiled on the top of patched @primer/doctocat to update the affected styled component in https://primer.style/design/app-2ac4de6e60c3247ee132.js. In turn, compiled bunch should be reuploaded to https://primer.style to get rid of the issue on the live site
• I've made no updates to @primer/css repo for this moment, I've just localized the possibly affected scss code and posted the warning along with this pull request. Thus the code in @primer/css/src/markdown/images.scss also needs to get the same patch to prevent the potential issue with Safari
• The guide on changeset accents the demand to declare how the consumers should update their code according to this patch to keep their projects safe. But as far as I see, the patched repo @primer/doctocat seems to be the service engine developed by @primer to cover internal purposes, and the repo @primer/design and the live site https://primer.style in turn are also @primer internal projects. On the contrary it seems the last affected repo @primer/css, which hasn't been patched, being officially recommended for public use, is widely utilized by third-party projects

Due to the above I find it difficult to provide the requested changeset, 'cause I can't figure, how this complicated case should be reported in readme designated to the one concrete patch in @primer/doctocat

[colebemis]

@juliyvchirkov Thank you for all your hard work on this! ❤️ I'll try to get around to reviewing this next week.

[github-actions]

Hi! This pull request has been marked as stale because it has been open with no activity for 60 days. You can comment on the pull request or remove the stale label to keep it open. If you do nothing, this pull request will be closed in 7 days.