Skip to content

Reland Display Click to Load placeholders on demand #296

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

Merged
merged 1 commit into from
Mar 6, 2023

Conversation

kzar
Copy link
Contributor

@kzar kzar commented Mar 6, 2023

These changes were landed and reverted last week, since some of the
integration tests started failing. But it turned out, that those failures
were just down to Puppeteer reporting blocked requests differently if
the corresponding elements are removed from the DOM very quickly.
See duckduckgo/duckduckgo-privacy-extension@8b5d389

The original commit message:

Historically, the Click to Load placeholders were only drawn once at
the point page load completed. That had two problems:

  1. Sometimes placeholders would take too long to display, if the page
    took a long time to load fully.
  2. Sometimes placeholders would not be displayed at all, if the
    blocked content was only created after page load had finished.

To fix that, let's display the placeholders when instructed by the
platform using a "displayClickToLoadPlaceholders" update message.

Let's also restructure the response messages, so that they can be
clearly distinguished.

These changes were landed and reverted last week, since some of the
integration tests started failing. But it turned out, that those failures
were just down to Puppeteer reporting blocked requests differently if
the corresponding elements are removed from the DOM very quickly.
See duckduckgo/duckduckgo-privacy-extension@8b5d389

The original commit message:

Historically, the Click to Load placeholders were only drawn once at
the point page load completed. That had two problems:
1. Sometimes placeholders would take too long to display, if the page
   took a long time to load fully.
2. Sometimes placeholders would not be displayed at all, if the
   blocked content was only created after page load had finished.

To fix that, let's display the placeholders when instructed by the
platform using a "displayClickToLoadPlaceholders" update message.

Let's also restructure the response messages, so that they can be
clearly distinguished.
@kzar kzar force-pushed the ctl-placeholder-on-block-reland branch from 36e6ada to daa0502 Compare March 6, 2023 17:04
Copy link
Contributor

@ladamski ladamski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me, though given the FB CTL tests seem to be timing out for me pretty much 100% of the time we should actually try to fix those soon (perhaps by switching to local hosting)

@kzar kzar merged commit a6acfa9 into duckduckgo:main Mar 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants