Margin inconsistently added to code-toolbar's div on Prism's site

[hoonweiting]

Information:

• Prism version: Latest
• Plugins: Autoloader, Toolbar, Show Language
• Environment: Browser? Prism's site?

Description
Toolbar adds some margin to its own div element on Prism's site, and it does so in a strange manner. For some code blocks it's margin: 8px 0px;, for others it's margin: 0px;. I have no idea why. (Screenshots below are from Chrome)

Example
You can check out #3104 yourself (though it's a little inconvenient), where I am writing some docs on Prism tokens. This can also be seen on the Toolbar, Show Language, and Copy to Clipboard plugin pages, if you open dev tools.

Extra info

• I can say with certainty that the Toolbar plugin is part of the problem, because when I commented it out, the problem went away.
• The behaviour exhibited in Firefox is also different from the behaviour exhibited in Chrome (in Firefox, the style is applied to the pre element instead of the div, but they all get style: 0px; instead of a mix), so that's another thing to look out for.

I don't know if this is a problem specific to Prism's website, and/or if there are any other elements at play. I'll poke around more and see what else I can find. Additional help will be appreciated!

Originally discovered here: #3104 (comment)

[hoonweiting]

A little more info, turns out that a style="margin: 0px;" is being applied to the pre element in Chrome as well, so the only difference between Chrome and Firefox is that Firefox doesn't have the style being applied to the code-toolbar div.

My own page doesn't have the problem, so I'm guessing the problem is specific to Prism's website. And that led me to poke around other parts of Prism's website.

I commented out the assets/code.js script and the problem went away:

I un-commented the script, but commented out the following two lines of code in assets/code.js and got the same result (screenshots from Chrome, Firefox respectively):

prism/assets/code.js

Lines 263 to 265 in d38592c

	// transfer margin of pre to wrapper
	wrapper.style.margin = window.getComputedStyle(pre).margin;
	pre.style.margin = '0';

So at least it isn't a problem with the Toolbar plugin itself, so I guess it's a little less pressing of an issue. But I'm not very sure how to solve it given that it was implemented because of another bug (#1966)?

I'm still not sure what is causing the margin to be transferred inconsistently in Chrome, though. May not be super necessary to figure out why, but I hope it's not a sign of another bug.

[hoonweiting]

Turns out the difference in behaviour in Chrome and Firefox is caused by window.getComputedStyle(pre).margin. According to this StackOverflow Q/A, Chrome is able to evaluate shorthand properties, but Firefox (and apparently Safari?) is unable to do the same. To resolve the discrepancy, we have to use wrapper.style.marginTop = window.getComputedStyle(pre).marginTop; instead (and do the same for the other sides). pre.style.margin = '0'; works fine though.

I added console.log(window.getComputedStyle(pre).marginTop; and here are the results in Chrome and Firefox respectively:

Seeing the console logs on Firefox made me wonder if the lazy loading of languages via Autoloader had anything to do with the discrepancies in marginTop (8px for of the 42 code blocks, 0px for 20...but there are 42 code blocks in total, so 20 of them got overwritten?). So I commented out Autoloader, and loaded the required languages myself, and the discrepancy went away. These are the results in Chrome and Firefox respectively:

To visualise what the code blocks look like after swapping out Autoloader for manual loading:

To summarise, the discrepancy between Chrome and Firefox is due to Firefox being unable to evaluate shorthand properties properly, while the discrepancies of some code blocks having style="margin: 8px 0px;" and others having style="margin: 0px;" has got to do with Autoloader.

[hoonweiting]

Okay I think the problem is with requestAnimationFrame();:

prism/assets/code.js

Lines 255 to 267 in 3f24dc7

	requestAnimationFrame(function () {
	if (!element.matches('div.code-toolbar > pre > code')) {
	return;
	}
	var pre = element.parentElement;
	var wrapper = pre.parentElement;
	// transfer margin of pre to wrapper
	wrapper.style.margin = window.getComputedStyle(pre).margin;
	pre.style.margin = '0';
	});

If my understanding isn't wrong (I'm really basing this off the console logs in Firefox, I've not looked at Autoloader's code), when Autoloader loads a new language, the relevant code blocks are repainted. I think it's easier to explain in point form:

1. Page is initially painted.
2. A bunch of resources load, including most of the languages.
3. requestAnimationFrame() runs, and the margins in the pre element of all code blocks (which are .5em 0 for all of the official themes, equivalent to 8px 0) are transferred to the code-toolbar div. Then, the margins of the pre elements are set to 0.
4. The remaining languages load.
5. requestAnimationFrame() is run again for the repainted code blocks, and the margins in the pre element of these code blocks (which are now 0, after Step 3) are transferred to the code-toolbar div. Then, the margins of the pre elements are set to 0 again.

---

So here I can offer a solution, which is to add an if statement to check if the margin transfer had already occurred. If not, then we transfer the margin. (This is on top of modifying the code to use marginTop etc instead of margin, so that it works in Firefox (and Safari?) as well.)

However, I think this is a bit of an XY problem. The original goal of the margin transfer is to give the toolbar some headspace in code blocks on Prism's site (#1966 (comment)). At first I wanted to suggest adding a rule for .toolbar (not .code-toolbar) in assets/styles.css to increase the value of top from the plugin's default (which is .3em), while removing the margin transfer entirely. But I wanted to see what the original bug is, so I commented out the fixes in #1966. And it looks...alright. It's got enough headspace. Or maybe I'm missing something. (Screenshot in Firefox, with the pre element selected in dev tools. Top edge of the code-toolbar div is where the top of the purple is.)

Anyway, I still went ahead and tried commenting out the margin transfer, and adding div.code-toolbar div.toolbar { top: .8em; } to assets/styles.css. An extra rule will be needed to reduce the space between two adjacent code blocks (to mimic adjacent margin collapsing), but yeah the result will be similar as the screenshot above.

As a bonus, the latter two solutions above will get rid of the scrollbar for single-line code blocks when the Coy theme is used (which you can see on the Copy to Clipboard page. I don't know why, but somehow it does.

To summarise, I have three solutions in mind:

1. Ensure margin is transferred only once. (I have not tried this myself, but I think it's possible.)
2. Revert Toolbar: Fixed overflow issue #1966. (I think I'm missing something here honestly,)
3. Remove the margin transfer, and add some CSS rules in assets/styles.css to reposition the toolbar.

---

@RunDevelopment what do you think? (And sorry for the text wall!)

[RunDevelopment]

Thank you for investigating this bug!

I think the CSS solution (solution 3) is the best solution. However, I think we can do something even simpler.

The main problem is that div.code-toolbar { overflow: auto } doesn't let the pre's margin escape the div. So how about we just remove the pre's margin? Instead of doing the margin transfer in hacky JS, we do it in simple CSS:

div.code-toolbar {
	display: block;
	overflow: auto;
	margin: .5em 0;
}

div.code-toolbar > pre {
	margin: 0;
}

(Sure, I just assumed that the margin of all pres is .5em 0 on the whole site, but that's not a problem. We control the styles.)

This means that we won't have to hack the toolbar into position and margin collapsing works naturally.

What do you think @hoonweiting?

[hoonweiting]

@RunDevelopment that's genius! And as for the margin of pre elements, yep all of Prism's official themes are .5em 0, so that's definitely not a problem! (Might be a problem in future if the unofficial themes can be previewed too (I didn't check the margins there), but we could still override that just for Prism's website.)

However, Coy doesn't want to play nicely with that :"( Here's a screenshot some code blocks on the Copy to Clipboard plugin page, with the margin transferred in assets/styles.css, with the code blocks scrolled down where possible:

Chrome	FF

(I checked the current website to compare as well, and at least it's the same thing, so no loss there.)

It's just Coy though, the other themes are fine. Maybe we can use that method when Coy gets replaced with Coy without Shadows in future? I very much prefer not using negative margins!

---

Also, regarding the problem of the pre margin being unable to escape the div, I see it now! But I'm wondering why the overflow: auto; property is needed at all? I didn't understand the problem before because I commented out the entire rule (instead of leaving the overflow: auto; in) and it looks okay (screenshots from FF, looks the same in Chrome):

Tomorrow Night	Coy

So I'm not sure what I'm missing!

[RunDevelopment]

However, Coy doesn't want to play nicely with that

It never does...

Let's just nuke the shadows:

div.code-toolbar > pre:after,
div.code-toolbar > pre:before {
	display: none !important;
}

Also, we can see that the usually grey border around code blocks disappeared (even before removing the shadows). This is because the pre element doesn't account for the 1px box-shadow of the codeelement. I would argue that this is an oversight with Coy's design and should be fixed by adding a padding: 1px to the pre. I'll make a PR for that (#3143).

Maybe we can use that method when Coy gets replaced with Coy without Shadows in future?

Yes, we should be. Coy is implemented in a pretty weird way which causes loads of problems but enables these cool shadows. Coy without Shadows takes the standard approach to themes but can't implement the shadows (easily).

But I'm wondering why the overflow: auto; property is needed at all?

Because that "cool" theme selector on the right side messes with the layout of pages and overlaps with elements. overflow: auto fixes that. Here's the div with overflow: auto:

and without:

[hoonweiting]

:") I must say Coy has really pretty colours (and the shadows are a really nice touch), it's just such a shame that its shadows don't play nicely with the rest of Prism!

Anyway to confirm, what's the plan? I am a little lost 😅 (I added the 1px padding on my end too for the screenshots)

Here's margin transfer with nuked shadows:	And here's shifted toolbar with negative div.code-toolbar:not(:first-of-type) margins:
We probably have to nudge the toolbar's position just a touch higher to account for single-line code blocks in Coy?	The shadows don't appear in the darker-coloured rows... I'm soooooo done

(Then again, sounds like the shadow nuking is a joke HAHA whoopsie)

Also, we can see that the usually grey border around code blocks disappeared (even before removing the shadows).

Whoa, nice catch!

Because that "cool" theme selector on the right side messes with the layout of pages and overlaps with elements.

Ah that's what I missed *facepalm* thanks!

[hoonweiting]

Ah I figured out why the shadows don't show up on the darker-coloured rows. It's because the z-index of the shadows are less than 0, and setting a z-index of 1 on the pre will fix it.

EDIT: And then the toolbar doesn't show up. But setting a z-index of 2 on the toolbar will fix that.

[RunDevelopment]

Anyway to confirm, what's the plan?

Let's do the simple CSS margin transfer + Coy shadow removal.

It doesn't look perfectly ok with Coy (as you said, the position is slightly off) but it's usable and consistent.

[hoonweiting]

Hmm, I guess I am a bit concerned about removing the shadows on Prism's site, because it's the only way users can 'preview' the theme. If they were to download it, they may be confused as to why their downloaded version has shadows, whereas there aren't shadows on Prism's site.

---

Also here's a screenshot of my previous comment, with the additional z-index properties set (sorry for not including it earlier):

[RunDevelopment]

it's the only way users can 'preview' the theme.

That's a very good point.

I made a PR (#3146) to fix the root cause of all of this: The theme selector's design.

[hoonweiting]

Aw man, you're amazing!