Problem/Motivation

When you have multiple footnotes on the page containing the same text 2 problems arise: 1) auto numbering results in repeat numbers if you have multiple text fields on the page and 2) some footnote links are broken.

Steps to reproduce

Create a content type with at least 3 rich text fields that are displayed on the page. Or use Paragraphs and embed at least 3 paragraphs that contain rich text fields.

Configure your text format to support Footnotes. The settings I'm using are:

  • Collapse footnotes with identical content: FALSE
  • Use footnotes CSS: TRUE
  • Open footnote references in a dialog: FALSE
  • Disable output of the footnotes footer: TRUE

Add the footnotes group block to output to your page using block layout (or whatever means you wish).

Add a footnote to the first text field on your node with text "A" (could be anything, just giving a specific example).

Add a footnote to the second text field on your node that contains the same text as in your first text field ("A").

Add a footnote to the third text field on your node that contains any text ("B").

In all cases above we're using auto-numbering (not providing a fixed number).

Expected behavior:
Text is output to the page with the following footnotes:
1 > linking to A
2 > linking to A
3 > linking to B

Actual behavior:
1 > linking to A
2 > linking to nothing
2 > linking to B

Using the "collapse footnotes with identical content" setting doesn't change this behavior.

Issue fork footnotes-3522443

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

azinck created an issue. See original summary.

azinck’s picture

Issue summary: View changes
azinck’s picture

I'm going to provide a fix that works for my needs, but I recognize that it's not a true fix for this module because it doesn't support many of the use-cases the module tries to support. There is a fundamental problem with the way that numbering, linking, and counting is managed in the module at present and I don't have a deep enough understanding of the things the module's trying to do to sort through the right solution right now.

A key part of this bug stems from the logic in \Drupal\footnotes\FootnotesGroup::add that avoids adding a footnote if it's a textual duplicate of a footnote that was added during a previous call to the same method. I honestly don't know why that logic exists. At the time it's called from the text processor the link in the text to the footnote has already been created, but now it's just nuking the footnote that the link references, breaking the link. It operates regardless of the status of the "Collapse footnotes with identical content" setting, but even if it were to be adjusted to honor that setting I think there are still problems, but I'm not 100% sure...I know there's some JS the module provides that tries to fix some footnote numbering in some of these situations so maybe that handles it, but this is where my lack of familiarity with the module comes in.

Anyhow, another outcome of that problem is that now we have a problem with our footnote numbering if the \Drupal\footnotes\FootnotesGroup::count is ever called again (which will happen for the next text field that gets processed). The count method just looks at the number of entries in the matches array, which is actually misaligned from the footnote numbers that are in the text on the page at this point (in our bug recreation steps, at the time the third text area is processed the count method reports the count is at 1, when in fact the second text block contains a now-broken link to a footnote 2), so the third text block starts its footnote count at 2.

Anyhow, my fix here is a blunt instrument. I don't care if the content of my footnotes matches. I want all the footnotes on the page, so this patch removes the text matching logic in \Drupal\footnotes\FootnotesGroup::add.

If someone with more familiarity with the nuances of how the collapsing logic is meant to work can weigh in and plot a path forward here I'd be happy to update the patch to be smarter.

scott_euser’s picture

Status: Active » Needs work

The non-JS option will always struggle in some scenarios because the only way a layout builder/experience builder/paragraphs based content can build up the combined footnotes as is is when they get rendered (or at least currently). You may get more mileage with the JS based grouping instead as the numbering, etc is all calculated after the page fully renders including getting recalculated if new elements arrive on the page (e.g. via bigpipe or via ajax).

But yeah if you did ever want something to get in:

  1. Needs to be via merge request so coding standards and test coverage gets run
  2. Needs to have test coverage run for the change
  3. If the change could potentially negatively affect existing sites, needs to have an opt-in feature
matthand’s picture

Seeing the same bug. When footnote text is the same it forces a grouping and breaks things. Regardless of how the configuration checkbox is set in the text filter.

matthand’s picture

Status: Needs work » Needs review

Issued a MR with a change to check the collapse footnotes setting when adding footnotes to a group.

scott_euser’s picture

Status: Needs review » Needs work

Thanks both for figuring out the problem and working out solution! Looks sensible

We should also add to the tests so it stays fixed. Ie, adding an example of two identical texts getting collapsed & not collapsed to FootnotesGroupBlockTest for example. FootnotesFilterPluginTest::testCollapseFootnotes() already has this outside of the block context, so the issue here is no coverage for within the Group block.

azinck’s picture

Here's the reason I didn't solve this using this approach in my initial patch: I'm not convinced it's right. It's definitely the correct approach if you don't want to collapse footnotes, but for people who have chosen "Collapse footnotes with identical content" how does this work correctly?

What is the intended behavior when "Collapse footnotes with identical content" is selected? Let's take an example where we have 3 footnotes where the footnotes contain the content: "A", "A", and "B", respectively.

When "Collapse footnotes" is selected do we expect to see our content annotated with [1], [1], [2] (since we have 2 instances of footnote 1)? Or with [1], [2], [3] where [1] and [2] both link to a single line in the footnotes block that reads something like [1],[2]: "A"?

scott_euser’s picture

When "Collapse footnotes" is selected do we expect to see our content annotated with [1], [1], [2] (since we have 2 instances of footnote 1)? Or with [1], [2], [3] where [1] and [2] both link to a single line in the footnotes block that reads something like [1],[2]: "A"?

[1], [1], [2] in that case and that's what FootnotesFilterPluginTest::testCollapseFootnotes() covers. Group is essentially an extension of that to handle multiple formatted texts on the same page (and the JS version, multiple on the same page with lazy loading/ajax loaded content). The thing not part of offline medium is the backlinks since linking back to source references is not a thing offline.

Ultimately the module has to take some opinionated decisions, and if people aren't happy with those opinions, needs to be flexible enough to allow control - but that of course adds complexity, hence the push always for more test coverage or it will end up being impossible for me to merge things confidently, so that was one of the bigger chunks of work when building out the current version. Some footnotes style guides for print media will say [1], [2], [3] and converting [2] to use `Ibid` when its a repeat of [1] - something I can't imagine being easy with the reference being formatted text, but anyways if someone wants to contribute that it'd be a separate feature needing its own test coverage.

azinck’s picture

Thanks for the explanation. Is the collapsing functionality meant to work at all without JS? I'm asking about not using JS because this is on a site I've inherited that has dramatically changed the template markup for the footnotes block in a way that breaks the JS functionality, so I've been mostly looking at this from a non-JS perspective. Because if it *is* meant to work without JS I'm confused as to *how* it would work, either with or without the fixes in this issue.

scott_euser’s picture

Yes meant to work, probably an oversight when Group functionality was merged (was a very long standing issue with many contributors over years) https://www.drupal.org/project/footnotes/issues/3098138 though I haven't revised that deeply to check.

So each footnote text has a hash of the text in the ID. The ID is unique but when stripped to get just the hash bit, if the hash matches, then the citation should target the same reference as already stored for output. If the hash doesn't exist the citation should cause a new reference to be created.

matthand’s picture

Status: Needs work » Needs review

Hi Scott, I added the test as you described and everything is now passing. Can you review again? Thank you!

advait_cs’s picture

Hi @scout_euser, could you please review this again? It seems that Matt has added the tests for collapsing in the footnotes group. Is there anything else that needs to be done?

scott_euser’s picture

Status: Needs review » Fixed

Added some further test coverage in, merged, thanks all!

  • scott_euser committed 9052ab46 on 4.0.x authored by matthand
    Issue #3522443 by matthand, scott_euser, azinck: Problems when footnotes...
advait_cs’s picture

Status: Fixed » Needs review

Thanks @scott_euser for merging. Can we expect a new release for this fix?

scott_euser’s picture

Status: Needs review » Fixed

Feel free to use patch for now, I don't want to make a release for every commit + important to let more people encounter this in case of regression not covered by test coverage. One of many projects I maintain. Please leave status as Fixed as release process is unrelated.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.