Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

CSS Code Hinting as new extension for Brackets #2498

Merged
merged 7 commits into from
Jan 14, 2013
Merged

CSS Code Hinting as new extension for Brackets #2498

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1e78bf9
CSS Code Hinting as extension for Brackets
Jan 9, 2013
694edef
Changing initialSelection-mechanic to highlight only for primaryKeys,…
Jan 13, 2013
9820cd7
correcting comments in css code hinting and apply css-related naming …
Jan 14, 2013
b149fd5
Merge remote-tracking branch 'upstream/master' into andrez/csscodehin…
Jan 14, 2013
c494f4f
Let Enter/Tab also trigger the codehinting, but remove the hinting af…
Jan 14, 2013
500a306
removed code for debugging
Jan 14, 2013
1e09459
Minor naming changes
Jan 14, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Let Enter/Tab also trigger the codehinting, but remove the hinting af… 
…ter { due to different keyboardlayouts and thus different behaviour
  • Loading branch information
Andre Zoufahl committed Jan 14, 2013
commit c494f4fab6a3812be9504136a8f3ef28a2dbb87d
2 changes: 2 additions & 0 deletions src/editor/CodeHintManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -450,6 +450,8 @@ define(function (require, exports, module) {
} else if (_inSession(editor) && hintList.isOpen()) {
// Pass event to the hint list, if it's open
hintList.handleKeyEvent(event);
} else {
lastChar = String.fromCharCode(event.keyCode);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a suggestion by @redmunds to extend the triggering of hints by pressing 'Tab' or 'Enter'

}
} else if (event.type === "keypress") {
// Last inserted character, used later by handleChange
Expand Down
8 changes: 6 additions & 2 deletions src/extensions/default/CSSCodeHints/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ define(function (require, exports, module) {
*/
function CssAttrHints() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In CSS, the terminology is "properties" (not "attributes"), so please rename all references to be consistent. Also rename CSSAttributes.json.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rename these:

CssAttrHints     -> CssPropHints
attrHintProvider -> cssPropHintProvider

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

this.primaryTriggerKeys = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ-()";
this.secondaryTriggerKeys = " :;{";
//this.secondaryTriggerKeys = " :;{\t\n\r";
this.secondaryTriggerKeys = " :;\t\n\r";
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: I removed { from the secondaryTriggerKeys.
This has 2 motivations:

  1. On different keyboard-layouts { are produced differently. E.g. on german layout "Alt + 8" is used to produce {, while most of you may use "Shift + [" to produce it. So what's the issue, you may ask ? (see Issue 2539)
    Using german keyboard layout entering { would previously trigger the hintlist and immediately close it, due to current CHM.handleKeyEvent mechanic to work with keyup events.
    To ensure same behaviour on every system I've removed { from the list of triggerKeys, until this issue is adressed properly.
  2. Due to the small change in CHM.handleKeyEvent proposed by @redmunds the hints can now be triggered using 'Enter'.
    In other words for the following fragment
body {

CSS property-name hints would not pop up after you enter the { , but after you hit Enter to move the cursor into the next line.


/**
Expand All @@ -39,7 +40,8 @@ define(function (require, exports, module) {
var cursor = this.editor.getCursorPos();

this.info = CSSUtils.getInfoAtPos(editor, cursor);
// console.log(this.info);
this.info.caller = "hasHints";
console.log(this.info);

if (implicitChar === null) {
if (this.info.context === CSSUtils.PROP_NAME || this.info.context === CSSUtils.PROP_VALUE) {
Expand Down Expand Up @@ -74,6 +76,8 @@ define(function (require, exports, module) {
*/
CssAttrHints.prototype.getHints = function (implicitChar) {
this.info = CSSUtils.getInfoAtPos(this.editor, this.editor.getCursorPos());
this.info.caller = "getHints";
console.log(this.info);

var needle = this.info.name,
valueNeedle = "",
Expand Down
149 changes: 89 additions & 60 deletions src/extensions/default/CSSCodeHints/unittests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,19 +34,6 @@ define(function (require, exports, module) {
"} \n";

var testDocument, testEditor;

beforeEach(function () {
// create Editor instance (containing a CodeMirror instance)
var mock = SpecRunnerUtils.createMockEditor(defaultContent, "css");
testEditor = mock.editor;
testDocument = mock.doc;
});

afterEach(function () {
SpecRunnerUtils.destroyMockEditor(testDocument);
testEditor = null;
testDocument = null;
});

// Ask provider for hints at current cursor position; expect it to return some
function expectHints(provider) {
Expand All @@ -61,12 +48,10 @@ define(function (require, exports, module) {
expect(provider.hasHints(testEditor, null)).toBe(false);
}

// Expect hintList to contain attribute names, starting with given value
function verifyAttrHints(hintList, expectedFirstHint) {
expect(hintList.indexOf("div")).toBe(-1);
expect(hintList[0]).toBe(expectedFirstHint);
}


function selectHint(provider, expectedHint) {
var hintList = expectHints(provider);
Expand All @@ -83,27 +68,40 @@ define(function (require, exports, module) {

describe("CSS attributes in general (selection of correct attribute based on input)", function () {

it("should list all hints right after curly bracket", function () {
beforeEach(function () {
// create Editor instance (containing a CodeMirror instance)
var mock = SpecRunnerUtils.createMockEditor(defaultContent, "css");
testEditor = mock.editor;
testDocument = mock.doc;
});

afterEach(function () {
SpecRunnerUtils.destroyMockEditor(testDocument);
testEditor = null;
testDocument = null;
});

it("should list all prop-name hints right after curly bracket", function () {
testEditor.setCursorPos({ line: 4, ch: 11 }); // after {
var hintList = expectHints(CSSCodeHints.attrHintProvider);
verifyAttrHints(hintList, "align-content"); // filtered on "empty string"
});

it("should list all hints in new line", function () {
it("should list all prop-namehints in new line", function () {
testEditor.setCursorPos({ line: 5, ch: 1 });

var hintList = expectHints(CSSCodeHints.attrHintProvider);
verifyAttrHints(hintList, "align-content"); // filtered on "empty string"
});

it("should list all hints starting with 'b' in new line", function () {
it("should list all prop-name hints starting with 'b' in new line", function () {
testEditor.setCursorPos({ line: 6, ch: 2 });

var hintList = expectHints(CSSCodeHints.attrHintProvider);
verifyAttrHints(hintList, "backface-visibility"); // filtered on "b"
});

it("should list all hints starting with 'bord' ", function () {
it("should list all prop-name hints starting with 'bord' ", function () {
// insert semicolon after previous rule to avoid incorrect tokenizing
testDocument.replaceRange(";", { line: 6, ch: 2 });

Expand All @@ -112,7 +110,7 @@ define(function (require, exports, module) {
verifyAttrHints(hintList, "border"); // filtered on "bord"
});

it("should list all hints starting with 'border-' ", function () {
it("should list all prop-name hints starting with 'border-' ", function () {
// insert semicolon after previous rule to avoid incorrect tokenizing
testDocument.replaceRange(";", { line: 7, ch: 5 });

Expand All @@ -121,7 +119,7 @@ define(function (require, exports, module) {
verifyAttrHints(hintList, "border-bottom"); // filtered on "border-"
});

it("should list only hint border-color", function () {
it("should list only prop-name hint border-color", function () {
// insert semicolon after previous rule to avoid incorrect tokenizing
testDocument.replaceRange(";", { line: 8, ch: 8 });

Expand All @@ -131,66 +129,76 @@ define(function (require, exports, module) {
expect(hintList.length).toBe(1);
});

it("should list hints at end of existing attribute+value finished by ;", function () {
it("should list prop-name hints at end of property-value finished by ;", function () {
testEditor.setCursorPos({ line: 10, ch: 19 }); // after ;
var hintList = expectHints(CSSCodeHints.attrHintProvider);
verifyAttrHints(hintList, "align-content"); // filtered on "empty string"
});

it("should list hints right after curly bracket", function () {
testEditor.setCursorPos({ line: 4, ch: 11 }); // inside .selector, after {
expectHints(CSSCodeHints.attrHintProvider);
});

it("should NOT list hints right before curly bracket", function () {
it("should NOT list prop-name hints right before curly bracket", function () {
testEditor.setCursorPos({ line: 4, ch: 10 }); // inside .selector, before {
expectNoHints(CSSCodeHints.attrHintProvider);
});
it("should NOT list hints after declaration of mediatype", function () {

it("should NOT list prop-name hints after declaration of mediatype", function () {
testEditor.setCursorPos({ line: 0, ch: 15 }); // after {
expectNoHints(CSSCodeHints.attrHintProvider);
});

it("should NOT list prop-name hints if previous property is not closed properly", function () {
testEditor.setCursorPos({ line: 16, ch: 6 }); // cursor directly after color
expectNoHints(CSSCodeHints.attrHintProvider);
});
});


describe("CSS attribute handleSelect", function () {
it("should insert colon followed by whitespace after attribute", function () {

describe("CSS attribute insertHint", function () {
beforeEach(function () {
// create Editor instance (containing a CodeMirror instance)
var mock = SpecRunnerUtils.createMockEditor(defaultContent, "css");
testEditor = mock.editor;
testDocument = mock.doc;
});

afterEach(function () {
SpecRunnerUtils.destroyMockEditor(testDocument);
testEditor = null;
testDocument = null;
});

it("should insert colon prop-name selected", function () {
// insert semicolon after previous rule to avoid incorrect tokenizing
testDocument.replaceRange(";", { line: 6, ch: 2 });

testEditor.setCursorPos({ line: 7, ch: 5 }); // cursor after 'bord'
selectHint(CSSCodeHints.attrHintProvider, "border");
expect(testDocument.getLine(7)).toBe(" border:");
expectCursorAt({ line: 7, ch: 8 });
});

it("should insert semicolon followed by newline after value added", function () {
it("should insert semicolon followed by newline after prop-value selected", function () {
// insert semicolon after previous rule to avoid incorrect tokenizing
testDocument.replaceRange(";", { line: 12, ch: 5 });
testEditor.setCursorPos({ line: 13, ch: 10 }); // cursor after 'display: '
selectHint(CSSCodeHints.attrHintProvider, "block");
expect(testDocument.getLine(13)).toBe(" display: block;");
});

it("should insert attribute directly after semicolon ", function () {
it("should insert prop-name directly after semicolon", function () {
testEditor.setCursorPos({ line: 10, ch: 19 }); // cursor after red;
selectHint(CSSCodeHints.attrHintProvider, "align-content");
expect(testDocument.getLine(10)).toBe(" border-color: red;align-content:");
});

it("should insert nothing if previous property not closed properly", function () {
testEditor.setCursorPos({ line: 16, ch: 6 }); // cursor directly after color
expectNoHints(CSSCodeHints.attrHintProvider);
});

it("should insert nothing but the closure if propertyvalue is already complete", function () {
it("should insert nothing but the closure(semicolon) if prop-value is fully written", function () {
testDocument.replaceRange(";", { line: 15, ch: 13 }); // insert text ;
testEditor.setCursorPos({ line: 16, ch: 6 }); // cursor directly after color
selectHint(CSSCodeHints.attrHintProvider, "color");
expect(testDocument.getLine(16)).toBe(" color:");
expectCursorAt({ line: 16, ch: 7 });
});

xit("should start new selection whenever there is a whitespace to last stringliteral", function () {
xit("should start new hinting whenever there is a whitespace last stringliteral", function () {
// topic: multi-value properties
// this needs to be discussed, whether or not this behaviour is aimed for
// if so, changes to CSSUtils.getInfoAt need to be done imho to classify this
testDocument.replaceRange(" ", { line: 16, ch: 6 }); // insert whitespace after color
Expand All @@ -201,8 +209,21 @@ define(function (require, exports, module) {
});
});

describe("CSS attribute value hints", function () {
it("should list all display-values after colon", function () {
describe("CSS prop-value hints", function () {
beforeEach(function () {
// create Editor instance (containing a CodeMirror instance)
var mock = SpecRunnerUtils.createMockEditor(defaultContent, "css");
testEditor = mock.editor;
testDocument = mock.doc;
});

afterEach(function () {
SpecRunnerUtils.destroyMockEditor(testDocument);
testEditor = null;
testDocument = null;
});

it("should list all prop-values for 'display' after colon", function () {
// insert semicolon after previous rule to avoid incorrect tokenizing
testDocument.replaceRange(";", { line: 12, ch: 5 });

Expand All @@ -211,7 +232,7 @@ define(function (require, exports, module) {
verifyAttrHints(hintList, "block"); // filtered after "display:"
});

it("should list all display-values after colon and whitespace", function () {
it("should list all prop-values for 'display' after colon and whitespace", function () {
// insert semicolon after previous rule to avoid incorrect tokenizing
testDocument.replaceRange(";", { line: 12, ch: 5 });

Expand All @@ -220,7 +241,7 @@ define(function (require, exports, module) {
verifyAttrHints(hintList, "block"); // filtered after "display: "
});

it("should list all display-values after colon and whitespace", function () {
it("should list all prop-values starting with 'in' for 'display' after colon and whitespace", function () {
// insert semicolon after previous rule to avoid incorrect tokenizing
testDocument.replaceRange(";", { line: 13, ch: 10 });

Expand All @@ -229,14 +250,14 @@ define(function (require, exports, module) {
verifyAttrHints(hintList, "inherit"); // filtered after "display: in"
});

it("should NOT list hints for unknown attribute", function () {
it("should NOT list prop-value hints for unknown prop-name", function () {
testEditor.setCursorPos({ line: 15, ch: 12 }); // at bordborder:
expectNoHints(CSSCodeHints.attrHintProvider);
});

});

describe("CSS attribute hint provider inside mixed htmlfiles", function () {
describe("CSS hint provider inside mixed htmlfiles", function () {
var defaultContent = "<html> \n" +
"<head><style>.selector{display: none;}</style></head> \n" +
"<body> <style> \n" +
Expand All @@ -257,37 +278,40 @@ define(function (require, exports, module) {
testDocument = mock.doc;
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might not need this beforeEach() function, because this same function is defined in outer scope.

Otherwise, there needs to be an afterEach() function to destroyMockEditor() (like you do above).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the MockEditormode is changed from "css" to "htmlmixed", thus this new function.
I didn't add afterEach() since this is the same for all describe-blocks and jasmine will call afterEach defined in the outer scope anyway.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both beforeEach() functions get called (as opposed to this beforeEach() overriding the function by the same name in the outer scope), so you need to keep beforeEach() and afterEach() balanced for the mock editor code. It might be cleanest to add afterEach() here (and below), and the move the other beforeEach()/afterEach() functions above to the inner scope(s).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.





it("should list hints right after curly bracket", function () {
afterEach(function () {
SpecRunnerUtils.destroyMockEditor(testDocument);
testEditor = null;
testDocument = null;
});

it("should list prop-name hints right after curly bracket", function () {
testEditor.setCursorPos({ line: 3, ch: 7 }); // inside body-selector, after {
expectHints(CSSCodeHints.attrHintProvider);
});

it("should list hints inside oneline styletags at start", function () {
it("should list prop-name hints inside single-line styletags at start", function () {
testEditor.setCursorPos({ line: 1, ch: 23 }); // inside style, after {
expectHints(CSSCodeHints.attrHintProvider);
});

it("should list hints inside oneline styletags after ;", function () {
it("should list prop-name hints inside single-line styletags after semicolon", function () {
testEditor.setCursorPos({ line: 1, ch: 37 }); // inside style, after ;
expectHints(CSSCodeHints.attrHintProvider);
});

it("should list hints inside multiline styletags with cursor in first line", function () {
it("should list prop-name hints inside multi-line styletags with cursor in first line", function () {
testEditor.setCursorPos({ line: 9, ch: 18 }); // inside style, after {
expectHints(CSSCodeHints.attrHintProvider);
});

it("should list hints inside multiline styletags with cursor in last line", function () {
it("should list prop-name hints inside multi-line styletags with cursor in last line", function () {
testEditor.setCursorPos({ line: 10, ch: 5 }); // inside style, after colo
var hintList = expectHints(CSSCodeHints.attrHintProvider);
verifyAttrHints(hintList, "color"); // filtered on "colo"
expect(hintList.length).toBe(1);
});

it("should NOT list hints between closed styletag and new opening style tag", function () {
it("should NOT list prop-name hints between closed styletag and new opening styletag", function () {
testEditor.setCursorPos({ line: 8, ch: 0 }); // right before <div
expectNoHints(CSSCodeHints.attrHintProvider);
});
Expand All @@ -303,9 +327,8 @@ define(function (require, exports, module) {
});

});


describe("CSS attribute hint provider in other filecontext (e.g. javascript)", function () {

describe("CSS hint provider in other filecontext (e.g. javascript)", function () {
var defaultContent = "function foobar (args) { \n " +
" /* do sth */ \n" +
" return 1; \n" +
Expand All @@ -317,6 +340,12 @@ define(function (require, exports, module) {
testDocument = mock.doc;
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might not need this beforeEach() function, because this same function is defined in outer scope.

Otherwise, there needs to be an afterEach() function to destroyMockEditor() (like you do above).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comment above (only that in this case mode is changed from "css" to "javascript"


afterEach(function () {
SpecRunnerUtils.destroyMockEditor(testDocument);
testEditor = null;
testDocument = null;
});

it("should NOT list hints after function declaration", function () {
testEditor.setCursorPos({ line: 0, ch: 24 }); // after { after function declaration
expectNoHints(CSSCodeHints.attrHintProvider);
Expand Down