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

(Auto)EmbedLiveSample is not working correctly #5005

Open
wbamberg opened this issue Nov 26, 2021 · 5 comments
Open

(Auto)EmbedLiveSample is not working correctly #5005

wbamberg opened this issue Nov 26, 2021 · 5 comments
Labels
🐛 bug Something isn't working, or isn't working as expected 🧑‍🤝‍🧑 community contributions by our wonderful community 🐌 idle Issues and PRs without recent activity. Flagged for maintainer follow-up. idle live-samples issues related to live samples (EmbedLiveSample macro) macros tracking issues related to kumascript macros p4 Not urgent, only if time allows

Comments

@wbamberg
Copy link
Collaborator

See also mdn/content#10758.

It seems that if a page has a structure like this:

## A heading

{{EmbedLiveSample}}

### HTML

(the HTML code block)

### JavaScript

(the JS code block)

...then the code blocks aren't found. But they should be, according to the spec (https://github.com/mdn/content/discussions/5803#discussioncomment-842706), because they are in the same section as the {{EmbedLiveSample}} call. Perhaps the implementation isn't looking in subsections of the current section?

@github-actions github-actions bot added the needs triage Triage needed by staff and/or partners. Automatically applied when an issue is opened. label Nov 26, 2021
@schalkneethling schalkneethling added 🐛 bug Something isn't working, or isn't working as expected 🚉 platform keeping the platform healthy and removed needs triage Triage needed by staff and/or partners. Automatically applied when an issue is opened. labels Nov 29, 2021
@schalkneethling schalkneethling added 🧑‍🤝‍🧑 community contributions by our wonderful community needs triage Triage needed by staff and/or partners. Automatically applied when an issue is opened. labels Apr 15, 2022
@github-actions github-actions bot added the 🐌 idle Issues and PRs without recent activity. Flagged for maintainer follow-up. label May 22, 2022
@OnkarRuikar
Copy link
Contributor

This works if we put the headings one level deeper:


### Example

{{EmbedLiveSample}}

#### HTML

(the HTML code block)

#### JavaScript

(the JS code block)

It doesn’t work if a live sample is spread across H2 and H3 sections. It is because yari puts H2 and H3 sections in separate <section>s. They are not in same hierarchy in DOM. Though they appear to be hierarchical due to styling, physically they are not.

The PR #1811 split H2 and H3. So H3 content is no longer placed under H2 header tree. That is why the macro call in H2 can’t find the code in H3 headings.


After all these years we can’t revert #1811 easily.

And if try to accommodate this in the auto embed live sample logic, then there will be a lot of hardcoding around H2 and H3 tags. And many possibilities will have to be handled around the H2-H3 boundaries. 😱 The logic will get even more complex. There will be a lot of bugs🐞🐛🪲.

Also, the pages that rely on this bug will break after we fix this. Finding them won’t be easy.

Do we prefer auto embed over explicit embed(i.e. provide id)? Which one is considered a better practice?

@wbamberg
Copy link
Collaborator Author

Well that's fun. Great digging @OnkarRuikar ! I can't comment on the implementation, but as for this:

Do we prefer auto embed over explicit embed(i.e. provide id)? Which one is considered a better practice?

...the history is that this was considered as a way to help address a problem that the Markdown conversion introduced. Before Markdown we could have explicit id attributes on headings (or anything for that matter) that EmbedLiveSample could reference. This meant that the ID could be stable when the heading text changed and in particular when the page was translated.

But in Markdown the ID is always taken from the heading text, so we were concerned that this would be a maintenance problem for authors. So we thought that if we can implicitly select code blocks from the page structure, this would help with that. There's background on this in mdn/content#3548.

In practice, because of bugs like this, we haven't used it much, and we're almost a year on from Markdown conversion at this point. If we do still want to start using it widely, I guess in practice it would be OK to disallow the particular usage that gives rise to this bug (it's certainly niche) as fixing it looks hard.

Also cc @SphinxKnight who has insight from the localization side :).

@github-actions github-actions bot removed the 🐌 idle Issues and PRs without recent activity. Flagged for maintainer follow-up. label Jun 23, 2022
@github-actions github-actions bot added the 🐌 idle Issues and PRs without recent activity. Flagged for maintainer follow-up. label Jul 30, 2022
@caugner
Copy link
Contributor

caugner commented Nov 11, 2022

@wbamberg or @SphinxKnight or @OnkarRuikar Could you please check if this is still an issue? If so, we would accept a PR that fixes this.

@caugner caugner added live-samples issues related to live samples (EmbedLiveSample macro) need more info Further information is requested macros tracking issues related to kumascript macros and removed 🚉 platform keeping the platform healthy 🐌 idle Issues and PRs without recent activity. Flagged for maintainer follow-up. needs triage Triage needed by staff and/or partners. Automatically applied when an issue is opened. labels Nov 11, 2022
@OnkarRuikar
Copy link
Contributor

The issue exists. Following code

---
title: Authoring MathML
slug: Web/MathML/Authoring
tags:
  - Beginner
  - MathML
  - MathML Project
---

## A heading

{{EmbedLiveSample}}

### HTML

```html
<h1>hi!</h1>
```

### CSS

```css
h1 {
  color:red;
}
```

throws error:

Error: MacroLiveSampleError within .../index.md, line 11 column 19 (unable to find any live code samples for "19" within /en-US/docs/Web/MathML/Authoring)

This is a very low severity issue as no page at the moment starts with a live sample in level two ## intro section.

@wbamberg
Copy link
Collaborator Author

wbamberg commented Nov 14, 2022

no page at the moment starts with a live sample in level two ## intro section.

Except for the two that are referenced in mdn/content#10758 (comment) ?

https://developer.mozilla.org/en-US/docs/web/html/element/input/date#examples
https://developer.mozilla.org/en-US/docs/web/html/element/input/month#examples

Still as I said above:

If we do still want to start using it widely, I guess in practice it would be OK to disallow the particular usage that gives rise to this bug (it's certainly niche) as fixing it looks hard.

@caugner caugner added p4 Not urgent, only if time allows and removed need more info Further information is requested labels Nov 14, 2022
@github-actions github-actions bot added the 🐌 idle Issues and PRs without recent activity. Flagged for maintainer follow-up. label Dec 21, 2022
@github-actions github-actions bot added the idle label Jan 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working, or isn't working as expected 🧑‍🤝‍🧑 community contributions by our wonderful community 🐌 idle Issues and PRs without recent activity. Flagged for maintainer follow-up. idle live-samples issues related to live samples (EmbedLiveSample macro) macros tracking issues related to kumascript macros p4 Not urgent, only if time allows
Projects
Development

No branches or pull requests

4 participants