✨ Recognize details element with open/[open] attributes and summary child element by westonruter · Pull Request #18440 · ampproject/amphtml

westonruter · 2018-09-28T16:22:40Z

I couldn't find a clear explanation on why the HTML5 details element is not allowed. The only mention of it is #1410 in relation to amp-accordion. It would be helpful to support details when converting HTML into AMP (such as in the WordPress plugin), since it could be used as-is without having to try to port it over to amp-accordion (even though the latter is more powerful), as a details-to-amp-accordion conversion so would would cause complications with existing styles that target the details element and its children.

In this PR:

Add details element.
Add details element's open boolean attribute.
Add [open] to amp-bind for managing the the details element's open boolean attribute.
Add the summary element as a child of details.

With these changes in place, an AMP document would be able to do:

<amp-state id="detailsOpen">false</amp-state>
<button on="tap:AMP.setState({ detailsOpen: ! detailsOpen })">Toggle Open</button>

<details [open]="detailsOpen">
	<summary>Learn more</summary>
	<p>This is more information.</p>
</details>

honeybadgerdontcare

Validation feedback only.

honeybadgerdontcare · 2018-09-28T22:53:12Z

validator/validator-main.protoascii

  }
 }
-# 4.4.8 The dl element
+# 4.11.1 (HTML5.3) The details element


It seems with 5.3 they've renumbered everything so this doesn't really follow our existing numbering. I'd still recommend putting it just before 4.11 Scripting below and include a 4.11 Interactive elements before it.

Done in 7255acd

honeybadgerdontcare · 2018-09-28T22:54:43Z

validator/validator-main.protoascii

+# 4.11.1 (HTML5.3) The details element
+tags: {
+  html_format: AMP
+  html_format: AMP4ADS


Lets begin with just AMP and leave the other formats for future PRs. If you believe these should be added to other formats then I'd recommend opening an issue asking for feedback from others as each one of these formats have a different spec. Same for below for SUMMARY.

EXPERIMENTAL would also be removed?

honeybadgerdontcare · 2018-09-28T22:55:33Z

validator/validator-main.protoascii

+  tag_name: "SUMMARY"
+  mandatory_parent: "DETAILS"
+}
+# 4.4.9 The dl element


Please leave as 4.4.8 as these are following the HTML5 spec.

Fixed in 7255acd

honeybadgerdontcare · 2018-09-28T22:56:50Z

@dvoytenko for including HTML5.3 spec items
@choumx for AMP4EMAIL spec
@lannka for AMP4ADS spec

dvoytenko · 2018-09-28T23:58:06Z

The element itself LG. We won't be providing any polyfills, will we?

Also limit html_format to just AMP.

westonruter · 2018-09-29T22:23:35Z

Why are the tests failing? Do I need to rebuild something and commit?

honeybadgerdontcare · 2018-09-30T01:34:15Z

The test failure mentions that the tag name is not defined. And it is not defined for summary.

westonruter · 2018-09-30T15:06:40Z

The element itself LG. We won't be providing any polyfills, will we?

@dvoytenko That's a good question. In terms of the actual clicking of the summary causing the open property to toggle, that could benefit from a polyfill. Otherwise if authors polyfill it themselves with amp-bind then perhaps they could cause double toggle in browsers that already have support, and thus the element's state could be stuck.

I think the styling of the element should be left to the author, however.

westonruter · 2018-09-30T21:27:09Z

On second thought, I wonder if open should be included in what amp-bind can manipulate. When using amp-bind to manage the open state, if a browser supports details natively then the state for whether the element is expanded would be stored in two separate places. As seen in the example added in 609221a, you can expand the details by clicking on the summary, without updating the amp-state:

Likewise, if you opened the details via an amp-state change then closing it via the component would also not cause the state to update:

Since the state can easily get out of sync, it seems the open attribute should be excluded from amp-bind.

Thoughts?

validator/validator-main.protoascii

westonruter · 2018-10-03T04:02:24Z

What about the question regarding whether the open attribute should be controllable via amp-bind? #18440 (comment)

Gregable · 2018-10-04T01:52:18Z

Re [open], I don't really have an opinion. Adding @choumx who might.

Gregable · 2018-10-04T01:53:24Z

LGTM once the amp-bind question is resolved.

dreamofabear · 2018-10-04T18:57:27Z

Either way is fine by me. I think amp-accordion has a similar issue and it's not a dealbreaker IMO.

westonruter · 2018-10-05T00:39:22Z

OK, cool. Let's keep [open] then I guess.

To deal with the inconsistent state issue, it would be great to introduce a way to connect a bound attribute with some state in a more declarative way so that updates to the attribute and to the state so that there could be two-way binding.

westonruter · 2018-11-06T04:31:26Z

@Gregable I believe the question is resolved.

westonruter · 2018-12-18T17:02:06Z

Anything left here to do?

honeybadgerdontcare

validation changes look good

* cl/225092847 Add validator rules for amp-video and amp-yotpo in EXPERIMENTAL. * cl/225106151 "add validator changes to support script templates -alabiaga@" * cl/225388113 Revision bump for #19854 * cl/225400099 Revision bump for #19871 * cl/225612473 Remove EXPERIMENTAL from amp-list and amp-state. * cl/225861155 Revision bump for #19872 * cl/225872246 Revision bump for #19894 * cl/225876987 Revision bump for #18700 * cl/226048698 Revision bump for #19928 * cl/226051527 Revision bump for #18440

…hild element (ampproject#18440) * Add details element with open boolean attr and summary child element * Add details@open attribute to amp-bind * Add Interative elements section with details and summary elements Also limit html_format to just AMP. * Add missing tag_name for summary element * Fix order of summary tag properties * Add amp-bind example for details element * Include details/summary elements in AMP4ADS, AMP4EMAIL, EXPERIMENTAL

* cl/225092847 Add validator rules for amp-video and amp-yotpo in EXPERIMENTAL. * cl/225106151 "add validator changes to support script templates -alabiaga@" * cl/225388113 Revision bump for ampproject#19854 * cl/225400099 Revision bump for ampproject#19871 * cl/225612473 Remove EXPERIMENTAL from amp-list and amp-state. * cl/225861155 Revision bump for ampproject#19872 * cl/225872246 Revision bump for ampproject#19894 * cl/225876987 Revision bump for ampproject#18700 * cl/226048698 Revision bump for ampproject#19928 * cl/226051527 Revision bump for ampproject#18440

…hild element (ampproject#18440) * Add details element with open boolean attr and summary child element * Add details@open attribute to amp-bind * Add Interative elements section with details and summary elements Also limit html_format to just AMP. * Add missing tag_name for summary element * Fix order of summary tag properties * Add amp-bind example for details element * Include details/summary elements in AMP4ADS, AMP4EMAIL, EXPERIMENTAL

* cl/225092847 Add validator rules for amp-video and amp-yotpo in EXPERIMENTAL. * cl/225106151 "add validator changes to support script templates -alabiaga@" * cl/225388113 Revision bump for ampproject#19854 * cl/225400099 Revision bump for ampproject#19871 * cl/225612473 Remove EXPERIMENTAL from amp-list and amp-state. * cl/225861155 Revision bump for ampproject#19872 * cl/225872246 Revision bump for ampproject#19894 * cl/225876987 Revision bump for ampproject#18700 * cl/226048698 Revision bump for ampproject#19928 * cl/226051527 Revision bump for ampproject#18440

westonruter added 2 commits September 28, 2018 09:03

Add details element with open boolean attr and summary child element

f71ae66

Add details@open attribute to amp-bind

1477cdb

googlebot added the cla: yes label Sep 28, 2018

westonruter mentioned this pull request Sep 28, 2018

implement alternative to <details> element #1410

Closed

honeybadgerdontcare requested changes Sep 28, 2018

View reviewed changes

Add Interative elements section with details and summary elements

7255acd

Also limit html_format to just AMP.

westonruter added 2 commits September 29, 2018 19:18

Add missing tag_name for summary element

f1f41ca

Fix order of summary tag properties

b24cec2

Add amp-bind example for details element

609221a

dreamofabear mentioned this pull request Oct 1, 2018

Support for details / summary tags #18461

Closed

Gregable requested changes Oct 2, 2018

View reviewed changes

validator/validator-main.protoascii Show resolved Hide resolved

validator/validator-main.protoascii Show resolved Hide resolved

Include details/summary elements in AMP4ADS, AMP4EMAIL, EXPERIMENTAL

f54381f

Gregable requested a review from dreamofabear October 4, 2018 01:52

Gregable approved these changes Nov 6, 2018

View reviewed changes

honeybadgerdontcare approved these changes Dec 18, 2018

View reviewed changes

Gregable merged commit 18b10a2 into ampproject:master Dec 18, 2018

honeybadgerdontcare mentioned this pull request Dec 18, 2018

Validator roll up #19965

Merged

westonruter deleted the add/details-element branch February 13, 2019 06:03

westonruter mentioned this pull request Sep 16, 2019

Improve validation of children of invalid elements ampproject/amp-wp#1511

Closed

westonruter mentioned this pull request Feb 24, 2020

Resources in <details> element do not load upon opening #26952

Closed

Conversation

westonruter commented Sep 28, 2018

Uh oh!

honeybadgerdontcare left a comment

Choose a reason for hiding this comment

Uh oh!

honeybadgerdontcare Sep 28, 2018

Choose a reason for hiding this comment

Uh oh!

westonruter Sep 29, 2018

Choose a reason for hiding this comment

Uh oh!

honeybadgerdontcare Sep 28, 2018

Choose a reason for hiding this comment

Uh oh!

westonruter Sep 29, 2018

Choose a reason for hiding this comment

Uh oh!

honeybadgerdontcare Sep 29, 2018

Choose a reason for hiding this comment

Uh oh!

honeybadgerdontcare Sep 28, 2018

Choose a reason for hiding this comment

Uh oh!

westonruter Sep 29, 2018

Choose a reason for hiding this comment

Uh oh!

honeybadgerdontcare commented Sep 28, 2018

Uh oh!

dvoytenko commented Sep 28, 2018

Uh oh!

westonruter commented Sep 29, 2018

Uh oh!

honeybadgerdontcare commented Sep 30, 2018

Uh oh!

westonruter commented Sep 30, 2018

Uh oh!

westonruter commented Sep 30, 2018

Uh oh!

Uh oh!

Uh oh!

westonruter commented Oct 3, 2018

Uh oh!

Gregable commented Oct 4, 2018

Uh oh!

Gregable commented Oct 4, 2018

Uh oh!

dreamofabear commented Oct 4, 2018

Uh oh!

westonruter commented Oct 5, 2018

Uh oh!

westonruter commented Nov 6, 2018

Uh oh!

westonruter commented Dec 18, 2018

Uh oh!

honeybadgerdontcare left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants