Microsoft CSS changes #156

jameshkramer · 2018-01-08T18:18:13Z

Changes to support Microsoft properties and pseudo-elements.

Removed spaces that preceded CRLF

Removed extraneous space

lahmatiy · 2018-01-17T00:12:00Z

css/properties.json

+    "status": "nonstandard"
+  },
+  "-ms-content-zoom-snap-points": {
+    "syntax": "snapInterval(<start zoomfactors>, <step zoomfactors>) | snapList(<list zoomfactors>)",


Type name can't contain a white spaces. Must be:

"syntax": "snapInterval(<start-zoomfactors>, <step-zoomfactors>) | snapList(<list-zoomfactors>)"

lahmatiy · 2018-01-17T00:12:49Z

css/properties.json

+    "status": "nonstandard"
+  },
+  "-ms-flow-from": {
+    "syntax": "none | identifier [ , none | identifier ] *",


Multipliers must stick to a group:

"syntax": "none | identifier [ , none | identifier ]*"

lahmatiy · 2018-01-17T00:12:57Z

css/properties.json

+    "status": "nonstandard"
+  },
+  "-ms-flow-into": {
+    "syntax": "none | identifier [ , none | identifier ] *",


Multipliers must stick to a group:

"syntax": "none | identifier [ , none | identifier ]*"

lahmatiy · 2018-01-17T00:13:54Z

css/properties.json

+    "status": "nonstandard"
+  },
+  "-ms-hyphenate-limit-chars": {
+    "syntax": "auto | <integer> {1,3}",


Multipliers must stick to a group:

"syntax": "auto | <integer>{1,3}",

lahmatiy · 2018-01-17T00:15:25Z

css/properties.json

+    "status": "nonstandard"
+  },
+  "-ms-scroll-limit": {
+    "syntax": "<'-ms-scroll-limit-x-min'> <'-ms-scroll-limit-y-min'> '<-ms-scroll-limit-x-max'> <'-ms-scroll-limit-y-max'>",


Wrongly placed apostrophe: '<-ms-scroll-limit-x-max'> -> <'-ms-scroll-limit-x-max'>

lahmatiy · 2018-01-17T00:18:06Z

css/properties.json

+    "status": "nonstandard"
+  },
+  "-ms-scroll-snap-points-x": {
+    "syntax": "snapInterval(<start length>, <step length>) | snapList(<list lengths>)",


Type name can't contain a white spaces. Must be:

"syntax": "snapInterval(<start-length>, <step-length>) | snapList(<list-lengths>)"

lahmatiy · 2018-01-17T00:18:13Z

css/properties.json

+    "status": "nonstandard"
+  },
+  "-ms-scroll-snap-points-y": {
+    "syntax": "snapInterval(<start length>, <step length>) | snapList(<list lengths>)",


Type name can't contain a white spaces. Must be:

"syntax": "snapInterval(<start-length>, <step-length>) | snapList(<list-lengths>)"

lahmatiy · 2018-01-17T00:47:43Z

Great contribution!

I leave a comments on syntaxes that can't be parsed according to CSS Values and Units Module Level 3 rules.

But even if fix those mistakes (as I ded), there are several problem properties:

<'-ms-content-zoom-snap-points'>, <'-ms-scroll-snap-points-x'> and <'-ms-scroll-snap-points-y'> contains undefined syntaxes like <start-zoomfactors>, <start-length> and so on
<'-ms-scroll-snap-x'> and <'-ms-scroll-snap-y'> that are using broken <'-ms-scroll-snap-points-x'> and <'-ms-scroll-snap-points-y'> respectively

The problem is around snapInterval( <start> , <step> ) | snapList( <list> ). I suppose it should be changed in such way:

for <'-ms-content-zoom-snap-points'> (according to MSDN)

snapInterval( <percentage> , <percentage> ) | snapList( <percentage># )

for <'-ms-scroll-snap-points-x'> and <'-ms-scroll-snap-points-y'> (according to MSDN)

snapInterval( <length-percentage> , <length-percentage> ) | snapList( <length-percentage># )

That's will solve the problem with undefined syntaxes.

lahmatiy · 2018-01-17T00:51:50Z

css/properties.json

+    "status": "nonstandard"
+  },
+  "-ms-scroll-limit-x-max": {
+    "syntax": "auto | <length> ",


Extra white space at the ending, should be removed.

lahmatiy · 2018-01-17T00:52:11Z

css/properties.json

+    "status": "nonstandard"
+  },
+  "-ms-scroll-limit-y-max": {
+    "syntax": "auto | <length> ",


Extra white space at the ending, should be removed.

lahmatiy · 2018-01-17T01:05:14Z

css/properties.json

+    "status": "nonstandard"
+  },
+  "-ms-filter": {
+    "syntax": "filtertype1 (parameter1, parameter2,...) | filtertype2 (parameter1, parameter2,...)",


This syntax is incorrect, despite it likely can be parsed:

function name should stick to parenthesis, otherwise it is a keyword and expression surrounded with parentheses

function name should be defined explicitly

parameter1, parameter2 means two keywords parameter1 and parameter2 separated with comma

... means 3 periods

| is a logical or

So this syntax have no sense. Moreover, -ms-filter requires that a value to be enclosed in single quotes or double quotes. So, that's just a string and syntax must be replaced for <string>, i.e.

"syntax": "<string>"

jameshkramer · 2018-02-01T18:05:30Z

Many thanks to @lahmatiy for his thorough review of my prior submission. His suggestions were very helpful to me.

Elchi3 · 2018-02-06T22:17:37Z

Many thanks to both of you @jameshkramer and @lahmatiy! I'm merging this.

lahmatiy · 2018-02-06T23:01:15Z

It's really impressive PR and will happy to see it merged. But I didn't review last changes and I see some issues here:

non-standard syntaxes were added to syntax.json with prefix ms-. That's for the first time when vendor syntaxes were added to that dictionary (correct me if I wrong), and I suppose such syntaxes should be added with -ms- prefix to differ from a standard syntaxes. Also some syntaxes are not spec compliant (e.g. progid:DXImageTransform.Microsoft.Alpha( ... )) – we need to decide what to do with them
function params (e.g. progid:DXImageTransform.Microsoft.Alpha( <ms-filter-property-specification> )) may to be more concrete, i.e. progid:DXImageTransform.Microsoft.Alpha( opacity=<integer> ) (but that's not a standard syntax)
definitions in full form (e.g. progid:DXImageTransform.Microsoft.Alpha( ... )) were deprecated since IE8, and (as I remember) it's highly recommended by Microsoft to use a string form of such values. Non-string values were supported until some IE version, not sure which one at the moment.
-ms-scrollbar-* properties currently have syntax defined as 'variant' and initial as 'false', that's incorrect and should be fixed.
-ms-scroll-snap-points-* properties have a semicolon in initial value, that's should be fixed.
and so on...

Also I didn't checked updates with CSSTree's syntax viewer yet. It can show problems with the connectivity of syntaxes.

@Elchi3 I think it's too early to merge it and better to revert it, until all issues are sorted out.

This reverts commit 55537b9.

Elchi3 · 2018-02-07T10:37:43Z

Thanks for your thorough review @lahmatiy and sorry for jumping on the merge button too early.

I thought these additions look good and I would like @jameshkramer to be unblocked for creating the MDN pages (see https://developer.mozilla.org/en-US/dashboards/revisions?user=jameshkramer).

Could we solve these issues in follow-up PRs? Otherwise I will revert this and we need to figure this out with @jameshkramer some more.

lahmatiy · 2018-02-07T10:41:00Z

@Elchi3 Thank you for merge reverting. However PR is still in Merged status. How to continue working on the PR review? Can you reopen it or new PR is needed?

Elchi3 · 2018-02-07T10:54:45Z

The revert PR is this #173
I don't think you can reopen a PR. 😢

So, likely a new PR needs to be opened if we merge the revert.
The alternative would be to leave this PR merged and not revert. Then fix issues you mention in new PRs.

lahmatiy · 2018-02-07T11:57:43Z

The alternative would be to leave this PR merged and not revert. Then fix issues you mention in new PRs.

In this case new release of mdn-data will be blocked until issues are sorted out.

@jameshkramer Could you create a new PR to continue work on issues?

Elchi3 · 2018-02-07T12:31:10Z

In this case new release of mdn-data will be blocked until issues are sorted out.

Yes, I agree. I will try to help as well. Sorry again for merging this too early.

jameshkramer · 2018-02-07T18:37:52Z

Hi @lahmatiy and @Elchi3,
I'll work on the necessary repairs as highest priority. We're investigating the technical issues now. I'm sure I'll be back with questions.
Thanks for your expert help.
Jim

jameshkramer · 2018-02-08T16:52:48Z

Hi @lahmatiy and @Elchi3,

I have been discussing the syntax work with @erikadoyle, and we have a proposal. We would like the -ms-filter page to be shorter and simpler for the benefit of the reader. Links would be provided to the MSDN pages that contain the details.

Please look at the current version of the -ms-filter page to see the proposal:

https://developer.mozilla.org/en-US/docs/Web/CSS/-ms-filter

Note that we use the "full form", per this MSDN page:

https://msdn.microsoft.com/en-us/library/ms533035#Downlevel_Support

Please let us know what you think.

Thanks,
Jim Kramer @jameshkramer

erikadoyle · 2018-02-08T17:00:43Z

Basically the idea is to unblock mdn-data and our effort to port -ms- properties to MDN by avoiding all the progid gobbledygook in the communal json file for now. I'm not sure if its worth eventually porting IE4-era filter MSDN reference, but that would be a project in itself for the backlog.

lahmatiy · 2018-02-09T13:16:07Z

I think moving syntax description on MDN page is a good option. So we can remove ms-filter-* and related syntaxes from the dictionary.

@jameshkramer Could you open a new PR, so we can continue a review?

lahmatiy · 2018-02-09T13:29:45Z

@Elchi3 I see you close reverting PR w/o merge. Plans have changed?

It's hard to fix issues without revert, since changes for a review can be found in that PR only.

lahmatiy · 2018-02-09T13:32:07Z

css/properties.json

+    "groups": [
+      "Microsoft Extensions"
+    ],
+    "initial": "false",


Should be one of: tb, rl, bt, lr

lahmatiy · 2018-02-09T13:32:48Z

css/properties.json

+    "groups": [
+      "Microsoft Extensions"
+    ],
+    "initial": "false",


Should be none or chained

lahmatiy · 2018-02-09T14:04:05Z

css/properties.json

+    "media": "interactive",
+    "inherited": false,
+    "animationType": "discrete",
+    "percentages": "maxZoomFactor",


Should describe the what the percentage actually refers to, not a meaning.

lahmatiy · 2018-02-09T14:04:33Z

css/properties.json

+    "media": "interactive",
+    "inherited": false,
+    "animationType": "discrete",
+    "percentages": "minZoomFactor",


Should describe the what the percentage actually refers to, not a meaning.

lahmatiy · 2018-02-09T14:09:14Z

css/properties.json

+    "groups": [
+      "Microsoft Extensions"
+    ],
+    "initial": "false",


Should be one of none | proximity | mandatory. I suppose none should be here

lahmatiy · 2018-02-09T14:37:50Z

css/properties.json

+    "groups": [
+      "Microsoft Extensions"
+    ],
+    "initial": "false",


Should be none (msdn)

lahmatiy · 2018-02-09T14:43:17Z

css/properties.json

+    "media": "visual",
+    "inherited": true,
+    "animationType": "discrete",
+    "percentages": "referToLineBox",


It's unclear what does it mean. A box has two dimensions (height and width), which one should be used with percentage?

lahmatiy · 2018-02-09T14:46:31Z

css/properties.json

+      "Microsoft Extensions"
+    ],
+    "initial": "0",
+    "appliesto": "exclusionElements",


I think we a definition (reference), what does it mean. Not sure we need introduce that

lahmatiy · 2018-02-09T14:49:09Z

css/properties.json

+      "Microsoft Extensions"
+    ],
+    "initial": "text",
+    "appliesto": "nonReplacedElements",


Unprefixed property user-select applies to allElements. I think this one should do the same.

lahmatiy · 2018-02-09T14:50:38Z

css/properties.json

+    "status": "nonstandard"
+  },
+  "-ms-filter": {
+    "syntax": "<ms-filter-specification>",


Should be <string>

jameshkramer added 4 commits January 8, 2018 09:58

Microsoft CSS changes

4c1befe

Changes to support Microsoft properties and pseudo-elements.

Fixed JSON errors

f09d157

Removed spaces that preceded CRLF

Fixed JSON errors

4b8bcc6

Removed spaces that preceded CRLF

Fixed JSON error

2e88988

Removed extraneous space

lahmatiy reviewed Jan 17, 2018

View reviewed changes

jameshkramer added 3 commits January 31, 2018 16:29

Changes to address issues raised by R. Dvornov

d3194bb

Fixed indentation error

787d8de

Fixed indentation

daaa9a3

Elchi3 merged commit 55537b9 into mdn:master Feb 6, 2018

Elchi3 added a commit that referenced this pull request Feb 7, 2018

Revert "Add Microsoft-specific CSS properties and syntaxes (#156)"

d263b8e

This reverts commit 55537b9.

lahmatiy reviewed Feb 9, 2018

View reviewed changes

jameshkramer mentioned this pull request Mar 7, 2018

Fixes to Microsoft-specific CSS per comments by @lahmatiy #191

Merged

wbamberg mentioned this pull request Apr 3, 2018

Regular NPM releases? #132

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Microsoft CSS changes #156

Microsoft CSS changes #156

jameshkramer commented Jan 8, 2018

lahmatiy Jan 17, 2018

lahmatiy Jan 17, 2018 •

edited

Loading

lahmatiy Jan 17, 2018 •

edited

Loading

lahmatiy Jan 17, 2018 •

edited

Loading

lahmatiy Jan 17, 2018

lahmatiy Jan 17, 2018

lahmatiy Jan 17, 2018

lahmatiy commented Jan 17, 2018

lahmatiy Jan 17, 2018

lahmatiy Jan 17, 2018

lahmatiy Jan 17, 2018

jameshkramer commented Feb 1, 2018

Elchi3 commented Feb 6, 2018

lahmatiy commented Feb 6, 2018

Elchi3 commented Feb 7, 2018

lahmatiy commented Feb 7, 2018

Elchi3 commented Feb 7, 2018

lahmatiy commented Feb 7, 2018

Elchi3 commented Feb 7, 2018

jameshkramer commented Feb 7, 2018

jameshkramer commented Feb 8, 2018

erikadoyle commented Feb 8, 2018

lahmatiy commented Feb 9, 2018

lahmatiy commented Feb 9, 2018

lahmatiy Feb 9, 2018

lahmatiy Feb 9, 2018

lahmatiy Feb 9, 2018

lahmatiy Feb 9, 2018

lahmatiy Feb 9, 2018

lahmatiy Feb 9, 2018

lahmatiy Feb 9, 2018

lahmatiy Feb 9, 2018

lahmatiy Feb 9, 2018

lahmatiy Feb 9, 2018

Microsoft CSS changes #156

Microsoft CSS changes #156

Conversation

jameshkramer commented Jan 8, 2018

Choose a reason for hiding this comment

lahmatiy Jan 17, 2018 • edited Loading

Choose a reason for hiding this comment

lahmatiy Jan 17, 2018 • edited Loading

Choose a reason for hiding this comment

lahmatiy Jan 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lahmatiy commented Jan 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jameshkramer commented Feb 1, 2018

Elchi3 commented Feb 6, 2018

lahmatiy commented Feb 6, 2018

Elchi3 commented Feb 7, 2018

lahmatiy commented Feb 7, 2018

Elchi3 commented Feb 7, 2018

lahmatiy commented Feb 7, 2018

Elchi3 commented Feb 7, 2018

jameshkramer commented Feb 7, 2018

jameshkramer commented Feb 8, 2018

erikadoyle commented Feb 8, 2018

lahmatiy commented Feb 9, 2018

lahmatiy commented Feb 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lahmatiy Jan 17, 2018 •

edited

Loading

lahmatiy Jan 17, 2018 •

edited

Loading

lahmatiy Jan 17, 2018 •

edited

Loading