resources-impl: fix alwayFixed

[samouri]

Fixes #30379.

Affected components:

• amp-addthis
• amp-analytics
• amp-bind-macro
• amp-state
• amp-sticky-ad
• amp-user-notifications

Root cause is related to intersect-resources experiment. Since measurement can (and often will) occur before the extension loads, measurement is then done while still a stub. At that moment, isAlwaysFixed is always false.

Therefore layoutBox computations were not adjusted for position:fixed and components would not be laid out. Band-aid fix in this PR is to recompute measurements once the extension loads using the same computed rect as earlier (but now taking the fixed position into account).

Future work: instead of relying upon some notion like isAlwaysFixed, we should have a simpler flag for shouldAlwaysLayout that doesn't need to fan out into more computations

[dvoytenko]

I think this is ok as a bug plug while we are reviewing all measurements anyway. But need tests here.

[micajuine-ho]

Thanks for quick resolution Jake!

[samouri]

I put this back in draft mode because I'm seeing some issues with the current solution :(. Investigating now

[samouri]

Argh. Yep. It is possible (especially in this example with scroll: smooth) that the viewport scroll actually changes in between the measure and the time when we realize the resource needs to be fixed.

Theres no way around performing a remeasure.

[jridgewell]

Can you actually reproduce with something like amp-user-notification?

For amp-analytics, amp-state, and amp-bind-macro, I'd rather we just remove this isAlwaysFixed. This was a legacy hack we added to force elements that were way down the page to appear as if they were in the first viewport. This forced us to call their layoutCallback regardless of how far away they were. This was always a hack, and a better solution would just be for us to provide a alwaysLayoutRegardlessOfDistance API that skips the usual distance to viewport checks.

[samouri]

Can you actually reproduce with something like amp-user-notification?

I haven't tried, but the logic is the same. When the measure happens before upgrade, then any fixed element may have its rect incorrect (esp. during a scroll event).

This was always a hack, and a better solution would just be for us to provide a...

I agree! In the PR description I actively called out the desire to replace the existing method with a flag like the one you mention (shouldAlwaysLayout), and that is definitely something I'm planning on doing. Given that it is a much further departure to the way of doing things than this fix though, I'd consider this to be a safer change. I'd like to defer the bigger scope change to future PRs (as opposed to at the same time as a bugfix).

[samouri]

@jridgewell: 🏓