I think we should allow each component to be undefined and avoid overriding here. This is inline with similar use cases.

Done

As described above, let's do (number|undefined)

Added a new type for specify the changes that allows undefined.

/* opt_margins */ undefined

Done

I'm wondering if we allow undefined values, would it be enough for us to just read the values from element.style? Calling getComputedStyle here for all elements (most of which do not care about resizing) is pretty expensive. If not - I think updating margin is a rare enough case and we should move it closer to the size updating code and only do this when margin changes have been requested.

I don't think element.style is sufficient; it would ignore anything that comes from the CSS style rules.

I've changed it so that the getLayoutMargins method calls getComputedStyle and reads the margins only when it's called and have also changed mutate() in resources-impl.js to only call getLayoutMargins() when there is actually a newMargins defined in the request. Does that seem OK?

Although it occurs to me that calling getComputedStyle would probably have to be done somewhere inside a vsync_.measure call I guess?

Yes, it has to be in the vsync.measure. I think we need to change this. Basically, we only need to read margins in a relatively rare case when margin change has been requested: they are not needed before or after otherwise. So, what we can do, is remove layoutMargins_ var and code from Resource file and modify this code here: https://github.com/ampproject/amphtml/blob/master/src/service/resources-impl.js#L1334

It'd be something like this:

if (resource.hasBeenMeasured() && !opt_newMargings) {
  // unchanged
} else {
  vsync.measure(() => {
    if (!resource.hasBeenMeasured()) {
     resource.measure();
    }
    let marginsDiff = null;
    if (opt_newMargin) {
      marginsDiff = // calculate existing margins and diff with opt_newMargin
    }
    this.completeScheduleChangeSize_(..., marginsDiff);
  });
}

And then we'd use marginsDiff in the resize code itself. E.g. marginsDiff && marginsDiff.top would mean that top margin has been modified, etc.

I've done basically what you suggest. I've passed the existing and the new margins along, since we need the existing margins in mutateWork_() to know where the bottom displaced boundary is. So doesn't make much sense to pass a diff around just to turn it back into the original value later on.

Shouldn't this be box.bottom + bottomDisplacedBoundary?

I'm not sure what your suggestion is here?
The point in this is to record the boundary that will have a visible change as far as the user is concerned. If there is no margin change then it will only appear as though the bottom of the border-box has moved (which is box.bottom). If the bottom margin is also being resized then the spacing between this element and the next will also change.

So for example, if there is an element where box.bottom is <10% inside the top of the viewport and it has a bottom margin of 100px, then a pure height resize is fine (and would be allowed under the existing logic, as well as this new logic). If the bottom margin, however, is being resized then it's not fine, since this will make things within the main 80% of the viewport move around.

Ok, got it. Let's do margin changes and I'll review this one more time - it should be good to go.

For simplicity, shouldn't we just do diff = (newHeight + newMargin) - (oldHeight + oldMargin) and operate with that as before? Why I'm asking is that naively, margin changes are visually very similar to padding changes and paddings would be captured inside the height. So it'd seem that the diff math should be the same?

I don't think just doing what you suggest is correct for either the case of changing padding or margins.

It seems that the existing logic uses the fact that the top edge of an element will not change as a result of a change in height (none of the rules for setting resize=true/false ever check what box.top is) and for changes to the height where margins and padding are held constant this is valid.

As far as a user is concerned the top edge of an element is the edge of whatever they can see. For the case of an image without a border, then this will be the top of the content box, for the case of something with a border or a background color this will be the border box (which is equivalent to layout box in AMP).

So in all cases, changing the top margin is more akin to re-positioning the element than it is to resizing it. And for elements with no border or background changing the top padding would also be more akin to re-positioning than resizing.

So changes in margins need to be considered somewhat separately from changes in height if we want to avoid user visible re-flows in the middle of the viewpoint.

Should this be opt_newMargins.top != null? I assume 0 would be a valid value here, right?

Done

Sorry for the furniture moving, but could we move these to layout-rect.js?

Done

"Returns the previously measured value..." is not longer true.

Done

Why aren't we giving changeSize a margins, too?

Done

Shouldn't this include the topMarginDiff?

No. It's supposed to be the current position of the bottom boundary that will be displaced. This is used in the check that the bottom of the element is above the viewport (or within the 10% offset allowed). As long as the bottom displaced boundary meets this criteria then it doesn't matter what happens in terms of height changes of top margin changes, since the scroll adjusting will act such that nothing appears to change in the viewport except possibly in that allowed 10% region.