Opened 17 years ago

Closed 13 years ago

#1376 closed New Feature (fixed)

"disabled" R/W property

Reported by: claudiu_cristea@… Owned by: Garry Yao
Priority: Normal Milestone: CKEditor 3.6
Component: General Version: 3.5.2
Keywords: IBM Confirmed Cc: saul11@…, Damian, joek, satya_minnekanti@…, fredrik@…

Description (last modified by Alfonso Martínez de Lizarrondo)

Like other input text controls on forms (<textarea>, <input type="text">, etc) FCKeditor need a new property on JavaScript API.

Something that can be change at runtime in this way:

var oEditor = FCKeditorAPI.GetInstance('instance_name'); oEditor.disabled = true;

This new property must be R/W (like those coresponding to <TEXTAREA>, <INPUT TYPE="TEXT"> from DOM).

This property must take a boolean value: true (by default), false;

Setting this property to "true" must freeze the toolbar too, not only the editor area.

The other hidden fields related to the editor must be treated in the same way.


Moved from Original author: Claudiu Cristea

Attachments (9)

1376.patch (5.1 KB) - added by Garry Yao 14 years ago.
readonly.ico (199.4 KB) - added by Garry Yao 14 years ago.
Toolbar Button Icon
1376_2.patch (6.4 KB) - added by Garry Yao 13 years ago.
1376_3.patch (13.2 KB) - added by Garry Yao 13 years ago.
1376_4.patch (21.8 KB) - added by Garry Yao 13 years ago.
1376_5.patch (24.1 KB) - added by Garry Yao 13 years ago.
1376_6.patch (25.9 KB) - added by Garry Yao 13 years ago.
1376_7.patch (25.9 KB) - added by Garry Yao 13 years ago.
1376_8.patch (22.3 KB) - added by Garry Yao 13 years ago.

Download all attachments as: .zip

Change History (62)

comment:1 Changed 17 years ago by Alfonso Martínez de Lizarrondo

Cc: saul11@… Alfonso Martínez de Lizarrondo added
Description: modified (diff)
Reporter: changed from Alfonso Martínez de Lizarrondo to claudiu_cristea@…

Date: 2006-08-31 14:44 Sender: claudiu_cristea

There is a mistake in the above post. Correct is: the default value must be "false"


Date: 2006-11-28 16:29 Sender: nobody

Any chance of it being implemented soon?


Date: 2006-11-08 20:19 Sender: nobody

this would be extremely useful.


Date: 2006-12-05 18:42 Sender: alfonsoml

https://sourceforge.net/tracker/index.php?func=detail&aid=1492728&group_id=75348&atid=543656 https://sourceforge.net/tracker/index.php?func=detail&aid=1400604&group_id=75348&atid=543656 https://sourceforge.net/tracker/index.php?func=detail&aid=865880&group_id=75348&atid=543656

Have been marked as duplicate of this report.

Right now I think that Internet Explorer doesn't allow to create setters and getters, so at best it would be a function like .SetEnabled( bNewValue)


Date: 2007-04-02 15:20 Sender: saul11

Independant of this request I have written a toggleFCKeditor function. It disables and re-enables the FCKeditor. When disbaled, the whole toolbar is grayed out and the editorArea will not receive any input. Optionally the toolbar can be hidden or collapsed. In this manner, when re-enabled, the toolbar will show again or be expanded. Note: When disabled, the editorArea is also grayed out in IE.

So no boolean, but a toggle function.

I have created a new page for things like this: http://fcksnippets.saulmade.nl/ (alias of http://www.saulmade.nl/FCKeditor/FCKSnippets.php) Here you can read more about the function, see a demo and get your download.

comment:2 Changed 16 years ago by Artur Formella

Keywords: Confirmed added

comment:3 Changed 15 years ago by Garry Yao

Milestone: CKEditor 3.x

We'll probably consider it for v3.2.

comment:4 Changed 14 years ago by Alfonso Martínez de Lizarrondo

#4996 has been marked as dup

comment:5 Changed 14 years ago by Sa'ar Zac Elias

when inputs and textarea that are disabled are converted into editors, they should automaticly become readonly.

comment:6 Changed 14 years ago by Alfonso Martínez de Lizarrondo

Cc: Damian joek satya_minnekanti@… added; Alfonso Martínez de Lizarrondo removed
Keywords: IBM added

#3944 and #5412 have been marked as dups

comment:7 Changed 14 years ago by sync

Version: 3.2.1

has this been added in to 3.2.1?

comment:8 Changed 14 years ago by Alfonso Martínez de Lizarrondo

Version: 3.2.1

Changed 14 years ago by Garry Yao

Attachment: 1376.patch added

Changed 14 years ago by Garry Yao

Attachment: readonly.ico added

Toolbar Button Icon

comment:9 Changed 14 years ago by Garry Yao

Keywords: Review? added
Owner: set to Garry Yao
Status: newassigned

comment:10 Changed 14 years ago by Sa'ar Zac Elias

With this patch, clicking on a link while readonly mode is enabled and afer you disable it, links in the editor are opened inside the editor frame.
Also, the context menu still works, yet all options in it are disabled.

comment:11 Changed 14 years ago by Alfonso Martínez de Lizarrondo

Keywords: Review- added; Review? removed

I think that we should define exactly what are the features of this configuration.

For example, I don't expect a button that the users can click to toggle it, that would be more along #3944, there are times when the user might want to see how the page will look like, and others where the developer is the one that marks the content as read only. I think that this ticket should focus on this part and #3944 is the one for users. Disabled form fields aren't sent back to the server, js can interact with them, but they aren't posted when the form is submitted. What should we do? Is the user allowed to switch to source mode? Should the content have some different (grayed) styling to signal that it's read only? Are the contents expected to be active (links, buttons, javascript, plugins...). A simple solution (maybe only for #3944 and depending on the requirements) would be to define a new mode "preview" or "readonly" similar to wysiwyg where the content is loaded in an iframe, but it's never marked as editable and the raw html (without protecting comments, javascript, plugins...) is used. This could avoid the need of defining "readyOnlyCommands" as each plugin could state if it can be enabled or not in "preview" or "readonly".

As I said, I would like to define first what are the expected behavior of this property and also how it relates and the differences with "preview" mode because that can help to avoid misunderstandings about what this property does.

comment:12 Changed 14 years ago by Frederico Caldeira Knabben

I had this feature in mind since the early ages of CKEditor 3. These is the way I thought it:

  • This is definitely not a feature to be activate by the user. It's an editor mode, defined by the developer.
  • Again, this is a "mode", so it must change the editor.mode. We should introduce a new values for it: "readonly".
  • Having a mode change makes things much simpler as commands are activated depending on the "modes" property.
  • The contents should be loaded into an iframe by dedicated code, but should still pass through the data processor (this means that things like anchors or placeholders will still be rendered and the page will not be alive). This step is necessary also because the input data may not be HTML.
  • Regarding the form post, we must just make it work like a <textarea disabled="disabled"> element. The thing to note is that the readonly mode will make no changes to the input data.
  • Context menu... I'm a bit unsure. Some features like "Copy" may still be useful.

comment:13 Changed 14 years ago by Garry Yao

Keywords: Discussion added; Review- removed
Milestone: CKEditor 3.xCKEditor 3.5

As we saw this feature desire a separate branch and more experimentation, defers it.

comment:14 Changed 14 years ago by Frederico Caldeira Knabben

Keywords: Discussion removed
Milestone: CKEditor 3.5

comment:15 Changed 14 years ago by Fredrik Wendt

Cc: fredrik@… added

My use case for this feature is to use it as a web 2.0-ish wiki-editor - default mode is to simply browse the wiki and read what others have written, but occasionally you want to change a speling error, or add a sentence. With a read-only mode, users wouldn't have to reload the page, scroll down, find the stuff you want to change, change it, save, watch page reload, scroll again - you'd just click "edit" in a floating toolbar, change and press save. No page reloads, no searching for the stuff you wanted to change - just simple, intuitive, inline editing.

comment:16 in reply to:  15 ; Changed 14 years ago by Garry Yao

Replying to fredrik_wendt:

My use case for this feature is to use it as a web 2.0-ish wiki-editor...

I hope you've checked our sample before saying so, for me it's not a use-case of the read-only mode.

comment:17 in reply to:  16 Changed 14 years ago by Fredrik Wendt

Replying to garry.yao:

Replying to fredrik_wendt:

My use case for this feature is to use it as a web 2.0-ish wiki-editor...

I hope you've checked our divreplace sample for me it's not a use-case of the read-only mode.

Please tell me if this discussion should be moved to the forum. The divreplace sample is pretty much how I put it together when I looked at it some months ago (http://wiki.wendt.se/) - except I used Ext-core to attach a double click event and that you edit the full page and not just one part of it (moving text from one div/part to the other seems to force too much work for the user when only editing parts of the page). I've also looked at full screen mode, and I tried to disable the editor (read-only) as suggest with the patch supplied on the forum (and in this ticket), but had to stop becuase of time constraints. With full screen and read only I had two points left to work out:

  • does the full page need to reload when following a link, or could just the wysiwygarea reload (and the browser's URL field/location field in the toolbar)
  • attach the double click event to the wysiwyg area, to have the editor change into edit mode
  • attach to escape button key press, and let that save data (ajax) and turn the editor back into readonly (browse) mode

The divreplace examples demonstrate perfect use for CMS:s, where you're only allowed to (and perhaps want to) edit parts of a complete page. For an enterprise wiki (such as confluence), with technical documentation, the use case is different.

comment:18 Changed 14 years ago by Fredrik Wendt

Ideally: this would also solve two issues:

  • after double clicking on the div, the word selected is selected when the editor instance is ready
  • when you edit a div which is longer than the height of the browser window (full page), the word/content double clicked should be in focus - basically with divreplace you have to handle scrolling issues - using a true read only-mode would not have this issue since you're already working in the wysiwyg area

comment:19 Changed 14 years ago by Damian

Could we add this to 3.5?

comment:20 Changed 14 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.6

Right now, adding this feature to the 3.5 could bring delays to it. I'm opening the 3.6 for it.

comment:21 Changed 14 years ago by Frederico Caldeira Knabben

@fredrik_wendt, note that "read-only" mode is not "browse" mode. As said before, the loaded contents will still be processed and rendered much like the "editing" (wysiwyg) mode. The only difference is that it'll not be possible to make changes to the contents.

This means that the editor contents will not be "active" in read-only mode, so things like links will not work (no navigation on click).

comment:22 Changed 14 years ago by tachyon42

I have an ASP.NET website where CKEditor is almost exactly what I need - just missing one thing: a readonly capability.

Requirement: Software to embed in an ASP.NET webpage so that one class of user can create a Word like document which another class of user can view (readonly) in another ASP.NET webpage.

I have used the current version CKEditor 3.4.1 (revision 5892) to create two separate CKEditor configurations. One configuration creates a CKEditor box with a limited number of toolbar buttons which successfully provides the required functionality in a webpage that outputs the HTML to a document that is saved to disk. The other configuration is used in a webpage that reads the document and renders the HTML to display the formatted text in the CKEditor box. This CKEditor configuration does not have any toolbar buttons so the user is unable to format any text. Visually it appears that this successfully provides the required functionality for the 'readonly' class of users. However, the user is still able to use a mouse or tab key to give focus to the CKEditor box and then add/delete text. This is not ideal since it may confuse users. This webpage does not provide any means by which this document can be updated so the document is safe from change by the 'readonly' class of user.

CKEditor is so close to being exactly what I need!

Please consider this scenario when designing any proposed 'readonly' modification to CKEditor.

Please contact me via PM if anyone wants me to test any 'readonly' modification code. Thanks

comment:23 Changed 13 years ago by Wiktor Walc

I'd not mix too many things in this ticket. Preview mode mentioned in comment:11 should stay as a separate issue as its much more complicated, one may want to replace some placeholders using server side language in preview mode and do some other stuff with the content.

readOnly mode should stay as simple as possible instead and do only the following: disallow user from changing the content inside of the editor

  • all buttons useless in readOnly mode should be disabled
  • buttons/commands that are useful should be still available (Maximize, Find, Copy etc.)
  • I'd not worry that much about form elements and eventually fix all issues related to them later in separate tickets (and consider in the meantime using fake elements #5804)

Changed 13 years ago by Garry Yao

Attachment: 1376_2.patch added

comment:24 Changed 13 years ago by Garry Yao

Status: assignedreview

New patch is a rough implementation of the idea following "readonly as a mode" - it behaves as a (special) parasitic mode that doesn't load/unload, which means it remain all functionality of an existing mode but just staying readonly at the API level, which set mutation restriction only inside of editor (user may change it anyway at DOM level). The patch is not meant to be complete, polishing will be following once the idea is accepted.

comment:25 Changed 13 years ago by decherneyge

Version: 3.5.2

this would prevent content editing as soon as the control is loaded but you can modify as needed by using the function executed on callback here in an onClick of a checkbox for example.

 <script type="text/javascript">
        window.onload = function () {

            CKEDITOR.on("instanceReady", function (ev) {
                var bodyelement = ev.editor.document.$.body;
                bodyelement.setAttribute("contenteditable", false);


            });
            CKEDITOR.replace('editor1');
        };
       
    </script>

comment:26 Changed 13 years ago by decherneyge

I discovered that the buttons would still format the text so if you choose the function above you should also customize the toolbar for that particular setting by modifing the CKEDITOR.replace() command with something similar to:

CKEDITOR.replace('editor1',{ toolbarStartupExpanded: false, Print','-', 'About?});

comment:27 Changed 13 years ago by Wiktor Walc

Status: reviewreview_failed

Looks like a way to go to me.

There are still few things to fix in the patch:

  • Clicking on a link should not open the Link dialog, in readonly mode link could behave just like a real link.
  • Editing elements using the context menu should not be possible. It looks like only the "Copy" item is useful in the readonly mode.
  • Few buttons are still enabled, like "Bold", "Italic" etc. in Safari/Firefox.
  • Switching to source mode and back disables the readonly mode (at least buttons don't look like disabled anymore).
  • The "ReadOnly" button should be removed from the final solution, it's just another editor mode that will be used by developers to programmatically disable CKEditor.
  • Ctrl+C should work in readonly mode.
  • If possible, the Find dialog could be still available, only the Replace tab should be hidden.
  • The About, Maximize, Preview, Select All, Print, Copy, Save(?) buttons could be still available I guess.

comment:28 in reply to:  25 ; Changed 13 years ago by Alejandro

Finally, the script worked for me, although I have prevented access to the buttons so they can not put links on selected text, for example. For now it works, but Firebug detects a javascript error: "uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMNSHTMLDocument.queryCommandEnabled]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: http://websopde/panel/v3/_include/editor3/ckeditor.js?t=B1GG4Z6 :: u :: line 5793" data: no]"

This line corresponds with the following code:

function u(x, y) {
   c && (t = 1);
   '''var z = y.document.$.queryCommandEnabled(x) ? 2 : 0;'''
   t = 0;
   return z;
};

This is my modified code to not write the editor:

config("toolbarCanCollapse") = false
config("toolbarStartupExpanded") = false
			
response.write "<script type=""text/javascript"">"
response.write "   window.onload = function () {"
response.write "      CKEDITOR.on(""instanceReady"", function (ev) {"
response.write "          var bodyelement = ev.editor.document.$.body;"
response.write "          bodyelement.setAttribute('contenteditable', false);"
response.write "      });"
response.write "      CKEDITOR.replace('" & <My_Editor> & "');"
response.write "      };"
response.write "</script>"

comment:29 in reply to:  28 ; Changed 13 years ago by Alejandro

I forgot to mention that this code also throw an exception:

uncaught exception: [CKEDITOR.editor] The instance "<My_Editor>" already exists

Is that normal?

Also I have to say that after several tests, I found that the blockade of the textarea does not always work. Sometimes, clicking the mouse inside the editor places the cursor and write to it.

Replying to andynedine:

Finally, the script worked for me, although I have prevented access to the buttons so they can not put links on selected text, for example. For now it works, but Firebug detects a javascript error: "uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMNSHTMLDocument.queryCommandEnabled]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: http://websopde/panel/v3/_include/editor3/ckeditor.js?t=B1GG4Z6 :: u :: line 5793" data: no]"

This line corresponds with the following code:

function u(x, y) {
   c && (t = 1);
   '''var z = y.document.$.queryCommandEnabled(x) ? 2 : 0;'''
   t = 0;
   return z;
};

This is my modified code to not write the editor:

config("toolbarCanCollapse") = false
config("toolbarStartupExpanded") = false
			
response.write "<script type=""text/javascript"">"
response.write "   window.onload = function () {"
response.write "      CKEDITOR.on(""instanceReady"", function (ev) {"
response.write "          var bodyelement = ev.editor.document.$.body;"
response.write "          bodyelement.setAttribute('contenteditable', false);"
response.write "      });"
response.write "      CKEDITOR.replace('" & <My_Editor> & "');"
response.write "      };"
response.write "</script>"

comment:30 in reply to:  29 Changed 13 years ago by Alejandro

After searching for information and find similar cases, I've found a way to put the editor in read mode. It also works if there are many editors on the same page, whether reading or writing. The operation of the editors are correct in all cases.

CKEDITOR.on(""instanceReady"", function (ev) {
	var editorCKE = CKEDITOR.instances.<My_Editor>;
	editorCKE.on('key', function(keyEvent) {
		keyEvent.cancel();
	});
CKEDITOR.replace('<My_Editor>');
});

To avoid possible uses of the buttons also have hidden and locked the toolbar.

config("toolbarCanCollapse") = false
config("toolbarStartupExpanded") = false

Changed 13 years ago by Garry Yao

Attachment: 1376_3.patch added

comment:31 Changed 13 years ago by Garry Yao

Status: review_failedreview

I found proposing 'readonly' as an mode looks awkward and requires much more hacks, for me it's more appropriate to just treat it as a configuration provided the 'editingblock' plugin, editing block relevant plugins must be ware of it (e.g. wysiwyg and sourcearea), while other plugins in most cases could just live in ignorance.

comment:32 Changed 13 years ago by Wiktor Walc

I haven't done a full review, but I've noticed two things:

  • switching to source and back enables the toolbar
  • "Preview" button is still disabled. I'd say it is still useful in preview mode due to fake elements (for example).

comment:33 Changed 13 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed

Additionally, I believe the state listening changes should not be handled by the editingblock plugin, being coded at the UI element level. This means that the editor should fire a "readonly" event that can be listened by these elements.

The command status updates could also be done in the editor.js code, instead of editingblock.

Basically, considering that we're not any more talking about a new edit mode, the editingblock code should have very little involvement with this tickets.

Changed 13 years ago by Garry Yao

Attachment: 1376_4.patch added

comment:34 Changed 13 years ago by Garry Yao

Status: review_failedreview

comment:35 Changed 13 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed
  • The "Print" and "Show Blocks" buttons are still enabled when switching to source view.
  • The editor becomes editable when using the Maximize buttons.
  • The "Select All" button is not working on read-only mode.
  • The editor.readOnly initialization should be done in the editor.js file, right after loading the configurations, not in the editingblock plugin.
  • The "mode" event should not fire the "readonly" event. If this happens it means that we're doing part of the processing wrongly and it's a hack. editor.readOnly must be checked on mode change instead.
  • CKEDITOR.editor.prototype.setReadOnly should be defined in editor.js. It should simply set editor.readOnly and fire "readonly" for the change. This is the only place to have this event fired.
  • Note that the "readonly" event is there not to say that the editor is in read-only mode, but to announce the readOnly property change, either to true or false (btw... do not fire if not changed). To understand if the editor is in read-only mode, the readOnly property is to be used.
  • My bad... the event name should be "readOnly".
  • There is no need to have the js alert when switching the read-only mode. This is distracting and the editor behavior change is quite evident.
  • An entry on the samples list is needed for the new sample file.

Changed 13 years ago by Garry Yao

Attachment: 1376_5.patch added

comment:36 Changed 13 years ago by Garry Yao

Status: review_failedreview

The "Select All" button is not working on read-only mode.

Looks troublesome in FF and Opera, let's have it disabled on those two browsers right now to not blocking the review.

comment:37 Changed 13 years ago by Sa'ar Zac Elias

Status: reviewreview_failed
  • The collapse toolbar and find commands should work.
  • How about setting readOnly mode on initialization of the editor for disabled textareas?
  • Selection like the following enables the unlink button (throws an exception when clicking):
    <p>
    	This is some <strong>sample text</strong>. You are using <a href="http://ckeditor.com/">CKE[ditor</a>.]</p>
    
  • Let's have the event fired only when the readOnly flag changes, i.e. if the editor is already readonly and one sets editor.setReadOnly(1), nothing should happen.

Changed 13 years ago by Garry Yao

Attachment: 1376_6.patch added

comment:38 Changed 13 years ago by Garry Yao

Status: review_failedreview

comment:39 Changed 13 years ago by Sa'ar Zac Elias

Status: reviewreview_failed

In find dialog, once you go back from readOnly mode to editing mode, the replace tab is still hidden.

Changed 13 years ago by Garry Yao

Attachment: 1376_7.patch added

comment:40 Changed 13 years ago by Garry Yao

Status: review_failedreview

Can't believe dialog api is not reopen-wise.

comment:41 Changed 13 years ago by Sa'ar Zac Elias

Status: reviewreview_failed

BACKSPACE key remains blocked when switiching back to editing mode.

Changed 13 years ago by Garry Yao

Attachment: 1376_8.patch added

comment:42 Changed 13 years ago by Garry Yao

Status: review_failedreview

comment:43 Changed 13 years ago by Frederico Caldeira Knabben

Owner: changed from Garry Yao to Frederico Caldeira Knabben

I'm proposing some changes and fixes to the last patch into a new dedicated branch for this feature:
http://svn.ckeditor.com/CKEditor/branches/features/readonly

Please check out the log to review the changes:
log:CKEditor/branches/features/readonly

Please feel free to make any necessary enhancement.

comment:44 Changed 13 years ago by Garry Yao

It doesn't make sense to have two both of us working on same ticket, even without any review comments to the last patch, for the branch:

  1. I didn't check native "readonly" property just reflect the functionality for us, that's great.
  2. The editing block change start from [6752] and subsequent changes are not good, it hacks things too much, introduced awkward code like below and make the "mode" event dirty with an empty state which will surely breaks back-compact, I'm just totally against it.
    editor.setMode();
    editor.setMode( 'wysiwyg' );
    

Anyway I'll not be able to review here since the reviewing role is already messed up.

comment:45 Changed 13 years ago by Sa'ar Zac Elias

Status: reviewreview_failed
  • I must agree with Garry here as for the changes in [6752] and subsequently, they introduce really ugly pieces of hacks like demonstrated above.
  • It's impossible to put the editor in read-only mode if it's maximized.

comment:46 in reply to:  44 Changed 13 years ago by Frederico Caldeira Knabben

Status: review_failedreview

Replying to garry.yao:

It doesn't make sense to have two both of us working on same ticket

That's ridiculous. You don't need to copy my comment from another ticket. This is a very different situation because we were not working on this ticket *at the same time*. I was the only person working on it. Here we had a r?. In the other ticket I got a r-, so we were both working on it at the same time.

---

  1. The editing block change start from [6752] and subsequent changes are not good, it hacks things too much, introduced awkward code like below and make the "mode" event dirty with an empty state which will surely breaks back-compact, I'm just totally against it.
    editor.setMode();
    editor.setMode( 'wysiwyg' );
    

I'm not chancing any feature here, simply *adding* a new feature. You should be clear when you say that it breaks things, eventually pointing a specific example.

The way it has been implemented is much clearer now. It's up to the mode to do the read-only change job, and the way the wysiwyg mode does now is very clear: unload and reload the mode. Having it done by the editingblock was a much uglier hack. Just see that the sourcearea way is totally different and doesn't need reload.

Anyway I'll not be able to review here since the reviewing role is already messed up.

There is no fixed review role. We're all developing this thing together. If I see something to be fixed while reviewing, I simply go ahead changing things. Especially in this case, where just pointing out issues on comments would simply delay things a lot.

The way I made the changes were quite clear. I've created a branch and applied your patch to it. Than, over your patch, I've made several changes which can be then reviewed by you based on the branch log. Then, if you have further changes, it's enough to you to commit things again, which will be then reviewed by me... and so on... this cycle would go until a stage that one of us, when reviewing, would be satisfied with the final result.

From two comments the only thing that can be considered is the maximized thing pointed by Saar, even if it's an edge case that can be dealt on a separated ticket. to avoid delaying things further, I'm putting this on review again,

comment:47 Changed 13 years ago by Garry Yao

Here we had a r?. In the other ticket I got a r-, so we were both working on it at the same time.

I agree in exact the contrast way, taken over should happen only on r-, this ensure that there's a good reason to neglect previous patch but not due to personal taste.

this cycle would go until a stage that one of us,when reviewing, would be satisfied with the final result.

I can tell this's simply the wrong approach as it makes dev review their own change, that's way I'm saying it breaks the review role.

Anyway doesn't look like the right place to go talk about dev process...

You should be clear when you say that it breaks things, eventually pointing a specific example.

It's quite clear that when set editor to read-only, WYSIWYG mode is loaded twice, and with once firing an empty mode, so the "mode" event is broken.

editor.on( 'mode', function() { alert( editor.mode ); })

comment:48 Changed 13 years ago by Frederico Caldeira Knabben

[6752] was an attempt to make it simple, because we were missing an API feature to retrieve the current mode object.

Fortunately it was quite easy to bring this feature. I've simply made public the private function we had for it, changing the way we handle read-only on the wysiwygarea plugin. That's all in [6765].

R? again.

comment:49 Changed 13 years ago by Garry Yao

Status: reviewreview_failed

Ok, it looks like the correct solution to have read-only working with modes, while one other caught - rich combos are now incorrectly stated.

Last edited 13 years ago by Garry Yao (previous) (diff)

comment:50 in reply to:  49 Changed 13 years ago by Garry Yao

Replying to garry.yao:

rich combos are now incorrectly stated.

This's fixed with [6771], as I've completed the rest of reviewing, it simply wait for the last review.

comment:51 Changed 13 years ago by Garry Yao

Owner: changed from Frederico Caldeira Knabben to Garry Yao
Status: review_failedreview

comment:52 Changed 13 years ago by Frederico Caldeira Knabben

Status: reviewreview_passed

I'm merging the branch into the 3.6.x right now.

comment:53 Changed 13 years ago by Frederico Caldeira Knabben

Resolution: fixed
Status: review_passedclosed

Feature introduced with [6772].

Note: See TracTickets for help on using tickets.
© 2003 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy