Closed Bug 964939 Opened 6 years ago Closed 5 years ago

Clicking on an autocomplete suggestion in the style editor should select it

Categories

(DevTools :: Source Editor, defect)

defect
Not set

Tracking

(firefox39 fixed)

RESOLVED FIXED
Firefox 39
Tracking Status
firefox39 --- fixed

People

(Reporter: Optimizer, Assigned: bgrins)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

Right now, only (Shift + ) Tab allows you to complete to one of the suggestions in the popup.

We should allow mouse click suggestion insertion too.
Unassigning myself as I am not working on this right now.
Assignee: scrapmachines → nobody
Duplicate of this bug: 987964
Can someone could fix this anoing bug? Now autocompletion for CodeMirror is broken when click by mouse in autocompletion box. This aplay to native Web Devs Tools or other addons which use CodeMirror, like Stylish (https://forum.userstyles.org/discussion/comment/95470/#Comment_95470). Now this bug causes:

- can't put autocompletion words by click (we can only select this word but click destroy next action - red below)
- when select word in autocompletion box by mouse click we destroy Tab insert and can't put this word by Tab (or jump to next word by Tab)
- when autocompletion box appear we can't use other button without make doubleclicking - first click must close autocompletion box and next click do correct action << it's very tiring.
Brian: is this 'by design' in Codemirrror? Can we work around it?

My STR: 

1. open up the style editor
2. type in 'bo' in a stylesheet -> I see a completion list containing 'body'
3. click on 'body'

Result: nothing happens
Expected: completion happens

The workaround is to use the keyboard. For the same example:

1. open up the style editor on this bugzilla page, create a new stylesheet, type '.yui'
2. arrow down through the list of selectors starting with .yui
3. hit enter/return to complete the correct selector
Flags: needinfo?(bgrinstead)
This has nothing to do with CodeMirror, we just don't have an onClick listener set for the AutocompletePopup.  This should fix that - pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4bce4c3eef6
Flags: needinfo?(bgrinstead)
Attached patch autocomplete-click.patch (obsolete) — Splinter Review
I needed to allow for an undefined commandDispatcher in the _fireOnSelect method for richlistbox.xml.  In our test environment, which is a xul window opened via Services.ww.openWindow, this line was throwing an exception.

Dão, you show up in the hg log for richlistbox so I'm asking you for review on that file only.  (blame points to an import from CVS: https://hg.mozilla.org/mozilla-central/annotate/1722c4635fac/toolkit/content/widgets/richlistbox.xml#l80).
Assignee: nobody → bgrinstead
Attachment #8576854 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8577532 - Flags: review?(dao)
It's not clear to me why commandDispatcher would be missing there. Is this a bug?
With the patch applied, if you ./mach test browser/devtools/sourceeditor/test/browser_editor_autocomplete_events.js it should pass, but if you revert the change in richlistbox, it throws this error:

*************************
A coding exception was thrown in a Promise resolution callback.
See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise
Full message: TypeError: document.commandDispatcher is undefined
Full stack: _fireOnSelect@chrome://global/content/bindings/richlistbox.xml:81:15
clearSelection@chrome://global/content/bindings/listbox.xml:365:11
set_selectedIndex@chrome://global/content/bindings/listbox.xml:105:13
AutocompletePopup.prototype.selectedIndex@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/shared/autocomplete-popup.js:366:5
AP_clearItems@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/shared/autocomplete-popup.js:331:5
AP_setItems@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/shared/autocomplete-popup.js:244:5
autoComplete/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/sourceeditor/autocomplete.js:218:5
*************************

The window is being created here: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/sourceeditor/test/head.js#18-24.  If I assert ok(win.document.commandDispatcher) inside of the load event there it fails and is undefined.
(In reply to Dão Gottwald [:dao] from comment #7)
> It's not clear to me why commandDispatcher would be missing there. Is this a
> bug?

See comment 8
Flags: needinfo?(dao)
Attached patch autocomplete-click.patch (obsolete) — Splinter Review
Patrick, I know you haven't worked too much with the source editor but this is a pretty small change to the autocomplete component bundled with some test updates.  Feel free to redirect to someone else if you'd prefer.

There was already a click handler provided by the AutocompletePopup class, but we just weren't subscribing to it for the CSS autocomplete popups.  Note that js autocompletes had some different code that was handling the click, but is meant to be refactored at some stage to share the code for CSS popups.  For now I just made a function and call it in both places.

For tests, I copied the addTab/promiseTab functionality from the shared tests folder and made some changes to make it easier to use.  The sourceeditor tests all sill need some refactoring to be based on add_task, but not planning to tackle that here.
Attachment #8578155 - Flags: review?(pbrosset)
Summary: Should allow click to autocomplete in CSS autocompletion. → Clicking on an autocomplete suggestion in the style editor should select it
Comment on attachment 8578155 [details] [diff] [review]
autocomplete-click.patch

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

The main code changes in autocomplete.js seem good to me, even if I don't know the specifics of this widget, the new version seems close enough to the old one, and if all tests still pass, I'm fine with it.
I'd like maybe the new test to be rewritten a bit to be more consistent with our current test standards (add_task and promise yielding generator functions), unless it would make it too different from the other tests in the same dir. In which case, can you file a follow-up bug to refactor the tests in devtools/sourceeditor/test/?
Last, the change to richlistbox.xml looks like it belongs in the other patch perhaps? In any case, I can't review this one.

::: 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><b1></b1><b2></b2><body></html>";
> +
> +function test() {

I suggest using the new add_task test declaration helper:

add_task(function*() {
  yield promiseTab(TEST_URI);
  yield runTests();
});

@@ +8,5 @@
> +const AUTOCOMPLETION_PREF = "devtools.editor.autocomplete";
> +const TEST_URI = "data:text/html;charset=UTF-8,<html><body><b1></b1><b2></b2><body></html>";
> +
> +function test() {
> +  waitForExplicitFinish();

No need for waitForExplicitFinish if you use add_task, also, promiseTab seems to call it too.

@@ +12,5 @@
> +  waitForExplicitFinish();
> +  promiseTab(TEST_URI).then(runTests);
> +}
> +
> +function runTests() {

If you use add_task, you can yield runTests()
and therefore, you can make this a generator function that yields promises too:

function* runTests() {
  let target = devtools.TargetFactory.forTab(gBrowser.selectedTab);
  yield target.makeRemote();
  let inspector = InspectorFront(target.client, target.form);
  let walker = yield inspector.getWalker();
  let {ed, win} = yield setup(); // I suggest making setup return a promise too, when no callback param is passed.
  ...
}

@@ +31,5 @@
> +    });
> +  });
> +}
> +
> +function testKeyboard(ed, win) {

Same here, if you make runTests a generator function, this can be one too, that yields promises. No need to create promise.defer() anymore.

@@ +53,5 @@
> +
> +  return waitForSuggestion.promise;
> +}
> +
> +function testMouse(ed, win) {

Ditto.

::: toolkit/content/widgets/richlistbox.xml
@@ +75,5 @@
>              var event = document.createEvent("Events");
>              event.initEvent("select", true, true);
>              this.dispatchEvent(event);
>  
> +            if (document.commandDispatcher) {

I can't review this change.
Attachment #8578155 - Flags: review?(pbrosset)
(In reply to Brian Grinstead [:bgrins] from comment #9)
> (In reply to Dão Gottwald [:dao] from comment #7)
> > It's not clear to me why commandDispatcher would be missing there. Is this a
> > bug?
> 
> See comment 8

Doesn't really explain whether you're working around a bug or just dealing with expected behavior.
Flags: needinfo?(dao)
Comment on attachment 8577532 [details] [diff] [review]
autocomplete-click.patch

Got some help in #developers and it turns out that setting the mime type on the window to application/vnd.mozilla.xul+xml fixes the commandDispatcher issue.
Attachment #8577532 - Attachment is obsolete: true
Attachment #8577532 - Flags: review?(dao)
Got rid of the change to richlistbox.xml by updating the mime type on the data URL and updated the test as suggested.  It does leave this test different than the others, but it's clearly an improvement.  I'll file a follow up to convert the other sourceeditor tests to use add_task.

I just went ahead and had setup() always return a promise for now, and we can get rid of the callback parameter once we convert the rest of the tests.
Attachment #8578155 - Attachment is obsolete: true
Attachment #8578810 - Flags: review?(pbrosset)
Blocks: 1144264
Attachment #8578810 - Flags: review?(pbrosset) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/2f64916c94aa
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/2f64916c94aa
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 39
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.