1154371 - In Style Editor, autocomplete popup opens and is selected after pressing ',' in a selector list

Status: RESOLVED FIXED

(DevTools :: Style Editor, defect, P1)

Description

[Brian Grinstead [:bgrins]]

STR:
Open about:home
Open style editor
After the first selector, press ',' (so the text looks like '.searchSuggestionTable,'
Press enter

Expected:
The cursor moves to a new line

Actual:
The '.searchSuggestionTable' text becomes replaced with 'div'

Comment 1

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Setting as P1 because the user experience is severe. The style editor is a less-used tool, although we do have some evidence from dev edition launch data that people did try it out and then stopped using it as much after a period of time, and we could theorize that papercuts like this in the editing experience might contribute to that:

http://note.io/1IP9Mgh

Comment 2

[Brian Grinstead [:bgrins]]

Note: (I think?) it'd still probably be OK if the popup showed up after a space or newline, but it definitely shouldn't after pressing a comma

Comment 3

[Matteo Ferretti [:zer0] [:matteo]]

Attachment: comma.patch

Brian, not sure if it's the proper way to obtain the desired behavior: who you would suggest as reviewer to check if it's OK?

Comment 4

[Brian Grinstead [:bgrins]]

Comment on attachment 8605193 [details] [diff] [review]
comma.patch

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

Forwarding review to tromey.  Also, I'd like to see a test case somewhere for this.  I'm not sure if it would fit into one of the statemachine test cases in devtools/sourceeditor/test or if it will need to go in browser_editor_autocomplete_events.js.

Comment 5

[Tom Tromey :tromey]

Comment on attachment 8605193 [details] [diff] [review]
comma.patch

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

I would also like to see a test case.

I noticed that the bug also occurs if I type ", " and then enter.
I suspect that the proposed patch will not help in this case.

I wonder if it would be better to just require some explicit acceptance
of the possible completions -- either typing tab or using the arrow keys
to select one; and not have a completion selected by default when the
list pops up.

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to Tom Tromey :tromey from comment #5)
> I wonder if it would be better to just require some explicit acceptance
> of the possible completions -- either typing tab or using the arrow keys
> to select one; and not have a completion selected by default when the
> list pops up.

Taking that a step further, I wonder if we should be opening it up at all if there were no alphanumeric characters typed beforehand.  So it just wouldn't open at all for "," or ", " or ",\n".  You could always open it with ctrl+space if you explicitly want to see a non-filtered listing of possible entries.

Comment 7

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to Tom Tromey :tromey from comment #5)

> I would also like to see a test case.

I'll check devtools/sourceeditor/test and browser_editor_autocomplete_events.js as suggested by Brian for similar test case; and add one.
 
> I noticed that the bug also occurs if I type ", " and then enter.

I forgot to mention in the bug – I mentioned to Brian, that, in my case, I wasn't able to reproduce the original error. It means that in my case, without the patch applied, if I type "," after ".searchSuggestionTable", I got the popup but if I type enter, ".searchSuggestionTable" is not replaced by "div" – the first selector suggested – but becomes ".searchSuggestionTable,div".

Give that, the fact that the popup appears after ", " is the desired behavior (see comment 2), unless of course, the "div" is going to replace the whole ".searchSuggestionTable, ", as in the original bug's description.

So Brian, Tom, could you please double checked that you still have the original issue with the recent fx-team changes? If ".searchSuggestionTable" is completely replaced by "div"? Because if it's so, the only thing I can think is that we have a platform differences (and that would make this bug more nasty). I'm on OS X, by the way.

> I wonder if it would be better to just require some explicit acceptance
> of the possible completions -- either typing tab or using the arrow keys
> to select one; and not have a completion selected by default when the
> list pops up.

That was something I suggested in IRC as well, avoiding to have the first suggestion automatically selected in the popup, but we end up that not display anything – unless you type something else – it seems a better user experience – and after applied the patch it seems the case.

Comment 8

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to Brian Grinstead [:bgrins] from comment #6)

> Taking that a step further, I wonder if we should be opening it up at all if
> there were no alphanumeric characters typed beforehand.  So it just wouldn't
> open at all for "," or ", " or ",\n".

It's an option. However, once I applied the patch and I start to work a bit with that for testing purpose, I find this patch behavior very natural.

> You could always open it with ctrl+space if you explicitly want to see a 
> non-filtered listing of possible entries.

About that – and that's because I raised the platform thingy above – ctrl+space is not working on OS X ( I suspect due the spotlight binding?); but the docs reports that as shortcut for OS X too, so I'm a bit confused here.

Comment 9

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #8)

> About that – and that's because I raised the platform thingy above –
> ctrl+space is not working on OS X ( I suspect due the spotlight binding?);
> but the docs reports that as shortcut for OS X too, so I'm a bit confused
> here.

Oh wait, the one I found was under scratchpad section (in MDN) where, indeed, it works. It's not the case of Style Editor, thou. So we should probably uniform the behavior there as well – shall I file a bug for that? Unless there is a reason for such difference, of course.

Comment 10

[Brian Grinstead [:bgrins]]

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #7)
> (In reply to Tom Tromey :tromey from comment #5)
> 
> > I would also like to see a test case.
> 
> I'll check devtools/sourceeditor/test and
> browser_editor_autocomplete_events.js as suggested by Brian for similar test
> case; and add one.
>  
> > I noticed that the bug also occurs if I type ", " and then enter.
> 
> I forgot to mention in the bug – I mentioned to Brian, that, in my case, I
> wasn't able to reproduce the original error. It means that in my case,
> without the patch applied, if I type "," after ".searchSuggestionTable", I
> got the popup but if I type enter, ".searchSuggestionTable" is not replaced
> by "div" – the first selector suggested – but becomes
> ".searchSuggestionTable,div".
> 
> Give that, the fact that the popup appears after ", " is the desired
> behavior (see comment 2), unless of course, the "div" is going to replace
> the whole ".searchSuggestionTable, ", as in the original bug's description.
> 
> So Brian, Tom, could you please double checked that you still have the
> original issue with the recent fx-team changes? If ".searchSuggestionTable"
> is completely replaced by "div"? Because if it's so, the only thing I can
> think is that we have a platform differences (and that would make this bug
> more nasty). I'm on OS X, by the way.

Yes, the behavior has changed for me since the bug report.  Now it opens up the popup and ends up with this text after pressing enter .searchSuggestionTable,div.

> > I wonder if it would be better to just require some explicit acceptance
> > of the possible completions -- either typing tab or using the arrow keys
> > to select one; and not have a completion selected by default when the
> > list pops up.
> 
> That was something I suggested in IRC as well, avoiding to have the first
> suggestion automatically selected in the popup, but we end up that not
> display anything – unless you type something else – it seems a better user
> experience – and after applied the patch it seems the case.

If we populated this in the same way as ctrl+space (just showing everything available) it seems like it'd be fine.  I'm not sure how much work that's going to be.  Maybe it wouldn't be too hard.  If the previous text is whitespace, then don't preselect anything in the popup.  There may be some updates needed to make sure the popup behaves well with nothing selected.

Either way, I think we should proceed with the current approach - I don't think the popup should open (preselected or not) immediately after pressing a comma.  We can file a follow up for not preselecting anything (and/or not showing the popup) if the popup is opened with whitespace-only in front of it.

Comment 11

[Brian Grinstead [:bgrins]]

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #8)
> > You could always open it with ctrl+space if you explicitly want to see a 
> > non-filtered listing of possible entries.
> 
> About that – and that's because I raised the platform thingy above –
> ctrl+space is not working on OS X ( I suspect due the spotlight binding?);
> but the docs reports that as shortcut for OS X too, so I'm a bit confused
> here.

ctrl+space works for me in both scratchpad and the style editor.  But I don't think it works all the time in the style editor (like it doesn't seem to work for the first sheet on a bugzilla URL but it does on about:home).

Comment 12

[Tom Tromey :tromey]

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #7)

Ugh, sorry about the delay on this.

> So Brian, Tom, could you please double checked that you still have the
> original issue with the recent fx-team changes? If ".searchSuggestionTable"
> is completely replaced by "div"? Because if it's so, the only thing I can
> think is that we have a platform differences (and that would make this bug
> more nasty). I'm on OS X, by the way.

I'm on Linux.  I don't see the original bug -- for me the "div" is inserted,
but does not replace the ".searchSuggestionTable".

Comment 13

[Matteo Ferretti [:zer0] [:matteo]]

Attachment: comma.patch

I wasn't sure were to put the tests, because it seems partially related to both statemachine and browser_editor_autocomplete_events; I decided to add it to the last one.

Comment 14

[Tom Tromey :tromey]

Comment on attachment 8613903 [details] [diff] [review]
comma.patch

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

Looks good.

::: browser/devtools/sourceeditor/test/browser_editor_autocomplete_events.js
@@ +7,5 @@
>  const {InspectorFront} = require("devtools/server/actors/inspector");
>  const AUTOCOMPLETION_PREF = "devtools.editor.autocomplete";
>  const TEST_URI = "data:text/html;charset=UTF-8,<html><body><bar></bar><div id='baz'></div><body></html>";
>  
> +const wait = (delay) => new Promise(resolve => setTimeout(resolve, delay));

setTimeout means the test is susceptible to races on slow hardware.

In this case it doesn't seem too bad, because the failure mode would be a false positive, not a test timeout.

If there is some other way to write the test -- say a way to see what completions are computed and test that it is the empty list -- then that would be better.  Otherwise I think this is fine.

@@ +102,5 @@
> +  EventUtils.synthesizeKey(",", { }, win);
> +
> +  yield wait(500);
> +
> +  ok(!isPopupOpened, "Autocompletion shouldn't be openend");

Typo: s/openend/opened/

Comment 15

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to Tom Tromey :tromey from comment #14)

> setTimeout means the test is susceptible to races on slow hardware.

Yeah, I know. I don't like use timeouts in tests for that very reason; but I thought that in this specific case it was acceptable.

> In this case it doesn't seem too bad, because the failure mode would be a
> false positive, not a test timeout.
> 
> If there is some other way to write the test -- say a way to see what
> completions are computed and test that it is the empty list -- then that
> would be better.

We could probably test the autocompleter itself; but I'm not sure how to test the "partial" rule and which test could fit it. I didn't find any css autocompleter test similar to what we need here to take inspiration – I think that this specific case is something in the middle between the autocompleter and the source editor.

> Otherwise I think this is fine.

If you have suggest about, it's more than welcome! Otherwise I would land it as is, at least for the time being.

> > +  ok(!isPopupOpened, "Autocompletion shouldn't be openend");
> 
> Typo: s/openend/opened/

oh boy, thanks for the catch. :)

Comment 16

[Matteo Ferretti [:zer0] [:matteo]]

Attachment: comma v2

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/d597146748b9

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d597146748b9