Use <span> instead of <div> for BigPipe placeholders

Hm… I think this actually makes sense. I'm tentatively +1 :) Great catch, and nice patch — thank you very much!

However, this should have test coverage to ensure we don't ever change this back again, and to ensure this behavior continues to work correctly.

A bit off-topic, but somehow relevant...

Isn't there too much repetitions of the markup for the placeholders?
It's not something that is going to change every day, but 2 utilities will not hinder performance that much?

bigPipePlaceholder($value)
bigPipePlaceholderNoJs($value)

Or one:
bigPipePlaceholder($value, [$js = FALSE])

@ndobromirov I have no idea what you mean. Please create a new issue.

Yeah, using a non-block level element makes definitely sense.

And if someone needs the old behavior, they can still target the bigpipe placeholders via the data- argument in CSS and use display: block;

So that is fine with me.

Alright, both maintainers (Fabianx and I) are +1. Now we just need JS tests.

This will also have to go into 8.2, not 8.1 (because no more releases there).

Note that even if we fix this, #2738685: BigPipe leaves wrapper <div>s in place, because AJAX system leaves them — this can cause some CSS to break will unfortunately still cause wrapper <div>s to be added, due to #736066: ajax.js insert command sometimes wraps content in a div, potentially producing invalid HTML and other bugs. But that doesn't mean we should not do this!

Now testing #2 against 8.3.x.

First, a straight reroll. Needed significant rebasing.

I started writing test coverage. And in doing so, I started wondering: how can you even get placeholders inside text? You can't do that with Drupal core by default I think.

Look at the test coverage, I have to do fairly strange things in order to do this.

The test-only patch is also the interdiff.

Now that #2828559: UpdatePathTestBase tests randomly failing is fixed, retesting.

#13 needed a rebase apparently. So, here's a straight reroll. No changes.

Ugh, this needs a separate 8.2.x patch now, because 8.2.x and 8.3.x have diverged slightly due to #2835758: Remove BigPipeInterface and move all of its docs to the implementation + #2569119: Use Crypt::hashBase64(), not hash('crc32b') or sha1 for placeholder tokens. So, rolling separate patches.

1. The test-only patch is unchanged, and applies to both 8.2.x and 8.3.x.
2. The 8.2.x and 8.3.x patches are identical. They differ only in size because of different contexts. They make the same exact changes. Same number of additions and deletions.
3. There's an interdiff, because I noticed 3 more lines where the docs mentioned a <div> which should now become a <span>.

There we are at last :)

Assigning to @Fabianx for review, and hopefully RTBC.

RTBC - looks great to me! :)

Committed/pushed to 8.4.x and cherry-picked to 8.3.x. Thanks!

Thanks, Fabianx & catch!

Is there a change record?

There's no need for a change record. IS updated to reflect that.