Julian Descottes [:jdescottes] Assignee
	Description • 8 years ago

STRs:
- open Inspector
- click in the rule view to create a new property
- type 'b'

Expected: User can see many properties.
Actual: User only sees 'backface-visibility' and several variants of background-*.

The autocomplete popup is currently limited to 10 items [1]. This limit is quite low and feels bad for learning/discovering properties, possible values. The popup should display more suggestions, using a scrollbar to keep its display small enough.

[1]https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/inplace-editor.js#36

	Patrick Brosset <:pbro>
	Comment 1 • 8 years ago

Triaging (filter on CLIMBING SHOES).

	Julian Descottes [:jdescottes] Assignee
	Comment 2 • 8 years ago

Now that bug 1168246 has landed, it would be nice to get this one in as soon as possible to make sure the best suggestion is always displayed.

	Julian Descottes [:jdescottes] Assignee
	Comment 3 • 8 years ago

Just a dirty prototype with the simplest fix possible: 
- increase the max of suggestions (actually keeping one as a safeguard, but that's it)
- reduce the max-height from 40em to 20em (I really feel 40em is too intrusive for an autocomplete popup)

My only concern here is the performance hit of building a much bigger list.
Want to test this out on older/slower hardware to see how it reacts. Otherwise it feels great, having access to all possible properties or values.

An alternative is to rewrite the autocomplete popup to render only the visible items. This would probably be the occasion to move it from XUL to HTML (or rather I would not engage the rewrite unless it's in HTML)

Quick landscape of the usage of the autocomplete popup today:
- inplace editor : limited to 10 suggestions
- inspector-search : limited to 15 suggestions
- sourceeditor autocomplete : no suggestion cap
- webconsole jsterm : no suggestion cap

	Julian Descottes [:jdescottes] Assignee
	Comment 6 • 8 years ago

Merging with Bug 1238747.

> [...] standard values will now be displayed, but they will still be after the "-moz-*" values.
> I think it makes sense to go beyond the basic alphabetical sort here and put prefixed values at the end
> of the list.
> 
> They are in general less popular than standard values and should not pollute the view.

	Julian Descottes [:jdescottes] Assignee
	Comment 8 • 8 years ago

Merging with Bug 1264631. 

!important should no longer be the default value suggested

	Julian Descottes [:jdescottes] Assignee
	Comment 9 • 8 years ago

Helen: I wanted to make a few adjustments to the order in which suggestions are displayed in the ruleview autocomplete:
- display all suggestions instead of just 10 (with a scrollbar and a slightly reduced max-height on the autocomplete popup)
- vendor-prefixed properties are moved to the end of the list (-moz- ...)
- !important is no longer the first suggested item

Any concern?

You should be able to grab builds on https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb9b958e9d6e89ba0ec2a21806c243f0637b2ce8 soon if you want to try it.

	Helen V. Holmes (:helenvholmes) (:✨)
	Comment 10 • 8 years ago

I pulled down one of the try-builds and noticed that the suggestion boxes were appearing dark. Seems like they're fine in dark theme—I assume this wasn't intentional, though. Any idea what's going on?

Other than that, the contents of the suggestion boxes seems great!

	Helen V. Holmes (:helenvholmes) (:✨)
	Comment 11 • 8 years ago

Seems like it's happening on the values too.

	Julian Descottes [:jdescottes] Assignee
	Comment 12 • 8 years ago

(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #10)
> Created attachment 8743527 [details]
> dark-suggestion-box.png
> 
> I pulled down one of the try-builds and noticed that the suggestion boxes
> were appearing dark. Seems like they're fine in dark theme—I assume this
> wasn't intentional, though. Any idea what's going on?
> 
> Other than that, the contents of the suggestion boxes seems great!

Thanks for testing! What you saw is definitely not intended :)
Which OS & build did you try?

	Helen V. Holmes (:helenvholmes) (:✨)
	Comment 13 • 8 years ago

(In reply to Julian Descottes [:jdescottes] from comment #12)
> Thanks for testing! What you saw is definitely not intended :)
> Which OS & build did you try?

I've gone through and re-downloaded a bunch of the try-builds and not been able to find the issue! I must conclude it was something that was only happening locally. I guess it was a red herring?

	Julian Descottes [:jdescottes] Assignee
	Comment 14 • 8 years ago

Usability changes to the ruleview autocomplete:
- max suggestions is now capped to 500 to make sure as many suggestions as possible
  as possible are displayed
- vendor-prefixed properties are moved to the end of the list
- !important is no longer the first suggested item

Review commit: https://reviewboard.mozilla.org/r/47669/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47669/

	Julian Descottes [:jdescottes] Assignee
	Comment 15 • 8 years ago

Try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=b05de625619f&selectedJob=19946061

	Patrick Brosset <:pbro>
	Comment 16 • 8 years ago

Comment on attachment 8744945 [details]
MozReview Request: Bug 1260419 - ruleview CSS autocomplete: more suggestions & better sorting;r=pbro

https://reviewboard.mozilla.org/r/47669/#review45793

Nice, thanks!
2 small things:
- the spacing between the scrollbar (when there is one) and the right edge of the dropdown is larger than the one above the scrollbar, making it look a bit misaligned (maybe that's only on windows, haven't tested elsewhere). This isn't a big deal, but I thought it was worth mentioned. Ok to move to another bug if needed.
- I can't use the scrollbar with the mouse, when I try to click/hold it, the dropdown goes away and the current value is selected. I guess this was expected and probably a bug that has always been here but just revealed by the 500 items cap. Also ok to move this to its own bug.

::: devtools/client/shared/inplace-editor.js:1274
(Diff revision 1)
>            if (match[1] == ":") {
>              // We are in CSS value completion
>              let propertyName =
>                query.match(/[;"'=]\s*([^"';:= ]+)\s*:\s*[^"';:=]*$/)[1];
>              list =
> -              ["!important;",
> +              ["!important",

I don't understand this change. Why did we have the `;` character before?

::: devtools/client/shared/inplace-editor.js:1325
(Diff revision 1)
>            break;
>          }
>        }
>  
> -      // Pick the best first suggestion from the provided list of suggestions.
> +      // Sort suggestions so that items starting with [a-z0-9] have a higher
> +      // priority than items starting with a special character.

Can you elaborate on this comment to explain that this includes vendor prefixed properties but also !important.

::: devtools/client/themes/common.css
(Diff revision 1)
> -:root[platform="linux"] .devtools-autocomplete-popup {
> -  max-height: 32rem;
>  }

Do you know why this was here before? It seems safe to get rid of it now that the height is 20rem, but might be worth looking at the blame history just to double check why it was added in the first place.

	Patrick Brosset <:pbro>
	Comment 17 • 8 years ago

This is the padding I mentioned in my last comment.

	Julian Descottes [:jdescottes] Assignee
	Comment 18 • 8 years ago

Comment on attachment 8744945 [details]
MozReview Request: Bug 1260419 - ruleview CSS autocomplete: more suggestions & better sorting;r=pbro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47669/diff/1-2/

	Julian Descottes [:jdescottes] Assignee
	Comment 19 • 8 years ago

https://reviewboard.mozilla.org/r/47669/#review45793

For the spacing issue I committed a change here. I'll wait for the try builds to test on Win7 and Linux. 
I will try to reproduce the scrollbar/click issue. It doesn't seem to occur on OSX, I'll try win7 and Linux. If I can't repro I might ask you to try under win8/10. (but I think I'll open a follow up to actually fix it).

> I don't understand this change. Why did we have the `;` character before?

I just reverted this. Having the ";"  here can make sense because we are autocompleting a style attribute from the markup view here.
So in theory we can safely append ";" to the "!important" suggestion. I don't think it's very useful but it's not the purpose of this bug.

> Can you elaborate on this comment to explain that this includes vendor prefixed properties but also !important.

Good point, fixed.

> Do you know why this was here before? It seems safe to get rid of it now that the height is 20rem, but might be worth looking at the blame history just to double check why it was added in the first place.

Good catch. I assumed it was just to have a smaller popup on Linux, but after tracking the original change : https://bugzilla.mozilla.org/show_bug.cgi?id=835899#c25 it's a bit more complicated. 

On Linux it looks like the root font size is a bit bigger than on other OSes. That's why we have a linux-only font-size: 80%; rule just a few lines above.
To remain consistent I updated it to 16rem on Linux and added a comment.

	Julian Descottes [:jdescottes] Assignee
	Comment 20 • 8 years ago

Try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=26632a7afca8

	Julian Descottes [:jdescottes] Assignee
	Comment 21 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/49053/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49053/

	Julian Descottes [:jdescottes] Assignee
	Comment 22 • 8 years ago

The "click on scrollbar hides the popup" issue is actually occurring on all OSX & Linux as well.
Fixing this behavior required a bit of change so I am moving it to a separate changeset. I don't think we should ship one without the other so let's keep everything in this bug.

The inplace editor listens to blur events fired by its input to:
- select an autocomplete suggestion
- commit and close the inlace editor

Clicking on the scrollbar of the autocomplete popup moves the focus to the popup and triggers this blur. 

Until now, clicking on the popup could only mean clicking on a selection. A blur would actually occur, picking the suggestion which just received the mousedown. In other words, today when we click a suggestion, it's actually the blur event which "picks" the suggestion, which is a bit weird.

The focus should remain at all times in the input. Here, we apply a custom classname on the autocomplete panel used by the inplace editor. This classname is used in CSS to remove the prevent focus on this autocomplete panel.

Since clicking the popup no longer triggers a blur, we need another mechanism to select a suggestion on click. Thankfully, the autocomplete-popup class supports a "onClick" option which is called when the listbox is clicked (which does *not* include the scrollbar). Here we reuse that with a small twist: normally the onClick handler is provided as a constructor argument to the autocomplete-popup. However the inplace-editor does not instantiate its autocomplete-popup, it is being injected. So we now have method to add and remove on click handlers dynamically.

I am not 100% sure about this approach, feel free to have a look if you want. Local tests are ok. Waiting for try before going further : https://treeherder.mozilla.org/#/jobs?repo=try&revision=659ddad0fbb5.

	Julian Descottes [:jdescottes] Assignee
	Comment 23 • 8 years ago

Comment on attachment 8745623 [details]
MozReview Request: Bug 1260419 - part2: do not hide inplace editor autocomplete popup when scrolling;r=pbro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49053/diff/1-2/

	Patrick Brosset <:pbro>
	Comment 24 • 8 years ago

Comment on attachment 8745623 [details]
MozReview Request: Bug 1260419 - part2: do not hide inplace editor autocomplete popup when scrolling;r=pbro

https://reviewboard.mozilla.org/r/49053/#review46267

I'd like to change add/removeOnClickHandler to on/off (event-emitter) unless there's big reason not to.
Code changes look good to me otherwise, not requesting another round of reviews.

::: devtools/client/shared/autocomplete-popup.js:150
(Diff revision 2)
>    /**
> +   * Attach a onClick handler that will be called when the richlistbox receives
> +   * a click event.
> +   * @param {Function} onClick
> +   *        A function that will be called when the richlistbox receives a
> +   *        click.
> +   */
> +  addOnClickHandler: function(onClickHandler) {
> +    this._list.addEventListener("click", onClickHandler, false);
> +  },
> +
> +  /**
> +   * If defined, remove the current onClick handler for the autocomplete popup.
> +   */
> +  removeOnClickHandler: function(onClickHandler) {
> +    this._list.removeEventListener("click", onClickHandler, false);
> +  },

I think it would be more consistent if the class was decorated as an event-emitter, and would always emit an event when a click occurred (with `this.emit("popup-clicked");`).

This way, consumers can do:
`this.popup.on("popup-clicked", this._onAutocompletePopupClick);`
and
`this.popup.off("popup-clicked", this._onAutocompletePopupClick);`

instead of having to use the custom event handler function `addOnClickHandler`.

But that means that you need a click listener at all times, even if it's not used. So up to you.

	Julian Descottes [:jdescottes] Assignee
	Comment 25 • 8 years ago

Comment on attachment 8744945 [details]
MozReview Request: Bug 1260419 - ruleview CSS autocomplete: more suggestions & better sorting;r=pbro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47669/diff/2-3/

	Julian Descottes [:jdescottes] Assignee
	Comment 26 • 8 years ago

Comment on attachment 8745623 [details]
MozReview Request: Bug 1260419 - part2: do not hide inplace editor autocomplete popup when scrolling;r=pbro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49053/diff/2-3/

	Julian Descottes [:jdescottes] Assignee
	Comment 27 • 8 years ago

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=941b2246b90c

	Pulsebot
	Comment 28 • 8 years ago

https://hg.mozilla.org/integration/fx-team/rev/7a4a32d82b15
https://hg.mozilla.org/integration/fx-team/rev/989b5ce5f7cf

	Ryan VanderMeulen [:RyanVM]
	Comment 29 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/7a4a32d82b15
https://hg.mozilla.org/mozilla-central/rev/989b5ce5f7cf

	Mohammad Maruf Rahman
	Comment 32 • 8 years ago

I have reproduced this bug with Nightly 48.0a1 (2016-03-29) on windows 7, 64 Bit!

This bug's fix is verified on latest developer edition, latest nightly !


Build ID   20160708004052
User Agent Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:49.0) Gecko/20100101 Firefox/49.0


Build ID   20160714030208
User Agent Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:50.0) Gecko/20100101 Firefox/50.0