Skip to content
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

Bento version of amp-lightbox does not respond to state-bound open attribute changes via amp-bind #37400

Closed
westonruter opened this issue Jan 15, 2022 · 3 comments · Fixed by #37619

Comments

@westonruter
Copy link
Member

westonruter commented Jan 15, 2022

Description

When using amp-lightbox v1.0 (Bento), using amp-bind to manage the open attribute has no effect. Example code:

<amp-state id="state">
  <script type="application/json">
  {"open": false}
  </script>
</amp-state>

<button on="tap:AMP.setState({'state':{'open': true }})">See Quote</button>
<amp-lightbox id="quote-lb" layout="nodisplay" data-amp-bind-open="state.open">
  <blockquote>
    "Don't talk to me about JavaScript fatigue" - Horse JS
  </blockquote>
  <button type="button" on="tap:AMP.setState({'state':{'open': false }})">Nice!</button>
</amp-lightbox>

See live examples comparing Bento and classic components with amp-bind for amp-lightbox, amp-video, and amp-twitter: https://bento-amp-with-amp-bind.glitch.me/

Note that only amp-lightbox appears to not work with amp-bind.

Reproduction Steps

See https://bento-amp-with-amp-bind.glitch.me/

Relevant Logs

No response

Browser(s) Affected

Chrome

OS(s) Affected

No response

Device(s) Affected

No response

AMP Version Affected

2112231523002

@westonruter
Copy link
Member Author

cc @alanorozco

@samouri
Copy link
Member

samouri commented Feb 9, 2022

This is an oversight in the R1 resources system. Since components are lazily built when displayed, beginning with nodisplay means the implementation isn't loaded when amp-bind tries to mutate it here:

amphtml/src/custom-element.js

Lines 1793 to 1804 in a5b5ef1

/**
* Called when one or more attributes are mutated.
* Note: Must be called inside a mutate context.
* Note: Boolean attributes have a value of `true` and `false` when
* present and missing, respectively.
* @param {!JsonObject<string, (null|boolean|string|number|Array|Object)>} mutations
*/
mutatedAttributesCallback(mutations) {
if (this.impl_) {
this.impl_.mutatedAttributesCallback(mutations);
}
}

Solution will likely be a single line of code (scheduleAsap), brainstorming now to ensure it is the right thing to do.

@samouri
Copy link
Member

samouri commented Feb 9, 2022

Potential solution: #37619

Bento automation moved this from In progress to Done Feb 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Bento
  
Done
Archived in project
4 participants