Amphtml: amp-list: auto-resize should only apply on first mutation

[choumx]

Not sure how I missed this but as-is auto-resize causes content jumping. We only should apply container layout on the invocation of mutatedAttributesCallback rather than render.

I hope nobody is using this in production yet.

[aghassemi]

Idea behind auto-resize is to be triggered the very first time amp-list is resized, whether because it was allowed to via attemptChangeHeight or whether user manually clicked the overflow button.

If it is causing content-jumping, that's definitely a bug to fix, but it should still be allowed outside of mutatedAttributesCallback.

[aghassemi]

I can repro content shifting even without auto-resize attribute: https://codepen.io/aghassemi/pen/NLQNMZ?editors=1000 this sample should be showing the "show more" button instead of resizing, but it is not.

Interestingly if I change the height from 30 to 20, it works correctly. There is something going on with attemptChangeHeight. ABE's overflow sample is also broken: https://ampbyexample.com/components/amp-list/#handling-list-overflow

[choumx]

Good call on clicking the overflow button, forgot about that.

The original logic was to allow resize the element is in the bottom 15% of the document. However, it wasn't working in some cases due to a bug (#14791).

So your example (and ABE) probably needs to put more content below the amp-list to trigger the overflow now that the bug is fixed.

[aghassemi]

@choumx Is there still a case you saw that auto-resize causes content shifting that's outside of the normal considerations?

[choumx]

Yes. https://github.com/ampproject/amphtml/issues/18376#issuecomment-424732871 was in response to your comment about behavior without auto-resize.

[aghassemi]

Sorry, I meant, is this bug still valid? Is there any issue you have seen caused by auto-resize attribute that causes content shifting outside of normal supported cases? Just wanna make sure we prioritise a fix if that's the case.

[choumx]

Yes. I modified your codepen to make this clear: https://codepen.io/choumx/pen/yxmdaa?editors=1000

• Expected: The amp-list does not resize on page load.
• Actual: The amp-list resizes on page load.

Removing auto-resize results in the correct behavior.

[aghassemi]

@choumx I still haven't been able to repro this. What I see is:

which looks correct

[aghassemi]

never mind, I can repro in Canary

@cathyxz let's treat as P0.5?

[aghassemi]

seems like changeToLayoutContainer_ is called regardless of whether attempChangeHeight succeeds. During initial render, it should only happen if attempChangeHeight does not fail.

[cathyxz]

Here's one way to deal with this problem:

1. Call changeToLayoutContainer iff attemptChangeHeight succeeds.
2. Add overflowCallback to BaseElement and override it for <amp-list> so that if auto-resize is on, it does a changeToLayoutContainer, and call this.implementation_.overflowCallback in custom-element.js in a similar way to what is done for all the other overridden callbacks. But I noted that overflowCallback is @package @final and therefore not intended to be overwritten. Is there a particular reason why?

EDIT: it should be added in base-element.js and has nothing to do with the @package @final in resources-impl.js.

[aghassemi]

Using Signals could be another approches. I suspect most cases we don't want to actually override overflow but rather do something after it is done making Signals a good option.

[cathyxz]

On second thought, to clarify that approach, what we want to override isn't actually overflowCallback, but rather the onclick function registered to the overflow element on showing the overflow element here:

https://github.com/ampproject/amphtml/blob/dca4483004b4d9cd3069e8fa67df433284587b73/src/custom-element.js#L1696-L1702

So we would add overflowClickedCallback to BaseElement:

 this.overflowElement_.onclick = () => {
  resources.mutateElement(this, () => {
    this.implementation_.overflowClickedCallback();
   });
};

And implement: BaseElement.js:overflowClickedCallback:

overflowClickedCallback() {
   const resources = this.element.getResources();
   resources./*OK*/changeSize(this, requestedHeight, requestedWidth);
   const overflowElement = this.getOverflowElement();
   overflowElement.classList.toggle('amp-visible', false);
   overflowElement.onclick = null;
}

And then in amp-list, we would override:

  overflowClickedCallback() {
    if (this.element.hasAttribute('auto-resize')) {
      this.applyAutoResize_();
    } else {
      super.overflowCallback();
    }
  }

For the Signals approach, would we add a new signal signifying that the overflow callback was called? That it succeeded? That attemptChangeSize was called/succeeded? There currently isn't a good signal in CommonSignals that would tell us anything about overflow being done.

[cathyxz]

Okay, another way to handle this is to add a 'overflow-clicked' signal, which we trigger in custom-element.js:overflowCallback, inside the onclick handler, also here:
https://github.com/ampproject/amphtml/blob/dca4483004b4d9cd3069e8fa67df433284587b73/src/custom-element.js#L1696-L1702

 this.overflowElement_.onclick = () => { 
   const resources = this.getResources(); 
   resources./*OK*/changeSize(this, requestedHeight, requestedWidth); 
   resources.mutateElement(this, () => { 
     this.overflowCallback( 
         /* overflown */ false, requestedHeight, requestedWidth); 
   }); 
   this.signals().signal('overflow-clicked');
};

And then within amp-list render, we can have

...signal().whenSignal('overflow-clicked').then(() => {
    if (this.element.hasAttribute('auto-resize')) {
        this.applyAutoResize_();
    }
});

[aghassemi]

Yep, I recommend the latter. Signals are nicer for this usecase than callbacks.

[cathyxz]

So I prototyped both approaches (https://github.com/ampproject/amphtml/pull/18543, https://github.com/ampproject/amphtml/pull/18542), and there's a slight problem. For the two test cases of changing the size of amp-list due to toggling accordion or toggling CSS classes, these two will not result in a call to attemptChangeSize, and will therefore not result in a call to overflowCallback and will therefore not trigger our logic to resize the container. It seems like we would then need some way to detect if the height of the contents of amp-list changed on accordion click, and on CSS class toggle affecting the amp-list.

[aghassemi]

That might be fine for now. If list has not resized yet (i.e. showing the overflow button) it is fine and maybe even expected to stay as is when something inside of it expands.

[cathyxz]

A third strategy proposed by @choumx is to use to listen to taps and add a MutationObserver to trigger resize when contents inside of amp-list mutate. This covers the accordion use case and tapping things inside amp-list that cause css class toggles. In order to cover tapping elements outside of <amp-list> that cause CSS class changes impacting height, expose a resize method that can be called on other actions (e.g. on="tap:amp-list-id.resize").

[cathyxz]

/cc @ericlindley-g

[cathyxz]

Summarising conversations with @aghassemi, @choumx and @ericlindley-g:

Here are two relatively non-controversial things that we can do:

1. Add a resize action.
2. Add a bindable solution to change to layout container.

This allows developers to manually dictate when they want a container resize to happen on some kind of user interaction. Enables good UX, but is non-ideal dev X.

These are options I discussed with @choumx and @aghassemi

Problem 1: A resize due to user interaction happens, what do we do?

Solution 1: Show the overflow element, change to container on click of said overflow element. (Note: this only handles increases in size, but not decreases in size).
Solution 2: Directly change to container.

Problem 2: How to detect a user-caused resize?

Solution 1: Listen to all taps inside the amp-list and assume that causes a resize.
Solution 2: Use a mutation observer and assume all detected mutations cause a resize.

Here's another IMO best if technically feasible option brought up by @ericlindley-g:

If no resize is necessary, directly change to layout container on load.

[ericlindley-g]

I'm in favor of getting amp-list into a container-like state as soon as possible, to simplify devX and eliminate one-off corner cases to mitigate with multiple features

So among the solutions described, if it's technically feasible, I'd opt for:

1. If on page load no resize is required (if either the content already fits) or the element is able to resize at load time (because it won't affect anything in the current viewport), change amp-list to layout container ASAP
2. If the overflow button is required (content doesn't fit initially and couldn't resize because it would have affected the current viewport when it would have resized), then make it so amp-list changes to layout container when the overflow button is tapped/clicked

[choumx]

Right. I think (1) is what Ali meant here and (2) would be covered by a resize action that's automatically triggered by overflow tap.

There's also some cases, let's call it (3), where we want resizing without tapping the overflow button:

<amp-list>
  <amp-accordion></amp-accordion>
</amp-list>

But certainly (1) and (2) can be implemented now without fuss.

[choumx]

(3) might be doable by simply tweaking our current focus-history.js behavior. #18772

[ericlindley-g]

Ah, I see — yeah, then sounds like we're on the same page for 1 & 2.

For (3), now I understand, and agree: more specifically, that tapping anything inside amp-list that causes a resize (amp-accordion for sure, and more debatably: a CSS checkbox hack that causes something to change height) should similarly trigger container layout (as if the overflow button were tapped as well).

[aghassemi]

I think (3) is a bit tricky. I don't think anything that changes height inside a not-yet-resized amp-list should resize the list and act as if overflow was tapped. Imagine a UI where we have:

<h1>Trending Items</h1>
<amp-list height=33vh>
   <button overflow>Show more trending items</button>
</amp-list>

<h1>New Items</h1>
<amp-list height=33vh>
   <button overflow>Show more new Items items</button>
</amp-list>

<h1>Sale Items</h1>
<amp-list height=33vh>
   <button overflow>Show more Sale items</button>
</amp-list>

Item template.

<tempalte>
<div item>
  <amp-img>
  <button> toggle item description</button>
  <p>Item Description</p>
</div>
</template>

In this design there are three lists each with 33% of view port height and are deliberately overflowing with an explicit "show more items" action. If we let interaction with each items expand the list, it may not make sense. In the example above, just clicking "show description" for an item in "Trending Items" would all of a sudden show all trending items and push the other 2 lists down the view.

My suggestion is:
If height of something changes inside a non-yet-resized amp-list:

• if overflow is not yet displayed, show it.
• if overflow is displayed, do nothing.

If author really wants some interaction inside the list to fully expand it, they can use the resize action.

[aghassemi]

That being said, I like the predictability of Eric's suggestion which also handles cases where authors were not thinking ahead to provide overflow action (which is common). Point is both solutions have some drawbacks. Best we can do is pick a default that we think handles the common case the best.

[cathyxz]

Here's a prototype of @ericlindley-g 's suggestion for part 1: https://github.com/ampproject/amphtml/pull/18814
Demo (minus autosuggest) here: https://amp-demo-project-1323a.firebaseapp.com/manual/amp-list-resize.amp.html.

The main difference here in the behavior, is we now change to layout container if attemptToFit was not called. I.e. scrollHeight is smaller than or equal to list height, which means a change to layout container will not actually result in a height change.

If we implement both (1) as linked above, and (2) via a resize action, then I actually think 3 becomes a bit of a minor edge case.

UPDATE: there's a slight issue with this approach (linked above) for solving part 1. It will still cause content jumping if the content height is smaller than the list height. So we can only safely change to layout container if the scrollHeight of the contents is equal to the list height.

[choumx]

Isn't predictability solved by gating under auto-resize attribute?

[choumx]

Tangent: maybe we should reconsider the naming of auto-resize since it seems to imply resizing on page load. There have already been a few confused parties about this feature.

[aghassemi]

yeah, agree. way more confusion than I expected (specially given it is not even documented yet). It's already in the validator though. we could soft deprecate (warning) and rename it while keeping auto-resize valid until we can fully deprecate it.

what would be a good name?
1- fluid
2- has-resizable-items
3- resize-when-items-resize
4- ???

most descriptive one is probably 3.

[choumx]

Since it's not documented I imagine the current usage is under the deprecation threshold. We may be able to get away with just removing it outright.

resizable-children? Let's bike-shed in design review.

[honeybadgerdontcare]

@choumx would you like me to test how many errors there may be on a large corpus with removing auto-resize.

[choumx]

@honeybadgerdontcare That would be great! 🙏

[aghassemi]

well, not until m.aliexpress.com removes usages.

[aghassemi]

(back to the main conversation)

We should keep in mind that simply using mutation observer and resizing the list if anything inside of it changes height will cause "resizing at load time" when components such as amp-twitter are used inside items since those components resize automatically at load time and change the height of the list.

We really need to detect "if height changed as result of user-action"

[choumx]

We really need to detect "if height changed as result of user-action"

To clarify, my original thought was to use both tap event and MutationObserver together to detect this:

• Tap event ensures that resize only happens on a user gesture.
• MutationObserver ensures that the DOM changed after the user gesture (not an accidental tap).

I also wonder how the focus behavior we saw in #18772 factors in. I.e. why doesn't resizing already work via the focus feature?

[leonalicious]

well, not until m.aliexpress.com removes usages.

we actually added this auto-resize feature in less than 24 hours, because of the upcoming double 11 festival, we will be having our code freeze in less than three days. Well, we think by including this option is just a way to play safe (AMP team can still release HotFix patch even after our code freeze has started). But after reading through the conversation, I think this option is unnecessary for us, we will need to have a full test without this option over the weekend with latest canary available, and get your guys updated on this

Tangent: maybe we should reconsider the naming of auto-resize since it seems to imply resizing on page load. There have already been a few confused parties about this feature.

Please let me guess, I think we know what this auto-resize is trying to solve, is it trying to provide a dynamic height on amp-list based on the size of the items within it ?

1. On our Search List Page, users can refine their search to only show list of items meeting certain criteria and this causes the amp-list to be resized every-time based on the size of items.
2. Our ShopCart List will resize the amp-list based on the items within it.

But we are achieving these effect with css since January, can we use auto-resize to achieve these effects in future ?

[leonalicious]

Tested with canary on, our pages are not working without the auto-resize option, but are fine with auto-resize option on, therefore, i think we will just keep the auto-resize option on. Appreciate everything to make this work, thank you as usual 🥇

/to: @aghassemi @choumx

[aghassemi]

Could you please tell us what exactly breaks when you remove auto-resize?

auto-resize should not really impact your site, I like to know how not having it breaks.

We are thinking about disabling the behavior of auto-resize until we make further fixes to it.

I recommend removing auto-resize, I am.somewhat surprised not having it breaks given you recently added that attribute and can't see what is relying on it's behavior.

[leonalicious]

the amp-list just somehow resize to the height of the placeholder I presume

[aghassemi]

After a bind reload I assume? Damn, okay. thanks for the feedback, I will note that.

[leonalicious]

wait a minute, i just tested with canary v1810191936340, seems like everything is working fine again without auto-resize, weird ~ just too weird ...

[aghassemi]

yeah that makes more sense actually. Please see if you can remove auto-resize for the list and cart pages. not having auto-resize makes things more stable since auto-resize is new and we are trying to fix bugs with it (and maybe even disable it temporarily #18849)

[leonalicious]

hmm, okay, I am agreed with you and we shall definitely work towards to go online without having auto-resize option. Okay, I will involve QA Team and we will run a full test on Monday, and hopefully deploy it into production on the same day if everything works as expected. And just FYI, the day after onwards, we are officially paralyzed with code freeze, IF there are anything suddenly turned unexpectedly, we are hoping the rectification should come from AMP Team. * Finger Crossed*

[aghassemi]

Sounds good, make sure testing is done on DevChannel since production does not have the placeholder resizing fix and it is only in Canary.

[leonalicious]

sorry for the late reply, as promised, we didn't implement any auto-resize option across our pages that were released 2 days ago. And just FYI, in order to make our pages work again (without being affected by this new auto-resize feature), we have added a dummy wrapper outside amp-list from prevent any height to be appended onto it.

For instance, we have added with class listed as below outside the amp-list to prevent the amp-list will be set to some random height during its initial rendered at https://m.ru.aliexpress.com/wishlist.html, a page where user can remove items from the list :

````css
.wish-wrap {
position: relative;
height: max-content;
}

````

Hope it helps.

[leonalicious]

@aghassemi may i know when will this be released ?

[aghassemi]

@leonalicious auto-resize is now turned off in Dev Channel and will be off on Prod Oct 30. Relaunch of this feature would be when we have solved all the issues. ~1-2 months I suspect.

[aghassemi]

@leonalicious actually Del Channel is a bit delayed, will be out soon today.

[cathyxz]

We can close this now that auto-resize has been rolled back and changeToLayoutContainer has been changed to an action and bindable attribute, gated under experiment amp-list-resizable-children.