Skip to content
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

3.17 - regex to remove data-rocket-location-hash mismatch #6910

Closed
DahmaniAdame opened this issue Aug 26, 2024 · 3 comments · Fixed by #6915
Closed

3.17 - regex to remove data-rocket-location-hash mismatch #6910

DahmaniAdame opened this issue Aug 26, 2024 · 3 comments · Fixed by #6915
Assignees
Labels
effort: [S] 1-2 days of estimated development time module: ALR Issues related to the Automatic Lazy Rendering feature priority: high Issues which should be resolved as quickly as possible severity: major Feature is not working as expected and no work around available type: bug Indicates an unexpected problem or unintended behavior
Milestone

Comments

@DahmaniAdame
Copy link
Contributor

Before submitting an issue please check that you’ve completed the following steps:
Tested in 3.17 alpha 1

Describe the bug
The current regex to clear data-rocket-location-hash has mismatches and removes whole elements.

$result = preg_replace( '/data-rocket-location-hash="(?:.*)"/i', '', $html );

https://gist.github.com/DahmaniAdame/92f8f5aeb6ffeffda846585bea6c20fa

Changing the regex pattern to /data-rocket-location-hash="[^"]*"/i picked the right elements (to be further tested by QA):

https://gist.github.com/DahmaniAdame/8f2ce6da9d1ee052f2c1d155776645d9

Testing sample: https://gist.github.com/DahmaniAdame/2146eec70321898bf1e8f42cd2cdf0f6

To Reproduce
N/A

Expected behavior
data-rocket-location-hash should be dismissed accurately without causing any missing elements.

Screenshots
N/A

Additional context
N/A

Acceptance Criteria (for WP Media team use only)
Clear instructions for developers, to be added before the grooming

@MathieuLamiot MathieuLamiot added this to the 3.17 milestone Aug 26, 2024
@Mai-Saad Mai-Saad added type: bug Indicates an unexpected problem or unintended behavior priority: high Issues which should be resolved as quickly as possible severity: major Feature is not working as expected and no work around available module: ALR Issues related to the Automatic Lazy Rendering feature labels Aug 26, 2024
@Mai-Saad
Copy link
Contributor

Can reproduce the issue here for elements below/above the fold when having 3 or more nested elements https://newer.rocketlabsqa.ovh/lrc_similive/
Here

    <header> header in body totally atf
        <div class="container"> div in header in body <div> div in div in header in body
            <h1>Powerful Electrical LLC</h1> </div>
            <nav>
                <ul>
                    <li><a href="#about1">About Us</a></li>
                    <li><a href="#services1">Services</a></li>
                    <li><a href="#contact1">Contact</a></li>
                </ul>
            </nav>
        </div>
        end header in body
    </header>
    <div> div in body <div> another div in body <header> header in div in div in body<div>another div in header in div in div in body</div></header></div></div>

and here <div> div in body<section>section in div<footer>footer in section in div</footer></section></div>

@Khadreal
Copy link
Contributor

Reproduce the issue ✅

Yes

Identify the root cause ✅

$result = preg_replace( '/data-rocket-location-hash="(?:.*)"/i', '', $html );

The pattern here doesn't handle values on a same line well which is why it break some nested elements, rightly changing to this /data-rocket-location-hash="[^"]*"/i as suggested by @DahmaniAdame fix the problem.

Scope a solution ✅

Change regex pattern to /data-rocket-location-hash="[^"]*"/i and add test

Estimate the effort ✅

[S]

@Khadreal Khadreal added the effort: [S] 1-2 days of estimated development time label Aug 26, 2024
@jeawhanlee
Copy link
Contributor

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort: [S] 1-2 days of estimated development time module: ALR Issues related to the Automatic Lazy Rendering feature priority: high Issues which should be resolved as quickly as possible severity: major Feature is not working as expected and no work around available type: bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants