[WIP] Visibility trigger for non-amp elements #26995

micajuine-ho · 2020-02-26T22:43:41Z

To be reviewed/merged after #26823's changes. Guarded with the same feature flag

For #20607

Within the IntersectionObserverPolyfill, if the element is a non-amp element (no getLayoutBox, no owner), asynchronously use getBoundingClientRect (offset by the hostViewport) to find the element's rect.

Besides affecting VisibilityManager, this change also impacts RefreshManager (for a4a). Since it manages a4a, we're guaranteed to always get an amp-element so this should not change behavior for it.

jridgewell · 2020-03-02T19:54:13Z

extensions/amp-analytics/0.1/analytics-root.js

+
+      // DOM search can "look" outside the boundaries of the root, thus make
+      // sure the result is contained. Length is not supported in all browsers
+      if (foundElements && foundElements.length) {


This is not necessary, querySelector is already guaranteed to contain everything that is returned.

What you're probably thinking of is that querySelector('div img') can match on <div><root><img/></root></div>, but this contains check isn't going to fix that. Use scopedQuerySelector from src/dom.js

src/layout-rect.js

mrjoro · 2020-06-12T17:37:47Z

Is this PR still active?

micajuine-ho · 2020-06-12T18:09:55Z

~~Yes, we are waiting for Intersection Observer experiment to be out for a while, then we will proceed with this PR.~~

Edit: Will pick up this work after TCFv2 changes, (end of June)

dvoytenko · 2020-06-12T22:37:53Z

Just an update: intersection observer experiment is fully launched. Just cleanup is remaining. It can be now used throughout.

amp-owners-bot · 2020-07-07T18:05:07Z

Hey @rsimha! These files were changed:

.vscode/settings.json
build-system/compile/closure-compile.js
build-system/compile/compile.js
build-system/pr-check/OWNERS
build-system/pr-check/build-targets.js
build-system/pr-check/build.js
build-system/pr-check/checks.js
build-system/pr-check/cross-browser-tests.js
build-system/pr-check/dist-bundle-size.js
build-system/pr-check/dist-tests.js
build-system/pr-check/e2e-tests.js
build-system/pr-check/experiment-tests.js
+18 more

Hey @erwinmombay! These files were changed:

build-system/babel-plugins/babel-plugin-amp-mode-transformer/index.js
build-system/babel-plugins/babel-plugin-amp-mode-transformer/test/fixtures/transform-assertions/no-transform/input.js
build-system/babel-plugins/babel-plugin-amp-mode-transformer/test/fixtures/transform-assertions/no-transform/options.json
build-system/babel-plugins/babel-plugin-amp-mode-transformer/test/fixtures/transform-assertions/no-transform/output.js
build-system/babel-plugins/babel-plugin-amp-mode-transformer/test/fixtures/transform-assertions/nomodule/input.js
build-system/babel-plugins/babel-plugin-amp-mode-transformer/test/fixtures/transform-assertions/nomodule/options.json
build-system/babel-plugins/babel-plugin-amp-mode-transformer/test/fixtures/transform-assertions/nomodule/output.mjs
build-system/babel-plugins/babel-plugin-amp-mode-transformer/test/fixtures/transform-assertions/transform/input.js
build-system/babel-plugins/babel-plugin-amp-mode-transformer/test/fixtures/transform-assertions/transform/options.json
build-system/babel-plugins/babel-plugin-amp-mode-transformer/test/fixtures/transform-assertions/transform/output.mjs
build-system/babel-plugins/babel-plugin-const-transformer/index.js
build-system/babel-plugins/babel-plugin-const-transformer/test/fixtures/transform-assertions/ignored-let/options.json
+295 more

Hey @jridgewell! These files were changed:

build-system/babel-plugins/babel-plugin-amp-mode-transformer/index.js
build-system/babel-plugins/babel-plugin-amp-mode-transformer/test/fixtures/transform-assertions/no-transform/input.js
build-system/babel-plugins/babel-plugin-amp-mode-transformer/test/fixtures/transform-assertions/no-transform/options.json
build-system/babel-plugins/babel-plugin-amp-mode-transformer/test/fixtures/transform-assertions/no-transform/output.js
build-system/babel-plugins/babel-plugin-amp-mode-transformer/test/fixtures/transform-assertions/nomodule/input.js
build-system/babel-plugins/babel-plugin-amp-mode-transformer/test/fixtures/transform-assertions/nomodule/options.json
build-system/babel-plugins/babel-plugin-amp-mode-transformer/test/fixtures/transform-assertions/nomodule/output.mjs
build-system/babel-plugins/babel-plugin-amp-mode-transformer/test/fixtures/transform-assertions/transform/input.js
build-system/babel-plugins/babel-plugin-amp-mode-transformer/test/fixtures/transform-assertions/transform/options.json
build-system/babel-plugins/babel-plugin-amp-mode-transformer/test/fixtures/transform-assertions/transform/output.mjs
build-system/babel-plugins/babel-plugin-const-transformer/index.js
build-system/babel-plugins/babel-plugin-const-transformer/test/fixtures/transform-assertions/ignored-let/options.json
+290 more

Hey @danielrozenberg! These files were changed:

build-system/compile/internal-version.js
build-system/global-configs/experiments-config.json
build-system/sauce_connect/OWNERS
build-system/sauce_connect/start_sauce_connect.sh
build-system/sauce_connect/stop_sauce_connect.sh
build-system/tasks/visual-diff/.percy.yaml
build-system/tasks/visual-diff/helpers.js
build-system/tasks/visual-diff/index.js
build-system/tasks/visual-diff/package.json
build-system/tasks/visual-diff/yarn.lock

Hey @alanorozco! These files were changed:

build-system/server/app-index/amphtml-helpers.js
build-system/server/app-index/api/api.js
build-system/server/app-index/file-list.js
build-system/server/app-index/form.js
build-system/server/app-index/html.js
build-system/server/app-index/template.js
build-system/server/app-index/url.js
build-system/server/app-index/util/listing.js
extensions/amp-3q-player/0.1/amp-3q-player.js
extensions/amp-3q-player/0.1/test/test-amp-3q-player.js
extensions/amp-3q-player/amp-3q-player.md
extensions/amp-brid-player/0.1/amp-brid-player.js
+39 more

Hey @estherkim! These files were changed:

build-system/tasks/e2e/README.md
build-system/tasks/e2e/amp-driver.js
build-system/tasks/e2e/controller-promise.js
build-system/tasks/e2e/describes-e2e.js
build-system/tasks/e2e/driver/query-xpath.js
build-system/tasks/e2e/expect.js
build-system/tasks/e2e/functional-test-controller.js
build-system/tasks/e2e/index.js
build-system/tasks/e2e/network-logger.js
build-system/tasks/e2e/package.json
build-system/tasks/e2e/puppeteer-controller.js
build-system/tasks/e2e/repl.js
+2 more

Hey @wassgha! These files were changed:

build-system/tasks/storybook/OWNERS
build-system/tasks/storybook/amp-env/decorator.js
build-system/tasks/storybook/amp-env/main.js
build-system/tasks/storybook/amp-env/register.js
build-system/tasks/storybook/amp-env/webpack.config.js
build-system/tasks/storybook/index.js
build-system/tasks/storybook/package.json
build-system/tasks/storybook/preact-env/main.js
build-system/tasks/storybook/preact-env/webpack.config.js
build-system/tasks/storybook/yarn.lock
extensions/amp-3q-player/0.1/amp-3q-player.js
extensions/amp-3q-player/0.1/test/test-amp-3q-player.js
+42 more

Hey @Jiaming-X! These files were changed:

extensions/amp-ad-network-adsense-impl/0.1/adsense-shared-state.js
extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js
extensions/amp-ad-network-adsense-impl/0.1/responsive-state.js
extensions/amp-ad-network-adsense-impl/0.1/test/test-amp-ad-network-adsense-impl.js
extensions/amp-ad-network-adsense-impl/0.1/test/test-responsive-state.js
extensions/amp-ad-network-adsense-impl/OWNERS

Hey @jeffkaufman! These files were changed:

extensions/amp-ad-network-adsense-impl/0.1/adsense-shared-state.js
extensions/amp-ad-network-adsense-impl/0.1/amp-ad-network-adsense-impl.js
extensions/amp-ad-network-adsense-impl/0.1/responsive-state.js
extensions/amp-ad-network-adsense-impl/0.1/test/test-amp-ad-network-adsense-impl.js
extensions/amp-ad-network-adsense-impl/0.1/test/test-responsive-state.js
extensions/amp-ad-network-adsense-impl/OWNERS
extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js
extensions/amp-ad-network-doubleclick-impl/0.1/flexible-ad-slot-utils.js
extensions/amp-ad-network-doubleclick-impl/0.1/safeframe-host.js
extensions/amp-ad-network-doubleclick-impl/0.1/sra-utils.js
extensions/amp-ad-network-doubleclick-impl/0.1/test/test-amp-ad-network-doubleclick-impl.js
extensions/amp-ad-network-doubleclick-impl/0.1/test/test-doubleclick-fluid.js
+8 more

Hey @gmajoulet! These files were changed:

extensions/amp-story-360/0.1/amp-story-360.css
extensions/amp-story-360/0.1/amp-story-360.js
extensions/amp-story-360/0.1/test/test-amp-story-360.js
extensions/amp-story-360/OWNERS
extensions/amp-story-education/0.1/amp-story-education.css
extensions/amp-story-education/0.1/amp-story-education.js
extensions/amp-story-education/0.1/test/test-amp-story-education.js
extensions/amp-story-education/OWNERS
extensions/amp-story-education/amp-story-education.md
extensions/amp-story-education/validator-amp-story-education.protoascii
extensions/amp-story/0.1/_locales/default.js
extensions/amp-story/0.1/_locales/en.js
+242 more

Hey @processprocess, @t0mg! These files were changed:

extensions/amp-story-360/0.1/amp-story-360.css
extensions/amp-story-360/0.1/amp-story-360.js
extensions/amp-story-360/0.1/test/test-amp-story-360.js
extensions/amp-story-360/OWNERS

Hey @newmuis! These files were changed:

extensions/amp-story/0.1/_locales/default.js
extensions/amp-story/0.1/_locales/en.js
extensions/amp-story/0.1/amp-story-base-layer.js
extensions/amp-story/0.1/amp-story-bookend.css
extensions/amp-story/0.1/amp-story-bookend.js
extensions/amp-story/0.1/amp-story-consent.css
extensions/amp-story/0.1/amp-story-consent.js
extensions/amp-story/0.1/amp-story-cta-layer.js
extensions/amp-story/0.1/amp-story-desktop-user-overridable.css
extensions/amp-story/0.1/amp-story-desktop.css
extensions/amp-story/0.1/amp-story-grid-layer.js
extensions/amp-story/0.1/amp-story-hint.css
+232 more

Hey @Enriqe! These files were changed:

extensions/amp-story/0.1/_locales/default.js
extensions/amp-story/0.1/_locales/en.js
extensions/amp-story/0.1/amp-story-base-layer.js
extensions/amp-story/0.1/amp-story-bookend.css
extensions/amp-story/0.1/amp-story-bookend.js
extensions/amp-story/0.1/amp-story-consent.css
extensions/amp-story/0.1/amp-story-consent.js
extensions/amp-story/0.1/amp-story-cta-layer.js
extensions/amp-story/0.1/amp-story-desktop-user-overridable.css
extensions/amp-story/0.1/amp-story-desktop.css
extensions/amp-story/0.1/amp-story-grid-layer.js
extensions/amp-story/0.1/amp-story-hint.css
+239 more

Hey @ampproject/wg-caching! These files were changed:

validator/AUTHORS
validator/BUILD
validator/README.md
validator/WORKSPACE
validator/build.py
validator/engine/amp4ads-parse-css.js
validator/engine/amp4ads-parse-css_test.js
validator/engine/htmlparser-interface.js
validator/engine/htmlparser.js
validator/engine/htmlparser_test.js
validator/engine/json-testutil.js
validator/engine/keyframes-parse-css.js
+235 more

Micajuine Ho added 4 commits February 19, 2020 11:03

Impl, unit test, & manual test

db28aad

Impl, unit tests, manual tests

ec0e4ae

Restrict data-vars, impl, unit, manual test

f2e6a66

Initial commits + tests

b9241d5

micajuine-ho requested a review from zhouyx February 26, 2020 22:43

amp-owners-bot bot requested a review from dvoytenko February 26, 2020 22:43

googlebot added the cla: yes label Feb 26, 2020

micajuine-ho removed the request for review from dvoytenko February 26, 2020 22:44

micajuine-ho force-pushed the visibility_non_amp branch from a7c7339 to 11b0bd8 Compare February 26, 2020 22:58

jridgewell reviewed Mar 2, 2020

View reviewed changes

Merge branch 'restrict_to_data_vars' into visibility_non_amp

75493a7

micajuine-ho force-pushed the visibility_non_amp branch from 11b0bd8 to 75493a7 Compare March 2, 2020 21:03

Micajuine Ho added 2 commits July 7, 2020 11:02

merge

650e259

New

e051c7e

micajuine-ho changed the title ~~Visibility trigger for non-amp elements~~ [WIP] Visibility trigger for non-amp elements Jul 7, 2020

micajuine-ho closed this Jul 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Visibility trigger for non-amp elements #26995

[WIP] Visibility trigger for non-amp elements #26995

micajuine-ho commented Feb 26, 2020

jridgewell Mar 2, 2020

mrjoro commented Jun 12, 2020

micajuine-ho commented Jun 12, 2020 •

edited

Loading

dvoytenko commented Jun 12, 2020

amp-owners-bot bot commented Jul 7, 2020 •

edited by ampprojectbot

Loading

[WIP] Visibility trigger for non-amp elements #26995

[WIP] Visibility trigger for non-amp elements #26995

Conversation

micajuine-ho commented Feb 26, 2020

jridgewell Mar 2, 2020

Choose a reason for hiding this comment

mrjoro commented Jun 12, 2020

micajuine-ho commented Jun 12, 2020 • edited Loading

dvoytenko commented Jun 12, 2020

amp-owners-bot bot commented Jul 7, 2020 • edited by ampprojectbot Loading

micajuine-ho commented Jun 12, 2020 •

edited

Loading

amp-owners-bot bot commented Jul 7, 2020 •

edited by ampprojectbot

Loading