-
Notifications
You must be signed in to change notification settings - Fork 31
Closes #1066: Fix upgrade modal layout shift by animating geometry and correcting viewport unit #1183
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
Honemo
wants to merge
3
commits into
develop
Choose a base branch
from
fix/1066-upgrade-modal-layout-shift
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+151
β5
Open
Closes #1066: Fix upgrade modal layout shift by animating geometry and correcting viewport unit #1183
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,165 @@ | ||
| import { test, expect } from '@playwright/test'; | ||
| import { loginAsAdmin } from '../fixtures/auth'; | ||
| import { screenshotElement } from '../fixtures/screenshot'; | ||
|
|
||
| /** | ||
| * Upgrade modal β layout shift / misalignment across the payment flow (#1066). | ||
| * | ||
| * The upgrade modal (`#imagify-pricing-modal` / `.imagify-payment-modal`) hosts three | ||
| * sequential view-states inside a single `.imagify-modal-content` box: | ||
| * 1. plan-selection (default) | ||
| * 2. payment iframe (`.imagify-iframe-viewing` modifier) | ||
| * 3. thank-you (`.imagify-success-viewing` modifier) | ||
| * | ||
| * `switchToView()` in `assets/js/pricing-modal.js` toggles those modifier classes to | ||
| * resize the shared box. Since the real payment flow cannot be driven end-to-end without | ||
| * a live Stripe/PayPal session, this spec simulates the step transitions by toggling the | ||
| * same modifier classes the controller uses, then asserts: | ||
| * - a CSS `transition` is declared on the geometry properties that change between steps | ||
| * (this is the actual fix β Option A from the spec β verified via computed style), | ||
| * - the box stays horizontally centered (no unexpected leftward/rightward "shift"), | ||
| * - no unexpected horizontal scrollbar appears on the modal at any step. | ||
| * | ||
| * Related issue: #1066 | ||
| */ | ||
| test.describe( 'Upgrade modal β layout shift across payment flow steps (#1066)', () => { | ||
| test.beforeEach( async ( { page } ) => { | ||
| // The modal (Imagify_Views::print_modal_payment()) and its upsell trigger | ||
| // (views/part-upsell.php) only render for an account in a specific quota/plan | ||
| // state (free plan with quota remaining, or any non-infinite plan near its | ||
| // quota limit) β real account data fetched from the live Imagify API. Like the | ||
| // other API-dependent specs in this suite (account-connection.spec.ts, | ||
| // bulk-optimization.spec.ts), skip rather than fail when no real key is | ||
| // configured, since we cannot fabricate that account state locally. | ||
| test.skip( ! process.env.IMAGIFY_TESTS_API_KEY, 'IMAGIFY_TESTS_API_KEY not set β cannot reach an account/quota state that renders the upgrade modal' ); | ||
|
|
||
| await loginAsAdmin( page ); | ||
| await page.goto( '/wp-admin/options-general.php?page=imagify' ); | ||
| await page.waitForLoadState( 'networkidle' ); | ||
|
|
||
| // The configured account's live plan/quota state (e.g. an "infinite" plan) may not | ||
| // satisfy Imagify_Views::get_user_info(), in which case the modal never prints at | ||
| // all β this is real, mutable production account state, not something this suite | ||
| // can seed. Skip (do not hard-fail) when the trigger genuinely cannot appear, same | ||
| // as the other account-state-dependent specs in this suite. | ||
| const triggerCount = await page.locator( '.imagify-modal-trigger[data-target="#imagify-pricing-modal"]' ).count(); | ||
| test.skip( triggerCount === 0, 'Configured account\'s plan/quota state does not render the upgrade modal trigger (see Imagify_Views::get_user_info()) β needs a free-plan test account with quota remaining.' ); | ||
| } ); | ||
|
|
||
| async function openPricingModal( page: import( '@playwright/test' ).Page ) { | ||
| const trigger = page.locator( '.imagify-modal-trigger[data-target="#imagify-pricing-modal"]' ).first(); | ||
| await trigger.click(); | ||
|
|
||
| const modalContent = page.locator( '#imagify-pricing-modal .imagify-modal-content' ); | ||
| await expect( modalContent ).toBeVisible( { timeout: 10000 } ); | ||
|
|
||
| return modalContent; | ||
| } | ||
|
|
||
| /** | ||
| * Assert the modal box is (approximately) horizontally centered in the viewport, and | ||
| * that no unexpected horizontal scrollbar has appeared. | ||
| */ | ||
| async function assertNoHorizontalShiftOrScrollbar( page: import( '@playwright/test' ).Page, modalContent: import( '@playwright/test' ).Locator ) { | ||
| const viewportSize = page.viewportSize(); | ||
| expect( viewportSize ).not.toBeNull(); | ||
|
|
||
| const box = await modalContent.boundingBox(); | ||
| expect( box ).not.toBeNull(); | ||
|
|
||
| const boxCenterX = box!.x + box!.width / 2; | ||
| const viewportCenterX = viewportSize!.width / 2; | ||
|
|
||
| // Allow a small tolerance for scrollbar width / sub-pixel rounding. | ||
| expect( | ||
| Math.abs( boxCenterX - viewportCenterX ), | ||
| `Expected .imagify-modal-content to stay horizontally centered (box center: ${ boxCenterX }, viewport center: ${ viewportCenterX })` | ||
| ).toBeLessThanOrEqual( 5 ); | ||
|
|
||
| const hasHorizontalScrollbar = await page.evaluate( () => { | ||
| return document.documentElement.scrollWidth > document.documentElement.clientWidth + 1; | ||
| } ); | ||
|
|
||
| expect( | ||
| hasHorizontalScrollbar, | ||
| 'Expected no unexpected horizontal scrollbar on the document while the modal is open.' | ||
| ).toBe( false ); | ||
| } | ||
|
|
||
| test( 'plan-selection step is centered with no horizontal scrollbar', async ( { page } ) => { | ||
| const modalContent = await openPricingModal( page ); | ||
|
|
||
| await assertNoHorizontalShiftOrScrollbar( page, modalContent ); | ||
| await screenshotElement( page, 'pricing-modal-step-1-plan-selection', modalContent ); | ||
| } ); | ||
|
|
||
| test( 'payment-iframe step is centered, transitions smoothly, and has no horizontal scrollbar', async ( { page } ) => { | ||
| const modalContent = await openPricingModal( page ); | ||
|
|
||
| // Simulate switchToView() moving to the payment-iframe step by toggling the same | ||
| // modifier class the controller applies (the real flow requires a live payment | ||
| // session, which is not reachable in this environment). | ||
| await modalContent.evaluate( ( el ) => el.classList.add( 'imagify-iframe-viewing' ) ); | ||
|
|
||
| // The resize is animated via a CSS transition (the fix for #1066) rather than | ||
| // instantaneous, so wait for it to settle before asserting the final geometry. | ||
| await page.waitForTimeout( 400 ); | ||
|
|
||
| const transitionProperty = await modalContent.evaluate( | ||
| ( el ) => window.getComputedStyle( el ).transitionProperty | ||
| ); | ||
|
|
||
| expect( | ||
| transitionProperty, | ||
| `Expected .imagify-modal-content to declare a CSS transition on its geometry properties, got transition-property: "${ transitionProperty }"` | ||
| ).not.toBe( 'all' ); | ||
| expect( transitionProperty ).not.toBe( 'none' ); | ||
|
|
||
| await assertNoHorizontalShiftOrScrollbar( page, modalContent ); | ||
| await screenshotElement( page, 'pricing-modal-step-2-payment-iframe', modalContent ); | ||
| } ); | ||
|
|
||
| test( 'thank-you step is centered, transitions smoothly, and has no horizontal scrollbar', async ( { page } ) => { | ||
| const modalContent = await openPricingModal( page ); | ||
|
|
||
| await modalContent.evaluate( ( el ) => el.classList.add( 'imagify-iframe-viewing' ) ); | ||
| await page.waitForTimeout( 400 ); | ||
|
|
||
| // Simulate switchToView() moving from the payment-iframe step to the thank-you step, | ||
| // mirroring the mutually-exclusive class toggle in `assets/js/pricing-modal.js`. | ||
| await modalContent.evaluate( ( el ) => { | ||
| el.classList.remove( 'imagify-iframe-viewing' ); | ||
| el.classList.add( 'imagify-success-viewing' ); | ||
| } ); | ||
| await page.waitForTimeout( 400 ); | ||
|
|
||
| await assertNoHorizontalShiftOrScrollbar( page, modalContent ); | ||
| await screenshotElement( page, 'pricing-modal-step-3-thank-you', modalContent ); | ||
| } ); | ||
|
|
||
| test( 'modal opened directly on a non-default step does not animate from a default state on first paint', async ( { page } ) => { | ||
| // Edge case from the spec: first paint should not "grow" from a default state when | ||
| // the modal is opened directly on the payment or success step (e.g. deep link / | ||
| // resumed session). A CSS `transition` never animates the very first computed value | ||
| // on load (no prior value to transition from), so we assert the box is already at | ||
| // its target size immediately after open, with no residual animation in progress. | ||
| const modalContent = await openPricingModal( page ); | ||
|
|
||
| await modalContent.evaluate( ( el ) => el.classList.add( 'imagify-success-viewing' ) ); | ||
|
|
||
| const widthImmediatelyAfterToggle = await modalContent.evaluate( | ||
| ( el ) => el.getBoundingClientRect().width | ||
| ); | ||
|
|
||
| await page.waitForTimeout( 400 ); | ||
|
|
||
| const widthAfterSettling = await modalContent.evaluate( | ||
| ( el ) => el.getBoundingClientRect().width | ||
| ); | ||
|
|
||
| // Both measurements should reflect the success-viewing width (450px) within a small | ||
| // tolerance β i.e. no drawn-out grow animation lingering well past the transition | ||
| // duration declared in the CSS (.3s). | ||
| expect( Math.abs( widthAfterSettling - widthImmediatelyAfterToggle ) ).toBeLessThanOrEqual( 5 ); | ||
| } ); | ||
| } ); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Medium β This modifies the shared, generic
openModal()utility inadmin.js, which is used by every modal in the plugin (visual comparison modal, media-frame comparison modal, settings-info modals, etc.), not just the pricing modal. The spec's Architectural Decision explicitly states this fix should be CSS-only ("no JS or PHP changes are required") specifically to avoid touching jQuery-based modal code, and frames the JS-vs-CSS tradeoff as a decision for the manager, not something to decide unilaterally. Feature-specific logic (the.imagify-modal-no-transitionclass toggle, referencingpricing-modal.cssin a comment) is now baked into a generic, multi-consumer function.\n\nFix: Either (a) flag this deviation explicitly for product/architecture sign-off before merging (it is documented in the PR description, but the spec itself was written to avoid exactly this outcome), or (b) scope the workaround so it doesn't run for every modal β e.g. gate it on$link.data('target') === '#imagify-pricing-modal'or a similar targeted check, so unrelated modals aren't paying the cost of a lookup/class-toggle they never use.