-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce a new attemptCollapse function #7532
Conversation
75ce93e
to
ea13475
Compare
return this.attemptChangeHeight(0).then(() => { | ||
this./*OK*/collapse(); | ||
}, () => {}); | ||
return this.attemptCollapse().then(() => {}, () => {}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just need a catch block.
src/base-element.js
Outdated
*/ | ||
attemptCollapse() { | ||
return this.element.getResources().attemptCollapse( | ||
this.element, 0, /* newWidth */ undefined); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to pass width
and height
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fail to cleanup, will delete
src/service/resource.js
Outdated
@@ -412,6 +413,10 @@ export class Resource { | |||
|
|||
this.element.updateLayoutBox(box); | |||
} | |||
/** | |||
* Collapse on changing height/width to 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My fault. Will clean up
* @return {!Promise} | ||
*/ | ||
attemptCollapse(element) { | ||
return new Promise((resolve, reject) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's just call #attemptChangeSize
:
attemptCollapse(element) {
this.attemptChangeSize(element, 0, undefined).then(() => {
const resource = Resource.forElement(element);
resource.completeCollapse();
});
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahhh. 👍
We need a really good test that will fail if anyone ever does #7532 (comment). |
ads/_ping_.js
Outdated
@@ -71,6 +71,8 @@ export function _ping_(global, data) { | |||
}); | |||
} | |||
} else { | |||
global.context.noContentAvailable(); | |||
window.setTimeout(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this work: global.setTimeout(global.context.noContentAvailable, 1000)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup, I changed to using global
@jridgewell You have any ideas on the test? |
I don't understand where the measure is coming from that is incorrect. But you could stub that, and make sure the element is |
@jridgewell that won't help me from preventing anyone from calling.
As a call like this is still valid. And they would both trigger measurement.
|
Sorry, I missed the second sentence in your comment. But, I was talking about prevent someone from doing the refactor I suggested. We need a test to make sure that doesn't happen. |
src/base-element.js
Outdated
/** | ||
* Return a promise that request the runtime to collapse one element | ||
* @return {!Promise} | ||
* @public |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deleted
@jridgewell Finally add some test coverage. Ugly test though... |
👀 |
@@ -566,6 +566,7 @@ var forbiddenTermsSrcInclusive = { | |||
// Functions | |||
'\\.changeHeight\\(': bannedTermsHelpString, | |||
'\\.changeSize\\(': bannedTermsHelpString, | |||
'\\.attemptChangeHeight\\(0\\)': 'please consider using `attemptCollapse()`', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use a dev().assert
? We could catch both height = 0
and width = 0
changes then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or, let's forward to #attemptCollapse
when we receive 0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍. Let's leave as is.
@@ -1824,6 +1833,67 @@ describe('Resources mutateElement and collapse', () => { | |||
}); | |||
}); | |||
|
|||
it('attemptCollapse should not call attemptChangeSize', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I confused the sync and async paths. I think re-ordering the calls might be best.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will go with re-ordering calls
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for going back and forth. But few things I found:
request.callback()
can do something to affect elementlayoutBox
. For example we are settingresource.isFixed
to false when we call#completeCollapse
in the callback. This won't be an issue for us because we will also manually update elementlayoutBox
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
when we try to collapse an element we do things like:
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#L1036In this PR I am trying to introduce a new method call
attemptCollapse
, and it will call resourcesscheduleChangeSize_
with a callback function to set display none.Still need to work on adding test coverage.