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

Add browser compat for HTML Interactive elements #318

Merged
merged 11 commits into from Sep 21, 2017

Conversation

teoli2003
Copy link
Member

@teoli2003 teoli2003 commented Aug 15, 2017

Add browser compat for <summary>, <details>, <menu>, <menulist> & <dialog>.

@jpmedley
Copy link
Collaborator

Please put true for Android Webview. Webview follows Chrome versioning, not Android.

@teoli2003
Copy link
Member Author

@jpmedley Sure! Quick question if the Chrome entry is behind a flag (that is true with a flag), I assume the Android Webview should be false (as there are no flag that can be changed.)

"notes": "<a href='https://developer.microsoft.com/en-us/microsoft-edge/platform/status/detailssummary?filter=f3f0000bf&search=details'>Under consideration</a>."
},
"firefox": {
"version_added": "49.0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

49.0 -> 49 (here and elsewhere)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

},
"firefox": {
"version_added": false,
"notes": "See <a href=\"https://bugzilla.mozilla.org/show_bug.cgi?id=840640\">bug 840640</a>"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is new: we added a rule to use short urls and single quotes here for easier readability.
Please use "See <a href='https://bugzil.la/840640'>bug 840640</a>."
If you rebase, the validator will complain about this, too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

},
"firefox_android": {
"version_added": true,
"notes": "Nested menus are not supported"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please end notes with a full stop.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@jpmedley
Copy link
Collaborator

@teoli2003 I apologize for the delay in getting back to you. I'm still trying to find someone who can tell me if Chrome flags apply to weview and if so, how to access them. I actually don't know because I don't list items behind a flag on MDN. This brings me to my second point. Since Google uses its own channels to communicate what's behind a flag, it's fine with me if you want to mark all behind a flag features as not supported.

@Elchi3 Elchi3 added the data:html 📄 Compat data for HTML elements. https://developer.mozilla.org/docs/Web/HTML label Aug 23, 2017
@teoli2003
Copy link
Member Author

@Elchi3 reviews addressed (and all rebased with new structure and mdn_url)

"notes": "<a href='https://developer.microsoft.com/en-us/microsoft-edge/platform/status/detailssummary?filter=f3f0000bf&search=details'>Under consideration</a>."
},
"firefox": {
"version_added": "49"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There has been a note added here in the meantime about Firefox 57 / Quantum. Do we want to add it? https://developer.mozilla.org/en-US/docs/Web/HTML/Element/details#Quantum_CSS_notes

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. I rephrased it to merely state Firefox 57 (rather than Quantum that is an internal-name only).

Also, Stylo will not be in Firefox 57 for Android (I think it will be in Fx 58 or 59), so I rephrased the comment for Android to state that there still is the stated bug.

}
}
},
"type": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type="context" is missing.

I would nest type="context" and type="toolbar" under type, but I'm not sure type needs __compat (aka basic support). So:

  • type
    • context
      • __compat
    • toolbar
      • __compat

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I nested context and toolbar; but without the basic support for type, the rendering macro is crashing, so I kept it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened #385

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Elchi3 I updated the PR to remove the basic support for type. It works locally, so this can land when #385 has landed.

@@ -0,0 +1,476 @@
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole file needs to be renamed to menuitem.json and the object should be html.elements.menuitem. The MDN URL is wrong, too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

"firefox_android": {
"version_added": "8",
"notes": [
"Only works for <code>&lt;menuitem&gt;</code> elements defined within a <code>&gt;menu&lt;</code> element assigned to an element via the <code>contextmenu</code> attribute.",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<code>&gt;menu&lt;</code> -> <code>&lt;menu&gt;</code> 😆

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

"version_added": "8",
"notes": [
"Only works for <code>&lt;menuitem&gt;</code> elements defined within a <code>&lt;menu&gt;</code> element assigned to an element via the <code>contextmenu</code> attribute.",
"The &lt;menuitem&gt; element requires a closing tag."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The <code>&lt;menuitem&gt;</code> (here and throughout the file)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

"chrome_android": {
"version_added": true
},
"edge": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add the same note as for the details element (under consideration)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed (and, meanwhile, MS moved it to In development, so I fixed this too)

@teoli2003
Copy link
Member Author

teoli2003 commented Sep 18, 2017

@Elchi3 Comments fixed, please take a special look at the one about <menu>.

@Elchi3 Elchi3 merged commit 1114c79 into mdn:master Sep 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data:html 📄 Compat data for HTML elements. https://developer.mozilla.org/docs/Web/HTML
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants