Steps: 1. Inspect <body> element 2. In the styles sidebar, in the Styles Attribute section, type "f" 3. Press Tab Expected: The most commonly used CSS property name that starts with "f" is autocompleted, e.g. "font-size". Actual: Alphabetically first CSS property name is autocompleted, e.g. "fill". Notes: I worked on Bug 96763 in 2012, but I don't think it ever landed. Recently, Microsoft released Global CSS Property Usage stats: https://developer.microsoft.com/en-us/microsoft-edge/platform/usage/ It clearly shows that "font-size" is more common than "fill", and "color" is more common than "caption-side".
Firefox DevTools started doing this: https://twitter.com/FirefoxDevTools/status/717365790010376193
<rdar://problem/25588888>
*** Bug 179830 has been marked as a duplicate of this bug. ***
Where typing in styles is something many developers do all day and hard to imagine the ordering being alphabetical versus popularity being intentional, so where this was reported a few years ago was wondering if this is still in someones queue. I know most would simply say to use Chrome where it works as expected, but with all the incredible improvements over the past year to Webkit dev tools, this personally has been one of the remaining major sticks in the mud compared to using Chrome's implementation. Just my two scents. Go team!
Created attachment 351706 [details] Patch
Comment on attachment 351706 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351706&action=review There are at least two approaches: 1. According to https://developer.microsoft.com/en-us/microsoft-edge/platform/tusage/Define, there are a lot of properties that are almost never used. We can provide a good experience out of the box if we store property name weights based on public data. 2. Web Inspector can remember (in localStorage) properties that are used the most and suggest them. Doing either of these would be a big improvement. Ideally, I want to have both. r- until we agree what we want to do here. > Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:425 > +WI.CSSProperty._nameCount = {}; Does this get lost after closing Web Inspector? We need to save this in localStorage.
Comment on attachment 351706 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351706&action=review (In reply to Nikita Vasilyev from comment #6) > 1. According to > https://developer.microsoft.com/en-us/microsoft-edge/platform/tusage/Define, > there are a lot of properties that are almost never used. We can provide a > good experience out of the box if we store property name weights based on > public data. My issue with this approach is that that list doesn't seem to take into account any sort of longhand property, which is something I'd definitely want to see as a user. The important thing for me is that I would still want the suggestions list to be alphabetical, but the initially selected value should also be the one that is most common. The approach that I took in this patch was to base usage off of the properties encountered by the developer. As an example, that list shows "float" to be much more common than "flex-*". If I'm not using "float" anywhere in my code, I wouldn't want it to suggest that by default. Furthermore, this approach is much more future-forward in that new properties can be added and we wouldn't have to do anything (e.g. update a list) in order to start suggesting it by default. I'm not saying that this approach is best, but moreso highlighting my reservations about using some sort of static list. > 2. Web Inspector can remember (in localStorage) properties that are used the > most and suggest them. Agreed. >> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:425 >> +WI.CSSProperty._nameCount = {}; > > Does this get lost after closing Web Inspector? We need to save this in localStorage. Oooo good idea.
Created attachment 352014 [details] Patch Updated patch with localStorage and tests
Comment on attachment 352014 [details] Patch r- because suggestion hint (the gray text right after the text caret) is never shown with this patch. I showed it to Devin offline. I'm going to run with this patch for a few days to see if the autocomplete actually selects what I want.
Created attachment 352469 [details] Patch
(In reply to Nikita Vasilyev from comment #9) > Comment on attachment 352014 [details] > Patch > > r- because suggestion hint (the gray text right after the text caret) is > never shown with this patch. I showed it to Devin offline. > > I'm going to run with this patch for a few days to see if the autocomplete > actually selects what I want. Any thoughts you'd like to share?
Created attachment 353610 [details] Patch Rebase
Comment on attachment 353610 [details] Patch r- because newly added properties don't get counted. Discussed with Devin offline.
Created attachment 353746 [details] Patch
Consider the following scenario: 1. Open https://webkit.org/ 2. Inspect <body> 3. Log JSON.parse(localStorage.getItem("com.apple.WebInspector.css-property-name-counts"))["background-color"] 4. Open https://apple.com/ in a new tab 5. Inspect <body> 6. Log JSON.parse(localStorage.getItem("com.apple.WebInspector.css-property-name-counts"))["background-color"] The logged number should be >= than the number logged the first time. This wasn't the case for me. Lately, I've been having issues when localStorage values not being persistent across sessions.
It seems like I have some issue with localStorage that is likely a regression. However, besides that, I don't think we should use WI.Setting for usage counts. Consider this case: 1. Open new window with https://webkit.org/ 2. Inspect <body> 3. Minimize this window 4. Use Web Inspector in another window for a month 5. Open the window with https://webkit.org/ again and add a new CSS property The whole month of CSS property count history will be erased and replaced by the old session. I think we should use localStorage directly. IndexedDB is also an option since it's asynchronous but the API is more complex.
(In reply to Nikita Vasilyev from comment #16) > I think we should use localStorage directly. IndexedDB is also an option > since it's asynchronous but the API is more complex. I experimented with using `WI.ObjectStore` (and IndexedDB wrapper) for this, but the main issue there is that it's async. I'll see if I can rework it to use some sort of value caching so that when we sort, we don't need to be async and await the result of the IndexedDB fetch, as the value will already be cached.
Created attachment 353903 [details] Patch
Comment on attachment 353903 [details] Patch Attachment 353903 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/9870203 New failing tests: inspector/css/css-property.html
Created attachment 353912 [details] Archive of layout-test-results from ews113 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 355544 [details] Patch When running tests, since `WI.ObjectStore.supported()` is `false`, the value never gets saved, meaning calling `get()` will return `undefined`. `WI.CSSProperty.prototype._updateName` would default to using `0` in this case, which wouldn't work for the test as the cached value would already be `1`. Instead, we should default to using the cached value if the `get()` doesn't work.
Comment on attachment 355544 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355544&action=review Looks good overall. A few questions below. > Source/WebInspectorUI/ChangeLog:11 > + Every time a `WI.CSSProperty` is created/updated, increment/decrement the count of that > + property's name in the global map of property name counts. When showing CSS completions for > + property names, initially focus the property with the highest count. At the high level, this patch counts CSS properties that were *displayed* in the styles sidebar. This provides suboptimal initial experience. In 20 minutes of me using Web Inspector, my default completion item for "c" changed from "caption-side", to "clear", to "color". That being said, after using it for 2 hours, most of the default completion items were what I'd expect. > Source/WebInspectorUI/UserInterface/Base/ObjectStore.js:55 > + const version = 2; // Increment this for every edit to `WI.objectStores`. Does the previous version of the data supposed to stay in IndexedDB forever? This is concerning. > Source/WebInspectorUI/UserInterface/Base/Utilities.js:1334 > +Object.defineProperty(Array.prototype, "minIndex", I looked at the tests to understand what this does. `minValueIndex` or even `indexOfMinValue` would be more clear. > Source/WebInspectorUI/UserInterface/Base/Utilities.js:1344 > + comparator = comparator || defaultComparator; > + > + let min = 0; Strange indentation. > Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:77 > + let canonicalPropertyNameA = WI.cssManager.canonicalNameForPropertyName(propertyNameA); > + let canonicalPropertyNameB = WI.cssManager.canonicalNameForPropertyName(propertyNameB); canonicalNameForPropertyName basically removes -webkit- and -apple- prefixes from a given string. Why do we need to use it here? > Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:463 > +WI.CSSProperty._cachedNameCounts = null; WI.CSSCompletions seems a bit more appropriate for this but I don't have a strong preference. > Source/WebInspectorUI/UserInterface/Views/CompletionSuggestionsView.js:82 > + if (this._delegate && this._delegate.completionSuggestionsSelectedCompletion) > + this._delegate.completionSuggestionsSelectedCompletion(this, selectedItemElement.textContent); Nice refactoring! Only the new styles sidebar uses completionSuggestionsSelectedCompletion, so it seems fairly safe to do.
Comment on attachment 355544 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355544&action=review >> Source/WebInspectorUI/ChangeLog:11 >> + property names, initially focus the property with the highest count. > > At the high level, this patch counts CSS properties that were *displayed* in the styles sidebar. > > This provides suboptimal initial experience. In 20 minutes of me using Web Inspector, my default completion item for "c" changed from "caption-side", to "clear", to "color". That being said, after using it for 2 hours, most of the default completion items were what I'd expect. Yes, that's somewhat expected. Perhaps it's worth limiting the property count "influence" to be only if it's been seen a significant (e.g. double the average) number of times. >> Source/WebInspectorUI/UserInterface/Base/ObjectStore.js:55 >> + const version = 2; // Increment this for every edit to `WI.objectStores`. > > Does the previous version of the data supposed to stay in IndexedDB forever? This is concerning. This won't destroy the data previously in the database. `IDBObjectStore` are only able to be created when a database is created or upgraded, so if we want to add new object stores, we need to upgrade the database (hence the version change). As an aside, even if it did, the only data in there right now is the default Audits, which are behind a feature flag and can be re-added very easily. >> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1334 >> +Object.defineProperty(Array.prototype, "minIndex", > > I looked at the tests to understand what this does. `minValueIndex` or even `indexOfMinValue` would be more clear. I was matching `Array.prototype.find` vs `Array.prototype.findIndex`. >> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1344 >> + let min = 0; > > Strange indentation. Oops. >> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:77 >> + let canonicalPropertyNameB = WI.cssManager.canonicalNameForPropertyName(propertyNameB); > > canonicalNameForPropertyName basically removes -webkit- and -apple- prefixes from a given string. Why do we need to use it here? I did this for two reasons: 1. I'd imagine that having both "-webkit-X" and "X" means that "X" is counted higher in both circumstances (with a bias towards the unprefixed version), which leads directly into (2) 2. if the property becomes unprefixed, all of the counts for the prefixed version will count towards the unprefixed version, meaning that there should be no observable difference >> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:463 >> +WI.CSSProperty._cachedNameCounts = null; > > WI.CSSCompletions seems a bit more appropriate for this but I don't have a strong preference. Since the value is changed in this file via private functions, it should be local to this file.
Comment on attachment 355544 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355544&action=review I'm done with the mock review. Actual reviewers welcome! >>> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:77 >>> + let canonicalPropertyNameB = WI.cssManager.canonicalNameForPropertyName(propertyNameB); >> >> canonicalNameForPropertyName basically removes -webkit- and -apple- prefixes from a given string. Why do we need to use it here? > > I did this for two reasons: > 1. I'd imagine that having both "-webkit-X" and "X" means that "X" is counted higher in both circumstances (with a bias towards the unprefixed version), which leads directly into (2) > 2. if the property becomes unprefixed, all of the counts for the prefixed version will count towards the unprefixed version, meaning that there should be no observable difference Makes sense!
Created attachment 356633 [details] Patch
Comment on attachment 356633 [details] Patch Attachment 356633 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/10281544 New failing tests: inspector/css/css-property.html
Created attachment 356646 [details] Archive of layout-test-results from ews106 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 356633 [details] Patch Attachment 356633 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/10281982 New failing tests: inspector/css/css-property.html webaudio/biquad-lowpass.html
Created attachment 356648 [details] Archive of layout-test-results from ews100 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 356633 [details] Patch Attachment 356633 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/10281659 New failing tests: inspector/css/css-property.html
Created attachment 356652 [details] Archive of layout-test-results from ews117 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 356657 [details] Patch Don't fall back to 0 if no previous count exists when updating
Comment on attachment 356657 [details] Patch r- because the math seems to be wrong. View in context: https://bugs.webkit.org/attachment.cgi?id=356657&action=review > Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:84 > + let existing = WI.CSSProperty.propertyNameUsage(property); It isn't trivial to guess what `propertyNameUsage` returns. Maybe `getUsageCount` or `getPropertyNameUsageCount` would be a better name. let existingCount = WI.CSSProperty.getPropertyNameUsageCount(property); > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:393 > + let index = completions.minIndex(WI.CSSProperty.sortByPropertyNameUsage); This method is pretty dense and hard to follow. To understand what it does, I had to read it once, read `minIndex` and `WI.CSSProperty.sortByPropertyNameUsage`, come back and read it a couple more times. `index` of what? You should either add more code comments or make variable names more descriptive (e.g. mostUsedPropertyIndex). > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:394 > + let usage = completions.reduce((accumulator, current) => accumulator + WI.CSSProperty.propertyNameUsage(current), 0); Nit: `totalUsage`. Nit: I'd add an `averageUsage` variable as well. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:399 > + if (WI.CSSProperty.propertyNameUsage(completions[index]) >= (usage / completions.length) * 0.9) > + return index; This math seems questionable. Let's say we have two completion items with the following usage counts 100 and 101. average = 100.5 100.5 * 0.9 = 90.45 101 >= 90.45 Did you mean if (WI.CSSProperty.propertyNameUsage(completions[index]) * 0.9 >= (usage / completions.length)) ? > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:402 > + return 0; // Select the first item Nit: this comment is obvious and unnecessary.
Comment on attachment 356657 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=356657&action=review >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:399 >> + return index; > > This math seems questionable. > > Let's say we have two completion items with the following usage counts 100 and 101. > > average = 100.5 > 100.5 * 0.9 = 90.45 > 101 >= 90.45 > > Did you mean > > if (WI.CSSProperty.propertyNameUsage(completions[index]) * 0.9 >= (usage / completions.length)) > > ? Please provide an example when falling back to the first item makes sense. I image a case when there are 3 items on the list. All close to average, but the 1st is used the least — falling back to it doesn't make sense to me.
(In reply to Nikita Vasilyev from comment #34) > I image a case when there are 3 items on the list. All close to average, but > the 1st is used the least — falling back to it doesn't make sense to me. *I imagine!
Comment on attachment 356657 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=356657&action=review >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:399 >> + return index; > > This math seems questionable. > > Let's say we have two completion items with the following usage counts 100 and 101. > > average = 100.5 > 100.5 * 0.9 = 90.45 > 101 >= 90.45 > > Did you mean > > if (WI.CSSProperty.propertyNameUsage(completions[index]) * 0.9 >= (usage / completions.length)) > > ? I had a whole bunch of reasoning for this, but after re-thinking it, I think we should just drop the 0.9 altogether. It's a valid assumption to say "we should only default-select the item that has been used _at least_ the average number of times for the given list". This would also "work" for your example of [100, 101]. Where this (both with and without the 0.9) still falls a bit short is the initial experience example (see comment 22), as values like [2, 1, 1] shouldn't really be considered "favored" or "significant" considering how few times they've been seen. A possible solution to this would be to just ignore these counts when they're below a certain threshold, but that seems somewhat arbitrary. Frankly, considering how "quick" I'd expect the user to get past the initial stage (any semi-large page would likely have enough properties within a few node clicks), I'm not sure how much effort it's worth to try to be smart about it, especially as that is the main delay in landing this as it is now. I think we should land the core logic and address the initial experience as a followup.
So far I'm not convinced this is better than simply selecting the first most commonly used property. Please provide a use case when this makes sense.
(In reply to Nikita Vasilyev from comment #37) > So far I'm not convinced this is better than simply selecting the first most commonly used property. Please provide a use case when this makes sense. What do you mean when you say "first most commonly used property"? If the user types "p", should we show a "padding-*" property or "position"? It may be possible that "padding-*" is used pretty frequently, but "position" may be used by every single element (e.g. making everything `position: relative;`). If we saw "padding-left" 100 times, and "position" 200 times, would you expect "padding-left" to be selected, or "position"? What about if its 100 and 101? What about if EVERY property has been seen 100 times? In all of those cases, I'd expect "position" to be selected as it's the most common. The issue I was speaking of in comment #36 was the initially poor experience, as EVERYTHING hasn't been seen yet, so the first few properties are _highly_ skewed towards favorability, even if those properties aren't actually used all that much (e.g. any user-agent style properties, like "-webkit-locale" or "-webkit-appearance"). I'm not sure how to tackle making false-positives not have a negative experience (e.g. "-webkit-appearance" being initially selected instead of the first item simply because the user selected a <button> or <input>).
Created attachment 357495 [details] Patch
(In reply to Devin Rousso from comment #39) > Created attachment 357495 [details] > Patch This looks good to me. I'd r+.
(In reply to Devin Rousso from comment #38) > (In reply to Nikita Vasilyev from comment #37) > > So far I'm not convinced this is better than simply selecting the first most commonly used property. Please provide a use case when this makes sense. > What do you mean when you say "first most commonly used property"? I meant that there could be two properties in the list with the same usage count. In this case, I'd select the alphabetically first one, that's all. This is what you're doing in your latest patch. > > If the user types "p", should we show a "padding-*" property or "position"? > It may be possible that "padding-*" is used pretty frequently, but > "position" may be used by every single element (e.g. making everything > `position: relative;`). If we saw "padding-left" 100 times, and "position" > 200 times, would you expect "padding-left" to be selected, or "position"? > What about if its 100 and 101? What about if EVERY property has been seen > 100 times? In all of those cases, I'd expect "position" to be selected as > it's the most common. Now that I think about it more, I'd want suggestions primarily based on properties I *type*. > > The issue I was speaking of in comment #36 was the initially poor > experience, as EVERYTHING hasn't been seen yet, so the first few properties > are _highly_ skewed towards favorability, even if those properties aren't > actually used all that much (e.g. any user-agent style properties, like > "-webkit-locale" or "-webkit-appearance"). I'm not sure how to tackle > making false-positives not have a negative experience (e.g. > "-webkit-appearance" being initially selected instead of the first item > simply because the user selected a <button> or <input>). Good observation! What if we don't count read-only properties (such as user agent styles of <button> or <input>)? This doesn't have to be in this patch, but it could.
(In reply to Nikita Vasilyev from comment #41) > Now that I think about it more, I'd want suggestions primarily based on properties I *type*. This was the main reason I wanted to have the heuristic be based on usage, not some static list. > Good observation! What if we don't count read-only properties (such as user agent styles of <button> or <input>)? This doesn't have to be in this patch, but it could. Along the lines of what I mentioned in comment #36, I think what is here now is a great starting point, and we can explore ways of making it better in future patches. Limiting the counting to user styles/attributes would definitely be a good improvement to make. For now, I'd like to get something landed sooner rather than later so that we can actually see how it feels and think about potential optimizations we can add in the future.
Comment on attachment 357495 [details] Patch Marking as r- to remove from the queue, given as there's no consensus.
Pull request: https://github.com/WebKit/WebKit/pull/1002
Committed r294862 (250994@main): <https://commits.webkit.org/250994@main> Reviewed commits have been landed. Closing PR #1002 and removing active labels.