| Summary: | Web Inspector: Add standard CSS Logical properties to CSS keyword completion | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Sonia Singla <soniasingla.1812> | ||||||||||||||||||||||
| Component: | Web Inspector | Assignee: | Sonia Singla <soniasingla.1812> | ||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||
| Severity: | Normal | CC: | ews-watchlist, fred.wang, gsnedders, hi, inspector-bugzilla-changes, nvasilyev, obrufau, pangle, rcaliman, rego, webkit-bug-importer | ||||||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||||||||
| Bug Blocks: | 228546 | ||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||
|
Description
Sonia Singla
2021-07-28 04:43:15 PDT
Created attachment 434419 [details]
Patch
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? Comment on attachment 434419 [details]
Patch
IIUC, this is removing autocompletion support for these legacy -webkit prefixes. Do we want that?
(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. (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. (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. (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? 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. Created attachment 434507 [details]
Patch
Created attachment 434508 [details]
Patch
Created attachment 434510 [details]
Patch
Created attachment 434513 [details]
Patch
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.
Created attachment 434517 [details]
Patch
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. > Seems a bit strange to have the logical padding properties but not the physical ones.
Made the changes thanks for review Oriol.
Created attachment 434525 [details]
Patch
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" :) Created attachment 434601 [details]
Patch
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.
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 ? 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. 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. 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) Created attachment 434745 [details]
Patch
Created attachment 434811 [details]
Patch
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]. |