Bug 156271

Summary:

Web Inspector: CSS autocomplete: suggestion hint should be the most commonly used property and not the alphabetically first one

Product:

WebKit

Reporter:

Nikita Vasilyev <nvasilyev>

Component:

Web Inspector

Assignee:

Devin Rousso <hi>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bburg, chris, eoconnor, ews-watchlist, graouts, hi, inspector-bugzilla-changes, rniwa, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

See Also:

https://bugs.webkit.org/show_bug.cgi?id=240986
https://bugs.webkit.org/show_bug.cgi?id=240994

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Archive of layout-test-results from ews113 for mac-sierra	none
Patch	none
Patch	none
Archive of layout-test-results from ews106 for mac-sierra-wk2	none
Archive of layout-test-results from ews100 for mac-sierra	none
Archive of layout-test-results from ews117 for mac-sierra	none
Patch	none
Patch	hi: review-

Nikita Vasilyev

Reported 2016-04-05 18:33:49 PDT

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".

Attachments
Patch (8.39 KB, patch) 2018-10-05 16:54 PDT, Devin Rousso	no flags	Details Formatted Diff Diff
Patch (15.16 KB, patch) 2018-10-10 21:45 PDT, Devin Rousso	no flags	Details Formatted Diff Diff
Patch (11.14 KB, patch) 2018-10-16 10:05 PDT, Devin Rousso	no flags	Details Formatted Diff Diff
Patch (17.62 KB, patch) 2018-11-01 09:49 PDT, Devin Rousso	no flags	Details Formatted Diff Diff
Patch (17.97 KB, patch) 2018-11-02 17:02 PDT, Devin Rousso	no flags	Details Formatted Diff Diff
Patch (80.30 KB, patch) 2018-11-05 13:35 PST, Devin Rousso	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews113 for mac-sierra (3.08 MB, application/zip) 2018-11-05 15:28 PST, EWS Watchlist	no flags	Details
Patch (80.33 KB, patch) 2018-11-23 17:58 PST, Devin Rousso	no flags	Details Formatted Diff Diff
Patch (81.08 KB, patch) 2018-12-05 11:56 PST, Devin Rousso	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews106 for mac-sierra-wk2 (3.34 MB, application/zip) 2018-12-05 13:00 PST, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews100 for mac-sierra (2.51 MB, application/zip) 2018-12-05 13:22 PST, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews117 for mac-sierra (1.98 MB, application/zip) 2018-12-05 13:45 PST, EWS Watchlist	no flags	Details
Patch (81.08 KB, patch) 2018-12-05 14:26 PST, Devin Rousso	no flags	Details Formatted Diff Diff
Patch (80.58 KB, patch) 2018-12-17 16:28 PST, Devin Rousso	hi: review-	Details Formatted Diff Diff
Show Obsolete (14) View All Add attachment proposed patch, testcase, etc.

Nikita Vasilyev

Comment 1 2016-04-06 16:23:06 PDT

Firefox DevTools started doing this: https://twitter.com/FirefoxDevTools/status/717365790010376193

Radar WebKit Bug Importer

Comment 2 2016-04-06 16:23:36 PDT

<rdar://problem/25588888>

Nikita Vasilyev

Comment 3 2017-11-17 16:25:13 PST

*** Bug 179830 has been marked as a duplicate of this bug. ***

Chris Chiera

Comment 4 2018-05-30 08:45:54 PDT

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!

Devin Rousso

Comment 5 2018-10-05 16:54:03 PDT

Created attachment 351706 [details] Patch

Nikita Vasilyev

Comment 6 2018-10-10 20:19:01 PDT

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.

Devin Rousso

Comment 7 2018-10-10 20:44:31 PDT

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.

Devin Rousso

Comment 8 2018-10-10 21:45:19 PDT

Created attachment 352014 [details] Patch Updated patch with localStorage and tests

Nikita Vasilyev

Comment 9 2018-10-15 15:35:29 PDT

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.

Devin Rousso

Comment 10 2018-10-16 10:05:54 PDT

Created attachment 352469 [details] Patch

Blaze Burg

Comment 11 2018-11-01 09:35:42 PDT

(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?

Devin Rousso

Comment 12 2018-11-01 09:49:35 PDT

Created attachment 353610 [details] Patch Rebase

Nikita Vasilyev

Comment 13 2018-11-02 11:23:28 PDT

Comment on attachment 353610 [details] Patch r- because newly added properties don't get counted. Discussed with Devin offline.

Devin Rousso

Comment 14 2018-11-02 17:02:53 PDT

Created attachment 353746 [details] Patch

Nikita Vasilyev

Comment 15 2018-11-03 14:55:56 PDT

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.

Nikita Vasilyev

Comment 16 2018-11-03 15:10:42 PDT

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.

Devin Rousso

Comment 17 2018-11-04 12:43:18 PST

(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.

Devin Rousso

Comment 18 2018-11-05 13:35:23 PST

Created attachment 353903 [details] Patch

EWS Watchlist

Comment 19 2018-11-05 15:28:51 PST Comment hidden (obsolete)

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

EWS Watchlist

Comment 20 2018-11-05 15:28:53 PST Comment hidden (obsolete)

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

Devin Rousso

Comment 21 2018-11-23 17:58:25 PST

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.

Nikita Vasilyev

Comment 22 2018-12-03 15:54:49 PST

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.

Devin Rousso

Comment 23 2018-12-03 16:11:29 PST

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.

Nikita Vasilyev

Comment 24 2018-12-03 16:33:48 PST

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!

Devin Rousso

Comment 25 2018-12-05 11:56:48 PST

Created attachment 356633 [details] Patch

EWS Watchlist

Comment 26 2018-12-05 13:00:54 PST Comment hidden (obsolete)

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

EWS Watchlist

Comment 27 2018-12-05 13:00:56 PST Comment hidden (obsolete)

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

EWS Watchlist

Comment 28 2018-12-05 13:22:28 PST Comment hidden (obsolete)

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

EWS Watchlist

Comment 29 2018-12-05 13:22:30 PST Comment hidden (obsolete)

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

EWS Watchlist

Comment 30 2018-12-05 13:45:20 PST Comment hidden (obsolete)

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

EWS Watchlist

Comment 31 2018-12-05 13:45:22 PST Comment hidden (obsolete)

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

Devin Rousso

Comment 32 2018-12-05 14:26:29 PST

Created attachment 356657 [details] Patch Don't fall back to 0 if no previous count exists when updating

Nikita Vasilyev

Comment 33 2018-12-14 16:41:16 PST

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.

Nikita Vasilyev

Comment 34 2018-12-14 16:56:53 PST

Nikita Vasilyev

Comment 35 2018-12-14 16:59:07 PST

(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!

Devin Rousso

Comment 36 2018-12-14 17:16:12 PST

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.

Nikita Vasilyev

Comment 37 2018-12-16 16:01:13 PST

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.

Devin Rousso

Comment 38 2018-12-16 19:25:37 PST

(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>).

Devin Rousso

Comment 39 2018-12-17 16:28:14 PST

Created attachment 357495 [details] Patch

Nikita Vasilyev

Comment 40 2018-12-17 18:12:44 PST

(In reply to Devin Rousso from comment #39) > Created attachment 357495 [details] > Patch This looks good to me. I'd r+.

Nikita Vasilyev

Comment 41 2018-12-17 18:32:34 PST

(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.

Devin Rousso

Comment 42 2018-12-19 23:02:28 PST

(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.

Devin Rousso

Comment 43 2019-08-23 10:16:55 PDT

Comment on attachment 357495 [details] Patch Marking as r- to remove from the queue, given as there's no consensus.

Devin Rousso

Comment 44 2022-05-24 19:22:52 PDT

Pull request: https://github.com/WebKit/WebKit/pull/1002

EWS

Comment 45 2022-05-25 19:17:41 PDT

Committed r294862 (250994@main): <https://commits.webkit.org/250994@main> Reviewed commits have been landed. Closing PR #1002 and removing active labels.

Note You need to log in before you can comment on or make changes to this bug.