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 overflow-clip-box-*, -moz-user-focus, and (-moz-)user-modify #2868

Merged
merged 13 commits into from Oct 4, 2018

Conversation

Projects
None yet
5 participants
@vinyldarkscratch
Contributor

vinyldarkscratch commented Sep 25, 2018

This PR is intended to check off a couple of the entries on #2684. I know different repositories have different preferences on sending multiple PRs or just one -- if requested, I'd be happy to cherry-pick into separate PRs!

@connorshea

Some changes needed

"properties": {
"-moz-user-modify": {
"__compat": {
"mdn_url": "https://developer.mozilla.org/docs/Mozilla/CSS/-moz-user-modify",

This comment has been minimized.

@connorshea

This comment has been minimized.

@vinyldarkscratch

vinyldarkscratch Sep 26, 2018

Contributor

I did mean that, haha, whoops! I manually typed out the links, though I pretty much just immediately woke up and worked on this, probably should have double-checked everything. 😛

Show resolved Hide resolved css/properties/overflow-clip-box.json
Show resolved Hide resolved css/properties/overflow-clip-box.json Outdated
"properties": {
"-moz-user-focus": {
"__compat": {
"mdn_url": "https://developer.mozilla.org/docs/Mozilla/CSS/-moz-user-focus",

This comment has been minimized.

@connorshea
"mdn_url": "https://developer.mozilla.org/docs/Mozilla/CSS/-moz-user-modify",
"support": {
"chrome": {
"prefix": "-webkit-",

This comment has been minimized.

@connorshea

connorshea Sep 26, 2018

Contributor

This is kind of a weird edge-case because multiple browsers implement this with different prefixes, maybe the file should cover user-modify instead of -moz-user-modify? I'd have assumed that you could determine the property name by prepending the prefix before the feature name, but this breaks that (e.g. you'd end up with -webkit-moz-user-modify).

This comment has been minimized.

@vinyldarkscratch

vinyldarkscratch Sep 26, 2018

Contributor

I kind of thought the same thing when running through this file, but decided to name it -moz-user-modify as that's what the link refers to. But since you're indicating that it breaks compatibility, creating a commit to accommodate now. 😉

@vinyldarkscratch vinyldarkscratch changed the title from Add overflow-clip-box-*, -moz-user-focus, and -moz-user-modify to Add overflow-clip-box-*, -moz-user-focus, and (-moz-)user-modify Sep 27, 2018

"chrome": {
"prefix": "-webkit-",
"version_added": "1",
"notes": "Also supported: <code>-webkit-user-modify: read-write-plaintext-only</code> (Richtext will be lost)."

This comment has been minimized.

@jpmedley

jpmedley Sep 27, 2018

Contributor

@Elchi3 Is read-user-modify supposed to be a separate entry?

This comment has been minimized.

@ExE-Boss

ExE-Boss Sep 29, 2018

Contributor

Yes, read-write-plaintext-only should be a sub‑feature of user-modify, and listed as unsupported in Firefox.

Show resolved Hide resolved css/properties/-moz-user-focus.json
},
"firefox": {
"prefix": "-moz-",
"version_added": false,

This comment has been minimized.

@ExE-Boss

ExE-Boss Sep 29, 2018

Contributor

true

"notes": "While the CSS property is parsed and accepted, it does not have any effect."
},
"firefox_android": {
"version_added": false

This comment has been minimized.

@ExE-Boss

ExE-Boss Sep 29, 2018

Contributor

Same as firefox

"properties": {
"user-modify": {
"__compat": {
"mdn_url": "https://developer.mozilla.org/docs/Web/CSS/-moz-user-modify",

This comment has been minimized.

@ExE-Boss
Show resolved Hide resolved css/properties/-moz-user-focus.json
"chrome": {
"prefix": "-webkit-",
"version_added": "1",
"notes": "Also supported: <code>-webkit-user-modify: read-write-plaintext-only</code> (Richtext will be lost)."

This comment has been minimized.

@ExE-Boss

ExE-Boss Sep 29, 2018

Contributor

Yes, read-write-plaintext-only should be a sub‑feature of user-modify, and listed as unsupported in Firefox.

}
}
}
}

This comment has been minimized.

@ExE-Boss

ExE-Boss Sep 29, 2018

Contributor

Please add trailing newlines to all files.

}
}
}
}

This comment has been minimized.

@ExE-Boss

ExE-Boss Sep 29, 2018

Contributor

ditto

}
}
}
}

This comment has been minimized.

@ExE-Boss

ExE-Boss Sep 29, 2018

Contributor

ditto

Show resolved Hide resolved css/properties/-moz-user-focus.json
"firefox": {
"version_added": true,
"partial_implementation": true,
"notes": "While the CSS property is parsed and accepted, it does not have any effect."

This comment has been minimized.

@ExE-Boss

ExE-Boss Sep 30, 2018

Contributor

Roll back this change.

"firefox_android": {
"version_added": true,
"partial_implementation": true,
"notes": "While the CSS property is parsed and accepted, it does not have any effect."

This comment has been minimized.

@ExE-Boss

ExE-Boss Sep 30, 2018

Contributor

Roll back this change.

},
"read-write-plaintext-only": {
"__compat": {
"description": "<code>read-write-plaintext-only</code> value",

This comment has been minimized.

@ExE-Boss

ExE-Boss Sep 30, 2018

Contributor

This line is unnecessary.

@ExE-Boss

LGTM, r+

@Elchi3

Thanks for your PR!

This PR is intended to check off a couple of the entries on #2684. I know different repositories have different preferences on sending multiple PRs or just one -- if requested, I'd be happy to cherry-pick into separate PRs!

I think review is easier and faster when you open smaller PRs. This PRs deals with different topics at the same time. I'm happy to continue here, but if you want reviews to be faster in the future, smaller PRs (or rather PRs that touch one topic) would be better :)

Thanks again for your work! Some change requests inline.

}
}
},
"overflow-clip-box-block": {

This comment has been minimized.

@Elchi3

Elchi3 Oct 2, 2018

Member

This is a separate CSS property and should live in its own file

}
}
},
"overflow-clip-box-inline": {

This comment has been minimized.

@Elchi3

Elchi3 Oct 2, 2018

Member

This is a separate CSS property and should live in its own file

"prefix": "-moz-",
"version_added": true,
"partial_implementation": true,
"notes": "While the CSS property is parsed and accepted, it does not have any effect."

This comment has been minimized.

@Elchi3

Elchi3 Oct 2, 2018

Member

Not sure this is actually true, but we should mention that it is planned to be removed in https://bugzilla.mozilla.org/show_bug.cgi?id=1388910.

This comment has been minimized.

@ExE-Boss
"prefix": "-moz-",
"version_added": true,
"partial_implementation": true,
"notes": "While the CSS property is parsed and accepted, it does not have any effect."

This comment has been minimized.

@Elchi3

Elchi3 Oct 2, 2018

Member

ditto

"prefix": "-moz-",
"version_added": true,
"partial_implementation": true,
"notes": "While the CSS property is parsed and accepted, it does not have any effect."

This comment has been minimized.

@ExE-Boss

ExE-Boss Oct 2, 2018

Contributor

This line should probably be changed to:

"notes": [
  "While the CSS property is parsed and accepted, it does not have any effect.",
  "Scheduled for removal (see <a href='https://bugzil.la/1388910'>bug 1388910</a>).
]
"prefix": "-moz-",
"version_added": true,
"partial_implementation": true,
"notes": "While the CSS property is parsed and accepted, it does not have any effect."

This comment has been minimized.

@ExE-Boss

ExE-Boss Oct 2, 2018

Contributor
@vinyldarkscratch

This comment has been minimized.

Contributor

vinyldarkscratch commented Oct 2, 2018

Fixed as requested, @Elchi3! I'll also be sure to cherry-pick changes much more frequently. 😉

@Elchi3

Elchi3 approved these changes Oct 4, 2018

Thank you @vinyldarkscratch and all the reviewers 🎉

@Elchi3 Elchi3 merged commit bcd662d into mdn:master Oct 4, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@vinyldarkscratch vinyldarkscratch deleted the vinyldarkscratch:css-compat-data branch Oct 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment