Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

classList usage in dom.{add|remove|toggle}CssClass + minor changes #1237

Merged
merged 5 commits into from
Feb 22, 2013
Merged

classList usage in dom.{add|remove|toggle}CssClass + minor changes #1237

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ebe03bb
remove dom.setText, use classList, document.head
danyaPostfactum Feb 6, 2013
0d08edc
fix colon deletion behavior
danyaPostfactum Feb 6, 2013
bb9f98e
Detect file dragging
danyaPostfactum Feb 6, 2013
930b5cf
Fix tooltip webkit gradient, reset ajax.org logo border
danyaPostfactum Feb 6, 2013
62b7d8c
Strip legacy xhr code out of lib/net
danyaPostfactum Feb 6, 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
4 changes: 3 additions & 1 deletion demo/kitchen-sink/demo.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -397,7 +397,9 @@ bindCheckbox("highlight_token", function(checked) {

/************** dragover ***************************/
event.addListener(container, "dragover", function(e) {
return event.preventDefault(e);
var types = e.dataTransfer.types;
if (types && Array.prototype.indexOf.call(types, 'Files') !== -1)
return event.preventDefault(e);
});

event.addListener(container, "drop", function(e) {
Expand Down
1 change: 1 addition & 0 deletions demo/kitchen-sink/styles.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ body {
#logo {
padding: 15px;
margin-left: 70px;
border: none;
}

#editor-container {
Expand Down
6 changes: 2 additions & 4 deletions lib/ace/css/editor.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@

.ace_gutter-tooltip {
background-color: #FFF;
background-image: -webkit-linear-gradient(to bottom, transparent, rgba(0, 0, 0, 0.1));
background-image: -webkit-linear-gradient(top, transparent, rgba(0, 0, 0, 0.1));
background-image: linear-gradient(to bottom, transparent, rgba(0, 0, 0, 0.1));
border: 1px solid gray;
border-radius: 1px;
Expand Down Expand Up @@ -342,9 +342,7 @@
-webkit-box-shadow: 0 1px 1px rgba(255, 255, 255, 0.2);
box-shadow: 0 1px 1px rgba(255, 255, 255, 0.2);
}




.ace_fold-widget.ace_invalid {
background-color: #FFB4B4;
border-color: #DE5555;
Expand Down
1 change: 0 additions & 1 deletion lib/ace/edit_session.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ define(function(require, exports, module) {
var config = require("./config");
var oop = require("./lib/oop");
var lang = require("./lib/lang");
var net = require("./lib/net");
var EventEmitter = require("./lib/event_emitter").EventEmitter;
var Selection = require("./selection").Selection;
var TextMode = require("./mode/text").Mode;
Expand Down
132 changes: 73 additions & 59 deletions lib/ace/lib/dom.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,72 +39,86 @@ exports.createElement = function(tag, ns) {
document.createElement(tag);
};

exports.setText = function(elem, text) {
if (elem.innerText !== undefined) {
elem.innerText = text;
}
if (elem.textContent !== undefined) {
elem.textContent = text;
if (typeof DOMTokenList === 'function') {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, DOMTokenList in IE10 is object, not function... What detection should be used here?
I think ("classList" in document.documentElement) would be good enough?

Copy link
Member

Choose a reason for hiding this comment

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

see danyaPostfactum@c93d1e2#lib/ace/lib/dom.js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's wrong with classList ?

Copy link
Member

Choose a reason for hiding this comment

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

nothing's wrong, but see https://github.com/ajaxorg/pilot/commit/7a1651f87af08b0523e6c7cf034a1924ab3d48f3#lib/pilot/dom.js, c93d1e2#commitcomment-793749

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, thanks. May be he is right. Ace calls these methods hardly ever ( but every mouse movement on gutter do it) to fight for every split millisecond. Also, we can not drop this code on dropping IE8. Anyway, I'll prepare jsperf test.

Copy link
Member

Choose a reason for hiding this comment

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

Gutter started to use hasCssClass after that discussion, so if classList is significantly faster it might be worth to use it. (I remember it was much faster on ff 3.6, but string performance have improved since then).
Should i cherrypick this now, and we could look into classList later?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

classList is not much faster as I know.

exports.hasCssClass = function(el, name) {
return el.classList.contains(name);
};

exports.addCssClass = function(el, name) {
el.classList.add(name);
};

exports.removeCssClass = function(el, name) {
el.classList.remove(name);
}
};

exports.hasCssClass = function(el, name) {
var classes = el.className.split(/\s+/g);
return classes.indexOf(name) !== -1;
};
exports.toggleCssClass = function(el, name) {
el.classList.toggle(name);
}

/*
* Add a CSS class to the list of classes on the given node
*/
exports.addCssClass = function(el, name) {
if (!exports.hasCssClass(el, name)) {
el.className += " " + name;
exports.setCssClass = function(el, name, include) {
el.classList[include ? 'add' : 'remove'](name);
}
};
}
else {
exports.hasCssClass = function(el, name) {
var classes = el.className.split(/\s+/g);
return classes.indexOf(name) !== -1;
};

/*
* Remove a CSS class from the list of classes on the given node
*/
exports.removeCssClass = function(el, name) {
var classes = el.className.split(/\s+/g);
while (true) {
var index = classes.indexOf(name);
if (index == -1) {
break;
/*
* Add a CSS class to the list of classes on the given node
*/
exports.addCssClass = function(el, name) {
if (!exports.hasCssClass(el, name)) {
el.className += " " + name;
}
classes.splice(index, 1);
}
el.className = classes.join(" ");
};
};

exports.toggleCssClass = function(el, name) {
var classes = el.className.split(/\s+/g), add = true;
while (true) {
var index = classes.indexOf(name);
if (index == -1) {
break;
/*
* Remove a CSS class from the list of classes on the given node
*/
exports.removeCssClass = function(el, name) {
var classes = el.className.split(/\s+/g);
while (true) {
var index = classes.indexOf(name);
if (index == -1) {
break;
}
classes.splice(index, 1);
}
add = false;
classes.splice(index, 1);
}
if(add)
classes.push(name);
el.className = classes.join(" ");
};

el.className = classes.join(" ");
return add;
};
exports.toggleCssClass = function(el, name) {
var classes = el.className.split(/\s+/g), add = true;
while (true) {
var index = classes.indexOf(name);
if (index == -1) {
break;
}
add = false;
classes.splice(index, 1);
}
if(add)
classes.push(name);

/*
* Add or remove a CSS class from the list of classes on the given node
* depending on the value of <tt>include</tt>
*/
exports.setCssClass = function(node, className, include) {
if (include) {
exports.addCssClass(node, className);
} else {
exports.removeCssClass(node, className);
}
};
el.className = classes.join(" ");
return add;
};

/*
* Add or remove a CSS class from the list of classes on the given node
* depending on the value of <tt>include</tt>
*/
exports.setCssClass = function(node, className, include) {
if (include) {
exports.addCssClass(node, className);
} else {
exports.removeCssClass(node, className);
}
};
}

exports.hasCssString = function(id, doc) {
var index = 0, sheets;
Expand Down Expand Up @@ -143,7 +157,7 @@ exports.importCssString = function importCssString(cssText, id, doc) {
if (id)
style.id = id;

var head = doc.getElementsByTagName("head")[0] || doc.documentElement;
var head = doc.head || doc.getElementsByTagName("head")[0] || doc.documentElement;
head.appendChild(style);
}
};
Expand All @@ -156,7 +170,7 @@ exports.importCssStylsheet = function(uri, doc) {
link.rel = 'stylesheet';
link.href = uri;

var head = doc.getElementsByTagName("head")[0] || doc.documentElement;
var head = doc.head || doc.getElementsByTagName("head")[0] || doc.documentElement;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Document without a head? Is it possible ? How ?

Copy link
Member

Choose a reason for hiding this comment

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

document.documentElement.removeChild(document.head), also xul documents

head.appendChild(link);
}
};
Expand Down Expand Up @@ -273,7 +287,7 @@ exports.getInnerText = function(el) {
if (document.body && "textContent" in document.body)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why not just test el.textContent ?

Copy link
Member

Choose a reason for hiding this comment

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

i'd say el.textContent || el.innerText || "" would be ok unless it throws on ie8 or something

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you please explain what do you mean?

Copy link
Member

Choose a reason for hiding this comment

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

I thought to replace whole thing with one line, but it can't be done, because el.textContent can be "", maybe use if ("textContent" in el)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we do this way:

if ("textContent" in document.documentElement) {
    exports.setInnerText = function(el, innerText) {
        el.textContent = innerText;
    };

    exports.getInnerText = function(el) {
        return el.textContent;
    };
}
else {
    exports.setInnerText = function(el, innerText) {
        el.innerText = innerText;

    };

    exports.getInnerText = function(el) {
        return el.innerText;
    };
}

or at least:

exports.setInnerText = function(el, innerText) {
    if ("textContent" in el)
        el.textContent = innerText;
    else
        el.innerText = innerText;
};

exports.getInnerText = function(el) {
    if ("textContent" in el)
        return el.textContent;
    else
         return el.innerText;
};

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By the way, textContent works on Webkit much slower then innerText. But I'm not sure innerText is future-proof:
http://wiki.whatwg.org/wiki/Specs_todo :

innerText and outerText, if browsers don't remove them entirely

Copy link
Member

Choose a reason for hiding this comment

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

i like the first one

if ("textContent" in document.documentElement) {
    exports.setInnerText = function(el, innerText) {
....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another way:

var textProperty = ("innerText" in document.documentElement) ? "innerText" : "textContent";

exports.setInnerText = function(el, innerText) {
    el[textProperty] = innerText;

};

exports.getInnerText = function(el) {
    return el[textProperty];
};

return el.textContent;
else
return el.innerText || el.textContent || "";
return el.innerText;
Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/ajaxorg/pilot/commit/1b6251c4bc1d24406d041c2949c3d3f99a112425, undefined was coming from textContent right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, we already detected that el has innerText, no need to use textContent here.

};

exports.getParentWindow = function(document) {
Expand Down
6 changes: 3 additions & 3 deletions lib/ace/lib/event.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,10 +301,10 @@ if (window.postMessage && !useragent.isOldIE) {


exports.nextFrame = window.requestAnimationFrame ||
window.oRequestAnimationFrame ||
window.msRequestAnimationFrame ||
window.mozRequestAnimationFrame ||
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Huge performance improvement he he ))

window.webkitRequestAnimationFrame;
window.webkitRequestAnimationFrame ||
window.msRequestAnimationFrame ||
window.oRequestAnimationFrame;

if (exports.nextFrame)
exports.nextFrame = exports.nextFrame.bind(window);
Expand Down
42 changes: 6 additions & 36 deletions lib/ace/lib/net.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,10 @@
define(function(require, exports, module) {
"use strict";

var useragent = require("./useragent");

exports.get = function (url, callback) {
var xhr = exports.createXhr();
var xhr = new XMLHttpRequest();
xhr.open('GET', url, true);
xhr.onreadystatechange = function (evt) {
xhr.onreadystatechange = function () {
//Do not explicitly handle errors, those should be
//visible via console output in the browser.
if (xhr.readyState === 4) {
Expand All @@ -23,47 +21,19 @@ exports.get = function (url, callback) {
xhr.send(null);
};

var progIds = ['Msxml2.XMLHTTP', 'Microsoft.XMLHTTP', 'Msxml2.XMLHTTP.4.0'];

exports.createXhr = function() {
//Would love to dump the ActiveX crap in here. Need IE 6 to die first.
var xhr, i, progId;
if (typeof XMLHttpRequest !== "undefined") {
return new XMLHttpRequest();
} else {
for (i = 0; i < 3; i++) {
progId = progIds[i];
try {
xhr = new ActiveXObject(progId);
} catch (e) {}

if (xhr) {
progIds = [progId]; // so faster next time
break;
}
}
}

if (!xhr) {
throw new Error("createXhr(): XMLHttpRequest not available");
}

return xhr;
};

exports.loadScript = function(path, callback) {
var head = document.getElementsByTagName('head')[0];
var head = document.head || document.getElementsByTagName('head')[0];
var s = document.createElement('script');

s.src = path;
head.appendChild(s);

if (useragent.isOldIE)
if (s.onload !== undefined)
s.onload = callback;
else
s.onreadystatechange = function () {
this.readyState == 'loaded' && callback();
};
else
s.onload = callback;
};

});
18 changes: 13 additions & 5 deletions lib/ace/mode/behaviour/css.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,11 +70,19 @@ var CssBehaviour = function () {
this.add("colon", "deletion", function (state, action, editor, session, range) {
var selected = session.doc.getTextRange(range);
if (!range.isMultiLine() && selected === ':') {
var line = session.doc.getLine(range.start.row);
var rightChar = line.substring(range.end.column, range.end.column + 1);
if (rightChar === ';') {
range.end.column ++;
return range;
var cursor = editor.getCursorPosition();
var iterator = new TokenIterator(session, cursor.row, cursor.column);
var token = iterator.getCurrentToken();
if (token && token.value.match(/\s+/)) {
token = iterator.stepBackward();
}
if (token && token.type === 'support.type') {
var line = session.doc.getLine(range.start.row);
var rightChar = line.substring(range.end.column, range.end.column + 1);
if (rightChar === ';') {
range.end.column ++;
return range;
}
}
}
});
Expand Down
9 changes: 9 additions & 0 deletions lib/ace/mouse/dragdrop.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ var DragdropHandler = function(mouseHandler) {
event.addListener(mouseTarget, "dragenter", function(e) {
if (editor.getReadOnly())
return;
var types = e.dataTransfer.types;
if (types && Array.prototype.indexOf.call(types, 'Files') !== -1)
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this check for presence of 'text/plain' instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're half right. Not instead of (anyway we should reject files, even with text/plain mime type), but along with.

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking about cases like types = ["-moz-image"]

return;
counter++;
if (!dragSelectionMarker) {
range = editor.getSelectionRange();
Expand All @@ -59,6 +62,9 @@ var DragdropHandler = function(mouseHandler) {
event.addListener(mouseTarget, "dragover", function(e) {
if (editor.getReadOnly())
return;
var types = e.dataTransfer.types;
if (types && Array.prototype.indexOf.call(types, 'Files') !== -1)
return;
x = e.clientX;
y = e.clientY;
return event.preventDefault(e);
Expand All @@ -73,6 +79,9 @@ var DragdropHandler = function(mouseHandler) {
event.addListener(mouseTarget, "dragleave", function(e) {
if (editor.getReadOnly())
return;
var types = e.dataTransfer.types;
if (types && Array.prototype.indexOf.call(types, 'Files') !== -1)
return;
counter--;
if (counter > 0)
return;
Expand Down
1 change: 0 additions & 1 deletion lib/ace/virtual_renderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ var dom = require("./lib/dom");
var event = require("./lib/event");
var useragent = require("./lib/useragent");
var config = require("./config");
var net = require("./lib/net");
var GutterLayer = require("./layer/gutter").Gutter;
var MarkerLayer = require("./layer/marker").Marker;
var TextLayer = require("./layer/text").Text;
Expand Down