Give TextColor plugin (foreColor and hiliteColor) support for custom color picker

[Nick Fenwick]

Same as #16881 but for dijit/_editor/plugins/TextColor.

[Nick Fenwick]

I'm starting work on this now, no patch available yet.

It seems dojox/editor/plugins/TextColor is basically the same as dijit/_editor/plugins/TextColor but with a different dropdown. First glace indicates this work would deprecate the dojox plugin, as we would be able to use the core dijit one configured to use whatever colour picker / dropdown you want.

[bill]

Makes sense, in that case dojox/editor/plugins/TextColor becomes a trivial (like 3 line) class.

[Nick Fenwick]

Patch on dojox just to fix dojox/widget/ColorPicker's habit of always firing onChange when value is set.

[Nick Fenwick]

Patch on dijit to give _editor/plugins/TextColor.js custom colour picker ability.

[Nick Fenwick]

New tests file to test behaviour of font and hilite plugins in a variety of situations.

[Nick Fenwick]

Just attached three files.

1. simple patch on dojox. When using dojox/widget/ColorPicker as an immediate popup from the font color picker, it appears, has its value set with .set('value', whatever, false), and immediately fires onChange and the popup closes, because ColorPicker?'s _setValueAttr ignored the fireOnChange argument.
2. and 3/ I'm being a bit of a cretin and can't see how to svn diff including my new tests file.

Because both a dropdown class and a picker class can be provided and the old TextColor? plugin supported async loading via require(), I have used dojo/promise/all to manage the two classes being loaded if necessary.

One problem stands out, if you drop down the plugins on the last example Editor, I have thrown in a very ugly example custom picker frame dijit, with TooltipDialog? .. its tooltipConnector is badly placed and I can't see why offhand. The overall popup seems to work fine.

[Nick Fenwick]

Replacement for earlier dijit patch and new files. Patches TextColor?.js, includes tests file, includes new file ConfirmTooltipDialog?.js.

[Nick Fenwick]

Just attached new patch. Please keep the original patch on dojox, that's still a good fix.

This patch includes work discussed with bill on IRC. Rather than port across the TablePlugins? internal dijit that allowed us to embed dojox/widget/ColorPicker in a TooltipDialog? with Set adn Cancel buttons, create a new dijit dijit/ConfirmTooltipDialog that can house any generic dijit that has a value.

Improvements over the old patch include:

1. No longer need internal dijit to TextColor?.js
2. From 1., no longer need to copy across nls/TextColor.js and all the associated translations, just to support the Set/Cancel? buttons on the dialog.
3. No longer have concept of "default" dropDownClass. Just pass in "dijit/ConfirmTooltipDialog" or its constructor to use it.

We should look at updating TablePlugins? to get rid of CellColorDropDown? and use this dijit instead.

[bill]

Thanks for the patch. I've been working on this but not sure it will make it into 1.9.

About ConfirmTooltipDialog, it wasn't laying out correctly (with the connector) because the new widget didn't have an orient() method like TooltipDialog does. The larger issue was that ConfirmTooltipDialog was a new widget with an *embedded* TooltipDialog, rather than an extension of TooltipDialog. So I had to change that.

With the new ConfirmTooltipDialog, one can easily construct a TooltipDialog containing a dojox/widget/ColorPicker and OK/Cancel buttons that's a functional replacement for dijit/ColorPalette. I.E. you can easily make a widget that the TextColor plugin can use in place of dijit/ColorPalette. Therefore I don't currently see the need for changing the TextColor plugin beyond adding the ability to specify which color picker widget to use.

There are two differences between using a TooltipDialog vs. a ColorPalette, but TextBox can compensate for them:

1. TooltipDialog has an onExecute() method instead of an onChange() method.

2. TooltipDialog, like Dialog, extends _FormMixin, meaning that it expects to have children with name=... settings, and get("value") returns a structure like {field1: va1, field2: val2}. I don't see a reason to deviate in the case of ConfirmTooltipDialog

So, if the TextColor plugin sees the dropdown has an onExecute() method it will connect to that, and assume get("value") returns something like {color: "#ff0000"} rather than "#ff0000".

[Nick Fenwick]

Latest patch, supercedes patch2

[Nick Fenwick]

I asked on IRC but got no answer.. bill, if this entire patch cannot make it into 1.9 because of the new dijit/ConfirmTooltipDialog, but the dijit/_editor/plugins/TextColor work is basically good, can we please split the patch so we can commit TextColor? for 1.9, adn leave ConfirmTooltipDialog? as a separate goodie for later?

TextColor? has been modified to work with any popup, it doesn't rely on ConfirmTooltipDialog?, that's just an option one can use to house the picker.

We'd have to remove ConfirmTooltipDialog? from the tests file but TextColor? itself doesn't depend on it.

Also, I need the ability for TextColor?'s popup picker to be able to access the plugin's data. My patch3 includes that, passing params in. This is the same as was done on dojox/editor/plugins/TablePlugins. I'd be happy just passing a reference to "parent: this" or similar, but whatever, we really need that ability. Perhaps if you make your code available I can have a look and feed back.

[bill]

No, sorry, it's passed the deadline for enhancements and also I don't want to add custom code in TextColor to create a TooltipDialog (i.e. the "dropDownClass" parameter) because that code won't be used by dijit at all.

I created #16930 to track the ConfirmTooltipDialog class. Note though that it works like TooltipDialog, i.e. like a dijit/form/Form. You can of course have just a single child widget, but I didn't build-in the code for get("value") to return this.getChildren()[0].get("value"), since that's specific to the Editor usage. Maybe consider adding it later.

[bill]

adds colorPicker property to TextColor? plugin; it can point to a ColorPalette? or a ConfirmTooltipDialog? containing a dojox/widget/ColorPicker, where get("value") and set("value") delegate to that ColorPicker?

[Nick Fenwick]

OK thank you bill. I'm not clear on what's going to get into 1.9 now. We may have to copy the TextColor? plugin and not use the core dijit one in our project, as we cannot wait for this functionality. Still, this is valuable work in general. I had my main SSD drive fail two days ago which has buggered my ability to keep up with this work.

[bill]

I'm sure you can just extend TextColor and override _initButton(), although making a copy is fine too. I'm not planning to put anything into 1.9 that isn't already there except for bug fixes.

[bill]

Might as well check this into SVN and then merge to github, so it's available for the 1.x and 2.x streams.

[Bill Keese <bill@…>]

In 457e674b0757dda57db3c7aee27aa45f85dab719/dijit:

Error: Processor CommitTicketReference failed

Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

[Bill Keese <bill@…>]

In 9f3e01c319cf418baaf709dab53b5f126097a05d/dojox:

Error: Processor CommitTicketReference failed

Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled?