Closed Bug 1168246 Opened 9 years ago Closed 8 years ago

UX - smarter suggestions for CSS properties in style editor and inspector

Categories

(DevTools :: Style Editor, defect, P2)

defect

Tracking

(firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: danemacmillan, Assigned: jdescottes, Mentored)

References

(Regressed 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [good next bug][lang=js][btpp-fix-later])

Attachments

(3 files, 8 obsolete files)

58 bytes, text/x-review-board-request
pbro
: review+
Details
58 bytes, text/x-review-board-request
pbro
: review+
Details
58 bytes, text/x-review-board-request
pbro
: review+
Details
This is nitpicky AF, but it would improve the UX in a minor way. I frequently use the "width" CSS property when debugging style in the browser. However, I have never used the "widows" CSS property. Nevertheless, it is always the first suggestion, which means it is never enough to just type "wid" and tab complete it; I need to type "widt," which is tantamount to typing the whole thing out anyway, so very often that is what happens. There is no denying how first-world-problem-y this reads, but like other developers who frequently debug CSS in the browser, I use "width" a lot, which means I verbosely type it out every time I need it.

It would be awesome if frequently used CSS properties were given more weight when suggested. Currently they are only suggested in alphabetical order.

Here is a screenshot that demonstrates this abominable, otherworldly nuisance: http://i.imgur.com/rHOfyLb.png
I completely agree, I've been bitten by this so many times, and yet never took the time to look into how to fix this.. it's one of those things you subconsciously learn to live with.
I seem to recall we had a bug about this already but can't find it now.
I wonder how far we should go with this: maintain an ordered list of all properties and values, or make something that learns which values you use most often? Or continue to suggest things in alphabetical order but flag certain often-used properties and values so they always end up at the top?
I'll do some research about how other devtools do it.
On ChromeDevTools:

Properties are always sorted alphabetically but there's always a default selected element in the list, whatever this list may be.
So if you type 't', the list contains (X shows which item is selected):

  tab-size
  table-layout
X text-align
  text-align-last
  ...
  top
  touch-action
  ...

But it's contextual to the list, so if I now enter 'o', the list becomes:

X top
  touch-action

I find this very nice, indeed, after I type 't' there are, say, 90% chances I mean text-align, but with 'to', I almost always want top.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: UX - smarter suggestions for CSS properties in style editor → UX - smarter suggestions for CSS properties in style editor and inspector
On Safari: no sorting or default selection mechanism at all, just like on Firefox.
On IE11: probably the worst of them all, typing 'wi' brings the whole list of properties that *contain* 'wi' in their names (so border-bottom-width is also in the list), sorted alphabetically, with the default selection being the first property that has 'wi' at the beginning of its name.
Chrome's auto-selecting is very convenient. I tested it after you wrote about its behaviour. With the "width" example, it auto-selects "width" from the moment "w" is typed; the only time it switches to "widows" is if I actually type "wido."

I tested to see if the auto-selected property could be updated by opting for "widows" every time I typed "w," but it did not. Mind you, I only ran a dozen tests, but at least anecdotally, it feels like the weight given to these auto-selected suggestions are hardcoded in, which I do not think is a negative in any way. Given that there are probably dozens of CSS properties no one will ever use, I think it is smart decision to hardcode some properties with more weight than others. Chrome's behaviour does not feel like it was trying to force my hand.

I do not think that hardcoding or flagging some properties in the shipped product would be a detriment or break anyone's experience with the tools; it would only improve it. It would likely also be the solution requiring the least amount of time, but would be very effective. It would also provide a base to build on, because I do not think it would prevent the future inclusion of some type of "learning" with regards to what properties users frequently choose.

To add your list of possible ways the devtools can suggest:

- Scan the loaded CSS and give priority to the ones already in the document, which will likely be most of the popular ones. I can see a performance overhead from doing that, though, so I could not really comment on the efficacy of this option. This would be similar to a suggestion I made in bug 1128058 regarding color values.
I forgot to include a screenshot of Chrome's behaviour: http://i.imgur.com/RVEHlwW.png
I investigated a little bit more, and here are some pointers to the code for anyone who wants to take a stab at fixing this:

- We need a new helper function that, given a list of properties, returns the index of the one that's the most used (see comment 7). Here's a WIP implementation: https://dl.dropboxusercontent.com/u/714210/most-used-props.js

- We need to make sure this is then used in the inspector (when editing a style attribute in the markup-view, or adding a new property in the rule-view).

2 components are involved: the inplace-editor and the autocomplete-popup

  - the inplace-editor is the field that appears when you edit an attribute or add a new property, that's what you type into. See inplace-editor.js, especially the _maybeSuggestCompletion function. This function builds a 'list' array which contains the things that will go into the autocomplete widget. So, somewhere in this function, we should be finding the index of the most used property, and using this index to prefill the field with the right value (search for setSelectionRange in this function). In this function, we should also pass this index to the autocomplete-popup widget so it knows which element to select by default (search for this.popup.setItems in this function).

  - the autocomplete-popup is the suggestion list that appears below the field. See autocomplete-popup.js, especially the setItems method that seems to always select the first item by default. Based on the changes made in the inplace-editor, we should be able to change this logic so it selects the right index instead.

- Finally, we need to make sure this is also used in the style-editor panel.

This panel is different as it uses a codeMirror editor (wrapped into a devtools sourceeditor wrapper, see /browser/devtools/sourceeditor/editor.js).
The interesting part (which generates the list of properties to be suggested) happens in css-autocompleter.js . There's a getCSSKeywords function that returns the whole list of properties, it is used in the completeProperties function, which is where we should also be finding the index of the most used property, and store it in the 'finalList'.
I think it should then be easy to make the rest work because the same autocomplete-popup widget is used.
Flagging as good *next* bug, this isn't an easy one, but I can help, and comment 8 should help get started.
Mentor: pbrosset
Whiteboard: [good next bug][lang=js]
If I wanted to have a crack at this, would you say the resources linked below are enough to get started? I pretty much just want to be sure I'm not going in with a blind spot; if you can expand on the recommended reading, if any, I'd really like that. I just want to make sure I'm reading relevant documentation. 

You tagged this report as a "good next bug," so whether or not I'm successful with it is not as important to me as actually wrapping my head around the development process here, and getting familiar with the codebase.

Resources:

- https://wiki.mozilla.org/DevTools/Hacking
- https://bugzilla.mozilla.org/show_bug.cgi?id=972655#c16 (I've had this bookmarked for months, waiting for some time to free up.)
(In reply to Dane MacMillan from comment #10)
> - https://wiki.mozilla.org/DevTools/Hacking
Yes, that's indeed the right place to get started.
This will redirect you to https://developer.mozilla.org/en-US/docs/Simple_Firefox_build, just make sure you install the pre-requisites for your platform and clone from http://hg.mozilla.org/integration/fx-team instead of https://hg.mozilla.org/mozilla-central , but the documentation says it all anyway.
> - https://bugzilla.mozilla.org/show_bug.cgi?id=972655#c16
Good of you to have bookmarked this, it's a good list of links to get started.

And, thanks for wanting to work on this!
I started work on this tonight, and I've got the Inspector panel component done. That leaves the Style panel component. I'll try and get a patch up soon-ish for initial review and feedback. I figure I'll mention this, so no one else picks it up.
Assignee: nobody → work
This patch resolves the first half of the bug. Namely, this is for ensuring the Inspector panel CSS property suggestions auto-select the most relevant property in the list for quicker tab completion.
Flags: needinfo?(pbrosset)
Attachment #8616520 - Flags: review+
Attachment #8616520 - Flags: feedback+
I also added an additional helper method to reduce the looping/checking redundancy in _maybeSuggestCompletion, which lightened up the method a bit.

Patrick, any and all feedback is definitely wanted. This is my first time writing code for Firefox, so I want to make sure I stay within the established conventions. Also, if you can suggest any tools or workflows that make debugging a bit more clear, if any, that'd be great. I found the terminal debug dumps massive, though eventually found a method to the madness.

Do you have any tips or tricks to the workflow? Right now I'm essentially writing code, rebuilding, and verifying changes. If there is some automated workflow I'm unaware, I'd love to know about it.

I also wanted to make a minor change to my patch, but realized my changes were removed from the "working tree," to speak Git, after creating the patch. Is it better to just commit and create patches from the commits? My mercurial experience is minimal.
I forgot to mention. I figure I would write a patch for the first half of the bug, and if all was okay, I would then wreak havoc on the Style panel component.
Comment on attachment 8616520 [details] [diff] [review]
relevant-css-property-select.patch

Hi Dane, I think you meant to flag pbrosset for feedback. Changed that for you :)

The reviewers usually r+ after a review of the code.
Flags: needinfo?(pbrosset)
Attachment #8616520 - Flags: review+
Attachment #8616520 - Flags: feedback?(pbrosset)
Attachment #8616520 - Flags: feedback+
Thanks, Gabriel. I thought I did, but I guess not.

@Patrick, something else I forgot to mention as one of the other reasons that compelled me to write the _getMostRelevantProperties method, was due to the fact that there was an inherent limitation with the suggestions.

What I mean is this: 1) The actual text string that populated the input, with part of its range selected, would be suggested from the list array, which contained all properties, and the first match found would be filled in. 2) The actual popup dropdown only suggested its contents from a limited subset of that list array. What that means is that both parts (1 & 2) were being built from two distinct data sources, which normally was not a problem because the first property in the alphabetized list array was being selected in the first. The problem becomes apparent when the first property from that popup dropdown is no longer being chosen first. For example, in the previous (existing) implementation, if the limit was 5 (instead of current 10), and I type "b"--the "background" suggestion will be highlighted in the popup dropdown, but "background-color" will be auto-filled in the input (this is because the list array had no filtering applied to it for this section, but still found the most relevant property).

I did not take a screenshot of the misbehaviour, but I took a screenshot of the correct behaviour, which I still elucidates the problem: http://imgur.com/a8KsKIY. Just imagine that instead of "background" being auto-filled, the input was auto-filled with "background-color" while the popup dropdown had "background" highlighted.

By ensuring that both sections of that code build their content from the same data set, the problem went away.
Comment on attachment 8616520 [details] [diff] [review]
relevant-css-property-select.patch

Review of attachment 8616520 [details] [diff] [review]:
-----------------------------------------------------------------

First of all, thanks Dane for the patch, I've applied it locally and it works really nicely, it feels so good to have this!
My main suggestion about the code is that I'd like to see a new separate module for CSS_PROPERTIES_MOST_USED and _getMostRelevantPropertyIndex (_getMostRelevantProperties doesn't look like it would belong in this new module though, better keep in in inplace-editor.js).

One way to do this:
- create a new file in /browser/devtools/shared, maybe named css-popular-properties.js
- reference it in /browser/devtools/shared/moz.build (just like node-attribute-parser.js)
- make sure you export the right functions from this module (exports.getMostRelevantPropertyIndex)
- you can then import it from other devtools modules (const {getMostRelevantPropertyIndex} = require("devtools/shared/css-popular-properties");)

This makes it more reusable and also more testable. You could, btw, create a new unit test in /browser/devtools/shared/test/unit. Tests in this directory are xpcshell types of tests (see https://developer.mozilla.org/en-US/docs/Mozilla/QA/Automated_testing and https://wiki.mozilla.org/DevTools/Hacking#Running_the_Developer_Tools_Tests)
Attachment #8616520 - Flags: feedback?(pbrosset) → feedback+
(In reply to Dane MacMillan from comment #17)
> Thanks, Gabriel. I thought I did, but I guess not.
> 
> @Patrick, something else I forgot to mention as one of the other reasons
> that compelled me to write the _getMostRelevantProperties method, was due to
> the fact that there was an inherent limitation with the suggestions.
> 
> What I mean is this: 1) The actual text string that populated the input,
> with part of its range selected, would be suggested from the list array,
> which contained all properties, and the first match found would be filled
> in. 2) The actual popup dropdown only suggested its contents from a limited
> subset of that list array. What that means is that both parts (1 & 2) were
> being built from two distinct data sources, which normally was not a problem
> because the first property in the alphabetized list array was being selected
> in the first. The problem becomes apparent when the first property from that
> popup dropdown is no longer being chosen first. For example, in the previous
> (existing) implementation, if the limit was 5 (instead of current 10), and I
> type "b"--the "background" suggestion will be highlighted in the popup
> dropdown, but "background-color" will be auto-filled in the input (this is
> because the list array had no filtering applied to it for this section, but
> still found the most relevant property).
> 
> I did not take a screenshot of the misbehaviour, but I took a screenshot of
> the correct behaviour, which I still elucidates the problem:
> http://imgur.com/a8KsKIY. Just imagine that instead of "background" being
> auto-filled, the input was auto-filled with "background-color" while the
> popup dropdown had "background" highlighted.
> 
> By ensuring that both sections of that code build their content from the
> same data set, the problem went away.
I think your changes make sense, I haven't looked at them for too long, but what I saw seemed clear to me and made the code better.
Alright, I've moved the code into a separate module. I want your feedback on the direction I went with it. It got me thinking about this in a more abstract sense.

CSS properties are just one kind of data that could benefit from more intelligent suggestions, but there are other good candidates as well. For example, this same concept of weight can be applied to other data, like JavaScript method suggestions (would be cool), or even pertinent CSS property values (e.g., bug 1128058). The patch I submitted is very self-explanatory, but this is it in summary:

- new suggest-meta.js module.
- suggest meta data of any sort can be defined.
- any defined evalutation can be performed to determine the relevancy of the provided set of data against the arbitrary meta data in the suggest-meta.js module.
- it gets instantiated: `let CssMeta = new SuggestMeta('css')`.
- then provided data is evaluated: `let bestIndex = CssMeta.getMostRelevantIndex(cssProperties);`.

I figured seeing as a whole new file, explicitly for CSS property evaluation was being created--and in a shared/common part of the devtools--it made sense to make the implementation more abstract to benefit other sections of the devtools, instead of looking like a CSS-only thing, should they ever want to make use of these kinds of evaluations. Again, the code itself is very self-explanatory.

Also, I don't seem to have the ability to flag individual people for feedback/reviews. I can add a "+" for review and feedback, but I cannot select a "requestee." I ended up not selecting any, because last time it emailed dozens of people.
Attachment #8616520 - Attachment is obsolete: true
Flags: needinfo?(pbrosset)
If all is good with the next round of feedback, I'll start integrating this functionality into the Style panel as well, unless you think that can be another ticket. If you think the patch as-is can be added just for the Inspector panel, then I will update the current patch with unit tests. What do you think?

I have to admit, I'm just asking this because I'm really excited to get this shipped into the latest devtools. I'm itching to use it. :D
(In reply to Dane MacMillan from comment #21)
> If all is good with the next round of feedback, I'll start integrating this
> functionality into the Style panel as well, unless you think that can be
> another ticket. If you think the patch as-is can be added just for the
> Inspector panel, then I will update the current patch with unit tests. What
> do you think?
> 
> I have to admit, I'm just asking this because I'm really excited to get this
> shipped into the latest devtools. I'm itching to use it. :D

I think a separate followup big is the typical pattern, just to keep the scope of any given bug as small as possible so it is easy to land.
Comment on attachment 8620782 [details] [diff] [review]
relevant-css-property-select.patch

Review of attachment 8620782 [details] [diff] [review]:
-----------------------------------------------------------------

That looks good!
Yeah, let's handle only the inspector for now, we can worry about the style-editor in a separate bug.
With regards to this patch, the approach is good, we only need a unit test for the new module, and probably fixing some tests in browser/devtools/markupview/test/ and browser/devtools/styleinspector/test/ that will fail because of the change, and then it's ready for a final review!

::: browser/devtools/shared/suggest-meta.js
@@ +5,5 @@
> +"use strict";
> +
> +/**
> + * SuggestMeta provides an abstraction for intelligently choosing suggestions.
> + *

I like this a lot, making this abstract is nice, there's already one clear other use case for this: the webconsole (it's very tedious right now when you want to autocomplete document.querySelectorAll and get queryCommandEnabled as the first suggestion).

I just have a few remarks further in the code, and also one about naming: I'm not sure about the term meta. This could mean a lot of things. How about this: suggestion-finder.js ? I'm bad at names, but I think we still need something a little more self-explanatory here.

@@ +27,5 @@
> + * over others.
> + *
> + * Example usage:
> + *  let CssMeta = new SuggestMeta('css');
> + *  let bestSuggestionIndex = CssMeta.getMostRelevantIndex(cssProperties);

I don't think there's a need right now to have this as a class that needs to be instantiated. Why not just a simple helper function:
getMostRelevantIndex(type, properties);

It would require callers to always pass the type, but I don't think that's too much of a downside, and I think it makes the code simpler/shorter for everyone.

I get that you might have done this so that it'd be easier to add new methods in the future, but we can always do that later.

@@ +34,5 @@
> + *        Tell constructor what suggest meta data to initialize.
> + *
> + * @constructor
> + */
> +function SuggestMeta(metaType)

I think metaType could in fact be an options object like:
suggestionOptions={type: String, weightedProperties: Array}

So that consumers can either pass a String type, or an array of weighted properties to use. So that we don't necessarily have to have all lists of properties in this module.

@@ +37,5 @@
> + */
> +function SuggestMeta(metaType)
> +{
> +  if (!metaType) {
> +    throw new Error("SuggestMeta must be instantiated with a metaType.");

Maybe also check that metaType is a valid type, and throw an error if not.

@@ +50,5 @@
> +
> +  // Determine which suggest meta data to compare against.
> +  this.metaWeight = (this.metaType in metaWeights)
> +    ? metaWeights[this.metaType]
> +    : [];

Not need for the empty array fallback if we ensure metaType is known.
Attachment #8620782 - Flags: feedback+
Patrick, I've implemented your feedback. I've also written the necessary unit tests. I looked to see if the modifications broke any pre-existing tests in the devtools (browser/devtools/markupview/test/ and browser/devtools/styleinspector/test/), but all of the devtools' xpcshell tests passed, including the new ones I added for the suggestion-picker.js module.

I ran these three commands to run the tests, and none of them failed with the modifications:

> ./mach xpcshell-test --tag devtools
> ./mach xpcshell-test browser/devtools/
> ./mach xpcshell-test browser/devtools/shared/

Output of tests:

> 1:05.89 LOG: MainThread INFO INFO | Result summary:
> 1:05.89 LOG: MainThread INFO INFO | Passed: 263
> 1:05.89 LOG: MainThread INFO INFO | Failed: 0
> 1:05.89 LOG: MainThread INFO INFO | Todo: 0
> 1:05.89 LOG: MainThread INFO INFO | Retried: 0
> 1:05.89 SUITE_END: MainThread
> Summary
> =======
> 
> Ran 266 tests
> Expected results: 263
> Unexpected results: 0
> Skipped: 3
> 
> OK
Attachment #8620782 - Attachment is obsolete: true
Flags: needinfo?(pbrosset)
Attachment #8622287 - Flags: review?(pbrosset)
Thanks for the new patch and for adding the new unit tests.
However, this patch breaks some of the "ui integration" tests (mochitest):

browser/devtools/markupview/test/browser_markupview_css_completion_style_attribute.js
browser/devtools/styleinspector/test/browser_ruleview_completion-existing-property_01.js
browser/devtools/styleinspector/test/browser_ruleview_completion-existing-property_02.js
browser/devtools/styleinspector/test/browser_ruleview_completion-new-property_01.js
browser/devtools/styleinspector/test/browser_ruleview_completion-new-property_02.js

You can run these with:
./mach mochitest path/to/test

You can also run all the tests in one dir:
./mach mochitest path/to/dir

Or all devtools mochitests:
./mach mochitest --subsuite devtools --tag devtools

More info here: https://wiki.mozilla.org/DevTools/Hacking#DevTools_Mochitests

Please investigate these failures (which are most probably due to the fact the these tests expect some other property to be selected by default).

I'll review the code changes now.
Comment on attachment 8622287 [details] [diff] [review]
relevant-css-property-select.patch

Review of attachment 8622287 [details] [diff] [review]:
-----------------------------------------------------------------

This looks nice. I like the new test and the code is easy to read.

I noted a bunch of minor code formatting things that make the new code different from most of the other code in devtools. It's not a big deal, but since you're re-visiting this patch to fix the other tests (see my previous comment) anyway, you might as well address these too. So, please take a look at this: https://wiki.mozilla.org/DevTools/CodingStandards
There are some guidelines here: https://wiki.mozilla.org/DevTools/CodingStandards#Code_style but it's easier to just run eslint to find these automatically.

I've also added a few comments below in the code

::: browser/devtools/shared/suggestion-picker.js
@@ +43,5 @@
> + *            passed, then built-in type will not be overridden, if one. Pass
> + *            an empty array to clear any built-in weights that match `type` in
> + *            the module already.
> + *
> + * @constructor

nit: we don't usually use the @constructor jsdoc tag.

@@ +50,5 @@
> +{
> +  if (!options
> +    || !options.type
> +    || typeof options.type != 'string'
> +    || !options.type.length

nit: we usually align these long conditions like this:

if (!options ||
    !options.type ||
    typeof options.type != "string" ||
    !options.type.length) {
  throw new Error ...
}

I have no preference, but it's better to make the code more consistent with the rest.

@@ +160,5 @@
> + * @param weightedItems {Array|null}
> + *        This is the same as the `weightedItems` parameter in SuggestionPicker.
> + *        This is optional. Null leaves built-in weightedItems intact.
> + */
> +function getMostRelevantIndex(type = '', items = [], weightedItems = null)

You should not default type to "" here, because this argument is mandatory, so if it's left undefined, it should reach the SuggestionPicker constructor as undefined, not as the empty string.

@@ +162,5 @@
> + *        This is optional. Null leaves built-in weightedItems intact.
> + */
> +function getMostRelevantIndex(type = '', items = [], weightedItems = null)
> +{
> +  let SuggestionPickerHelper = new SuggestionPicker({

nit: no first-cap for a variable name, we try to keep those for constructors only.

@@ +164,5 @@
> +function getMostRelevantIndex(type = '', items = [], weightedItems = null)
> +{
> +  let SuggestionPickerHelper = new SuggestionPicker({
> +    type: type,
> +    weightedItems: weightedItems

With es6, you can simplify this to:

let suggestionPickerHelper = new SuggestionPicker({type, weightedItems});

::: browser/devtools/shared/test/unit/test_suggestion-picker.js
@@ +116,5 @@
> +{
> +  do_print("Running ensureMostRelevantIndexProvidedByHelperFunction()");
> +
> +  for (let i = 0, l = TEST_DATA.length; i < l; i++) {
> +    let suggestionTestData = TEST_DATA[i];

Using a for-of loop and destructuring would make this loop simpler:

for (let {suggestions, mostRelevantSuggestion, mostRelevantIndex} of TEST_DATA) {
  ...
}

@@ +121,5 @@
> +
> +    let mostRelevantIndex = getMostRelevantIndex(
> +      'css:property',
> +      suggestionTestData.suggestions,
> +      suggestionTestData.mostRelevantSuggestion

Am I missing something here? I thought the third parameter was supposed to be an array of custom weighted properties to use, but here you're passing mostRelevantSuggestion which is a string that I don't think you should pass at all.

@@ +140,5 @@
> +{
> +  do_print("Running ensureMostRelevantIndexProvidedByClassMethod()");
> +
> +  for (let i = 0, l = TEST_DATA.length; i < l; i++) {
> +    let suggestionTestData = TEST_DATA[i];

Same remark about using a for..of loop
Attachment #8622287 - Flags: review?(pbrosset)
Blocks: 990800
Relevant to this: https://groups.google.com/forum/#!topic/mozilla.dev.platform/ktoEer_cdJA
tl;dr: we will start accumulating CSS property usage data in Telemetry.

Dane, are you still interested in working on this?
Flags: needinfo?(work)
Hi, Patrick--yes, I'm still interested. I've been away for some time. The feature is complete--I just need to write the tests. I will try to get this done in the next couple weeks.
Flags: needinfo?(work)
No longer blocks: 990800
Blocks: 1229671
Patrick, if it's not urgent, I will be able to finally get this finished before the end of the month. I really want this in.
(In reply to Dane MacMillan [:danemacmillan] from comment #29)
> Patrick, if it's not urgent, I will be able to finally get this finished
> before the end of the month. I really want this in.
Awesome! I was about the ping you earlier today :)
Priority: -- → P2
Whiteboard: [good next bug][lang=js] → [good next bug][lang=js][btpp-fix-later]
Hi Dane! Do you still want to finish this bug? Otherwise, I might pick it up :)
Flags: needinfo?(work)
Attached patch bug1168246.rebased.patch (obsolete) — Splinter Review
In the meantime, here is a rebased version that works with the current codebase.

About the suggestion list, in an autocomplete context, I feel "background" should rank higher than "background-color". It is more open ended, and still very easy to reach background-color ("b", "TAB", "-c", "TAB").
Taking the bug for now, as I started working on it.
Assignee: work → jdescottes
Status: NEW → ASSIGNED
Attached patch bug1168246.part1.v1.patch (obsolete) — Splinter Review
Made a few modifications on the original patch. Most importantly I modified the sorted list of properties. After checking the source used (https://www.chromestatus.com/metrics/css/popularity), the position of a property on the percentage of pages that have this property at least once.

This leads to some strange priorities, such as "cursor" having higher priority than "color". I checked the current Chrome devtools implementation and I think they use a mix of :
- https://www.chromestatus.com/metrics/css/popularity
- https://gist.github.com/NV/3751436

The second source actually counts the number of time a given property is used to compute the property weight. The new list makes a bit more sense in my opinion.
Attachment #8622287 - Attachment is obsolete: true
Attachment #8734938 - Attachment is obsolete: true
Attached patch bug1168246.part2.v1.patch (obsolete) — Splinter Review
Fix the existing tests.
(In reply to Julian Descottes [:jdescottes] from comment #35)
> (https://www.chromestatus.com/metrics/css/popularity), the position of a
> property on the percentage of pages that have this property at least once.

Read "the position of a property depends on the percentage of pages that have this property at least once."
Attached patch bug1168246.part1.v2.patch (obsolete) — Splinter Review
Attachment #8735293 - Attachment is obsolete: true
Attached patch bug1168246.part2.v2.patch (obsolete) — Splinter Review
Attachment #8735294 - Attachment is obsolete: true
Based on the original patch from danemacmillan.

* suggestion-picker.js
Add a new shared util to find the most popular css property in an array.
The list of popular css properties is extracted from chrome devtools code.
* autocomplete-popup.js
Can specify selected item index when opening the popup or setting items.
* inplace-editor.js
Use the suggestion-picker to select a default property.

Review commit: https://reviewboard.mozilla.org/r/42789/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42789/
Attachment #8735467 - Flags: review?(pbrosset)
Attachment #8735468 - Flags: review?(pbrosset)
Attachment #8735421 - Attachment is obsolete: true
Attachment #8735422 - Attachment is obsolete: true
Attachment #8735467 - Flags: review?(pbrosset)
Comment on attachment 8735467 [details]
MozReview Request: Bug 1168246 - part1: CSS autocomplete picks most popular prop;r=pbrosset

https://reviewboard.mozilla.org/r/42789/#review39449

Looking really good, thanks.
Cancelling review simply to have a discussion about my 4th comment. I can quickly R+ after that.

::: devtools/client/shared/autocomplete-popup.js:161
(Diff revision 1)
> +   * Select item at the provided index.
> +   *
> +   * @param {Number} index
> +   *        The position of the item to select.
> +   */
> +  selectItemAtIndex: function selectItemAtIndex(index)

Maybe I can convince you to add a third commit that gets rid of ESLint errors in this file? (and therefore not have to add this useless function name here)

::: devtools/client/shared/autocomplete-popup.js:164
(Diff revision 1)
> +   *        The position of the item to select.
> +   */
> +  selectItemAtIndex: function selectItemAtIndex(index)
> +  {
> +    if (typeof index != "undefined") {
> +      this.selectedIndex = Math.max(index, 0);

You're doing some input validation here by getting rid of negative number. Should we also do some more by defaulting to 0 if index is not a number?

::: devtools/client/shared/inplace-editor.js:1288
(Diff revision 1)
>        this._doValidation();
>      }, 0);
> +  },
> +
> +  /**
> +   * Display the suggestions popup

please add jsdoc for the params here.

::: devtools/client/shared/inplace-editor.js:1291
(Diff revision 1)
> +    // Resize and center the suggestions list to make sure the provided index is
> +    // displayed and the size is < MAX_POPUP_ENTRIES.
> +    let startIndex = 0;
> +    if (index >= MAX_POPUP_ENTRIES) {
> +      startIndex = index - Math.floor(MAX_POPUP_ENTRIES / 2);
> +    }
> +    let endIndex = startIndex + MAX_POPUP_ENTRIES;
> +    let popupList = suggestions.slice(startIndex, endIndex);

So, this might be outside of this bug, but we can potentially have somewhat weird behaviors with this. Imagine if there are more than MAX_POPUP_ENTRIES items in suggestions, with this we'd be cutting off some of the items in this list.

I don't know if that can practically happen though. I tested locally by making MAX_POPUP_ENTRIES=5 and typing "b" in the rule-view.
"border" gets selected by default, and "background" isn't visible.

Before this change, we were just selecting the first one always, so you'd always see the beginning of the list.

I think this is unrelated though and should be fixed in another bug where we make the suggestion popup either bigger (show more items), or show all items with a scrollbar if needed. 
Related: in our implementation, if you press <DOWN> several times, the selection will cycle in the *visible* list of suggestions, not in *all* suggestions, which feels wrong too.

So, I guess we should just make sure that we're not hiding important properties with this change. I don't think we are, and frankly, this makes the UX so much better anyway, that I don't think we should delay landing this. 
But if we are, we should discuss about fixing this either now or in a follow-up bug.

::: devtools/client/shared/suggestion-picker.js:52
(Diff revision 1)
> + * List based on the one used by Chrome devtools :
> + * https://code.google.com/p/chromium/codesearch#chromium/src/third_party/
> + * WebKit/Source/devtools/front_end/sdk/CSSMetadata.js&q=CSSMetadata&
> + * sq=package:chromium&type=cs&l=676
> + *
> + * The data is a mix of https://www.chromestatus.com/metrics/css and usage
> + * metrics from popular sites collected via https://gist.github.com/NV/3751436

Off-topic, but could you check if we can have metrics on javascript methods/properties too? I'm thinking it would be nice to have this for the auto-complete in the web console too.

::: devtools/client/shared/suggestion-picker.js:176
(Diff revision 1)
> +function findMostRelevantCssPropertyIndex(items) {
> +  return findMostRelevantIndex(items, SORTED_CSS_PROPERTIES);
> +}
> +
> +exports.findMostRelevantIndex = findMostRelevantIndex;
> +exports.SORTED_CSS_PROPERTIES = SORTED_CSS_PROPERTIES;

This doesn't seem to be imported anywhere outside of this module, it shouldn't be exported then I guess.
Comment on attachment 8735468 [details]
MozReview Request: Bug 1168246 - part2: fix tests relying on CSS suggestions order;r=pbrosset

https://reviewboard.mozilla.org/r/42791/#review39457
Attachment #8735468 - Flags: review?(pbrosset) → review+
https://reviewboard.mozilla.org/r/42789/#review39449

> Maybe I can convince you to add a third commit that gets rid of ESLint errors in this file? (and therefore not have to add this useless function name here)

Sure, I will add a part3 :)

> You're doing some input validation here by getting rid of negative number. Should we also do some more by defaulting to 0 if index is not a number?

Changed the implementation a bit (I will push to review soon).
In the last version, I simply get a default index if no index was provided (or was not a number).
"-1" should actually be accepted as a valid input.

> please add jsdoc for the params here.

Done

> So, this might be outside of this bug, but we can potentially have somewhat weird behaviors with this. Imagine if there are more than MAX_POPUP_ENTRIES items in suggestions, with this we'd be cutting off some of the items in this list.
> 
> I don't know if that can practically happen though. I tested locally by making MAX_POPUP_ENTRIES=5 and typing "b" in the rule-view.
> "border" gets selected by default, and "background" isn't visible.
> 
> Before this change, we were just selecting the first one always, so you'd always see the beginning of the list.
> 
> I think this is unrelated though and should be fixed in another bug where we make the suggestion popup either bigger (show more items), or show all items with a scrollbar if needed. 
> Related: in our implementation, if you press <DOWN> several times, the selection will cycle in the *visible* list of suggestions, not in *all* suggestions, which feels wrong too.
> 
> So, I guess we should just make sure that we're not hiding important properties with this change. I don't think we are, and frankly, this makes the UX so much better anyway, that I don't think we should delay landing this. 
> But if we are, we should discuss about fixing this either now or in a follow-up bug.

We were already cutting off items after MAX_POPUP_ENTRIES (additional exit condition in a for loop). Now we get as many items as possible based on the search query, then try to find the "best" match and center the list around it. I agree our implementation is bad, especially with only 10 items max allowed. I'd prefer to increase the number of items displayed in a separate Bug : https://bugzilla.mozilla.org/show_bug.cgi?id=1260419

[off-topic]
  There is one issue directly linked to this implementation though. 
  If the best suggestion is almost at the end of the list, endIndex will be greater than the
  suggestions' array length, and some more items will be cutoff. Example:
  - ["a", "b", "c", "d", "e", "f"]
  - "f" most relevant
  - max entries: 5
  => will display ["d", "e", "f"], but should display ["b", "c", "d", "e", "f"]
[off-topic]

I will restore the previous behavior: always show the beginning of the list. This means the most relevant item will not be always be displayed; if you type 'b' you will get 'background' instead of 'border'. When we fix Bug 1260419, this issue will go away.

> Off-topic, but could you check if we can have metrics on javascript methods/properties too? I'm thinking it would be nice to have this for the auto-complete in the web console too.

Don't know where to find it yet, but I agree it would be nice to have this for common DOM APIs too (getElementById, querySelector, etc...).
Comment on attachment 8735467 [details]
MozReview Request: Bug 1168246 - part1: CSS autocomplete picks most popular prop;r=pbrosset

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42789/diff/1-2/
Comment on attachment 8735468 [details]
MozReview Request: Bug 1168246 - part2: fix tests relying on CSS suggestions order;r=pbrosset

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42791/diff/1-2/
Attachment #8735855 - Flags: review?(pbrosset)
Comment on attachment 8735467 [details]
MozReview Request: Bug 1168246 - part1: CSS autocomplete picks most popular prop;r=pbrosset

https://reviewboard.mozilla.org/r/42789/#review39487
Attachment #8735467 - Flags: review?(pbrosset) → review+
Comment on attachment 8735855 [details]
MozReview Request: Bug 1168246 - part3: ES lint fixes for inplace-editor and autocomplete-popup;r=pbrosset

https://reviewboard.mozilla.org/r/42981/#review39489
Attachment #8735855 - Flags: review?(pbrosset) → review+
https://hg.mozilla.org/mozilla-central/rev/7aacc3495dc8
https://hg.mozilla.org/mozilla-central/rev/d8fb6dbbb478
https://hg.mozilla.org/mozilla-central/rev/484cdaff3121
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Related: Microsoft just released their CSS usage data: https://developer.microsoft.com/en-us/microsoft-edge/platform/usage/
Thanks for the info! I compiled the diff of their top 100 with our list :

[
  "-ms-text-size-adjust", 

  "-webkit-appearance", "-webkit-border-horizontal-spacing", "-webkit-border-vertical-spacing", "-webkit-font-smoothing", "-webkit-tap-highlight-color", 
  
  "backface-visibility", 
  
  "background-attachment", "background-position-x", "background-position-y", "background-repeat-x", "background-repeat-y",
  
  "border-bottom-style", "border-left-style", "border-right-style", "border-top-style",

  "font-size-adjust", "font-stretch",

  "orphans", 

  "outline-color", "outline-style", "outline-width",

  "page-break-after", "page-break-inside",

  "transition-delay", "transition-duration", "transition-property", "transition-timing-function",

  "widows"
]

Not worried about the prefixed ones. For background-* border-* font-* transition-* and outline-* we have the initial word in our list, which is what matters most.

Remains orphans, page-* and widows. I don't think we need to add them right now, but it's good to keep an eye on this.
(just to clarify, the properties I listed above are the ones we don't have in our list)
As I saw, against the bug's summary the smart suggestions are currently only available within the 'Rules' side panel but *not* within the Style Editor. Should I create a new bug for implementing the feature within the Style Editor?

Because Julian now finished this, I removed the needinfo for Dane.

Sebastian
Flags: needinfo?(work) → needinfo?(jdescottes)
See Also: → 1263097
Thanks for pointing that out!
Filed Bug 1263097 to do apply the same logic to the Style Editor.
Flags: needinfo?(jdescottes)
No longer blocks: 1229671
I've added a note on this: https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/Examine_and_edit_CSS#Edit_rules.

Please let me know if this covers it!
Flags: needinfo?(jdescottes)
Nice! Thanks Will.
Flags: needinfo?(jdescottes)
See Also: → 1270015
See Also: → 1270026
Depends on: 1270494
Product: Firefox → DevTools
Depends on: 1552606
No longer depends on: 1552606
Regressions: 1552606
You need to log in before you can comment on or make changes to this bug.