Closed sleemanj closed 3 years ago
2.0
normal
to enhancement
trunk
mharrisonline commented:
Actually, that editor is a spin-off of an early version of Kevin Roth's editor at http://www.kevinroth.com. While Kevin made his editor public domain, the "developer" of the editor you linked to made some modifications to Kevin's and put it under a BSD license. He later made a more feature-rich version of Kevin's early editor and placed it under a LGPL license.
Meanwhile, Kevin's editor (which does use an iframe that appears next to the button for choosing colors rather than a popup) is still being developed by several people at the original site that are happy to work on a public domain project and not have their name on a license. If you take the code from tinyRTE you will have to stay within the confines of the BSD license, if you borrow the code from Kevin's editor you could use it without any legal formalities.
mokhet commented:
Here is a patch using the colorpicker from gogo
it adds 2 variables to editor.config
- config.colorPickerGranularity, to choose the size of cells. default set to '6px'
- config.colorPickerPosition, to choose position. default set to 'bottom,right'
Index: htmlarea.js =================================================================== --- htmlarea.js (revision 548) +++ htmlarea.js (working copy) @@ -510,6 +510,11 @@ killword: [ "Clear MSOffice tags", ["ed_buttons_main.gif",4,3], false, function(e) { e.execCommand("killword"); } ] }; + // size of color picker cells + this.colorPickerGranularity = '6px'; + // position of color picker from toolbar button + this.colorPickerPosition = 'bottom,right'; + /* ADDING CUSTOM BUTTONS * --------------------- * @@ -1434,6 +1439,12 @@ HTMLArea._loadback(_editor_url + 'popupwin.js', this.generate, this ); return false; } + + if ( typeof colorPicker == 'undefined' ) + { + HTMLArea._loadback(_editor_url + 'popups/color_picker.js', this.generate, this ); + return false; + } if ( _editor_skin !== "" ) { @@ -4221,6 +4232,7 @@ HTMLArea.prototype._colorSelector = function(cmdID) { var editor = this; // for nested functions + var btn = editor._toolbarObjects[cmdID].element; if ( cmdID == 'hilitecolor' ) { if ( HTMLArea.is_ie ) @@ -4236,17 +4248,10 @@ } catch (ex) {} } } - this._popupDialog( - editor.config.URIs.select_color, - function(color) - { - // selection not canceled - if ( color ) - { - editor._doc.execCommand(cmdID, false, "#" + color); - } - }, - HTMLArea._colorToRgb(this._doc.queryCommandValue(cmdID))); + + var colorPicker = new colorPicker({cellsize:editor.config.colorPickerGranularity,callback:function(color){editor._doc.execCommand(cmdID, false, "#" + color);}}); + colorPicker.open(editor.config.colorPickerPosition, btn); + }; // the execCommand function (intercepts some commands and replaces them with
is it ok to commit to trunk ?
@sleemanj commented:
Sounds good mokhet, always meant to get around to doing that myself. Commit away.
applied patch in changeset:550 with 2 minors updates
"var colorPicker = new colorPicker();" was obviously an error, replaced to "var picker = new colorPicker();"
and "editor._doc.execCommand(cmdID, false, "#" + color)" was wrong because the colorPicker widget is already providing the # with the color, so it has been changed to "editor._doc.execCommand(cmdID, false, color)"
It probably can be improved maybe if we use a global colorPicker instead of creating a brand new one everytime and update the callback function at needs, but i dont have investigated in this way.
Anyway closing this ticket now.
fixed
new
to closed
There is a problem with this color picker in IE. Go to the examples page in IE and select some text. Click the foreground color button. When you click in the color picker, whatever was selected in xinha is deselected before the callback function is executed, so nothing happens. I guess we have to intercept the click and prevent it from being handled in xinha, but this is getting into unfamiliar territory for me. Does anyone have any ideas how to do this?
2.0
to Version 1.0
fixed
to ``enhancement
to normal
closed
to reopened
mokhet commented:
Damn IE. I've fixed it in my working copy of /branches/mokhet. Once again, i'll commit all the updates as soon as possible.
I've tried to intercept events, but i was getting nowhere. IE empty the selection when you click anywhere on the page. So I changed my mind and instead of intercepts events, it creates a copy of the current selection when the button is clicked and when the callback method (color assignement) is called, the selection is .. reselected.
Anyway, here is a patch for trunk
Index: htmlarea.js =================================================================== --- htmlarea.js (revision 553) +++ htmlarea.js (working copy) @@ -4253,7 +4253,17 @@ } catch (ex) {} } } - var picker = new colorPicker({cellsize:editor.config.colorPickerGranularity,callback:function(color){editor._doc.execCommand(cmdID, false, color);}}); + var cback = function(color) { editor._doc.execCommand(cmdID, false, color); }; + if ( HTMLArea.is_ie ) + { + var range = editor._createRange(editor._getSelection()); + cback = function(color) + { + range.select(); + editor._doc.execCommand(cmdID, false, color); + }; + } + var picker = new colorPicker({cellsize:editor.config.colorPickerGranularity,callback:cback}); picker.open(editor.config.colorPickerPosition, btn); };
Is it ok to commit ?
Hope this time we will really close this ticket :p
wymsy commented:
Clever. I say go ahead and commit. Even if there is still a problem in IE5.5, as reported in the forum (I can't confirm that because I don't have IE5.5 any more), this fixes it for the great majority of IE users.
As a side note, I have been working on some fixes and enhancements to color_picker.js. My version adds an iframe behind the picker to fix the problem with select lists showing through, it does boundary checking to keep the picker within the browser window, and it improves the user interface in several small ways. A click (instead of a mouseover) on the color grid now selects a color for preview, and a double click accepts the color and closes the picker. This allows you to find a color with the desired hue/saturation first, then adjust brightness, and then go back and adjust hue/saturation again until you get the right color. The current color and brightness settings are highlighted and can be dragged. I also added an 'OK' button so you can type in a color. I will attach a copy of the file to this ticket - comments are welcome.
I mention this because there is a small quirk in the display in IE with this bug fix and my version of the picker. With the current picker, the dropping and reselecting of the selection happens so fast it is not visible, but in my version, the selection is dropped on the first mousedown, and it isn't reselected until the picker is closed. The fix still works, but the dropped highlighting might be disconcerting to some users. Not a major issue, and I doubt there is an easy solution, but I thought I would at least mention it.
Enhanced color picker Attachment: color_picker.js
mokhet commented:
Hi wymsy, I get the following error (quite a lot) with the attached version.
Erreur : picker.saved_cells[row][col] has no properties
Fichier source : http://mokhet.com/xinha/popups/color_picker.js
Ligne : 387
And there's a skin problem, at least with blue_look, the buttons (ok and X to quit) are black on dark blue and not totally at the correct places.
I've commited, changeset:554 the previous patch to let IE6.0 users be able to use the colorpicker. However, i'm intrigued to know what can be wrong with IE5.5. It is only using existing methods in Xinha, so it has to work :)
Here is a patch I propose to fix - only - the "select" elements behind the colorpicker, by appending an iframe - only - if it's IE, it is using conditionals comments.
Index: color_picker.js =================================================================== --- color_picker.js (revision 553) +++ color_picker.js (working copy) @@ -288,6 +288,11 @@ { this.table.style.left = (left - (this.table.offsetWidth - element.offsetWidth)) + 'px'; } + // IE ONLY - prevent windowed elements (<SELECT>) to render above the colorpicker + /*@cc_on + this.iframe.style.top = this.table.style.top; + this.iframe.style.left = this.table.style.left; + @*/ }; /** Draw the color picker. */ @@ -466,10 +471,29 @@ td.appendChild(sampleTable); - this.tbody.appendChild(tr); document.body.appendChild(this.table); - + // IE ONLY - prevent windowed elements (<SELECT>) to render above the colorpicker + /*@cc_on + if ( !this.iframe ) + { + this.iframe = document.createElement('iframe'); + this.iframe.style.zIndex = 999; + this.table.style.zIndex = 1000; + this.iframe.style.position = 'absolute'; + this.iframe.style.width = this.table.offsetWidth; + this.iframe.style.height = this.table.offsetHeight; + this.iframe.border = 0; + this.iframe.frameBorder = 0; + this.iframe.frameSpacing = 0; + this.iframe.marginWidth = 0; + this.iframe.marginHeight = 0; + this.iframe.hspace = 0; + this.iframe.vspace = 0; + document.body.appendChild(this.iframe); + } + this.iframe.style.display = ''; + @*/ } else { @@ -495,6 +519,10 @@ this.close = function() { this.table.style.display = 'none'; + // IE ONLY - prevent windowed elements (<SELECT>) to render above the colorpicker + /*@cc_on + if ( this.iframe ) { this.iframe.style.display = 'none'; } + @*/ }; } \ No newline at end of file
Do you think it's ok to commit ? I'll look more into your updates to colorpicker. I like the idea of the ok button. But i'm running out of time to look deeper into your changes, can you provide a patch, it's easier to understand the globals idea with a patch than with a whole new file. :p
wymsy commented:
Thanks for your comments. I'll look into the problems you report. The patch for my changes is pretty big, so I'll attach it in a file.
I agree with you that changeset:554 should work in IE5.5. It would be useful to get confirmation from someone that there is (or is not) a problem.
Your iframe patch is similar to mine, except I didn't make mine conditional (too lazy) and you set a few more attributes than I did. I found I didn't need to set the z-index explicitly - the order in which the elements are added seems to take care of it (I used insertBefore instead of appendChild). I didn't actually try yours, but it looks like it should work.
diff for enhanced color_picker.js Attachment: color_picker_patch.txt
mokhet commented:
Hi wymsy, thanks for the diff file it will help the review - hope i'll have time any soon to look at it - , or better, more ppl looking at it and commenting :)
Anyway, since i'm actually busy working on enhancing some functionalities in Xinha for a client, i've run into the same problem as mharrisonline in [http://xinha.gogo.co.nz/punbb/viewtopic.php?pid=3796#p3787 forums]. It's nearly impossible to select twice time the same color with the colorpicker. So I have implemented a solution and here is the patch I'm using, unfortunatly it includes also my previous patch (fix windowed element).
Index: color_picker.js =================================================================== --- color_picker.js (revision 554) +++ color_picker.js (working copy) @@ -77,6 +77,7 @@ var picker = this; this.callback = params.callback?params.callback:function(color){alert('You picked ' + color )}; + this.websafe = params.websafe?params.websafe:false; this.cellsize = params.cellsize?params.cellsize:'10px'; this.side = params.granularity?params.granularity:18; @@ -288,6 +289,11 @@ { this.table.style.left = (left - (this.table.offsetWidth - element.offsetWidth)) + 'px'; } + // IE ONLY - prevent windowed elements (<SELECT>) to render above the colorpicker + /*@cc_on + this.iframe.style.top = this.table.style.top; + this.iframe.style.left = this.table.style.left; + @*/ }; /** Draw the color picker. */ @@ -338,7 +344,7 @@ picker.backSample.style.color = 'black'; } } - td.onclick = function() { picker.callback(this.colorCode); picker.close(); } + td.onclick = function() { colorPicker.remember(this.colorCode); picker.callback(this.colorCode); picker.close(); } td.appendChild(document.createTextNode(' ')); td.style.cursor = 'pointer'; tr.appendChild(td); @@ -413,7 +419,7 @@ picker.backSample.style.color = 'black'; } } - td.onclick = function() { picker.callback(this.colorCode); picker.close(); } + td.onclick = function() { colorPicker.remember(this.colorCode); picker.callback(this.colorCode); picker.close(); } td.appendChild(document.createTextNode(' ')); td.style.cursor = 'pointer'; tr.appendChild(td); @@ -429,6 +435,8 @@ td.colSpan = this.side + 3; td.style.padding = '3px'; + if ( this.websafe ) + { var div = document.createElement('div'); var label = document.createElement('label'); label.appendChild(document.createTextNode('Web Safe: ')); @@ -439,6 +447,7 @@ label.style.fontSize = 'x-small'; div.appendChild(label); td.appendChild(div); + } var div = document.createElement('div'); var label = document.createElement('label'); @@ -466,10 +475,60 @@ td.appendChild(sampleTable); + var savedColors = document.createElement('div'); + savedColors.style.clear = 'both'; + function createSavedColors(color) + { + var is_ie = false; + /*@cc_on is_ie = true; @*/ + var div = document.createElement('div'); + div.style.width = '13px'; + div.style.height = '13px'; + div.style.margin = '1px'; + div.style.border = '1px solid black'; + div.style.cursor = 'pointer'; + div.style.backgroundColor = color; + div.style[ is_ie ? 'styleFloat' : 'cssFloat'] = 'left'; + div.onclick = function() { picker.callback(color); picker.close(); }; + div.onmouseover = function() + { + picker.chosenColor.value = color; + picker.backSample.style.backgroundColor = color; + picker.foreSample.style.color = color; + }; + savedColors.appendChild(div); + } + for ( var savedCols = 0; savedCols < colorPicker.savedColors.length; savedCols++ ) + { + createSavedColors(colorPicker.savedColors[savedCols]); + } + + td.appendChild(savedColors); + this.tbody.appendChild(tr); document.body.appendChild(this.table); - + // IE ONLY - prevent windowed elements (<SELECT>) to render above the colorpicker + /*@cc_on + if ( !this.iframe ) + { + this.iframe = document.createElement('iframe'); + this.iframe.style.zIndex = 999; + this.table.style.zIndex = 1000; + this.iframe.style.position = 'absolute'; + this.iframe.style.width = this.table.offsetWidth; + this.iframe.style.height = this.table.offsetHeight; + this.iframe.border = 0; + this.iframe.frameBorder = 0; + this.iframe.frameSpacing = 0; + this.iframe.marginWidth = 0; + this.iframe.marginHeight = 0; + this.iframe.hspace = 0; + this.iframe.vspace = 0; + document.body.appendChild(this.iframe); + } + this.iframe.style.display = ''; + @*/ } else { @@ -495,6 +554,28 @@ this.close = function() { this.table.style.display = 'none'; + // IE ONLY - prevent windowed elements (<SELECT>) to render above the colorpicker + /*@cc_on + if ( this.iframe ) { this.iframe.style.display = 'none'; } + @*/ }; + } - } \ No newline at end of file +colorPicker.savedColors = []; +/** add the color to the savedColors */ +colorPicker.remember = function(color) +{ + // check if this color is known + for ( var i = colorPicker.savedColors.length; i--; ) + { + if ( colorPicker.savedColors[i] == color ) + { + return false; + } + } + // insert the new color + colorPicker.savedColors.splice(0, 0, color); + // limit elements + colorPicker.savedColors = colorPicker.savedColors.slice(0, 21); + return true; +};
wymsy commented:
It's an interesting idea, but the problem with your savedColors approach is that it is short-term memory only. As soon as you submit and close the xinha editor, it is all lost. I think one of the keys to helping people find the same color again is limiting the number of choices. The original picker does this by using only web-safe colors. This picker can do it by using a smaller granularity, say 10 cells per row or less, as well as the web-safe approach.
Speaking of granularity, there is a problem in htmlarea.js in the config settings for the new picker. These lines
// size of color picker cells this.colorPickerGranularity = '6px'; // position of color picker from toolbar button this.colorPickerPosition = 'bottom,right';
at line 364 should be changed to
// size of color picker cells this.colorPickerCellsize = '6px'; // granularity of color picker cells (number per column/row) this.colorPickerGranularity = 18; // position of color picker from toolbar button this.colorPickerPosition = 'bottom,right';
and this line
var picker = new colorPicker({cellsize:editor.config.colorPickerGranularity,callback:cback});
around line 4290 should be changed to
var picker = new colorPicker({cellsize:editor.config.colorPickerCellsize,callback:cback,granularity:colorPickerGranularity});
I have fixed the problem in my updated color_picker.js that you reported above, along with a few other small tweaks. I will post the new file and a diff as attachments.
Enhanced color picker, v2 Attachment: color_picker.2.js
diff for color_picker.js v2 Attachment: color_picker_patch-v2.txt
mokhet commented:
Yes, are right, the editor.config.colorPickerGranularity was misused. I get fooled by default configuration. Anyway, it has been fixed in changeset:555 . Now, ppl can custom cells size and granularity, which should help to select the correct color.
But even with this update, the short-term memory you noticed is a real issue i wasnt seeing when doing it. The only "generic" solution (meaning not relying on Xinha to be used) i'm able to find is using a cookie. Here is ( again ) a patch to fix this issue. I've removed the previous patch with the IE fix to make it easier to read.
Concerning your last file attached, i have found, i think, a little visual bug. Or perhaps it is intended ? If so, i find it quite disturbing.
- Select a color
- Select a gray color (the bottom cells from white to black)
- select another color
- the old selection effect (white or black border, i call it "cursor selection") is still shown in the color picker
- select again a gray color
- and here again the border is not removed
- '''expected result''' : the old border should disappear
Do you think it is possible to add a 1px solid border to all cells which will get the cursor selection. I mean, without, the colorPicker keeps resizing by a few pixels everytime we select a color. With a preset border of 1px with the same color as the background, the cursor selection will not make the whole picker update it's layout.
Concerning the iframe, i think you can save some writing and some calculs by replacing this part
if(/top/.test(anchorage)) { if(top - this.table.offsetHeight > 0) { this.table.style.top = (top - this.table.offsetHeight) + 'px'; this.iframe.style.top = (top - this.table.offsetHeight) + 'px'; } else { this.table.style.top = 0; this.iframe.style.top = 0; } } else { this.table.style.top = (top + element.offsetHeight) + 'px'; this.iframe.style.top = (top + element.offsetHeight) + 'px'; }
to something like this : (i'm using it and it looks ok from here)
if(/top/.test(anchorage)) { if(top - this.table.offsetHeight > 0) { this.table.style.top = (top - this.table.offsetHeight) + 'px'; } else { this.table.style.top = 0; } } else { this.table.style.top = (top + element.offsetHeight) + 'px'; } // HERE ONLY COPY THE FINAL TABLE TOP POSITION, // INSTEAD OF DOING TWICE THE CALCULATIONS this.iframe.style.top = this.table.style.top;
And the same goes for the left position.
Talking about the iframe fix, I think we should decide of a resolution. Use your solution or mine or another if someone else got a better fix than ours, but we need to use one of them to help thoses poor IE users to actually see the colorpicker despite the
I like very much the idea of click to select then ok, or doubleclick to validate. Lot of my Xinha users are not very smart and i know they gonna love to be able to select colors this way.
I think this patch combined to your changes could be a very nice enhancement. Here is the patch with the colors saving in a cookie
Index: color_picker.js =================================================================== --- color_picker.js (revision 555) +++ color_picker.js (working copy) @@ -74,9 +74,15 @@ function colorPicker(params) { + // if the savedColors is empty, try to read the savedColors from cookie + if ( colorPicker.savedColors.length == 0 ) + { + colorPicker.loadColors(); + } var picker = this; this.callback = params.callback?params.callback:function(color){alert('You picked ' + color )}; + this.websafe = params.websafe?params.websafe:false; this.cellsize = params.cellsize?params.cellsize:'10px'; this.side = params.granularity?params.granularity:18; @@ -338,7 +344,7 @@ picker.backSample.style.color = 'black'; } } - td.onclick = function() { picker.callback(this.colorCode); picker.close(); } + td.onclick = function() { colorPicker.remember(this.colorCode); picker.callback(this.colorCode); picker.close(); } td.appendChild(document.createTextNode(' ')); td.style.cursor = 'pointer'; tr.appendChild(td); @@ -413,7 +419,7 @@ picker.backSample.style.color = 'black'; } } - td.onclick = function() { picker.callback(this.colorCode); picker.close(); } + td.onclick = function() { colorPicker.remember(this.colorCode); picker.callback(this.colorCode); picker.close(); } td.appendChild(document.createTextNode(' ')); td.style.cursor = 'pointer'; tr.appendChild(td); @@ -429,6 +435,8 @@ td.colSpan = this.side + 3; td.style.padding = '3px'; + if ( this.websafe ) + { var div = document.createElement('div'); var label = document.createElement('label'); label.appendChild(document.createTextNode('Web Safe: ')); @@ -439,6 +447,7 @@ label.style.fontSize = 'x-small'; div.appendChild(label); td.appendChild(div); + } var div = document.createElement('div'); var label = document.createElement('label'); @@ -466,7 +475,36 @@ td.appendChild(sampleTable); + var savedColors = document.createElement('div'); + savedColors.style.clear = 'both'; + function createSavedColors(color) + { + var is_ie = false; + /*@cc_on is_ie = true; @*/ + var div = document.createElement('div'); + div.style.width = '13px'; + div.style.height = '13px'; + div.style.margin = '1px'; + div.style.border = '1px solid black'; + div.style.cursor = 'pointer'; + div.style.backgroundColor = color; + div.style[ is_ie ? 'styleFloat' : 'cssFloat'] = 'left'; + div.onclick = function() { picker.callback(color); picker.close(); }; + div.onmouseover = function() + { + picker.chosenColor.value = color; + picker.backSample.style.backgroundColor = color; + picker.foreSample.style.color = color; + }; + savedColors.appendChild(div); + } + for ( var savedCols = 0; savedCols < colorPicker.savedColors.length; savedCols++ ) + { + createSavedColors(colorPicker.savedColors[savedCols]); + } + td.appendChild(savedColors); + this.tbody.appendChild(tr); document.body.appendChild(this.table); @@ -496,5 +534,45 @@ { this.table.style.display = 'none'; }; + } - } \ No newline at end of file +// array of the saved colors +colorPicker.savedColors = []; + +// add the color to the savedColors +colorPicker.remember = function(color) +{ + // check if this color is known + for ( var i = colorPicker.savedColors.length; i--; ) + { + if ( colorPicker.savedColors[i] == color ) + { + return false; + } + } + // insert the new color + colorPicker.savedColors.splice(0, 0, color); + // limit elements + colorPicker.savedColors = colorPicker.savedColors.slice(0, 21); + //[mokhet] probably some more parameters to send to the cookie definition + // like domain, secure and such, especially with https connection i presume + // save the cookie + var expdate = new Date(); + expdate.setMonth(expdate.getMonth() + 1); + + document.cookie = 'XinhaColorPicker=' + escape (colorPicker.savedColors.join('-')) + ';expires=' + expdate.toGMTString(); + return true; +}; + +// try to read the colors from the cookie +colorPicker.loadColors = function() +{ + var index = document.cookie.indexOf('XinhaColorPicker'); + if ( index != -1 ) + { + var begin = (document.cookie.indexOf('=', index) + 1); + var end = document.cookie.indexOf(';', index); + if ( end == -1 ) { end = document.cookie.length; } + colorPicker.savedColors = unescape(document.cookie.substring(begin, end)).split('-'); + } +};
I wonder what gogo is thinking. And I wonder where the hell is gone the right scrollbars when including and viewing patches :)
And the demo is still here : http://mokhet.com/xinha/branch_events/examples/on_demand.php
wymsy commented:
Mokhet, I agree, combining all these changes into one package will make a pretty slick color picker! So I have done that, including your suggestion about border colors and a few more little details. I'll attach it as before. One thing I am still thinking about is, should we change the click on saved colors to a double click to accept, and change the mouseover to a click to view? This would be more consistent with the color grid, but on the other hand it works ok as it is. What do you think?
BTW, there is an error in the patch I gave you for changeset:555. The second part should have been
var picker = new colorPicker({cellsize:editor.config.colorPickerCellsize,callback:cback,granularity:editor.colorPickerGranularity});
Sorry!
Enhanced color_picker.js v3 Attachment: color_picker.3.js
diff for color_picker.js v3 Attachment: color_picker_patch-v3.txt
Committed a slightly updated version in changeset:600.
fixed
reopened
to closed
Id like to see the color choosers roll out in the same window such as they do at: http://www.alfmiti.net/demo/tinyRTE_demo.htm
I believe that site states the code is free for use so we might be able to use the exact method they do.
Thanks to anyone who can add this!
Reported by anonymous, migrated from http://trac.xinha.org/ticket/85