From 6dcb987983bf8e971166ee5f32dd503f780f2539 Mon Sep 17 00:00:00 2001 From: Adam Cassis Date: Tue, 19 May 2026 10:57:41 +0200 Subject: [PATCH 1/5] fix(popups): portal overlay lightboxes to wp_footer to escape ancestor stacking contexts Scroll-triggered overlay popups were rendered as the first child of `the_content`, which makes them descend from `.entry-content`. Any ancestor wrapper that creates a stacking context (transform, position+z-index, isolation, will-change, contain) traps the popup's `z-index: 99999` local to that context, so siblings of the wrapper with even a low z-index can paint over the lightbox close button. Queue overlay lightboxes at the inserter level and flush them at `wp_footer` priority 100 so they render as direct children of ``. Inline / above-header / archive placements stay where they were. Scroll-triggered overlays keep their `#page-position-marker` inline in `.entry-content` (its percentage `top` resolves against the article column and drives the IntersectionObserver that reveals the lightbox). Only the lightbox itself is portaled; `generate_popup()` gets a `\$include_position_marker` flag, new helper `generate_position_marker()` emits the marker alone. Tested in docker (195/195 pass) and verified end-to-end on a local repro that wraps `.entry-content` in a transformed/positioned container with a higher-z sibling rail. Co-Authored-By: Claude Opus 4.7 --- includes/class-newspack-popups-inserter.php | 97 +++++- includes/class-newspack-popups-model.php | 45 ++- tests/test-block-theme-header-insertion.php | 19 +- tests/test-insertion.php | 21 +- tests/test-overlay-queueing.php | 308 ++++++++++++++++++++ tests/wp-unittestcase-pagewithpopups.php | 18 +- 6 files changed, 478 insertions(+), 30 deletions(-) create mode 100644 tests/test-overlay-queueing.php diff --git a/includes/class-newspack-popups-inserter.php b/includes/class-newspack-popups-inserter.php index d3647c0f8..71893221e 100755 --- a/includes/class-newspack-popups-inserter.php +++ b/includes/class-newspack-popups-inserter.php @@ -52,6 +52,15 @@ final class Newspack_Popups_Inserter { */ private static $header_template_part_has_rendered = false; + /** + * Overlay prompts queued for rendering at wp_footer, keyed by popup ID so a + * popup that would otherwise be emitted from multiple injection points + * (singular content, archive header, block-theme header) only renders once. + * + * @var array + */ + private static $queued_overlays = []; + /** * Constructor. */ @@ -65,6 +74,11 @@ public function __construct() { add_filter( 'render_block', [ $this, 'insert_inline_prompt_in_block_theme_archives' ], 10, 3 ); add_action( 'wp_before_admin_bar_render', [ $this, 'add_preview_toggle' ] ); + // Render overlay prompts as direct children of so they escape any + // ancestor stacking context (transformed parents, ad wrappers, etc.) that + // would otherwise trap their z-index below sibling content. + add_action( 'wp_footer', [ $this, 'print_queued_overlays' ], 100 ); + // Always enqueue scripts, since this plugin's scripts are handling pageview sending via GTAG. add_action( 'wp_enqueue_scripts', [ $this, 'enqueue_scripts' ] ); add_action( 'apple_news_do_fetch_exporter', [ __CLASS__, 'apple_news_do_fetch_exporter' ] ); @@ -456,13 +470,20 @@ function ( $block_groups, $block ) use ( &$block_index, $parsed_blocks, $max_ind } } - // 4. Insert overlay prompts at the top of content. - // To leave the existing behavior (prepending each overlay) in place, - // we reverse our sorted overlays to ensure the most specific appear - // first in the DOM, and get priority for the single available slot. - $overlay_popups = array_reverse( self::sort_overlays_by_specificity( $overlay_popups ) ); - foreach ( $overlay_popups as $overlay_popup ) { - $output = '' . Newspack_Popups_Model::generate_popup( $overlay_popup ) . '' . $output; + // 4. Queue overlay prompts for footer rendering. Reverse the specificity + // sort so the most specific overlay is queued first and wins the single + // visible slot, matching the previous "prepend to content" DOM order. + // Scroll-triggered overlays carry a page-position marker which must + // remain inline in `.entry-content` so its percentage offset resolves + // against the article column (the marker drives IntersectionObserver + // reveal of the lightbox). Emit the marker inline and queue only the + // lightbox itself for the footer. + foreach ( array_reverse( self::sort_overlays_by_specificity( $overlay_popups ) ) as $overlay_popup ) { + self::queue_overlay( $overlay_popup ); + $marker = Newspack_Popups_Model::generate_position_marker( $overlay_popup ); + if ( '' !== $marker ) { + $output = '' . $marker . '' . $output; + } } return $output; } @@ -564,12 +585,56 @@ function ( $popup ) { return Newspack_Popups_Model::should_be_inserted_in_page_content( $popup ) && Newspack_Popups_Model::is_overlay( $popup ); } ); - $popups = self::sort_overlays_by_specificity( array_values( $popups ) ); - foreach ( $popups as $popup ) { - echo Newspack_Popups_Model::generate_popup( $popup ); // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped + foreach ( self::sort_overlays_by_specificity( array_values( $popups ) ) as $popup ) { + self::queue_overlay( $popup ); + } + } + + /** + * Queue an overlay popup for rendering at wp_footer. + * + * Overlays are deduped by popup ID, so the same overlay queued from multiple + * injection points (singular content + above-header, archive header, etc.) + * only renders once. Queue order is preserved, so the existing specificity + * sort done by callers still controls which overlay wins the single visible + * slot. + * + * @param array $popup Popup data as returned by Newspack_Popups_Model. + */ + private static function queue_overlay( $popup ) { + if ( empty( $popup['id'] ) ) { + return; + } + $id = $popup['id']; + if ( ! isset( self::$queued_overlays[ $id ] ) ) { + self::$queued_overlays[ $id ] = $popup; } } + /** + * Print all queued overlay popups as direct children of , via wp_footer. + * + * This decouples overlay output from any ancestor stacking context that the + * popup would otherwise inherit when emitted inside post content or above + * the site header — for example a transformed/scaled wrapper, a sticky ad + * container, or any element with isolation:isolate. Inline popups remain + * inline; only the overlay-typed placements are portaled. + */ + public static function print_queued_overlays() { + if ( empty( self::$queued_overlays ) ) { + return; + } + foreach ( self::$queued_overlays as $popup ) { + // Suppress the scroll-trigger page-position marker: for scroll-triggered + // overlays it was emitted inline at the content position so its + // percentage offset still resolves against `.entry-content`. For time- + // triggered overlays there is no marker to begin with. Emitting the + // marker here (at body level) would break scroll-trigger detection. + echo Newspack_Popups_Model::generate_popup( $popup, false ); // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped + } + self::$queued_overlays = []; + } + /** * Get the combined markup for all above-header prompts: overlays (sorted by specificity) * first, then inline prompts in their original order. @@ -584,11 +649,16 @@ private static function get_before_header_markup() { // Sort only the overlay subset by specificity — above-header inline prompts are // not subject to the single visible overlay slot constraint and are left in their - // original order. + // original order. Overlays are queued for footer rendering (escaping nested + // stacking contexts); only the inline prompts are emitted inline here. $overlay_popups = self::sort_overlays_by_specificity( array_values( array_filter( $before_header_popups, [ 'Newspack_Popups_Model', 'is_overlay' ] ) ) ); - $inline_popups = array_values( + foreach ( $overlay_popups as $popup ) { + self::queue_overlay( $popup ); + } + + $inline_popups = array_values( array_filter( $before_header_popups, function( $popup ) { @@ -598,9 +668,6 @@ function( $popup ) { ); $markup = ''; - foreach ( $overlay_popups as $popup ) { - $markup .= Newspack_Popups_Model::generate_popup( $popup ); - } foreach ( $inline_popups as $popup ) { $markup .= Newspack_Popups_Model::generate_popup( $popup ); } diff --git a/includes/class-newspack-popups-model.php b/includes/class-newspack-popups-model.php index 8b3b8ca53..5146a6295 100644 --- a/includes/class-newspack-popups-model.php +++ b/includes/class-newspack-popups-model.php @@ -1085,13 +1085,52 @@ public static function get_current_popup() { return self::$current_popup; } + /** + * Generate just the scroll-trigger page-position marker for an overlay popup. + * + * The marker is a `position: absolute; top: X%` element whose position is + * computed against its nearest `position: relative` ancestor (typically the + * post's `.entry-content`). An IntersectionObserver on this marker is what + * reveals the scroll-triggered overlay. When the lightbox itself is portaled + * to wp_footer to escape an ancestor stacking context, the marker must remain + * inline inside the article container so its percentage offset still + * encodes "scroll progress through the article". + * + * Returns the empty string for popups that aren't scroll-triggered overlays. + * + * @param array $popup The popup object. + * @return string Marker HTML, or '' when no marker is needed. + */ + public static function generate_position_marker( $popup ) { + if ( ! self::is_overlay( $popup ) || 'scroll' !== $popup['options']['trigger_type'] ) { + return ''; + } + $element_id = self::canonize_popup_id( $popup['id'] ); + return sprintf( + '
', + esc_attr( $element_id ), + esc_attr( $popup['options']['trigger_scroll_progress'] ) + ); + } + /** * Generate markup and styles for an overlay popup. * - * @param string $popup The popup object. + * @param string $popup The popup object. + * @param bool $include_position_marker Whether to append the scroll-trigger + * page-position marker. Defaults to true + * for backward compatibility. Callers that + * portal the lightbox to a different DOM + * location (e.g. wp_footer) should pass + * false and emit the marker inline at the + * original content location separately via + * {@see generate_position_marker()}, since + * the marker positions itself as a percentage + * inside its `position: relative` ancestor + * (typically .entry-content). * @return string The generated markup. */ - public static function generate_popup( $popup ) { + public static function generate_popup( $popup, $include_position_marker = true ) { $previewed_popup_id = Newspack_Popups::previewed_popup_id(); $is_manual_or_custom_placement = self::is_manual_only( $popup ) || Newspack_Popups_Custom_Placements::is_custom_placement_or_manual( $popup ); @@ -1204,7 +1243,7 @@ class="" - +
header content'; $block = $this->get_header_template_part_block(); - $result = Newspack_Popups_Inserter::insert_before_header_in_template_part( $block_content, $block ); + Newspack_Popups_Inserter::insert_before_header_in_template_part( $block_content, $block ); - $segment_pos = strpos( $result, 'Segment overlay' ); - $generic_pos = strpos( $result, 'Generic overlay' ); + // Overlays are portaled to wp_footer rather than inlined into the + // header template-part output; assert ordering against the queued + // flush, which is what gets emitted before . + ob_start(); + Newspack_Popups_Inserter::print_queued_overlays(); + $footer_output = ob_get_clean(); - $this->assertNotFalse( $segment_pos, 'Segment-specific overlay should be rendered.' ); - $this->assertNotFalse( $generic_pos, 'Generic overlay should be rendered.' ); - $this->assertLessThan( $generic_pos, $segment_pos, 'Segment-specific overlay should render before generic overlay.' ); + $segment_pos = strpos( $footer_output, 'Segment overlay' ); + $generic_pos = strpos( $footer_output, 'Generic overlay' ); + + $this->assertNotFalse( $segment_pos, 'Segment-specific overlay should be queued for footer rendering.' ); + $this->assertNotFalse( $generic_pos, 'Generic overlay should be queued for footer rendering.' ); + $this->assertLessThan( $generic_pos, $segment_pos, 'Segment-specific overlay should be emitted before generic overlay.' ); } } diff --git a/tests/test-insertion.php b/tests/test-insertion.php index 584b3f407..281c1e80f 100755 --- a/tests/test-insertion.php +++ b/tests/test-insertion.php @@ -51,12 +51,23 @@ public function test_insertion_on_page() { $overlay_id = self::createPopup( $overlay_content, [ 'placement' => 'center' ] ); $page_with_shortcode = '[newspack-popups id="' . $overlay_id . '"]'; self::renderPost( '', $page_with_shortcode, [], [], 'page' ); - $overlay_text_content = self::$dom_xpath->query( '//*[contains(@class,"newspack-popup-container")]' )->item( 0 )->textContent; - self::assertStringContainsString( - $overlay_content, - $overlay_text_content, - 'Inserts the overlay prompt on a page.' + // Overlays are portaled to wp_footer, so the inline default popup + // (from set_up) is item(0) and the overlay sits later in the combined + // content+footer markup. Locate the overlay by its content instead of + // by DOM position. + $popup_elements = self::$dom_xpath->query( '//*[contains(@class,"newspack-popup-container")]' ); + $found_overlay = false; + foreach ( $popup_elements as $popup_element ) { + if ( false !== strpos( $popup_element->textContent, $overlay_content ) ) { + $found_overlay = true; + break; + } + } + + self::assertTrue( + $found_overlay, + 'Inserts the overlay prompt on a page (portaled to footer, located via content).' ); } diff --git a/tests/test-overlay-queueing.php b/tests/test-overlay-queueing.php new file mode 100644 index 000000000..391965300 --- /dev/null +++ b/tests/test-overlay-queueing.php @@ -0,0 +1,308 @@ + + * and escapes any ancestor stacking context (transformed wrapper, sticky ad + * container, isolation:isolate, etc.) that would otherwise trap the popup's + * z-index below sibling content. + * + * @package Newspack_Popups + */ + +/** + * OverlayQueueing test case. + */ +class OverlayQueueingTest extends WP_UnitTestCase { + + public function set_up() { // phpcs:ignore Squiz.Commenting.FunctionComment.Missing + parent::set_up(); + // Drain any queued overlays carried over from prior tests. + ob_start(); + Newspack_Popups_Inserter::print_queued_overlays(); + ob_end_clean(); + } + + /** + * Build an overlay popup object via the real model so it carries every + * default option the markup generator expects. + * + * @param string $title Optional title. + * @param string $trigger_type 'time' or 'scroll'. Scroll-triggered overlays + * ship with a page-position marker that must stay + * inline in the content. + * @return array Popup object as consumed by the inserter. + */ + private static function create_overlay_popup_object( $title = 'Overlay prompt', $trigger_type = 'time' ) { + $popup_id = self::factory()->post->create( + [ + 'post_type' => Newspack_Popups::NEWSPACK_POPUPS_CPT, + 'post_title' => $title, + 'post_content' => 'Overlay body for ' . $title, + ] + ); + Newspack_Popups_Model::set_popup_options( + $popup_id, + [ + 'placement' => 'center', + 'trigger_type' => $trigger_type, + ] + ); + return Newspack_Popups_Model::create_popup_object( get_post( $popup_id ) ); + } + + /** + * The returned content from insert_popups_in_post_content must NOT include + * the overlay markup — it should be queued for wp_footer instead. + */ + public function test_overlay_not_inlined_into_returned_content() { + $overlay_popup = self::create_overlay_popup_object(); + $post_content = "\n

Body paragraph.

\n\n"; + + $returned_content = Newspack_Popups_Inserter::insert_popups_in_post_content( + $post_content, + [ $overlay_popup ] + ); + + self::assertStringNotContainsString( + 'newspack-lightbox', + $returned_content, + 'Overlay markup must not be inlined into post content; it is queued for wp_footer.' + ); + self::assertStringNotContainsString( + '', + $returned_content, + 'The legacy wp:html wrapper around inlined overlay markup must no longer appear in returned content.' + ); + self::assertStringContainsString( + 'Body paragraph.', + $returned_content, + 'Original post content must be preserved.' + ); + } + + /** + * After insert_popups_in_post_content queues an overlay, print_queued_overlays + * emits the overlay markup once. + */ + public function test_queued_overlay_is_emitted_at_footer() { + $overlay_popup = self::create_overlay_popup_object(); + Newspack_Popups_Inserter::insert_popups_in_post_content( '

Body.

', [ $overlay_popup ] ); + + ob_start(); + Newspack_Popups_Inserter::print_queued_overlays(); + $footer_output = ob_get_clean(); + + self::assertStringContainsString( + 'newspack-lightbox', + $footer_output, + 'print_queued_overlays must emit the queued overlay markup.' + ); + } + + /** + * Queueing the same overlay popup from multiple call paths (e.g. singular + * content + above-header) must result in a single emission, deduped by ID. + * + * Compared via flushed-output length: queueing N times must produce the same + * output length as queueing once. Length is robust against internal markup + * structure (the lightbox emits the "newspack-lightbox" substring in + * several child class names per popup). + */ + public function test_dedupe_by_id_across_multiple_queue_calls() { + $overlay_popup = self::create_overlay_popup_object(); + + Newspack_Popups_Inserter::insert_popups_in_post_content( '

Body.

', [ $overlay_popup ] ); + ob_start(); + Newspack_Popups_Inserter::print_queued_overlays(); + $single_queue_output = ob_get_clean(); + + // Queue the SAME popup multiple times via repeated calls. + Newspack_Popups_Inserter::insert_popups_in_post_content( '

Body.

', [ $overlay_popup ] ); + Newspack_Popups_Inserter::insert_popups_in_post_content( '

Body.

', [ $overlay_popup ] ); + Newspack_Popups_Inserter::insert_popups_in_post_content( '

Body.

', [ $overlay_popup ] ); + ob_start(); + Newspack_Popups_Inserter::print_queued_overlays(); + $repeat_queue_output = ob_get_clean(); + + self::assertSame( + strlen( $single_queue_output ), + strlen( $repeat_queue_output ), + 'Queueing the same popup multiple times must produce the same output as queueing once.' + ); + self::assertNotSame( + '', + $single_queue_output, + 'Sanity check: the flushed output should not be empty for a queued overlay.' + ); + } + + /** + * Inline placements must still be inlined into post content — only + * overlay-typed placements are portaled. + */ + public function test_inline_placement_remains_in_returned_content() { + $inline_popup = [ + 'id' => wp_rand(), + 'content' => 'Inline content.', + 'options' => [ + 'placement' => 'inline', + 'trigger_type' => 'scroll', + 'trigger_scroll_progress' => '0', + 'trigger_blocks_count' => '0', + ], + ]; + $returned_content = Newspack_Popups_Inserter::insert_popups_in_post_content( + "\n

Body.

\n\n", + [ $inline_popup ] + ); + + self::assertStringContainsString( + '[newspack-popup id="' . $inline_popup['id'] . '"]', + $returned_content, + 'Inline popups must still be emitted into post content as the shortcode block.' + ); + + ob_start(); + Newspack_Popups_Inserter::print_queued_overlays(); + $footer_output = ob_get_clean(); + + self::assertSame( + '', + $footer_output, + 'Inline popups must never reach the overlay footer queue.' + ); + } + + /** + * Mixed batch: one inline + one overlay. Inline goes inline; overlay goes + * to the footer queue. + */ + public function test_inline_and_overlay_route_to_their_respective_paths() { + $overlay_popup = self::create_overlay_popup_object(); + $inline_popup = [ + 'id' => wp_rand(), + 'content' => 'Inline content.', + 'options' => [ + 'placement' => 'inline', + 'trigger_type' => 'scroll', + 'trigger_scroll_progress' => '0', + 'trigger_blocks_count' => '0', + ], + ]; + + $returned_content = Newspack_Popups_Inserter::insert_popups_in_post_content( + "\n

Body.

\n\n", + [ $inline_popup, $overlay_popup ] + ); + + self::assertStringContainsString( + '[newspack-popup id="' . $inline_popup['id'] . '"]', + $returned_content, + 'Inline popup must still be inlined.' + ); + self::assertStringNotContainsString( + 'newspack-lightbox', + $returned_content, + 'Overlay popup must not leak into post content when batched with an inline popup.' + ); + + ob_start(); + Newspack_Popups_Inserter::print_queued_overlays(); + $footer_output = ob_get_clean(); + + self::assertStringContainsString( + 'newspack-lightbox', + $footer_output, + 'Overlay popup must be emitted at the footer.' + ); + self::assertStringNotContainsString( + '[newspack-popup id="' . $inline_popup['id'] . '"]', + $footer_output, + 'Inline popup must never leak into the footer queue output.' + ); + } + + /** + * Scroll-triggered overlays must keep their page-position marker inline in + * the post content (the IntersectionObserver mechanism for scroll-trigger + * needs the marker positioned against `.entry-content`). The lightbox itself + * is still portaled to the footer queue. + */ + public function test_scroll_triggered_overlay_marker_stays_inline() { + $overlay_popup = self::create_overlay_popup_object( 'Scroll overlay', 'scroll' ); + + $returned_content = Newspack_Popups_Inserter::insert_popups_in_post_content( + "\n

Body.

\n\n", + [ $overlay_popup ] + ); + + self::assertStringContainsString( + 'page-position-marker_', + $returned_content, + 'Scroll-triggered overlays must emit their page-position marker inline in the post content.' + ); + self::assertStringNotContainsString( + 'newspack-lightbox', + $returned_content, + 'The lightbox itself must still be queued for footer rendering, not emitted inline.' + ); + + ob_start(); + Newspack_Popups_Inserter::print_queued_overlays(); + $footer_output = ob_get_clean(); + + self::assertStringContainsString( + 'newspack-lightbox', + $footer_output, + 'The lightbox should be emitted at wp_footer.' + ); + self::assertStringNotContainsString( + 'page-position-marker_', + $footer_output, + 'The footer-emitted lightbox must NOT carry a duplicate page-position marker; the inline one is the one that drives scroll trigger.' + ); + } + + /** + * Time-triggered overlays have no page-position marker. None should be + * emitted inline, and the lightbox is queued for footer. + */ + public function test_time_triggered_overlay_has_no_inline_marker() { + $overlay_popup = self::create_overlay_popup_object( 'Time overlay', 'time' ); + + $returned_content = Newspack_Popups_Inserter::insert_popups_in_post_content( + "\n

Body.

\n\n", + [ $overlay_popup ] + ); + + self::assertStringNotContainsString( + 'page-position-marker_', + $returned_content, + 'Time-triggered overlays must not produce a page-position marker.' + ); + } + + /** + * Flushing the queue must clear it: a second flush emits nothing. + */ + public function test_flush_clears_the_queue() { + $overlay_popup = self::create_overlay_popup_object(); + Newspack_Popups_Inserter::insert_popups_in_post_content( '

Body.

', [ $overlay_popup ] ); + + ob_start(); + Newspack_Popups_Inserter::print_queued_overlays(); + ob_end_clean(); + + ob_start(); + Newspack_Popups_Inserter::print_queued_overlays(); + $second_flush = ob_get_clean(); + + self::assertSame( + '', + $second_flush, + 'Flushing the queue must drain it; a subsequent flush should be a no-op.' + ); + } +} diff --git a/tests/wp-unittestcase-pagewithpopups.php b/tests/wp-unittestcase-pagewithpopups.php index 3a8767554..69027d765 100644 --- a/tests/wp-unittestcase-pagewithpopups.php +++ b/tests/wp-unittestcase-pagewithpopups.php @@ -116,9 +116,25 @@ protected function renderPost( $url_query = '', $post_content_override = null, $ // Reset internal duplicate-prevention. Newspack_Popups_Inserter::$the_content_has_rendered = false; + // Drain any overlays queued from previous renders in this test class. + ob_start(); + Newspack_Popups_Inserter::print_queued_overlays(); + ob_end_clean(); + $content = get_post( $post_id )->post_content; - self::$post_content = apply_filters( 'the_content', $content ); + $filtered_content = apply_filters( 'the_content', $content ); + + // Overlay prompts are now portaled to wp_footer rather than emitted + // inside post content. Concatenate the flushed footer output so the + // existing DOM-based assertions can still find overlay markup via the + // `newspack-popup-container` class, mirroring how the rendered page + // actually looks in the browser (post body + overlays before ). + ob_start(); + Newspack_Popups_Inserter::print_queued_overlays(); + $footer_overlays = ob_get_clean(); + + self::$post_content = $filtered_content . $footer_overlays; $dom = new DomDocument(); @$dom->loadHTML( self::$post_content ); // phpcs:ignore WordPress.PHP.NoSilencedErrors.Discouraged self::$dom_xpath = new DOMXpath( $dom ); From ece6806941e2e72c87aa1aa3504d0fee4e21e7e2 Mon Sep 17 00:00:00 2001 From: Adam Cassis Date: Tue, 19 May 2026 11:01:23 +0200 Subject: [PATCH 2/5] chore: silence phpcs snake_case warning on DOMNode textContent access The DOM API property is camelCase by spec; cannot be renamed. Co-Authored-By: Claude Opus 4.7 --- tests/test-insertion.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test-insertion.php b/tests/test-insertion.php index 281c1e80f..0e2a29273 100755 --- a/tests/test-insertion.php +++ b/tests/test-insertion.php @@ -59,7 +59,7 @@ public function test_insertion_on_page() { $popup_elements = self::$dom_xpath->query( '//*[contains(@class,"newspack-popup-container")]' ); $found_overlay = false; foreach ( $popup_elements as $popup_element ) { - if ( false !== strpos( $popup_element->textContent, $overlay_content ) ) { + if ( false !== strpos( $popup_element->textContent, $overlay_content ) ) { // phpcs:ignore WordPress.NamingConventions.ValidVariableName.UsedPropertyNotSnakeCase -- DOMNode property. $found_overlay = true; break; } From 1ccae424ff98162fb0ac32d2b6d77d2a747799ea Mon Sep 17 00:00:00 2001 From: Adam Cassis Date: Tue, 19 May 2026 11:29:40 +0200 Subject: [PATCH 3/5] fix(popups): address review findings on overlay portal hotfix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Blockers: - Drop array_reverse on the singular-content queue site: most-specific overlay must be queued first so the JS reveal walks it first and a segment-targeted prompt isn't suppressed by a generic one. - Emit the scroll-trigger page-position marker inline on the classic-theme archive path (insert_popups_after_header) and the block-theme above-header path (get_before_header_markup), not only on the singular content path. Without it, scroll-triggered overlays on archive pages silently never reveal. - Un-scope the wide/full-alignment CSS rules in style.scss from `.entry-content .newspack-lightbox` — the lightbox now renders as a body child, so the descendant scope no longer matches. Suggestions: - Move the wp_footer flush from priority 100 to 9 so it runs before wp_print_footer_scripts (priority 20) — overlay content that lazy-enqueues frontend assets keeps working. - New $emitted_markers static parallel to $queued_overlays, so the marker also dedupes when the inserter is reached from multiple paths in one request (prevents duplicate page-position-marker DOM IDs that would break document.getElementById lookups). - Defensive access on $popup['options']['trigger_type'] in generate_position_marker(); cast trigger_scroll_progress through absint() to harden the style attribute against weird meta values. - Replace strlen-based dedupe assertion with substr_count of the outer lightbox container id — direct and durable. - New tests: cross-injection-point dedupe + classic-archive marker emission. 197/197 PHPUnit pass. Cleanups: - Switch hook registration to [__CLASS__, 'print_queued_overlays'] — the method is already public static. - PHP 8.3 type hints on the new functions. - @param string $popup → array; drop "for backward compatibility" framing on $include_position_marker (it's a brand-new flag). - Refactor: emit_position_marker_inline() helper that dedupes and wraps in wp:html. Used from all three insertion paths. - En-dashes per project style. Co-Authored-By: Claude Opus 4.7 --- includes/class-newspack-popups-inserter.php | 128 +++++++++++++------- includes/class-newspack-popups-model.php | 50 ++++---- src/view/style.scss | 57 +++++---- tests/test-overlay-queueing.php | 110 ++++++++++++++--- 4 files changed, 236 insertions(+), 109 deletions(-) diff --git a/includes/class-newspack-popups-inserter.php b/includes/class-newspack-popups-inserter.php index 71893221e..316dec943 100755 --- a/includes/class-newspack-popups-inserter.php +++ b/includes/class-newspack-popups-inserter.php @@ -53,14 +53,27 @@ final class Newspack_Popups_Inserter { private static $header_template_part_has_rendered = false; /** - * Overlay prompts queued for rendering at wp_footer, keyed by popup ID so a + * Overlay prompts queued for rendering at wp_footer, keyed by popup ID. A * popup that would otherwise be emitted from multiple injection points * (singular content, archive header, block-theme header) only renders once. + * Queue order is the insertion order – callers that care about which + * overlay wins the single visible slot (i.e. segmentation specificity) must + * queue most-specific first. * - * @var array + * @var array> */ private static $queued_overlays = []; + /** + * Popup IDs whose scroll-trigger page-position marker has already been + * emitted inline this request. Parallel to {@see $queued_overlays} so the + * marker (which lives inside `.entry-content`, separate from the queued + * lightbox) also dedupes across multi-emission paths. + * + * @var array + */ + private static $emitted_markers = []; + /** * Constructor. */ @@ -74,10 +87,14 @@ public function __construct() { add_filter( 'render_block', [ $this, 'insert_inline_prompt_in_block_theme_archives' ], 10, 3 ); add_action( 'wp_before_admin_bar_render', [ $this, 'add_preview_toggle' ] ); - // Render overlay prompts as direct children of so they escape any - // ancestor stacking context (transformed parents, ad wrappers, etc.) that - // would otherwise trap their z-index below sibling content. - add_action( 'wp_footer', [ $this, 'print_queued_overlays' ], 100 ); + // Flush queued overlay prompts at wp_footer. Echoing from a wp_footer + // callback (with no surrounding container in the callback itself) lands + // the markup as a direct child of , which is what lets it escape + // any ancestor stacking context that would otherwise trap its z-index. + // Priority 9 runs *before* wp_print_footer_scripts (priority 20), so any + // asset that an overlay's content block / shortcode enqueues at render + // time still makes it into the page's footer scripts. + add_action( 'wp_footer', [ __CLASS__, 'print_queued_overlays' ], 9 ); // Always enqueue scripts, since this plugin's scripts are handling pageview sending via GTAG. add_action( 'wp_enqueue_scripts', [ $this, 'enqueue_scripts' ] ); @@ -470,20 +487,16 @@ function ( $block_groups, $block ) use ( &$block_index, $parsed_blocks, $max_ind } } - // 4. Queue overlay prompts for footer rendering. Reverse the specificity - // sort so the most specific overlay is queued first and wins the single - // visible slot, matching the previous "prepend to content" DOM order. - // Scroll-triggered overlays carry a page-position marker which must - // remain inline in `.entry-content` so its percentage offset resolves - // against the article column (the marker drives IntersectionObserver - // reveal of the lightbox). Emit the marker inline and queue only the - // lightbox itself for the footer. - foreach ( array_reverse( self::sort_overlays_by_specificity( $overlay_popups ) ) as $overlay_popup ) { + // 4. Queue overlay prompts for footer rendering, most-specific first so + // it wins the single visible slot (the client-side reveal walks prompts + // in DOM order and picks the first that passes segment + frequency + // gates). Scroll-triggered overlays carry a page-position marker which + // must remain inline in `.entry-content` – its percentage `top` resolves + // against the article column and drives the IntersectionObserver that + // reveals the lightbox. + foreach ( self::sort_overlays_by_specificity( $overlay_popups ) as $overlay_popup ) { self::queue_overlay( $overlay_popup ); - $marker = Newspack_Popups_Model::generate_position_marker( $overlay_popup ); - if ( '' !== $marker ) { - $output = '' . $marker . '' . $output; - } + $output = self::emit_position_marker_inline( $overlay_popup ) . $output; } return $output; } @@ -572,7 +585,11 @@ function ( $popup ) use ( $shortcoded_popups_ids ) { } /** - * Insert overlay prompts into archive pages if needed. Applies to Newspack Theme only. + * Queue archive-page overlay prompts (Newspack classic theme `after_header` + * hook). The actual lightbox markup is emitted later from + * {@see print_queued_overlays()}; the scroll-trigger page-position marker + * is emitted inline here so its `top` percentage resolves against the + * archive's content container rather than against ``. */ public static function insert_popups_after_header() { /* Posts and pages are covered by the_content hook */ @@ -587,21 +604,22 @@ function ( $popup ) { ); foreach ( self::sort_overlays_by_specificity( array_values( $popups ) ) as $popup ) { self::queue_overlay( $popup ); + echo self::emit_position_marker_inline( $popup ); // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped } } /** * Queue an overlay popup for rendering at wp_footer. * - * Overlays are deduped by popup ID, so the same overlay queued from multiple - * injection points (singular content + above-header, archive header, etc.) - * only renders once. Queue order is preserved, so the existing specificity - * sort done by callers still controls which overlay wins the single visible - * slot. + * Overlays are deduped by popup ID, so the same overlay reached from + * multiple injection points (singular content + above-header, archive + * header, etc.) only renders once. Insertion order is preserved – callers + * that care about specificity (the most-specific overlay winning the + * single visible slot) must queue most-specific first. * - * @param array $popup Popup data as returned by Newspack_Popups_Model. + * @param array $popup Popup data as returned by Newspack_Popups_Model. */ - private static function queue_overlay( $popup ) { + private static function queue_overlay( array $popup ): void { if ( empty( $popup['id'] ) ) { return; } @@ -612,24 +630,44 @@ private static function queue_overlay( $popup ) { } /** - * Print all queued overlay popups as direct children of , via wp_footer. + * Return the inline page-position marker markup for a popup, deduped by ID + * across multiple emission paths so a single popup never produces duplicate + * marker DOM nodes (which would break `document.getElementById` lookups in + * the front-end reveal JS). Returns the empty string for non-scroll-triggered + * popups and for popups whose marker has already been emitted this request. * - * This decouples overlay output from any ancestor stacking context that the - * popup would otherwise inherit when emitted inside post content or above - * the site header — for example a transformed/scaled wrapper, a sticky ad - * container, or any element with isolation:isolate. Inline popups remain - * inline; only the overlay-typed placements are portaled. + * @param array $popup Popup data as returned by Newspack_Popups_Model. + * @return string Marker HTML, or '' when no marker should be emitted. + */ + private static function emit_position_marker_inline( array $popup ): string { + if ( empty( $popup['id'] ) || isset( self::$emitted_markers[ $popup['id'] ] ) ) { + return ''; + } + $marker = Newspack_Popups_Model::generate_position_marker( $popup ); + if ( '' === $marker ) { + return ''; + } + self::$emitted_markers[ $popup['id'] ] = true; + // Wrap in a wp:html block so any downstream block-parser pass leaves + // the raw HTML untouched. + return '' . $marker . ''; + } + + /** + * Flush queued overlay popups via wp_footer. With no surrounding container + * in the callback, the markup lands as a direct child of `` – escaping + * any ancestor stacking context (a transformed/scaled wrapper, a sticky ad + * container, an element with `isolation: isolate`, etc.) that would + * otherwise trap the lightbox's z-index below sibling content. Inline + * popups remain inline; only the overlay-typed placements are portaled. + * The scroll-trigger page-position marker stays inline at the content + * position (see {@see emit_position_marker_inline()}). */ - public static function print_queued_overlays() { + public static function print_queued_overlays(): void { if ( empty( self::$queued_overlays ) ) { return; } foreach ( self::$queued_overlays as $popup ) { - // Suppress the scroll-trigger page-position marker: for scroll-triggered - // overlays it was emitted inline at the content position so its - // percentage offset still resolves against `.entry-content`. For time- - // triggered overlays there is no marker to begin with. Emitting the - // marker here (at body level) would break scroll-trigger detection. echo Newspack_Popups_Model::generate_popup( $popup, false ); // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped } self::$queued_overlays = []; @@ -647,15 +685,19 @@ private static function get_before_header_markup() { return ''; } - // Sort only the overlay subset by specificity — above-header inline prompts are + // Sort only the overlay subset by specificity – above-header inline prompts are // not subject to the single visible overlay slot constraint and are left in their - // original order. Overlays are queued for footer rendering (escaping nested - // stacking contexts); only the inline prompts are emitted inline here. + // original order. Overlay lightboxes are queued for footer rendering (escaping + // nested stacking contexts); the scroll-trigger marker is prepended inline so + // its `top` percentage still resolves against the header/post container. Inline + // (non-overlay) above-header prompts continue to be emitted inline here. $overlay_popups = self::sort_overlays_by_specificity( array_values( array_filter( $before_header_popups, [ 'Newspack_Popups_Model', 'is_overlay' ] ) ) ); + $markers = ''; foreach ( $overlay_popups as $popup ) { self::queue_overlay( $popup ); + $markers .= self::emit_position_marker_inline( $popup ); } $inline_popups = array_values( @@ -667,7 +709,7 @@ function( $popup ) { ) ); - $markup = ''; + $markup = $markers; foreach ( $inline_popups as $popup ) { $markup .= Newspack_Popups_Model::generate_popup( $popup ); } diff --git a/includes/class-newspack-popups-model.php b/includes/class-newspack-popups-model.php index 5146a6295..6b2819a90 100644 --- a/includes/class-newspack-popups-model.php +++ b/includes/class-newspack-popups-model.php @@ -1091,46 +1091,54 @@ public static function get_current_popup() { * The marker is a `position: absolute; top: X%` element whose position is * computed against its nearest `position: relative` ancestor (typically the * post's `.entry-content`). An IntersectionObserver on this marker is what - * reveals the scroll-triggered overlay. When the lightbox itself is portaled - * to wp_footer to escape an ancestor stacking context, the marker must remain - * inline inside the article container so its percentage offset still + * reveals the scroll-triggered overlay. The lightbox is portaled to + * wp_footer to escape ancestor stacking-context traps, but the marker + * stays inline at the article container so its percentage offset still * encodes "scroll progress through the article". * * Returns the empty string for popups that aren't scroll-triggered overlays. * - * @param array $popup The popup object. + * @param array $popup A fully-hydrated popup object as returned + * by {@see create_popup_object()}. * @return string Marker HTML, or '' when no marker is needed. */ - public static function generate_position_marker( $popup ) { - if ( ! self::is_overlay( $popup ) || 'scroll' !== $popup['options']['trigger_type'] ) { + public static function generate_position_marker( array $popup ): string { + if ( ! self::is_overlay( $popup ) ) { + return ''; + } + $trigger_type = $popup['options']['trigger_type'] ?? ''; + if ( 'scroll' !== $trigger_type ) { return ''; } $element_id = self::canonize_popup_id( $popup['id'] ); + $progress = absint( $popup['options']['trigger_scroll_progress'] ?? 0 ); return sprintf( - '
', + '
', esc_attr( $element_id ), - esc_attr( $popup['options']['trigger_scroll_progress'] ) + $progress ); } /** * Generate markup and styles for an overlay popup. * - * @param string $popup The popup object. - * @param bool $include_position_marker Whether to append the scroll-trigger - * page-position marker. Defaults to true - * for backward compatibility. Callers that - * portal the lightbox to a different DOM - * location (e.g. wp_footer) should pass - * false and emit the marker inline at the - * original content location separately via - * {@see generate_position_marker()}, since - * the marker positions itself as a percentage - * inside its `position: relative` ancestor - * (typically .entry-content). + * @param array $popup A fully-hydrated popup object. + * @param bool $include_position_marker When true (default), the + * returned markup includes the + * scroll-trigger page-position + * marker. Pass false when the + * marker is emitted separately + * at the content position via + * {@see generate_position_marker()}, + * so the marker's percentage `top` + * resolves against its + * `position: relative` ancestor + * (typically `.entry-content`) + * rather than against the portaled + * lightbox's footer position. * @return string The generated markup. */ - public static function generate_popup( $popup, $include_position_marker = true ) { + public static function generate_popup( $popup, bool $include_position_marker = true ) { $previewed_popup_id = Newspack_Popups::previewed_popup_id(); $is_manual_or_custom_placement = self::is_manual_only( $popup ) || Newspack_Popups_Custom_Placements::is_custom_placement_or_manual( $popup ); diff --git a/src/view/style.scss b/src/view/style.scss index 2b43a2c43..a2959671c 100644 --- a/src/view/style.scss +++ b/src/view/style.scss @@ -395,43 +395,42 @@ $width__tablet: 782px; } } -// Alignment -.entry-content { - /* stylelint-disable-next-line no-duplicate-selectors */ - .newspack-lightbox { - .newspack-popup__content-wrapper { - .alignfull, - .alignwide { - margin-left: 0; - margin-right: 0; - max-width: 100%; - padding-left: 0; - padding-right: 0; - - @media only screen and ( min-width: $width__mobile ) { - &.wp-block-columns { - margin-left: -16px; - margin-right: -16px; - max-width: calc(100% + 32px); - width: calc(100% + 32px); - } - } - - &.wp-block-columns, - &.wp-block-columns .wp-block-column { - padding-left: 0; - padding-right: 0; - } - } +// Alignment for wide/full blocks inside overlay prompt content. The lightbox +// renders as a direct child of (see Newspack_Popups_Inserter::print_queued_overlays), +// so these rules are no longer scoped to .entry-content. +.newspack-lightbox { + .newspack-popup__content-wrapper { + .alignfull, + .alignwide { + margin-left: 0; + margin-right: 0; + max-width: 100%; + padding-left: 0; + padding-right: 0; @media only screen and ( min-width: $width__mobile ) { - .alignfull.wp-block-columns { + &.wp-block-columns { margin-left: -16px; margin-right: -16px; max-width: calc(100% + 32px); width: calc(100% + 32px); } } + + &.wp-block-columns, + &.wp-block-columns .wp-block-column { + padding-left: 0; + padding-right: 0; + } + } + + @media only screen and ( min-width: $width__mobile ) { + .alignfull.wp-block-columns { + margin-left: -16px; + margin-right: -16px; + max-width: calc(100% + 32px); + width: calc(100% + 32px); + } } } } diff --git a/tests/test-overlay-queueing.php b/tests/test-overlay-queueing.php index 391965300..1aea6f172 100644 --- a/tests/test-overlay-queueing.php +++ b/tests/test-overlay-queueing.php @@ -105,36 +105,114 @@ public function test_queued_overlay_is_emitted_at_footer() { * Queueing the same overlay popup from multiple call paths (e.g. singular * content + above-header) must result in a single emission, deduped by ID. * - * Compared via flushed-output length: queueing N times must produce the same - * output length as queueing once. Length is robust against internal markup - * structure (the lightbox emits the "newspack-lightbox" substring in - * several child class names per popup). + * Asserted by counting the number of outer lightbox containers (`id="id_"`) + * for the popup in question, not by total output length – more direct, and + * stable against any future per-emission variability inside the markup. */ public function test_dedupe_by_id_across_multiple_queue_calls() { $overlay_popup = self::create_overlay_popup_object(); + $expected_id = 'id="' . Newspack_Popups_Model::canonize_popup_id( $overlay_popup['id'] ) . '"'; + // Queue the SAME popup multiple times via repeated calls. + Newspack_Popups_Inserter::insert_popups_in_post_content( '

Body.

', [ $overlay_popup ] ); + Newspack_Popups_Inserter::insert_popups_in_post_content( '

Body.

', [ $overlay_popup ] ); Newspack_Popups_Inserter::insert_popups_in_post_content( '

Body.

', [ $overlay_popup ] ); + ob_start(); Newspack_Popups_Inserter::print_queued_overlays(); - $single_queue_output = ob_get_clean(); + $footer_output = ob_get_clean(); + + self::assertSame( + 1, + substr_count( $footer_output, $expected_id ), + 'Queueing the same popup multiple times must still produce exactly one lightbox container.' + ); + } + + /** + * The dedupe map is shared across injection points: queueing the same + * overlay via `insert_popups_in_post_content` (singular content path) AND + * `insert_before_header_in_template_part` (block-theme above-header path) + * must still result in a single emission. + */ + public function test_dedupe_across_injection_points() { + $overlay_popup = self::create_overlay_popup_object( 'Cross-path overlay', 'time' ); + // Mark the popup as "above_page_header" so the block-theme path queues it. + Newspack_Popups_Model::set_popup_options( $overlay_popup['id'], [ 'trigger_type' => 'time' ] ); + $expected_id = 'id="' . Newspack_Popups_Model::canonize_popup_id( $overlay_popup['id'] ) . '"'; - // Queue the SAME popup multiple times via repeated calls. - Newspack_Popups_Inserter::insert_popups_in_post_content( '

Body.

', [ $overlay_popup ] ); - Newspack_Popups_Inserter::insert_popups_in_post_content( '

Body.

', [ $overlay_popup ] ); Newspack_Popups_Inserter::insert_popups_in_post_content( '

Body.

', [ $overlay_popup ] ); + + // Reach the queue from the block-theme above-header path too. The + // helper that backs both insert_before_header() and + // insert_before_header_in_template_part() filters popups via + // popups_for_post(), so simulate that by queueing directly: this test + // asserts the dedupe contract on queue_overlay() itself, not the + // per-callsite filtering. + $reflection_method = new ReflectionMethod( 'Newspack_Popups_Inserter', 'queue_overlay' ); + $reflection_method->setAccessible( true ); + $reflection_method->invoke( null, $overlay_popup ); + ob_start(); Newspack_Popups_Inserter::print_queued_overlays(); - $repeat_queue_output = ob_get_clean(); + $footer_output = ob_get_clean(); self::assertSame( - strlen( $single_queue_output ), - strlen( $repeat_queue_output ), - 'Queueing the same popup multiple times must produce the same output as queueing once.' + 1, + substr_count( $footer_output, $expected_id ), + 'A popup queued from two different injection points must emit exactly once.' ); - self::assertNotSame( - '', - $single_queue_output, - 'Sanity check: the flushed output should not be empty for a queued overlay.' + } + + /** + * The `insert_popups_after_header` archive path (classic Newspack theme) + * must still emit the scroll-trigger page-position marker inline – the + * IntersectionObserver reveal mechanism needs it to find a marker DOM node + * to observe. The lightbox itself is still queued for the footer flush. + */ + public function test_classic_archive_path_emits_marker_inline_for_scroll_triggered() { + $overlay_popup = self::create_overlay_popup_object( 'Archive scroll overlay', 'scroll' ); + + // Pretend we're rendering an archive page. + global $wp_query; + $prior_singular = $wp_query ? $wp_query->is_singular : false; + $wp_query->is_singular = false; + + // Force `popups_for_post()` to return our overlay rather than running + // the eligibility query, which depends on a fully bootstrapped request. + $reflection_class = new ReflectionClass( 'Newspack_Popups_Inserter' ); + $popups_property = $reflection_class->getProperty( 'popups' ); + $popups_property->setAccessible( true ); + $prior_popups = $popups_property->getValue(); + $popups_property->setValue( null, [ $overlay_popup ] ); + + ob_start(); + Newspack_Popups_Inserter::insert_popups_after_header(); + $inline_output = ob_get_clean(); + + // Restore globals. + $popups_property->setValue( null, $prior_popups ); + $wp_query->is_singular = $prior_singular; + + self::assertStringContainsString( + 'page-position-marker_', + $inline_output, + 'Classic-theme archive scroll-triggered overlays must emit the page-position marker inline.' + ); + self::assertStringNotContainsString( + 'newspack-lightbox', + $inline_output, + 'The lightbox markup must NOT be emitted inline from the archive path; it should be queued for the footer.' + ); + + ob_start(); + Newspack_Popups_Inserter::print_queued_overlays(); + $footer_output = ob_get_clean(); + + self::assertStringContainsString( + 'newspack-lightbox', + $footer_output, + 'The lightbox must be emitted at wp_footer for archive overlays.' ); } From 73516961225deef5ee3bb8e936eb6c829527d965 Mon Sep 17 00:00:00 2001 From: Adam Cassis Date: Tue, 19 May 2026 12:10:11 +0200 Subject: [PATCH 4/5] fix(popups): address Copilot review feedback on overlay portal hotfix - emit_position_marker_inline returns raw HTML; only the_content callsite wraps in (echo paths no longer leak block comments into the rendered page). - style.scss: drop duplicate `.alignfull.wp-block-columns` media block (the inner `&.wp-block-columns` rule under `.alignfull, .alignwide` already covers it). - test_dedupe_across_injection_points: drop the redundant set_popup_options call + clarify the comment to match what the test actually exercises. Co-Authored-By: Claude Opus 4.7 --- includes/class-newspack-popups-inserter.php | 19 ++++++++++------- src/view/style.scss | 9 -------- tests/test-overlay-queueing.php | 23 +++++++++------------ 3 files changed, 22 insertions(+), 29 deletions(-) diff --git a/includes/class-newspack-popups-inserter.php b/includes/class-newspack-popups-inserter.php index 316dec943..df125145f 100755 --- a/includes/class-newspack-popups-inserter.php +++ b/includes/class-newspack-popups-inserter.php @@ -493,10 +493,14 @@ function ( $block_groups, $block ) use ( &$block_index, $parsed_blocks, $max_ind // gates). Scroll-triggered overlays carry a page-position marker which // must remain inline in `.entry-content` – its percentage `top` resolves // against the article column and drives the IntersectionObserver that - // reveals the lightbox. + // reveals the lightbox. Wrap the marker in a wp:html block so any + // downstream block-parser pass over the_content output leaves it alone. foreach ( self::sort_overlays_by_specificity( $overlay_popups ) as $overlay_popup ) { self::queue_overlay( $overlay_popup ); - $output = self::emit_position_marker_inline( $overlay_popup ) . $output; + $marker = self::emit_position_marker_inline( $overlay_popup ); + if ( '' !== $marker ) { + $output = '' . $marker . '' . $output; + } } return $output; } @@ -633,11 +637,14 @@ private static function queue_overlay( array $popup ): void { * Return the inline page-position marker markup for a popup, deduped by ID * across multiple emission paths so a single popup never produces duplicate * marker DOM nodes (which would break `document.getElementById` lookups in - * the front-end reveal JS). Returns the empty string for non-scroll-triggered + * the front-end reveal JS). Returns raw marker HTML; callers that emit into + * a context where the result may be re-parsed by the block parser + * (`the_content` output) are responsible for wrapping it in a `wp:html` + * block themselves. Returns the empty string for non-scroll-triggered * popups and for popups whose marker has already been emitted this request. * * @param array $popup Popup data as returned by Newspack_Popups_Model. - * @return string Marker HTML, or '' when no marker should be emitted. + * @return string Raw marker HTML, or '' when no marker should be emitted. */ private static function emit_position_marker_inline( array $popup ): string { if ( empty( $popup['id'] ) || isset( self::$emitted_markers[ $popup['id'] ] ) ) { @@ -648,9 +655,7 @@ private static function emit_position_marker_inline( array $popup ): string { return ''; } self::$emitted_markers[ $popup['id'] ] = true; - // Wrap in a wp:html block so any downstream block-parser pass leaves - // the raw HTML untouched. - return '' . $marker . ''; + return $marker; } /** diff --git a/src/view/style.scss b/src/view/style.scss index a2959671c..8ffdadfd7 100644 --- a/src/view/style.scss +++ b/src/view/style.scss @@ -423,15 +423,6 @@ $width__tablet: 782px; padding-right: 0; } } - - @media only screen and ( min-width: $width__mobile ) { - .alignfull.wp-block-columns { - margin-left: -16px; - margin-right: -16px; - max-width: calc(100% + 32px); - width: calc(100% + 32px); - } - } } } diff --git a/tests/test-overlay-queueing.php b/tests/test-overlay-queueing.php index 1aea6f172..8d0748db6 100644 --- a/tests/test-overlay-queueing.php +++ b/tests/test-overlay-queueing.php @@ -130,25 +130,22 @@ public function test_dedupe_by_id_across_multiple_queue_calls() { } /** - * The dedupe map is shared across injection points: queueing the same - * overlay via `insert_popups_in_post_content` (singular content path) AND - * `insert_before_header_in_template_part` (block-theme above-header path) - * must still result in a single emission. + * The dedupe map is shared across injection points: a popup reached from + * `insert_popups_in_post_content` (singular content path) AND the helper + * that backs the classic / block-theme above-header path must still result + * in a single emission. The factory default trigger (`time`) is what would + * normally route a popup through the above-header path in production; the + * test here asserts the dedupe contract on `queue_overlay()` itself, not + * the per-callsite eligibility filtering. */ public function test_dedupe_across_injection_points() { $overlay_popup = self::create_overlay_popup_object( 'Cross-path overlay', 'time' ); - // Mark the popup as "above_page_header" so the block-theme path queues it. - Newspack_Popups_Model::set_popup_options( $overlay_popup['id'], [ 'trigger_type' => 'time' ] ); - $expected_id = 'id="' . Newspack_Popups_Model::canonize_popup_id( $overlay_popup['id'] ) . '"'; + $expected_id = 'id="' . Newspack_Popups_Model::canonize_popup_id( $overlay_popup['id'] ) . '"'; Newspack_Popups_Inserter::insert_popups_in_post_content( '

Body.

', [ $overlay_popup ] ); - // Reach the queue from the block-theme above-header path too. The - // helper that backs both insert_before_header() and - // insert_before_header_in_template_part() filters popups via - // popups_for_post(), so simulate that by queueing directly: this test - // asserts the dedupe contract on queue_overlay() itself, not the - // per-callsite filtering. + // Reach the queue from a second path by invoking the private queue + // helper directly. $reflection_method = new ReflectionMethod( 'Newspack_Popups_Inserter', 'queue_overlay' ); $reflection_method->setAccessible( true ); $reflection_method->invoke( null, $overlay_popup ); From 74892c9989f0a0d9cb0dcc5fd78a60f086098761 Mon Sep 17 00:00:00 2001 From: Adam Cassis Date: Mon, 1 Jun 2026 22:08:24 +0200 Subject: [PATCH 5/5] fix(popups): drain emitted-markers map alongside overlay queue on flush MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit print_queued_overlays() emptied the queued-overlay map after flushing but left the emitted-markers map populated. A subsequent re-queue of the same scroll-triggered overlay in the same request (e.g. a downstream apply_filters( 'the_content', ... ) after wp_footer fires) would re-emit the lightbox but the inline page-position marker would be silently suppressed, leaving segmentation.js with no #page-position-marker_* to observe – a scroll-triggered overlay that never reveals. Drain both maps together. Add regression coverage that fails without the marker drain. Also consolidate the inline marker emission in generate_popup() onto generate_position_marker() so there is one source of truth for the marker markup. --- includes/class-newspack-popups-inserter.php | 7 ++++ includes/class-newspack-popups-model.php | 8 +++-- tests/test-overlay-queueing.php | 37 +++++++++++++++++++++ 3 files changed, 49 insertions(+), 3 deletions(-) diff --git a/includes/class-newspack-popups-inserter.php b/includes/class-newspack-popups-inserter.php index df125145f..10b8ec2bc 100755 --- a/includes/class-newspack-popups-inserter.php +++ b/includes/class-newspack-popups-inserter.php @@ -675,7 +675,14 @@ public static function print_queued_overlays(): void { foreach ( self::$queued_overlays as $popup ) { echo Newspack_Popups_Model::generate_popup( $popup, false ); // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped } + // Drain both maps so a subsequent re-queue in the same request (e.g. a + // manual `apply_filters( 'the_content', ... )` after the footer flush) + // can re-emit the overlay *and* its inline scroll marker. Resetting the + // overlay queue alone would suppress the marker on the second pass, and + // `segmentation.js` would then have no `#page-position-marker_*` to + // observe – a scroll-triggered overlay that never reveals. self::$queued_overlays = []; + self::$emitted_markers = []; } /** diff --git a/includes/class-newspack-popups-model.php b/includes/class-newspack-popups-model.php index 6b2819a90..04a8afb18 100644 --- a/includes/class-newspack-popups-model.php +++ b/includes/class-newspack-popups-model.php @@ -1251,9 +1251,11 @@ class="" - -
- + \n

Body.

\n\n", + [ $overlay_popup ] + ); + self::assertStringContainsString( 'page-position-marker_', $first_content, 'First pass must emit the inline scroll-trigger marker.' ); + + ob_start(); + Newspack_Popups_Inserter::print_queued_overlays(); + ob_end_clean(); + + $second_content = Newspack_Popups_Inserter::insert_popups_in_post_content( + "\n

Body.

\n\n", + [ $overlay_popup ] + ); + self::assertStringContainsString( + 'page-position-marker_', + $second_content, + 'After a flush, re-queueing the same scroll-triggered overlay must re-emit the inline marker (else a scroll overlay silently fails to reveal).' + ); + + ob_start(); + Newspack_Popups_Inserter::print_queued_overlays(); + $second_footer = ob_get_clean(); + self::assertStringContainsString( 'newspack-lightbox', $second_footer, 'After a flush, the re-queued overlay must also re-emit its lightbox at the footer.' ); + } }