✨ Auto-detect images to lightbox under experiment #20393
Conversation
alanorozco
force-pushed
the
amp-lightbox-gallery-detection
branch
2 times, most recently
from
902c98c
to
a056f4f
Compare
alanorozco
force-pushed
the
amp-lightbox-gallery-detection
branch
from
a056f4f
to
307452b
Compare
alanorozco
force-pushed
the
amp-lightbox-gallery-detection
branch
from
23f0231
to
c5b8cf4
Compare
|
@rsimha for bundle size approval. This gates |
|
/to @jridgewell or @choumx for integration bundle size check |
builtins/amp-img.js
Outdated
| @@ -50,6 +52,15 @@ export class AmpImg extends BaseElement { | |||
|
|
|||
| /** @private {?UnlistenDef} */ | |||
| this.unlistenError_ = null; | |||
|
|
|||
| /** @private {function()} */ | |||
| this.detectIfLightboxable_ = once(() => { | |||
There was a problem hiding this comment.
This may makes sense to be outside if amp-img since it does not belong to a single image instance (although once handles that). Also we want a good to exclude this in different runtimes.
I think something like
installAutoLightbox() in runtime.js and then calling it from amp.js (and not from amp-shadow.js, amp-inabox) is better.
There was a problem hiding this comment.
(Ideally installAutoLightbox gets called in a chuck created with very low-priority inside amp.js)
There was a problem hiding this comment.
(we can check to see if there is a single amp-img however before install it)
builtins/amp-img.js
Outdated
|
|
||
| /** @private {function()} */ | ||
| this.detectIfLightboxable_ = once(() => { | ||
| if (!isExperimentOn(this.win, 'amp-lightbox-gallery-detection')) { |
There was a problem hiding this comment.
let's stick with amp-auto-lightbox as everywhere else.
| export const VIEWPORT_AREA_RATIO = 0.4; | ||
|
|
||
| const ACTIONABLE_ANCESTORS = 'a[href], amp-selector, amp-script, amp-story'; | ||
| const TAP_ACTION_REGEX = /(;|\s|^)tap\:/; |
There was a problem hiding this comment.
actionService has a hasAction(element, eventName) that can tell you this. not sure which is faster though.
There was a problem hiding this comment.
It's pretty similar performance since they both traverse up the tree, but using hasAction is more readable, changed.
| * @return {boolean} | ||
| */ | ||
| static meetsAll(element) { | ||
| return Criteria.meetsPlaceholderCriteria(element) && |
There was a problem hiding this comment.
the order here sort of matters for performance, it should be a combination of (cost * likeliness to exclude the image) so we can optimally short circuit. Honestly I have no idea what order is the best. (Imagine if you could tell JS that these checks are side-effect free and V8 would optimize the order for you!)
There was a problem hiding this comment.
We can roll that ourselves, basically by making each criteria async and failing as soon as one of them fails. For now I think this is a premature optimization.
| /** Factor of renderArea vs viewportArea to lightbox. */ | ||
| export const VIEWPORT_AREA_RATIO = 0.4; | ||
|
|
||
| const ACTIONABLE_ANCESTORS = 'a[href], amp-selector, amp-script, amp-story'; |
There was a problem hiding this comment.
amp-selector is fine being inside a amp-selector > [option] match isn't.
| */ | ||
| static getAllImages(doc) { | ||
| const imgs = toArray(doc.querySelectorAll('amp-img')) | ||
| .filter(img => !img.hasAttribute(LIGHTBOXABLE_ATTR)); |
There was a problem hiding this comment.
I thought the idea was if lightbox is present even on a single one, we opt out completely. So maybe move this to another high-level rule similar to the sizing and placeholder?
There was a problem hiding this comment.
That check is already in place in isEnabledForDoc.
|
|
||
| const promises = imgs.map(whenLoadedOrNull); | ||
|
|
||
| return Promise.all(promises).then(imagesOrNull => |
There was a problem hiding this comment.
i don't think this would work. IF we wait for all images to load first, the ones below 3 viewport won't be loaded and we don't lightbox anything. It would actually be amazing to just do the lightboxable check as images finish loading, provided a decent spacing and works on demand. (one problem is lightbox gallery wound't pick up late addition, but there can be a simple API to tell it to add a new image to its list). For now, I guess just don't wait for load event and add a TODO.
| } | ||
|
|
||
| iterateCursor(lightboxable, element => { | ||
| element.setAttribute(LIGHTBOXABLE_ATTR, ''); |
There was a problem hiding this comment.
you wanna generate a unique "group" key here as set it as the value of lightbox=<group> otherwise they all end up in the "default" group.
alanorozco
force-pushed
the
amp-lightbox-gallery-detection
branch
4 times, most recently
from
e729016
to
4cae170
Compare
alanorozco
force-pushed
the
amp-lightbox-gallery-detection
branch
from
4cae170
to
447f155
Compare
src/amp.js
Outdated
| @@ -121,6 +122,7 @@ if (shouldMainBootstrapRun) { | |||
|
|
|||
| maybeValidate(self); | |||
| makeBodyVisible(self.document); | |||
| detectAutoLightbox(ampdoc); | |||
There was a problem hiding this comment.
This should happen in your extension's initialization process, not here.
There was a problem hiding this comment.
There needs to be a path to the extension from the runtime, this function only checks for existence of amp-img and then includes amp-auto-lightbox. This is essentially a runtime feature.
There was a problem hiding this comment.
I agree, this shouldn't be happening here. If the publisher wants auto-lightbox treatment, they should be including amp-auto-lightbox.js.
There was a problem hiding this comment.
This is supposed to be backported to documents that do not include the extension. /cc @aghassemi
For internal details see go/amp-auto-lightbox. Public I2I TBD.
There was a problem hiding this comment.
@choumx @jridgewell: the whole feature is lightboxing images without publisher's involvements (because they don't do it and end user suffers as the result). We will be running experiments and monitoring perf metrics as this progresses. Initially we will be starting with a limited environment to enable this feature (likely just AMP pages in SERP viewer that are of type Article). This PR is purely to put it behind an experiment flag so we can enable manually and start looking at pages to refine the heuristics.
There was a problem hiding this comment.
(I also assume tree shaking is smart enough not to include all of auto-lightbox.js here in v0. detectAutoLightbox simply checks for couple of conditions and then dynamically loads another extension that would take care of the rest of heuristics)
There was a problem hiding this comment.
Have we considered fetching the extension binary at runtime for experiment-enabled users? Or injecting the extension script via cache transform?
There was a problem hiding this comment.
Have we considered fetching the extension binary at runtime for experiment-enabled users?
That's exactly what detectLightbox does unless I'm missing something.
Or injecting the extension script via cache transform?
That would be a nice improvement over this, I don't see how they're mutually exclusive?
src/auto-lightbox.js
Outdated
| return; | ||
| } | ||
| chunk(ampdoc, () => { | ||
| if (!ampdoc.getRootNode().querySelector('amp-img')) { |
There was a problem hiding this comment.
Every page has an amp-img.
There was a problem hiding this comment.
True, might be an unnecessary optimization. Removed.
src/amp.js
Outdated
| @@ -121,6 +122,7 @@ if (shouldMainBootstrapRun) { | |||
|
|
|||
| maybeValidate(self); | |||
| makeBodyVisible(self.document); | |||
| detectAutoLightbox(ampdoc); | |||
There was a problem hiding this comment.
I agree, this shouldn't be happening here. If the publisher wants auto-lightbox treatment, they should be including amp-auto-lightbox.js.
| */ | ||
| static getAllImages(doc) { | ||
| const imgs = toArray(doc.querySelectorAll('amp-img')).filter(img => | ||
| !img.hasAttribute(LIGHTBOXABLE_ATTR) && |
There was a problem hiding this comment.
Add these as part of the query selector.
| !img.hasAttribute(VISITED_ATTR)); | ||
|
|
||
| imgs.forEach(img => { | ||
| img.setAttribute(VISITED_ATTR, ''); |
There was a problem hiding this comment.
VISITED_ATTR is a public attribute (data-amp-auto-lightbox-visited), so it must be done in a mutate phase.
| * @return {!Promise<?Element>} | ||
| */ | ||
| function whenLoadedOrNull(el) { | ||
| return new Promise(resolve => { |
There was a problem hiding this comment.
| return new Promise(resolve => { | |
| return el.signals.whenSignal(CommonSignals.LOAD_END).then(() => el, () => null) |
| } | ||
|
|
||
| const maybeApplyAll = () => Scanner.getAllImages(ampdoc.win.document) | ||
| .map(imgPromise => imgPromise.then(imgOrNull => { |
There was a problem hiding this comment.
map here is useless, the return map is filled with all undefined.
There was a problem hiding this comment.
The signal is used in tests. For the execution path it's not necessary to go through the result of the promise, but it's useful to yield to all the promises returned here in tests.
| const scriptTags = ampdoc.getHeadNode().querySelectorAll('script'); | ||
|
|
||
| const currentScriptTag = find(scriptTags, | ||
| t => t.getAttribute('custom-element') == REQUIRED_EXTENSION); |
There was a problem hiding this comment.
This should be handled by src/service/extensions-impl.js
There was a problem hiding this comment.
Yeah, not sure why it's not provided as a public API. I don't know what the best way to expose this may be, since there is some extension holder logic I'm not particularly familiar with.
| export const VIEWPORT_AREA_RATIO = 0.3; | ||
|
|
||
| const ACTIONABLE_ANCESTORS = | ||
| 'a[href], amp-selector > [option], amp-script, amp-story'; |
There was a problem hiding this comment.
option does not need to be direct child of amp-selector, so amp-selector [option] (my bad from previous comment)
| /** Factor of renderArea vs viewportArea to lightbox. */ | ||
| export const VIEWPORT_AREA_RATIO = 0.3; | ||
|
|
||
| const ACTIONABLE_ANCESTORS = |
There was a problem hiding this comment.
another fun one to add button ( <button type="submit" value="OK"> <amp-img src="https://via.placeholder.com/150" width=20 height=20> </button> )
There was a problem hiding this comment.
Done. Test added as well.
| .then( | ||
| () => | ||
| Mutation.mutate(img, () => { | ||
| img.setAttribute(VISITED_ATTR, ''); |
There was a problem hiding this comment.
when can we endup scanning the same tree twice? (worth adding a comment) also for the name of VISITED_ATTR, let's do i-amphtml- instead of data- as prefix.
| function usesLightboxExplicitly(ampdoc) { | ||
| const scriptTags = ampdoc.getHeadNode().querySelectorAll('script'); | ||
|
|
||
| const currentScriptTag = find(scriptTags, |
There was a problem hiding this comment.
why not querySelectorAll(`script [custom-element=${REQUIRED_EXTENSION}]`) ?;
| */ | ||
| export function isEnabledForDoc(ampdoc) { | ||
| const {win} = ampdoc; | ||
| if (!Services.viewerForDoc(ampdoc).isEmbedded() && |
There was a problem hiding this comment.
&& viewer.isTrustedViewer() (will limit to SERP)
| } | ||
| chunk(ampdoc, () => { | ||
| Services.extensionsFor(win) | ||
| .installExtensionForDoc(ampdoc, 'amp-auto-lightbox'); |
There was a problem hiding this comment.
Would be nice to define to a constant, but isExperimentOn only takes literals per lint rules.
src/auto-lightbox.js
Outdated
| /** | ||
| * @param {!./service/ampdoc-impl.AmpDoc} ampdoc | ||
| */ | ||
| export function detectAutoLightbox(ampdoc) { |
There was a problem hiding this comment.
Nit: installAutoLightboxExtension
Partial for ampproject#20395. Introduces `amp-auto-lightbox` experiment and extension for field testing.
Partial for #20395.
Introduces
amp-auto-lightboxexperiment and extension for field testing.