Bug 228548 – Web Inspector: Add standard CSS Logical properties to CSS keyword completion

• Component: Web Inspector
• Status: RESOLVED FIXED

[Sonia Singla]

Replace -webkit-*-* logical properties to standard logical properties in WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js

[Radar WebKit Bug Importer]

<rdar://problem/81212400>

[Sonia Singla]

Created attachment 434419 [details]
Patch

[Manuel Rego Casasnovas]

Comment on attachment 434419 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=434419&action=review

Thank you! This seems to be a good change as other logical properties are already using the standard name on that file.

> Source/WebInspectorUI/ChangeLog:8
> +        * UserInterface/Models/CSSKeywordCompletions.js:

The file seems to be sorted alphabetically, could you keep it sorted after the renames?

[Frédéric Wang (:fredw)]

Comment on attachment 434419 [details]
Patch

IIUC, this is removing autocompletion support for these legacy -webkit prefixes. Do we want that?

[Sam Sneddon [:gsnedders]]

(In reply to Frédéric Wang (:fredw) from comment #4)
> Comment on attachment 434419 [details]
> Patch
> 
> IIUC, this is removing autocompletion support for these legacy -webkit
> prefixes. Do we want that?

Yeah, we probably want to keep both autocompleting till we remove the -webkit-* properties.

[Manuel Rego Casasnovas]

(In reply to Sam Sneddon [:gsnedders] from comment #5)
> (In reply to Frédéric Wang (:fredw) from comment #4)
> > Comment on attachment 434419 [details]
> > Patch
> > 
> > IIUC, this is removing autocompletion support for these legacy -webkit
> > prefixes. Do we want that?
> 
> Yeah, we probably want to keep both autocompleting till we remove the
> -webkit-* properties.

This is just making a few prefixed logical properties behave like the rest.
For example we have support for "-webkit-margin-before" which is the old version of "margin-block-start", and we only have "margin-block-start" in CSSKeywordCompletions.js.

That's why we thought it'd be ok to do the same with the remaining logical properties there that have standard ones.

[Manuel Rego Casasnovas]

(In reply to Manuel Rego Casasnovas from comment #6)
> (In reply to Sam Sneddon [:gsnedders] from comment #5)
> > (In reply to Frédéric Wang (:fredw) from comment #4)
> > > Comment on attachment 434419 [details]
> > > Patch
> > > 
> > > IIUC, this is removing autocompletion support for these legacy -webkit
> > > prefixes. Do we want that?
> > 
> > Yeah, we probably want to keep both autocompleting till we remove the
> > -webkit-* properties.
> 
> This is just making a few prefixed logical properties behave like the rest.
> For example we have support for "-webkit-margin-before" which is the old
> version of "margin-block-start", and we only have "margin-block-start" in
> CSSKeywordCompletions.js.

Actually "margin-block-start" is not even on this file. So maybe we don't want to do that and just add the standard logical properties in this file. Sorry.

[Manuel Rego Casasnovas]

(In reply to Manuel Rego Casasnovas from comment #7)
> (In reply to Manuel Rego Casasnovas from comment #6)
> > (In reply to Sam Sneddon [:gsnedders] from comment #5)
> > > (In reply to Frédéric Wang (:fredw) from comment #4)
> > > > Comment on attachment 434419 [details]
> > > > Patch
> > > > 
> > > > IIUC, this is removing autocompletion support for these legacy -webkit
> > > > prefixes. Do we want that?
> > > 
> > > Yeah, we probably want to keep both autocompleting till we remove the
> > > -webkit-* properties.
> > 
> > This is just making a few prefixed logical properties behave like the rest.
> > For example we have support for "-webkit-margin-before" which is the old
> > version of "margin-block-start", and we only have "margin-block-start" in
> > CSSKeywordCompletions.js.
> 
> Actually "margin-block-start" is not even on this file. So maybe we don't
> want to do that and just add the standard logical properties in this file.
> Sorry.

Not that one, but what I explained happens for "border-block-end-style", it has a prefixed version "-webkit-border-after-style", but only the standard one is listed on CSSKeywordCompletions.js.
That's why I got confused.

Would it be a good idea to add the standard unprefixed properties to this file and don't remove anything for now?

[Devin Rousso]

Yeah we don't want to remove any legacy properties so long as they're still supported by the engine or any version of the engine still found on commonly used iOS devices.

[Sonia Singla]

Created attachment 434507 [details]
Patch

[Sonia Singla]

Created attachment 434508 [details]
Patch

[Sonia Singla]

Created attachment 434510 [details]
Patch

[Sonia Singla]

Created attachment 434513 [details]
Patch

[Manuel Rego Casasnovas]

Comment on attachment 434513 [details]
Patch

Looks good, now that we're adding these I'd also include other properties like margin-block-start, padding-*, etc. thought those only have "auto" value.

[Sonia Singla]

Created attachment 434517 [details]
Patch

[Oriol Brufau]

Comment on attachment 434517 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=434517&action=review

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:608
> +    ],

Seems a bit strange to have the logical padding properties but not the physical ones.

[Sonia Singla]

> Seems a bit strange to have the logical padding properties but not the physical ones.

Made the changes thanks for review Oriol.

[Sonia Singla]

Created attachment 434525 [details]
Patch

[Oriol Brufau]

Comment on attachment 434525 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=434525&action=review

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:620
> +    ],

Wait, actually the padding properties don't accept "auto" :)

[Sonia Singla]

Created attachment 434601 [details]
Patch

[Manuel Rego Casasnovas]

Comment on attachment 434601 [details]
Patch

Thanks. This looks good to me.
But I'd prefer if someone more involved on the inspector like Devin Rousso reviews this.

[Frédéric Wang (:fredw)]

Comment on attachment 434601 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=434601&action=review

> Source/WebInspectorUI/ChangeLog:3
> +        Web Inspector: Add standard logical properties in WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js

Shouldn't we also add update ContextualDocumentationDatabase.js to add the unprefixed properties ?

[Frédéric Wang (:fredw)]

Comment on attachment 434601 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=434601&action=review

>> Source/WebInspectorUI/ChangeLog:3
>> +        Web Inspector: Add standard logical properties in WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js
> 
> Shouldn't we also add update ContextualDocumentationDatabase.js to add the unprefixed properties ?

And it seems the JetStream test is also using these -webkit-* values to test WebInspector CSSKeywordCompletions, so this is related to the change here too.
I wonder whether you should call this "Web Inspector: Add standard logical properties to CSS keyword completion".

PerformanceTests/JetStream2/code-load/inspector-payload.js
PerformanceTests/JetStream2/code-load/inspector-payload-minified.js
Websites/browserbench.org/JetStream2.0/code-load/inspector-payload-minified.js
Websites/browserbench.org/JetStream2.0/code-load/inspector-payload.js

BTW, bugzilla's bug title should be updated too.

[Patrick Angle]

Comment on attachment 434601 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=434601&action=review

>>> Source/WebInspectorUI/ChangeLog:3
>>> +        Web Inspector: Add standard logical properties in WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js
>> 
>> Shouldn't we also add update ContextualDocumentationDatabase.js to add the unprefixed properties ?
> 
> And it seems the JetStream test is also using these -webkit-* values to test WebInspector CSSKeywordCompletions, so this is related to the change here too.
> I wonder whether you should call this "Web Inspector: Add standard logical properties to CSS keyword completion".
> 
> PerformanceTests/JetStream2/code-load/inspector-payload.js
> PerformanceTests/JetStream2/code-load/inspector-payload-minified.js
> Websites/browserbench.org/JetStream2.0/code-load/inspector-payload-minified.js
> Websites/browserbench.org/JetStream2.0/code-load/inspector-payload.js
> 
> BTW, bugzilla's bug title should be updated too.

`ContextualDocumentationDatabase.js` is a file derived from information in the `vscode-custom-data` open source project, so we shouldn't add anything manually. Properties without documentation available are gracefully handled, so the presence of an entry in that file is not required.

[Devin Rousso]

Comment on attachment 434601 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=434601&action=review

r=me

>>>> Source/WebInspectorUI/ChangeLog:3
>>>> +        Web Inspector: Add standard logical properties in WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js
>>> 
>>> Shouldn't we also add update ContextualDocumentationDatabase.js to add the unprefixed properties ?
>> 
>> And it seems the JetStream test is also using these -webkit-* values to test WebInspector CSSKeywordCompletions, so this is related to the change here too.
>> I wonder whether you should call this "Web Inspector: Add standard logical properties to CSS keyword completion".
>> 
>> PerformanceTests/JetStream2/code-load/inspector-payload.js
>> PerformanceTests/JetStream2/code-load/inspector-payload-minified.js
>> Websites/browserbench.org/JetStream2.0/code-load/inspector-payload-minified.js
>> Websites/browserbench.org/JetStream2.0/code-load/inspector-payload.js
>> 
>> BTW, bugzilla's bug title should be updated too.
> 
> `ContextualDocumentationDatabase.js` is a file derived from information in the `vscode-custom-data` open source project, so we shouldn't add anything manually. Properties without documentation available are gracefully handled, so the presence of an entry in that file is not required.

You definitely don't need to update any of the perf tests.  Those are not meant to represent actual Web Inspector and I believe are just a way to test large JS payloads.  They are also _extremely_ out of date.

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:466
> +        "auto", "intrinsic", "min-intrinsic", "min-content", "-webkit-min-content", "max-content",
> +        "-webkit-max-content", "-webkit-fill-available", "fit-content", "-webkit-fit-content", "calc()"

Style: Please don't split this into two lines.  Either keep it all on one line (preferred) or split every item onto it's own line.

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:550
> +        "auto", "intrinsic", "min-intrinsic", "min-content", "-webkit-min-content",
> +        "max-content", "-webkit-max-content", "-webkit-fill-available", "fit-content", "-webkit-fit-content", "calc()"

ditto (:465)

> Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js:559
> +        "auto"

Style: missing trailing comma

(ditto with most of the other added lines in this patch too)

[Sonia Singla]

Created attachment 434745 [details]
Patch

[Sonia Singla]

Created attachment 434811 [details]
Patch

[EWS]

Committed r280588 (240210@main): <https://commits.webkit.org/240210@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 434811 [details].