Problem/Motivation

When big-pipe placeholders occur inside elements that don't allow block level elements, browsers split up the parent element.

For example:

<p>Hi <div></div>, ...</p>

Is splitted into:

<p>Hi </p>{placeholder result}<p>, ...</p>

Proposed resolution

Use <span> instead of <div> for big-pipe placeholders

Remaining tasks

None.

User interface changes

None.

API changes

None. Only an internal functionality change: <span> elements are used instead of <div> for BigPipe placeholders.

Data model changes

None.

Comments

casey created an issue. See original summary.

casey’s picture

Status: Active » Needs review
FileSize
12.38 KB
Wim Leers’s picture

Issue tags: +Needs JS testing

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.

ndobromirov’s picture

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])

Wim Leers’s picture

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

Fabianx’s picture

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.

Wim Leers’s picture

Version: 8.1.x-dev » 8.2.x-dev
Status: Needs review » Needs work

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).

Wim Leers’s picture

Issue tags: +Needs tests
Wim Leers’s picture

Wim Leers’s picture

Now testing #2 against 8.3.x.

Wim Leers’s picture

First, a straight reroll. Needed significant rebasing.

Status: Needs review » Needs work

The last submitted patch, 11: span-instead-of-div-2802923-11.patch, failed testing.

Wim Leers’s picture

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.

The last submitted patch, 11: span-instead-of-div-2802923-11.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 13: 2802923-13.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
Wim Leers’s picture

Wim Leers’s picture

Title: Use <span> instead of <div> for big-pipe placeholders » Use <span> instead of <div> for BigPipe placeholders

The last submitted patch, 17: 2802923-17-test-only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 17: 2802923-17.patch, failed testing.

Wim Leers’s picture

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>.

The last submitted patch, 21: 2802923-21-test-only.patch, failed testing.

Wim Leers’s picture

Assigned: Unassigned » Fabianx

There we are at last :)

Assigning to @Fabianx for review, and hopefully RTBC.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC - looks great to me! :)

catch’s picture

Status: Reviewed & tested by the community » Fixed

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

  • catch committed df70190 on 8.4.x
    Issue #2802923 by Wim Leers, casey: Use <span> instead of <div> for...

  • catch committed 3395dff on 8.3.x
    Issue #2802923 by Wim Leers, casey: Use <span> instead of <div> for...
Wim Leers’s picture

Thanks, Fabianx & catch!

Wim Leers’s picture

Assigned: Fabianx » Unassigned
cilefen’s picture

Is there a change record?

Wim Leers’s picture

Issue summary: View changes

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

Status: Fixed » Closed (fixed)

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