Problem/Motivation
When using the big_pipe
module with JavaScript, under certain circumstances, placeholders generated from #lazy_builder
callbacks to history_attach_timestamp
in comments cause Drupal.behaviors
for the entire document to be detached. This issue became apparent after custom theming the comments field to put the comment form above the list of comments rather than below it. The user-facing issue was that, shortly after being attached to the comment_body
field, the CKEditor was subsequently detached, leaving a plain textarea field, and producing the following warning in the browser's developer console:
[CKEDITOR] Error code: editor-incorrect-destroy.
[CKEDITOR] For more information about this error go to http://docs.ckeditor.com/#!/guide/dev_errors-section-editor-incorrect-de...
The specific reason for this issue is as follows. After making a theme change to move the comment form above the list of existing comments, the order of content with placeholders changed in the document, putting the placeholder for the comment form above the history_attach_timestamp
. Big Pipe uses the order of the placeholders in the markup to set the order in which it renders the <script>
tags it appends to the document body, and Big Pipe's JavaScript, in turn, reads the script tags in the order they appear. The result in this case was that the placeholder for history_attach_timestamp
was evaluated after the one for the comment form.
Steps to reproduce:
- Ensure
comment
,big_pipe
, andhistory
modules are enabled. - Create a node with a few comments.
- Edit
field--comment.html.twig
to move the comment form above the list of comments on a node - Rebuild caches and note that the WYSIWYG is gone from the form, and the errors above are in the Chrome or Firefox console
- Attach patch and note the issue is fixed
Here is a sample of the script tags that Big Pipe appended to the end of the document body after the comment form was moved above the comment list:
<script type="application/json" data-big-pipe-event="start"></script> <script type="application/json" data-big-pipe-placeholder="callback=Drupal%5CCore%5CRender%5CElement%5CStatusMessages%3A%3ArenderMessages&args[0]&token=a8c34b5e" data-drupal-ajax-processor="big_pipe"> [{"command":"settings","settings":{"ajaxPageState":{"theme":"bartik","libraries":"ajax_comments\/commands,bartik\/global-styling,big_pipe\/big_pipe,classy\/base,classy\/messages,comment\/drupal.comment-by-viewer,comment\/drupal.comment-new-indicator,contextual\/drupal.contextual-links,contextual\/drupal.contextual-toolbar,core\/drupal.active-link,core\/html5shiv,core\/normalize,history\/mark-as-read,shortcut\/drupal.shortcut,statistics\/drupal.statistics,system\/base,toolbar\/toolbar,toolbar\/toolbar.escapeAdmin,tour\/tour,user\/drupal.user.icons"},"pluralDelimiter":"\u0003","user":{"uid":"1","permissionsHash":"50f490420ed4c54558b56e853ec07d868d9317d8e1f2e7b76962c15f61956d44"}},"merge":true},{"command":"add_css","data":"\u003Clink rel=\u0022stylesheet\u0022 href=\u0022\/core\/themes\/classy\/css\/components\/messages.css?o4m2v0\u0022 media=\u0022all\u0022 \/\u003E\n"},{"command":"insert","method":"replaceWith","selector":"[data-big-pipe-selector=\u0022callback=Drupal%5CCore%5CRender%5CElement%5CStatusMessages%3A%3ArenderMessages\u0026args[0]\u0026token=a8c34b5e\u0022]","data":"\n ","settings":null}] </script> <script type="application/json" data-big-pipe-placeholder="callback=Drupal%5Cblock%5CBlockViewBuilder%3A%3AlazyBuilder&args[0]=bartik_local_tasks&args[1]=full&args[2]&token=7daf9ccc" data-drupal-ajax-processor="big_pipe"> [{"command":"insert","method":"replaceWith","selector":"[data-big-pipe-selector=\u0022callback=Drupal%5Cblock%5CBlockViewBuilder%3A%3AlazyBuilder\u0026args[0]=bartik_local_tasks\u0026args[1]=full\u0026args[2]\u0026token=7daf9ccc\u0022]","data":"\u003Cdiv id=\u0022block-bartik-local-tasks\u0022 class=\u0022contextual-region block block-core block-local-tasks-block\u0022\u003E\n \n \u003Cdiv data-contextual-id=\u0022block:block=bartik_local_tasks:langcode=en\u0022\u003E\u003C\/div\u003E\n \u003Cnav class=\u0022tabs\u0022 role=\u0022navigation\u0022 aria-label=\u0022Tabs\u0022\u003E\n \u003Ch2 class=\u0022visually-hidden\u0022\u003EPrimary tabs\u003C\/h2\u003E\n \u003Cul class=\u0022tabs primary\u0022\u003E\u003Cli class=\u0022is-active\u0022\u003E\u003Ca href=\u0022\/qanda\/question\/2\u0022 data-drupal-link-system-path=\u0022node\/2\u0022\u003EView\u003Cspan class=\u0022visually-hidden\u0022\u003E(active tab)\u003C\/span\u003E\u003C\/a\u003E\u003C\/li\u003E\n\u003Cli\u003E\u003Ca href=\u0022\/node\/2\/edit\u0022 data-drupal-link-system-path=\u0022node\/2\/edit\u0022\u003EEdit\u003C\/a\u003E\u003C\/li\u003E\n\u003Cli\u003E\u003Ca href=\u0022\/node\/2\/delete\u0022 data-drupal-link-system-path=\u0022node\/2\/delete\u0022\u003EDelete\u003C\/a\u003E\u003C\/li\u003E\n\u003C\/ul\u003E\n\n \u003C\/nav\u003E\n \u003C\/div\u003E\n","settings":null}] </script> <script type="application/json" data-big-pipe-placeholder="callback=comment.lazy_builders%3ArenderForm&args[0]=node&args[1]=2&args[2]=field_question_comments&args[3]=comment&token=6313718a" data-drupal-ajax-processor="big_pipe"> [{"command":"settings","settings":{"ajaxPageState":{"theme":"bartik","libraries":"ajax_comments\/commands,ajax_comments\/commands,bartik\/global-styling,big_pipe\/big_pipe,ckeditor\/drupal.ckeditor,classy\/base,classy\/messages,comment\/drupal.comment-by-viewer,comment\/drupal.comment-new-indicator,contextual\/drupal.contextual-links,contextual\/drupal.contextual-toolbar,core\/drupal.active-link,core\/html5shiv,core\/jquery.form,core\/normalize,filter\/drupal.filter,history\/mark-as-read,shortcut\/drupal.shortcut,statistics\/drupal.statistics,system\/base,toolbar\/toolbar,toolbar\/toolbar.escapeAdmin,tour\/tour,user\/drupal.user.icons"},"ajaxTrustedUrl":{"\/comment\/reply\/node\/2\/field_question_comments":true,"\/ajax_comments\/reply\/node\/2\/field_question_comments\/0":true},"ajax":{"edit-ajax-comments-reply-form-node-2-field-question-comments-0-0":{"url":"\/ajax_comments\/reply\/node\/2\/field_question_comments\/0","wrapper":".ajax-comments-wrapper","method":"replaceWith","effect":"fade","event":"mousedown","keypress":true,"prevent":"click","dialogType":"ajax","submit":{"_triggering_element_name":"op","_triggering_element_value":"Save"}}},"editor":{"formats":{"links_only":{"format":"links_only","editor":"ckeditor","editorSettings":{"allowedContent":{"*":{"attributes":"lang,dir","styles":false,"classes":false},"a":{"attributes":"href,hreflang","styles":false,"classes":false}},"contentsCss":["\/core\/modules\/ckeditor\/css\/ckeditor-iframe.css","\/core\/modules\/system\/css\/components\/align.module.css","\/core\/themes\/bartik\/css\/base\/elements.css","\/core\/themes\/bartik\/css\/components\/captions.css","\/core\/themes\/bartik\/css\/components\/table.css","\/core\/themes\/bartik\/css\/components\/text-formatted.css"],"customConfig":"","disallowedContent":{"*":{"attributes":"on*"}},"drupalExternalPlugins":{"drupallink":"\/core\/modules\/ckeditor\/js\/plugins\/drupallink\/plugin.js"},"drupalLink_dialogTitleAdd":"Add Link","drupalLink_dialogTitleEdit":"Edit Link","entities":false,"extraPlugins":"drupallink","justifyClasses":["text-align-left","text-align-center","text-align-right","text-align-justify"],"language":"en","pasteFromWordPromptCleanup":true,"resize_dir":"vertical","stylesSet":false,"toolbar":[{"name":"Links","items":["DrupalLink","-","DrupalUnlink"]},"\/"]},"editorSupportsContentFiltering":true,"isXssSafe":false}}},"pluralDelimiter":"\u0003","user":{"uid":"1","permissionsHash":"50f490420ed4c54558b56e853ec07d868d9317d8e1f2e7b76962c15f61956d44"}},"merge":true},{"command":"add_css","data":"\u003Clink rel=\u0022stylesheet\u0022 href=\u0022\/core\/assets\/vendor\/jquery.ui\/themes\/base\/button.css?o4m2v0\u0022 media=\u0022all\u0022 \/\u003E\n\u003Clink rel=\u0022stylesheet\u0022 href=\u0022\/core\/assets\/vendor\/jquery.ui\/themes\/base\/resizable.css?o4m2v0\u0022 media=\u0022all\u0022 \/\u003E\n\u003Clink rel=\u0022stylesheet\u0022 href=\u0022\/core\/assets\/vendor\/jquery.ui\/themes\/base\/dialog.css?o4m2v0\u0022 media=\u0022all\u0022 \/\u003E\n\u003Clink rel=\u0022stylesheet\u0022 href=\u0022\/core\/themes\/stable\/css\/ckeditor\/ckeditor.css?o4m2v0\u0022 media=\u0022all\u0022 \/\u003E\n\u003Clink rel=\u0022stylesheet\u0022 href=\u0022\/core\/themes\/stable\/css\/filter\/filter.admin.css?o4m2v0\u0022 media=\u0022all\u0022 \/\u003E\n\u003Clink rel=\u0022stylesheet\u0022 href=\u0022\/core\/themes\/classy\/css\/components\/dialog.css?o4m2v0\u0022 media=\u0022all\u0022 \/\u003E\n"},{"command":"insert","method":"append","selector":"body","data":"\u003Cscript src=\u0022\/core\/assets\/vendor\/jquery.ui\/ui\/widget-min.js?v=1.11.4\u0022\u003E\u003C\/script\u003E\n\u003Cscript src=\u0022\/core\/assets\/vendor\/jquery-form\/jquery.form.min.js?v=3.51\u0022\u003E\u003C\/script\u003E\n\u003Cscript src=\u0022\/core\/modules\/filter\/filter.js?v=8.0.5\u0022\u003E\u003C\/script\u003E\n\u003Cscript src=\u0022\/core\/assets\/vendor\/jquery.ui\/ui\/button-min.js?v=1.11.4\u0022\u003E\u003C\/script\u003E\n\u003Cscript src=\u0022\/core\/assets\/vendor\/jquery.ui\/ui\/mouse-min.js?v=1.11.4\u0022\u003E\u003C\/script\u003E\n\u003Cscript src=\u0022\/core\/assets\/vendor\/jquery.ui\/ui\/draggable-min.js?v=1.11.4\u0022\u003E\u003C\/script\u003E\n\u003Cscript src=\u0022\/core\/assets\/vendor\/jquery.ui\/ui\/position-min.js?v=1.11.4\u0022\u003E\u003C\/script\u003E\n\u003Cscript src=\u0022\/core\/assets\/vendor\/jquery.ui\/ui\/resizable-min.js?v=1.11.4\u0022\u003E\u003C\/script\u003E\n\u003Cscript src=\u0022\/core\/assets\/vendor\/jquery.ui\/ui\/dialog-min.js?v=1.11.4\u0022\u003E\u003C\/script\u003E\n\u003Cscript src=\u0022\/core\/misc\/dialog\/dialog.js?v=8.0.5\u0022\u003E\u003C\/script\u003E\n\u003Cscript src=\u0022\/core\/misc\/dialog\/dialog.position.js?v=8.0.5\u0022\u003E\u003C\/script\u003E\n\u003Cscript src=\u0022\/core\/misc\/dialog\/dialog.jquery-ui.js?v=8.0.5\u0022\u003E\u003C\/script\u003E\n\u003Cscript src=\u0022\/core\/modules\/editor\/js\/editor.js?v=8.0.5\u0022\u003E\u003C\/script\u003E\n\u003Cscript src=\u0022\/core\/assets\/vendor\/ckeditor\/ckeditor.js?v=4.5.5\u0022\u003E\u003C\/script\u003E\n\u003Cscript src=\u0022\/core\/modules\/ckeditor\/js\/ckeditor.js?v=8.0.5\u0022\u003E\u003C\/script\u003E\n","settings":null},{"command":"insert","method":"replaceWith","selector":"[data-big-pipe-selector=\u0022callback=comment.lazy_builders%3ArenderForm\u0026args[0]=node\u0026args[1]=2\u0026args[2]=field_question_comments\u0026args[3]=comment\u0026token=6313718a\u0022]","data":"\u003Cform class=\u0022comment-comment-form comment-form ajax-comments-reply-form-node-2-field_question_comments-0-0 ajax-comments-form-add\u0022 id=\u0022ajax-comments-reply-form-node-2-field-question-comments-0-0\u0022 data-drupal-selector=\u0022comment-form\u0022 action=\u0022\/comment\/reply\/node\/2\/field_question_comments\u0022 method=\u0022post\u0022 accept-charset=\u0022UTF-8\u0022\u003E\n \u003Cinput data-drupal-selector=\u0022form-nvvpaonymtye-9r0lpvyrd0nfz0uwjiohyu7kqrdrvc\u0022 type=\u0022hidden\u0022 name=\u0022form_build_id\u0022 value=\u0022form-NvvpAOnYMTYE-9r0lpVyRD0nfZ0UwjIOHYu7kqrdRvc\u0022 \/\u003E\n\u003Cinput data-drupal-selector=\u0022edit-comment-comment-form-form-token\u0022 type=\u0022hidden\u0022 name=\u0022form_token\u0022 value=\u00221PB7abbCZbeRIzubQ8qx8qrWw5J_FsRs8dykDr4WVp4\u0022 \/\u003E\n\u003Cinput data-drupal-selector=\u0022edit-comment-comment-form\u0022 type=\u0022hidden\u0022 name=\u0022form_id\u0022 value=\u0022comment_comment_form\u0022 \/\u003E\n\u003Cinput data-drupal-selector=\u0022edit-html-id\u0022 type=\u0022hidden\u0022 name=\u0022html_id\u0022 value=\u0022ajax-comments-reply-form-node-2-field-question-comments-0-0\u0022 \/\u003E\n\u003Cdiv class=\u0022field--type-text-long field--name-comment-body field--widget-text-textarea js-form-wrapper form-wrapper\u0022 data-drupal-selector=\u0022edit-comment-body-wrapper\u0022 id=\u0022edit-comment-body-wrapper\u0022\u003E \u003Cdiv class=\u0022js-text-format-wrapper text-format-wrapper js-form-item form-item\u0022\u003E\n \u003Cdiv class=\u0022js-form-item form-item js-form-type-textarea form-type-textarea js-form-item-comment-body-0-value form-item-comment-body-0-value form-no-label\u0022\u003E\n \u003Cdiv class=\u0022form-textarea-wrapper\u0022\u003E\n \u003Ctextarea class=\u0022js-text-full text-full form-textarea required resize-vertical\u0022 data-drupal-selector=\u0022edit-comment-body-0-value\u0022 id=\u0022edit-comment-body-0-value\u0022 name=\u0022comment_body[0][value]\u0022 rows=\u00225\u0022 cols=\u002260\u0022 placeholder=\u0022\u0022 required=\u0022required\u0022 aria-required=\u0022true\u0022\u003E\u003C\/textarea\u003E\n\u003C\/div\u003E\n\n \u003C\/div\u003E\n\u003Cdiv data-drupal-selector=\u0022edit-comment-body-0-format\u0022 id=\u0022edit-comment-body-0-format\u0022 class=\u0022js-form-wrapper form-wrapper\u0022\u003E\u003Cinput data-editor-for=\u0022edit-comment-body-0-value\u0022 type=\u0022hidden\u0022 name=\u0022comment_body[0][format]\u0022 value=\u0022links_only\u0022 \/\u003E\n\u003C\/div\u003E\n\n \u003C\/div\u003E\n\n \u003C\/div\u003E\n\u003Cdiv class=\u0022field--type-language field--name-langcode field--widget-language-select js-form-wrapper form-wrapper\u0022 data-drupal-selector=\u0022edit-langcode-wrapper\u0022 id=\u0022edit-langcode-wrapper\u0022\u003E \n \u003C\/div\u003E\n\u003Cdiv data-drupal-selector=\u0022edit-actions\u0022 class=\u0022form-actions js-form-wrapper form-wrapper\u0022 id=\u0022edit-actions\u0022\u003E\u003Cinput data-drupal-selector=\u0022edit-ajax-comments-reply-form-node-2-field-question-comments-0-0\u0022 type=\u0022submit\u0022 id=\u0022edit-ajax-comments-reply-form-node-2-field-question-comments-0-0\u0022 name=\u0022op\u0022 value=\u0022Save\u0022 class=\u0022button button--primary js-form-submit form-submit\u0022 \/\u003E\n\u003C\/div\u003E\n\n\u003C\/form\u003E\n","settings":null}] </script> <script type="application/json" data-big-pipe-placeholder="callback=history_attach_timestamp&args[0]=2&token=15cf9f98" data-drupal-ajax-processor="big_pipe"> [{"command":"settings","settings":{"ajaxPageState":{"theme":"bartik","libraries":"ajax_comments\/commands,ajax_comments\/commands,bartik\/global-styling,big_pipe\/big_pipe,ckeditor\/drupal.ckeditor,classy\/base,classy\/messages,comment\/drupal.comment-by-viewer,comment\/drupal.comment-new-indicator,contextual\/drupal.contextual-links,contextual\/drupal.contextual-toolbar,core\/drupal.active-link,core\/html5shiv,core\/jquery.form,core\/normalize,filter\/drupal.filter,history\/mark-as-read,shortcut\/drupal.shortcut,statistics\/drupal.statistics,system\/base,toolbar\/toolbar,toolbar\/toolbar.escapeAdmin,tour\/tour,user\/drupal.user.icons"},"history":{"lastReadTimestamps":{"2":1458959751}},"pluralDelimiter":"\u0003","user":{"uid":"1","permissionsHash":"50f490420ed4c54558b56e853ec07d868d9317d8e1f2e7b76962c15f61956d44"}},"merge":true},{"command":"insert","method":"replaceWith","selector":"[data-big-pipe-selector=\u0022callback=history_attach_timestamp\u0026args[0]=2\u0026token=15cf9f98\u0022]","data":"","settings":null}] </script> <script type="application/json" data-big-pipe-placeholder="callback=comment.lazy_builders%3ArenderLinks&args[0]=12&args[1]=full&args[2]=en&args[3]=&token=2231368d" data-drupal-ajax-processor="big_pipe"> [{"command":"insert","method":"replaceWith","selector":"[data-big-pipe-selector=\u0022callback=comment.lazy_builders%3ArenderLinks\u0026args[0]=12\u0026args[1]=full\u0026args[2]=en\u0026args[3]=\u0026token=2231368d\u0022]","data":"\u003Cul class=\u0022links inline\u0022\u003E\u003Cli class=\u0022comment-delete\u0022\u003E\u003Ca href=\u0022\/comment\/12\/delete\u0022 class=\u0022use-ajax-comments ajax-comments-delete ajax-comments-delete-12\u0022 hreflang=\u0022en\u0022\u003EDelete\u003C\/a\u003E\u003C\/li\u003E\u003Cli class=\u0022comment-edit\u0022\u003E\u003Ca href=\u0022\/comment\/12\/edit\u0022 class=\u0022use-ajax-comments ajax-comments-edit ajax-comments-edit-12\u0022 hreflang=\u0022en\u0022\u003EEdit\u003C\/a\u003E\u003C\/li\u003E\u003Cli class=\u0022comment-reply\u0022\u003E\u003Ca href=\u0022\/comment\/reply\/node\/2\/field_question_comments\/12\u0022 class=\u0022use-ajax-comments ajax-comments-reply ajax-comments-reply-2-field_question_comments-12\u0022\u003EReply\u003C\/a\u003E\u003C\/li\u003E\u003C\/ul\u003E","settings":null}] </script> <script type="application/json" data-big-pipe-placeholder="callback=history_attach_timestamp&args[0]=2&token=15cf9f98" data-drupal-ajax-processor="big_pipe"> [{"command":"settings","settings":{"ajaxPageState":{"theme":"bartik","libraries":"ajax_comments\/commands,ajax_comments\/commands,bartik\/global-styling,big_pipe\/big_pipe,ckeditor\/drupal.ckeditor,classy\/base,classy\/messages,comment\/drupal.comment-by-viewer,comment\/drupal.comment-new-indicator,contextual\/drupal.contextual-links,contextual\/drupal.contextual-toolbar,core\/drupal.active-link,core\/html5shiv,core\/jquery.form,core\/normalize,filter\/drupal.filter,history\/mark-as-read,shortcut\/drupal.shortcut,statistics\/drupal.statistics,system\/base,toolbar\/toolbar,toolbar\/toolbar.escapeAdmin,tour\/tour,user\/drupal.user.icons"},"history":{"lastReadTimestamps":{"2":1458959751}},"pluralDelimiter":"\u0003","user":{"uid":"1","permissionsHash":"50f490420ed4c54558b56e853ec07d868d9317d8e1f2e7b76962c15f61956d44"}},"merge":true},{"command":"insert","method":"replaceWith","selector":"[data-big-pipe-selector=\u0022callback=history_attach_timestamp\u0026args[0]=2\u0026token=15cf9f98\u0022]","data":"","settings":null}] </script> <script type="application/json" data-big-pipe-placeholder="callback=comment.lazy_builders%3ArenderLinks&args[0]=96&args[1]=full&args[2]=en&args[3]=&token=9e4892ed" data-drupal-ajax-processor="big_pipe"> [{"command":"insert","method":"replaceWith","selector":"[data-big-pipe-selector=\u0022callback=comment.lazy_builders%3ArenderLinks\u0026args[0]=96\u0026args[1]=full\u0026args[2]=en\u0026args[3]=\u0026token=9e4892ed\u0022]","data":"\u003Cul class=\u0022links inline\u0022\u003E\u003Cli class=\u0022comment-delete\u0022\u003E\u003Ca href=\u0022\/comment\/96\/delete\u0022 class=\u0022use-ajax-comments ajax-comments-delete ajax-comments-delete-96\u0022 hreflang=\u0022en\u0022\u003EDelete\u003C\/a\u003E\u003C\/li\u003E\u003Cli class=\u0022comment-edit\u0022\u003E\u003Ca href=\u0022\/comment\/96\/edit\u0022 class=\u0022use-ajax-comments ajax-comments-edit ajax-comments-edit-96\u0022 hreflang=\u0022en\u0022\u003EEdit\u003C\/a\u003E\u003C\/li\u003E\u003Cli class=\u0022comment-reply\u0022\u003E\u003Ca href=\u0022\/comment\/reply\/node\/2\/field_question_comments\/96\u0022 class=\u0022use-ajax-comments ajax-comments-reply ajax-comments-reply-2-field_question_comments-96\u0022\u003EReply\u003C\/a\u003E\u003C\/li\u003E\u003C\/ul\u003E","settings":null}] </script><script type="application/json" data-big-pipe-event="stop"></script>
As the above code sample demonstrates, the script tags with the identifier data-big-pipe-placeholder="callback=history_attach_timestamp&args[0]=2&token=15cf9f98"
appear after the one with the identifier data-big-pipe-placeholder="callback=comment.lazy_builders%3ArenderForm&args[0]=node&args[1]=2&args[2]=field_question_comments&args[3]=comment&token=6313718a"
, and as a result the code in the big_pipe.js
file evaluates the script tags with callback=history_attach_timestamp
after the script tag containing the JSON for the comment form placeholder. This logic appears in the following code excerpt from bigPipeProcessDocument()
in big_pipe.js
:
$(context).find('script[data-big-pipe-replacement-for-placeholder-with-id]')
.once('big-pipe')
.each(bigPipeProcessPlaceholderReplacement);
When a given node has more than one comment (the above example has two, which is why there are two tags with data-big-pipe-placeholder="callback=history_attach_timestamp&args[0]=2&token=15cf9f98"
), the comment
module generates identical #lazy_builder
callbacks to history_attach_timestamp
for all of them, which big_pipe
in turn uses to generate identical placeholders. The identical placeholders become an issue when the bigPipeProcessPlaceholderReplacement()
function simulates an ajax success response using the identical script tags. The success()
method in the ajax system invokes the simulated command defined in the JSON content of the script tags, which uses the replaceWith
method. This triggers invocation of Drupal.AjaxCommands.prototype.insert()
command, which detaches behaviors attached to any DOM elements in the context of the placeholder:
Drupal.detachBehaviors(wrapper.get(0), settings);
The first time this executes on a placeholder generated for the #lazy_builder
callback for history_attach_timestamp
, the value of wrapper.get(0)
is <div data-big-pipe-placeholder-id="callback=history_attach_timestamp&args[0]=2&token=15cf9f98"></div>
. However, a few lines below that, the jQuery replaceWith
method is triggered:
// Add the new content to the page.
wrapper[method](new_content);
After this line executes, the DOM node <div data-big-pipe-placeholder-id="callback=history_attach_timestamp&args[0]=2&token=15cf9f98"></div>
is replaced with an empty div (<div></div>
), because the value of response.data
in the simulated ajax response is an empty string. The problem is that the next time this code executes on an identical placeholder from the script tags at the bottom of the document body, all instances of these placeholder divs in the DOM have been replaced with empty divs. The value for the wrapper
variable is now an empty object, because the selector in response.selector
refers to DOM nodes that have been removed:
// The value of response.selector is [data-big-pipe-placeholder-id="callback=history_attach_timestamp&args[0]=2&token=15cf9f98"]
var wrapper = response.selector ? $(response.selector) : $(ajax.wrapper);
Then, a few lines down, when Drupal.detachBehaviors()
runs, the value of wrapper.get(0)
evaluates to undefined
:
Drupal.detachBehaviors(wrapper.get(0), settings);
Drupal.detachBehaviors()
sets the context
in which it operates to the entire document if the context
is undefined; see the first line of the function:
Drupal.detachBehaviors = function (context, settings, trigger) {
context = context || document;
settings = settings || drupalSettings;
trigger = trigger || 'unload';
var behaviors = Drupal.behaviors;
// Execute all of them.
for (var i in behaviors) {
if (behaviors.hasOwnProperty(i) && typeof behaviors[i].detach === 'function') {
// Don't stop the execution of behaviors in case of an error.
try {
behaviors[i].detach(context, settings, trigger);
}
catch (e) {
Drupal.throwError(e);
}
}
}
};
The result is that behaviors are detached from the entire document, causing behaviors that were attached earlier in the execution process to be removed.
Proposed resolution
Proposed solution is to modify \Drupal\comment\CommentViewBuilder::buildComponents()
to prevent duplicate copies of the placeholder from being generated. The #lazy_builder
callback in the render array that causes Big Pipe to generate the placeholder seems to have one purpose: to add the timestamp for the comments' parent node to the drupalSettings
object.
The #lazy_builder
for the history_attach_timestamp
in the comment view builder is:
// Embed the metadata for the comment "new" indicators on this node.
$build[$id]['history'] = [
'#lazy_builder' => ['history_attach_timestamp', [$commented_entity->id()]],
'#create_placeholder' => TRUE,
];
The callback function for history_attach_timestamp()
is:
function history_attach_timestamp($node_id) {
$element = [];
$element['#attached']['drupalSettings']['history']['lastReadTimestamps'][$node_id] = (int) history_read($node_id);
return $element;
}
The history_attach_timestamp()
callback function always returns a render array that is empty, except for the settings being added to drupalSettings
. Since the settings are per node ID and are the same for all comments attached to the same node, subsequent invocations of this callback function merely result in this property of the drupalSettings
object being overwritten with the same value.
Proposed code change is to set this #lazy_builder
only once for the entire comment list:
diff --git a/core/modules/comment/src/CommentViewBuilder.php b/core/modules/comment/src/CommentViewBuilder.php
index 9b47b7c..435bae5 100644
--- a/core/modules/comment/src/CommentViewBuilder.php
+++ b/core/modules/comment/src/CommentViewBuilder.php
@@ -148,18 +148,18 @@ public function buildComponents(array &$build, array $entities, array $displays,
$build[$id]['#attached']['library'][] = 'comment/drupal.comment-by-viewer';
if ($this->moduleHandler->moduleExists('history') && $this->currentUser->isAuthenticated()) {
$build[$id]['#attached']['library'][] = 'comment/drupal.comment-new-indicator';
-
- // Embed the metadata for the comment "new" indicators on this node.
- $build[$id]['history'] = [
- '#lazy_builder' => ['history_attach_timestamp', [$commented_entity->id()]],
- '#create_placeholder' => TRUE,
- ];
}
}
if ($build[$id]['#comment_threaded']) {
// The final comment must close up some hanging divs.
$build[$id]['#comment_indent_final'] = $current_indent;
}
+
+ // Embed the metadata for the comment "new" indicators on this node.
+ $build['history'] = [
+ '#lazy_builder' => ['history_attach_timestamp', [$commented_entity->id()]],
+ '#create_placeholder' => TRUE,
+ ];
}
/**
Patch file attached.
Remaining tasks
Patch needs review.
User interface changes
None.
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#32 | interdiff2.txt | 1.02 KB | danmuzyka |
#30 | big_pipe_multi_occurence_js_placeholder-2698811-30.patch | 12.95 KB | danmuzyka |
#24 | big_pipe_multi_occurence_js_placeholder-2698811-24.patch | 13.85 KB | Wim Leers |
Comments
Comment #3
danmuzyka CreditAttribution: danmuzyka at Phase2 for Workday, Inc. commentedCorrected patch attached.
Comment #4
danmuzyka CreditAttribution: danmuzyka at Phase2 for Workday, Inc. commentedComment #6
danmuzyka CreditAttribution: danmuzyka at Phase2 for Workday, Inc. commentedUpdating patch file for 8.1.x branch.
Comment #7
jhedstromI've updated the IS with steps to reproduce, and also verified myself that this patch fixes the issue using those steps.
Can this bit now be moved below as well since it is the same
if
check?Comment #8
jhedstromThis patch is for manual testing purposes only. It moves the comment form above the list of comments in Bartik.
Comment #9
jhedstromI started on a javascript test, but did not get far enough to reproduce the error. For some reason, the custom test theme declared here isn't being used, so the comment form is still below the comments.
Comment #10
danmuzyka CreditAttribution: danmuzyka at Phase2 for Workday, Inc. commentedAfter further investigation, this issue seems to be related more generally to the way that Big Pipe processes placeholders. There are circumstances under which the original patch to the comment module won't fix the issue (e.g., if there are multiple comment fields on the same page), so a better solution seems to be for Big Pipe not to add duplicate copies of the placeholder JSON in the page footer.
I'm attaching a new patch that fixes this in Big Pipe instead of Comment, and includes JavaScript functional tests.
Comment #12
danmuzyka CreditAttribution: danmuzyka at Phase2 for Workday, Inc. commentedRerolled patch.
Comment #13
jhedstrom@danmuzyka could you attach patch that just contains the new test to illustrate the failure without the fix?
Comment #14
danmuzyka CreditAttribution: danmuzyka at Phase2 for Workday, Inc. commentedSure, here is the patch file with the test only.
Comment #16
jhedstromThis is looking good to me, both the fix and the test. Back to NR and tagging in hopes of getting a few more eyes on this for review.
Comment #17
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedNice work on the patch!
I would like to skip re-rendering placeholders that have already been replaced though - if possible and not just skip the JS replacement part.
Comment #18
Wim LeersCommentViewBuilder
. Having multiple lazy builders map to the same placeholder is very much intentional, and is an architectural requirement: the whole point is that if independent code paths need the same placeholder, that we only replace that placeholder once.history_attach_timestamp()
(verify yourself by putting a breakpoint at\Drupal\big_pipe\Render\Placeholder\BigPipeStrategy::doProcessPlaceholders()
), yet BigPipe wants to render each occurrence of a placeholder separately. i.e. the problem is\Drupal\big_pipe\Render\BigPipe::getPlaceholderOrder()
. That code relies on parsing to determine which placeholders to replace. This means the solution in #14 is also wrong.Patch attached.
This should still get a
\Drupal\big_pipe\Tests\BigPipeTest::testBigPipeJsMultipleOccurrencePlaceholders()
test. That would be a generic test. However, it's awesome that the current patch already contains a functional JS regression test! I'd like to keep that, but rename it to be specifically for regression tests.The changes here should be reverted.
s/comment/big_pipe/
BigPipeRegressionTest
This should not be necessary.
Can you move this into the test method?
testCommentFormRegression_2698811()
Comment #19
Wim LeersOops, forgot the patch.
Comment #20
danmuzyka CreditAttribution: danmuzyka at Phase2 for Workday, Inc. commentedHere's an updated patch file with the new, generic test, and an interdiff of it relative to the patch I posted in #12.
Comment #21
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedQuestion:
Will this also work when JS is off?
As there we can only replace one placeholder at a time ...
Comment #22
Wim Leers#20: splendid work! :) Thanks so much!
Here's first a reroll that fixes all my nits. That avoids needless back-and-forth. Zero code changes, just renaming, simplifying, strictifying.
Comment #23
Wim LeersYes, this already was only a problem when JS was turned on. My analysis in #18 already applies only to JS placeholders:
getPlaceholderOrder()
only runs for JS placeholders.We could look into the no-JS placeholder case too… and I decided to do so here.
In order to test that better, it's important that we can prove that no-JS placeholders are rendered multiple times. An easy way to test that, is by doing something evil: using a
static
inside a lazy builder, so we can increment a counter. Then the JS version would show "Count=1. Count=1. Count=1." The no-JS version would show "Count=1. Count=2. Count=3." And that's exactly what happens.Attached patch expands that test coverage, and fails.
Comment #24
Wim LeersAnd here are then the changes for
\Drupal\big_pipe\Render\BigPipe::sendNoJsPlaceholders()
.As far as I'm concerned, this is RTBC. If Fabian agrees, he can RTBC this right away.
Comment #25
Wim LeersOh, and this should go in 8.1, not 8.2. BigPipe is experimental, so we can commit this to 8.1 :)
Also testing #24 against 8.1.
Comment #26
Wim Leers@danmuzyka & @jhedstrom: I just wanted to say thanks again for your excellent work here. These are much higher quality comments and patches than usual, so please know it is very much appreciated. I've repaid the favor by helping to land this issue swiftly. I'm sorry for not having seen it sooner, if I would have, then I'd have helped land it right then!
Comment #28
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedRTBC - Yes, indeed fantastic work on the patches and tests.
It is great to have both a JS regression test (yeah!) and functional tests.
Also good that we now no longer re-replace multiple occurences, which will help for all "token" based placeholders, so a nice perf improvement, too.
Comment #29
Wim LeersIndeed!
This seems like a perfect model:
At least, I personally think that's the perfect model. It allows me to have very strong confidence that it works as intended.
Comment #30
danmuzyka CreditAttribution: danmuzyka at Phase2 for Workday, Inc. commentedThanks @Wim Leers and @Fabianx !
If you feel like fast-tracking any other core bugs, I have two other open ones at the moment: ;-)
I think in the latest patch, the callback function
placeholderContent()
is no longer needed. I'm attaching an updated patch that removes it.Comment #31
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented#30: Could you make interdiffs with git diff OR with diff -u?
They are very hard to read atm. ...
Comment #32
danmuzyka CreditAttribution: danmuzyka at Phase2 for Workday, Inc. commentedSorry, this one should be better...
Comment #33
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedIndeed, back to RTBC
Comment #35
jhedstromThose fails do not look related to this patch.
Comment #38
catchAgreed the patch and tests look great. Committed/pushed to 8.2.x and cherry-picked to 8.1.x. Thanks!
Comment #39
Wim LeersAlso committed & pushed to the contrib module for Drupal 8.0: http://drupalcode.org/project/big_pipe.git/commit/f60bceb.