Introduce a new attemptCollapse function by zhouyx · Pull Request #7532 · ampproject/amphtml

zhouyx · 2017-02-14T03:00:36Z

when we try to collapse an element we do things like:

element.attemptChangeHeight(0).then(() => {
  element.collapse();
})

We do some measurements after height is set to 0, and before we call element.collapse(); However with display:inline-block, it is still possible for layout to change from height = 0 to display: none. [Looks like a well know CSS issue] Then the previous measure will give wrong result. https://github.com/ampproject/amphtml/blob/master/src/service/resources-impl.js#L1036

In this PR I am trying to introduce a new method call attemptCollapse, and it will call resources scheduleChangeSize_ with a callback function to set display none.

Still need to work on adding test coverage.

jridgewell · 2017-02-14T18:15:38Z

extensions/amp-fx-flying-carpet/0.1/amp-fx-flying-carpet.js

-        return this.attemptChangeHeight(0).then(() => {
-          this./*OK*/collapse();
-        }, () => {});
+        return this.attemptCollapse().then(() => {}, () => {});


Just need a catch block.

jridgewell · 2017-02-14T18:16:01Z

src/base-element.js

+   * @public
+   */
+  attemptCollapse() {
+    return this.element.getResources().attemptCollapse(


Why do we need to pass width and height?

Fail to cleanup, will delete

jridgewell · 2017-02-14T18:16:13Z

src/service/resource.js

    this.element.updateLayoutBox(box);
  }
+  /**
+   * Collapse on changing height/width to 0


My fault. Will clean up

jridgewell · 2017-02-14T18:18:38Z

src/service/resources-impl.js

+   * @return {!Promise}
+   */
+  attemptCollapse(element) {
+    return new Promise((resolve, reject) => {


Let's just call #attemptChangeSize:

attemptCollapse(element) { this.attemptChangeSize(element, 0, undefined).then(() => { const resource = Resource.forElement(element); resource.completeCollapse(); }); }

That's the thing that I want to fix in this PR.

#attemptChangeSize success, resources manager does some calculation to adjust scrollTop, we then immediately collapse the element the previous measurement gives incorrect value which results in a page jump.

jridgewell · 2017-02-14T18:51:09Z

We need a really good test that will fail if anyone ever does #7532 (comment).

lannka · 2017-02-14T19:03:11Z

ads/_ping_.js

    }
  } else {
-    global.context.noContentAvailable();
+    window.setTimeout(() => {


does this work: global.setTimeout(global.context.noContentAvailable, 1000) ?

yup, I changed to using global

zhouyx · 2017-02-14T19:30:40Z

@jridgewell You have any ideas on the test?
The best I can think of is a presubmit check of calling #attemptChangeHeight(0)

jridgewell · 2017-02-14T19:36:12Z

I don't understand where the measure is coming from that is incorrect. But you could stub that, and make sure the element is display: none.

zhouyx · 2017-02-14T19:43:31Z

@jridgewell that won't help me from preventing anyone from calling.

attemptChangeHeight(0).then(() => {
  collapse()
});

As a call like this is still valid. And they would both trigger measurement.

attemptChangeHeight(0).then(() => {
  // do something that don't affect change layout
});

jridgewell · 2017-02-14T19:53:27Z

Sorry, I missed the second sentence in your comment. dev.assert() that newWidth and newHeight do not equal 0.

But, I was talking about prevent someone from doing the refactor I suggested. We need a test to make sure that doesn't happen.

dvoytenko · 2017-02-14T20:22:43Z

src/base-element.js

+
+  /**
+   * Return a promise that request the runtime to collapse one element
+   * @return {!Promise}


zhouyx · 2017-02-15T02:24:46Z

@jridgewell Finally add some test coverage. Ugly test though...
BTW I add a presubmit check to warn on calling of attemptChangeHeight(0) I think when people try to change height to 0, they are most likely trying to do the same thing as attemptCollapse().

zhouyx · 2017-02-15T22:06:40Z

👀

jridgewell · 2017-02-15T22:17:12Z

build-system/tasks/presubmit-checks.js

  // Functions
  '\\.changeHeight\\(': bannedTermsHelpString,
  '\\.changeSize\\(': bannedTermsHelpString,
+  '\\.attemptChangeHeight\\(0\\)': 'please consider using `attemptCollapse()`',


Why not use a dev().assert? We could catch both height = 0 and width = 0 changes then.

That's good point. But it is possible that an iframe request leads runtime to call attemptChangeSize(newWidth=0, newHeight=0). It is perfectly valid without setting element display to none afterwards.
The thing I want avoid is us explicitly put 0 as the input. dev().assert is too strict, maybe I can switch to dev().warn. honestly i feel presubmit check should be sufficient.

Or, let's forward to #attemptCollapse when we receive 0?

That's what I was planning to do at first, just add a check inside #attemptChangeSize. Then we realize once an iframe send us request to resize to (0, 0), they will never be able to resize again because we don't remove the display:none for them.

👍. Let's leave as is.

jridgewell · 2017-02-15T22:27:52Z

test/functional/test-resources.js

    });
  });

+  it('attemptCollapse should not call attemptChangeSize', () => {


We're targeting the scrollAdjSet path, right?

If so, we should be able to avoid the getScrollHeight stub, and just assert that resource1#completeCollapse has ben called before resource1#getLayoutBox.

but #completeCollapse gets called after #getLayoutBox.
https://github.com/ampproject/amphtml/blob/master/src/service/resources-impl.js#L1029
Or should I reorder the call, as I feel the order doesn't matter here. It makes more sense to call #changeSize first and then measure layout box.

Ah, I confused the sync and async paths. I think re-ordering the calls might be best.

will go with re-ordering calls

Sorry for going back and forth. But few things I found:

request.callback() can do something to affect element layoutBox. For example we are setting resource.isFixed to false when we call #completeCollapse in the callback. This won't be an issue for us because we will also manually update element layoutBox in #completeCollapse call.

So I can still go with re-ordering calls, and assert #completeCollapse gets called before #getLayoutBox. However just like what I do with #getScrollHeight #getLayoutBox gets called 2 times in the test path. here and here. I fell I will still need to stub #getLayoutBox to assert on the second call. which makes no difference between my current test approach.

One other thing that's not related to this PR:
if we resize an fixed element, is it likely that it won't affect other elements layout on the page. Maybe we can add some optimization by not updating minTop here if element is position fixed.

* still need tests * need to fix test * fix lint * nit * add test coverage

zhouyx added 2 commits February 13, 2017 17:43

still need tests

537f69d

need to fix test

2deac94

zhouyx assigned jridgewell, dvoytenko and lannka Feb 14, 2017

zhouyx requested review from dvoytenko, jridgewell and lannka February 14, 2017 03:00

zhouyx force-pushed the auto-collapse branch from 75ce93e to ea13475 Compare February 14, 2017 17:58

fix lint

ea13475

jridgewell requested changes Feb 14, 2017

View reviewed changes

nit

9855a63

lannka approved these changes Feb 14, 2017

View reviewed changes

dvoytenko approved these changes Feb 14, 2017

View reviewed changes

add test coverage

9ad80a9

jridgewell reviewed Feb 15, 2017

View reviewed changes

jridgewell approved these changes Feb 15, 2017

View reviewed changes

zhouyx merged commit 3e02f1c into ampproject:master Feb 16, 2017

zhouyx mentioned this pull request Feb 21, 2017

Weekly Status 2017-02-20 #7679

Closed

mrjoro pushed a commit to mrjoro/amphtml that referenced this pull request Apr 28, 2017

Introduce a new attemptCollapse function (ampproject#7532)

446f368

* still need tests * need to fix test * fix lint * nit * add test coverage

Conversation

zhouyx commented Feb 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhouyx Feb 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jridgewell commented Feb 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhouyx commented Feb 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jridgewell commented Feb 14, 2017

Uh oh!

zhouyx commented Feb 14, 2017

Uh oh!

jridgewell commented Feb 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhouyx commented Feb 15, 2017

Uh oh!

zhouyx commented Feb 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

zhouyx Feb 14, 2017 •

edited

Loading

zhouyx commented Feb 14, 2017 •

edited

Loading