Revert "⏪ Revert "♻️ Support img element (#34028)"" #34630

kristoferbaxter · 2021-06-01T14:05:54Z

Reverts #34589

This reverts commit 3e8f1fc.

lgtm-com · 2021-06-01T14:32:42Z

This pull request introduces 1 alert when merging 80274dc into 63d8ca9 - view on LGTM.com

new alerts:

1 for Comparison between inconvertible types

lgtm-com · 2021-06-02T14:58:44Z

This pull request introduces 1 alert when merging be10828 into 36c4832 - view on LGTM.com

new alerts:

1 for Comparison between inconvertible types

lgtm-com · 2021-06-02T15:41:35Z

This pull request introduces 1 alert when merging 2e976b4 into 9d449c9 - view on LGTM.com

new alerts:

1 for Comparison between inconvertible types

lgtm-com · 2021-06-02T17:29:12Z

This pull request introduces 1 alert when merging d33e5b4 into b5d55ee - view on LGTM.com

new alerts:

1 for Comparison between inconvertible types

lgtm-com · 2021-06-04T15:00:04Z

This pull request introduces 1 alert when merging 1d1da19 into 1d3757a - view on LGTM.com

new alerts:

1 for Comparison between inconvertible types

amp-owners-bot · 2021-06-04T20:51:04Z

Hey @alanorozco! These files were changed:

extensions/amp-brid-player/0.1/amp-brid-player.js
extensions/amp-kaltura-player/0.1/amp-kaltura-player.js
extensions/amp-springboard-player/0.1/amp-springboard-player.js
extensions/amp-viqeo-player/0.1/amp-viqeo-player.js

Hey @gmajoulet, @newmuis! These files were changed:

extensions/amp-story/1.0/amp-story-page.js

Hey @jridgewell! These files were changed:

src/core/dom/propagate-attributes.js

kristoferbaxter · 2021-06-04T20:52:33Z

Ready for review, additional change for supporting img filters amp-img>img from most auto upgrade paths, allowing previous behaviour on amp-img to remain untouched.

extensions/amp-auto-lightbox/0.1/amp-auto-lightbox.js

rcebulko · 2021-06-07T20:19:56Z

test/unit/test-amp-img.js

@@ -406,7 +407,10 @@ describes.sandboxed('amp-img', {}, (env) => {
    el.getLayoutSize = () => ({width: 300, height: 200});

    const impl = new AmpImg(el);
-    const propagateAttributesSpy = sandbox.spy(impl, 'propagateAttributes');
+    const propagateAttributesSpy = sandbox.spy(


I know this is how the code was beforehand, but it seems like instead of testing that this helper is called we should directly test that the expected attributes were propagated.

That's a really good point. I'll switch this over.

Unfortunately this test ensures the order of attributes being propagated, and ensures src is last or double downloads can occur in Webkit.

I'll see if I can come up with a different method of verification.

Perhaps instead spy on the element itself a la something like env.spy(impl.element, 'setAttribute')?

That is better than the change I made (validated order of attributes to propagate).

…9-revert-img

lgtm-com · 2021-06-08T19:41:55Z

This pull request introduces 1 alert when merging e48ab85 into 3fe7dcb - view on LGTM.com

new alerts:

1 for Comparison between inconvertible types

…attributes helper

lgtm-com · 2021-06-08T20:09:09Z

This pull request introduces 1 alert when merging 1dcc3fc into 3fe7dcb - view on LGTM.com

new alerts:

1 for Comparison between inconvertible types

…9-revert-img

lgtm-com · 2021-06-09T15:46:43Z

This pull request introduces 1 alert when merging 115d045 into e734418 - view on LGTM.com

new alerts:

1 for Comparison between inconvertible types

lgtm-com · 2021-06-10T16:50:15Z

This pull request introduces 1 alert when merging 85a1676 into 2ee75c4 - view on LGTM.com

new alerts:

1 for Comparison between inconvertible types

lgtm-com · 2021-06-10T18:10:00Z

This pull request introduces 1 alert when merging 05d435f into 01e11e4 - view on LGTM.com

new alerts:

1 for Comparison between inconvertible types

extensions/amp-auto-lightbox/0.1/amp-auto-lightbox.js

lgtm-com · 2021-06-10T18:48:56Z

This pull request introduces 1 alert when merging 4eab890 into cd94152 - view on LGTM.com

new alerts:

1 for Comparison between inconvertible types

…9-revert-img

rcebulko · 2021-06-10T20:16:02Z

extensions/amp-lightbox-gallery/0.1/amp-lightbox-gallery.js

@@ -1250,6 +1263,10 @@ export class AmpLightboxGallery extends AMP.BaseElement {
  closeGallery_() {
    return this.mutateElement(() => {
      this.container_.removeAttribute('gallery-view');
+      Services.ownersForDoc(this.element)./*OK*/ scheduleUnlayout(


nit: This is done 4x in this class, make it a helper

Agreed, but would prefer in a different PR.

There are lots of examples of where this extension is breaking rules and I am fearful of making significant changes without:

Better tests

More consideration of what the implementation should own.

lgtm-com · 2021-06-10T20:32:08Z

This pull request introduces 1 alert when merging 598d63f into cd94152 - view on LGTM.com

new alerts:

1 for Comparison between inconvertible types

rcebulko · 2021-06-11T15:27:58Z

@kristoferbaxter Felt bad about constantly torpedoing this PR with my core PRs, so I resolved the conflicts introduced by #34818 since I figured it would be easier for me to do anyway

kristoferbaxter · 2021-06-11T15:29:40Z

Thanks @rcebulko! Appreciate it.

Also, no worries about the conflicts... happy to see the changes you're working on landing cleanly!

lgtm-com · 2021-06-11T15:54:38Z

This pull request introduces 1 alert when merging 0d0348b into 6efb0c0 - view on LGTM.com

new alerts:

1 for Comparison between inconvertible types

jridgewell · 2021-06-11T21:49:07Z

extensions/amp-lightbox-gallery/0.1/amp-lightbox-gallery.js

@@ -936,6 +938,13 @@ export class AmpLightboxGallery extends AMP.BaseElement {
    };

    const mutate = () => {
+      if (enter) {
+        Services.ownersForDoc(this.element)./*OK*/ scheduleUnlayout(


Can you comment why you're calling unlayout when you're entering? Shouldn't this have been called when we closed the lightbox?

Good call, this implementation doesn't appear to clean up after itself well.

lgtm-com · 2021-06-15T13:40:28Z

This pull request introduces 1 alert when merging 9057386 into bafe128 - view on LGTM.com

new alerts:

1 for Comparison between inconvertible types

test/unit/test-amp-img.js

lgtm-com · 2021-06-15T16:14:51Z

This pull request introduces 1 alert when merging d94dbe0 into 1f2308b - view on LGTM.com

new alerts:

1 for Comparison between inconvertible types

rcebulko · 2021-06-15T16:27:26Z

extensions/amp-auto-lightbox/0.1/amp-auto-lightbox.js

-          // remove it from DOM.
-          if (!candidate.signals().get(CommonSignals.LOAD_END)) {
+          if (
+            !candidate.tagName === 'IMG' &&


This looks like a bug

Suggested change

!candidate.tagName === 'IMG' &&

candidate.tagName !== 'IMG' &&

lgtm-com · 2021-06-15T21:59:23Z

This pull request introduces 1 alert when merging 184ccf3 into 7702194 - view on LGTM.com

new alerts:

1 for Comparison between inconvertible types

rcebulko · 2021-06-16T17:59:59Z

kristoferbaxter · 2021-06-17T01:26:38Z

Let's hope the night metrics don't have another fun discovery awaiting us!

Revert "Revert "♻️ Support img element (#34028)" (#34589)"

80274dc

This reverts commit 3e8f1fc.

kristoferbaxter added 2 commits June 2, 2021 07:15

Merge conflicts

9cec618

Support image viewer and lightbox

be10828

Fix amp-img>img detection

2e976b4

Checks

d33e5b4

Merge latest main

1d1da19

kristoferbaxter marked this pull request as ready for review June 4, 2021 20:50

kristoferbaxter requested a review from rcebulko June 4, 2021 20:51

rcebulko reviewed Jun 7, 2021

View reviewed changes

Merge branch 'main' of github.com:ampproject/amphtml into revert-3458…

e48ab85

…9-revert-img

switch to use the internal attributes instead of spying on propagate …

1dcc3fc

…attributes helper

kristoferbaxter added 2 commits June 9, 2021 07:44

Merge branch 'main' of github.com:ampproject/amphtml into revert-3458…

05cdaba

…9-revert-img

Add test for not selecting amp-img>img, but amp-img instead

115d045

Merge conflicts

85a1676

Path changes from merge

05d435f

rcebulko reviewed Jun 10, 2021

View reviewed changes

extensions/amp-auto-lightbox/0.1/amp-auto-lightbox.js Outdated Show resolved Hide resolved

Relative path instead of absolute

4eab890

kristoferbaxter added 2 commits June 10, 2021 12:53

Merge branch 'main' of github.com:ampproject/amphtml into revert-3458…

4da4cb8

…9-revert-img

Correct usage of closest by selector

598d63f

rcebulko reviewed Jun 10, 2021

View reviewed changes

kristoferbaxter requested a review from jridgewell June 11, 2021 00:08

Merge branch 'main' into revert-34589-revert-img

0d0348b

jridgewell approved these changes Jun 11, 2021

View reviewed changes

kristoferbaxter added 3 commits June 15, 2021 06:00

Merge conflicts

1e96f7b

Merge conflicts

7232a74

Accidental left over merge

9057386

rcebulko reviewed Jun 15, 2021

View reviewed changes

test/unit/test-amp-img.js Outdated Show resolved Hide resolved

More merge conflicts

d94dbe0

rcebulko suggested changes Jun 15, 2021

View reviewed changes

More merges

184ccf3

merge conflicts

426b53d

rcebulko approved these changes Jun 15, 2021

View reviewed changes

caroqliu mentioned this pull request Jun 16, 2021

Tracking Issue: Flaky (Bento?) unit tests #34885

Open

Linting fixes

57b1e2e

kristoferbaxter merged commit fa14e5c into main Jun 16, 2021

rsimha deleted the revert-34589-revert-img branch August 27, 2021 16:29

caroqliu mentioned this pull request Sep 23, 2021

🐛(amp-lightbox-gallery): opens to selected image, resolve #35920 #36103

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert "⏪ Revert "♻️ Support img element (#34028)"" #34630

Revert "⏪ Revert "♻️ Support img element (#34028)"" #34630

kristoferbaxter commented Jun 1, 2021

lgtm-com bot commented Jun 1, 2021

lgtm-com bot commented Jun 2, 2021

lgtm-com bot commented Jun 2, 2021

lgtm-com bot commented Jun 2, 2021

lgtm-com bot commented Jun 4, 2021

amp-owners-bot bot commented Jun 4, 2021 •

edited

Loading

kristoferbaxter commented Jun 4, 2021

rcebulko Jun 7, 2021

kristoferbaxter Jun 8, 2021

kristoferbaxter Jun 8, 2021

rcebulko Jun 8, 2021

kristoferbaxter Jun 8, 2021

lgtm-com bot commented Jun 8, 2021

lgtm-com bot commented Jun 8, 2021

lgtm-com bot commented Jun 9, 2021

lgtm-com bot commented Jun 10, 2021

lgtm-com bot commented Jun 10, 2021

lgtm-com bot commented Jun 10, 2021

rcebulko Jun 10, 2021

kristoferbaxter Jun 10, 2021

lgtm-com bot commented Jun 10, 2021

rcebulko commented Jun 11, 2021

kristoferbaxter commented Jun 11, 2021

lgtm-com bot commented Jun 11, 2021

jridgewell Jun 11, 2021

kristoferbaxter Jun 12, 2021

lgtm-com bot commented Jun 15, 2021

lgtm-com bot commented Jun 15, 2021

rcebulko Jun 15, 2021

lgtm-com bot commented Jun 15, 2021

rcebulko commented Jun 16, 2021

kristoferbaxter commented Jun 17, 2021

	!candidate.tagName === 'IMG' &&
	candidate.tagName !== 'IMG' &&

Revert "⏪ Revert "♻️ Support img element (#34028)"" #34630

Revert "⏪ Revert "♻️ Support img element (#34028)"" #34630

Conversation

kristoferbaxter commented Jun 1, 2021

lgtm-com bot commented Jun 1, 2021

lgtm-com bot commented Jun 2, 2021

lgtm-com bot commented Jun 2, 2021

lgtm-com bot commented Jun 2, 2021

lgtm-com bot commented Jun 4, 2021

amp-owners-bot bot commented Jun 4, 2021 • edited Loading

kristoferbaxter commented Jun 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Jun 8, 2021

lgtm-com bot commented Jun 8, 2021

lgtm-com bot commented Jun 9, 2021

lgtm-com bot commented Jun 10, 2021

lgtm-com bot commented Jun 10, 2021

lgtm-com bot commented Jun 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Jun 10, 2021

rcebulko commented Jun 11, 2021

kristoferbaxter commented Jun 11, 2021

lgtm-com bot commented Jun 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Jun 15, 2021

lgtm-com bot commented Jun 15, 2021

Choose a reason for hiding this comment

lgtm-com bot commented Jun 15, 2021

rcebulko commented Jun 16, 2021

kristoferbaxter commented Jun 17, 2021

amp-owners-bot bot commented Jun 4, 2021 •

edited

Loading