Opened 10 years ago

Closed 10 years ago

Last modified 6 years ago

#9745 closed enhancement (fixed)

[PATCH][CCLA]: Provide FullScreen and ViewSource Plugins for dijit.Editor

Reported by: Jared Jurkiewicz Owned by: bill
Priority: high Milestone: 1.4
Component: Editor Version: 1.3.2
Keywords: Cc:
Blocked By: Blocking:

Description (last modified by bill)

I've been working on a series of plugins for Editor, the first of which is a 'Fullscreen', which allows the editor to take over the full viewport of the browser when activated. The second is a ViewSource plugin, which allows the user to edit the markup from the editor.

I've tested both heavily together, as they indirectly interact, and I've made sure they play well with each other

The FullScreen Plugin provides the following:
1.) Keyboard hotkey: CTRL-SHIFT-F11 to go in and out of FullScreen mode (for A11Y)
. Complimentary to several browser's use of F11 for Browser fullscreen, as well as several word processors.
2.) Icon for !Fullscreen button (enabled and disabled).
3.) When activated, it tries to control TAB so that the user can only tab between the editor body and the menu bar for editor, since the editor is completely overlaying the page as it is. This seems to work fairly well. FireFox? and IE work great, Webkit based browsers work 'okay'.
4.) Restores original state of editor on deactivate.



The ViewSource Plugin provides the following:
1.) Keyboard hotkey: CTRL-SHIFT-F12 to go in and out of ViewSource mode (for A11Y)
2.) Icon for ViewSource button (enabled and disabled).
3.) When activated, it it works with the tab controls of ViewSource so that when in full-screen mode, tab will only go between the menubar and text area. This seems to work fairly well. FireFox? and IE work great, Webkit based browsers work 'okay'.
4.) Replaces the content in the editor with that from the textarea so that edits are applied when toggled off.
5.) Provides some basic filters by default to help prevent XSS. These filters strip <script> tags, comment blocks, and <iframe> tags, common sources of XSS injection.
6.) The filters can be disabled individually in the plugin load definition.
7.) 11+ DOH Robot tests to test the interactions, mode switching, and XSS filtering.

Notes:


Included are basic DOH Robot Tests for Keyboard and Mouse activation of the plugin, as well as verifying basic screen state between each. It does not have TAB control checking in Robot as I was having issues getting Robot to do that right (Any advice here?)

The CSS and NLS 'Command' are in separate files currently to make this easy to test standalone, but if accepted, these should be merged into the base editor ones (which I'll do once you're happy with it).

Browsers I've tested on during its development have been: FireFox? 2, FireFox? 3, FireFox? 3.5, Opera 9.6, Internet Explorer 7.0, 6.0, and 8.0, as well as Safari 3.2.3 and Chrome 2.0.

I'd like to get this into Dojo 1.4. More are coming. :-)

Attachments (1)

FullScreen_ViewSource.zip (21.0 KB) - added by Jared Jurkiewicz 10 years ago.
Updated plugins based on Bill's comments.

Download all attachments as: .zip

Change History (33)

comment:1 Changed 10 years ago by Jared Jurkiewicz

Description: modified (diff)

comment:2 Changed 10 years ago by Jared Jurkiewicz

I also will do all documentation on docs.dojocampus.org for this plugin if/when accepted.

comment:3 Changed 10 years ago by Jared Jurkiewicz

Owner: set to bill

comment:4 Changed 10 years ago by Jared Jurkiewicz

Description: modified (diff)
Summary: [PATCH][CCLA]: Provide FullScreen Plugin for dijit.Editor[PATCH][CCLA]: Provide FullScreen and ViewSource Plugins for dijit.Editor

comment:5 Changed 10 years ago by Jared Jurkiewicz

Description: modified (diff)

comment:6 Changed 10 years ago by bill

Milestone: tbd1.4

About "TAB control testing", we've got lots of tests that do testing about the TAB key (TAB and shift-TAB), including some for the editor itself; I don't have any special advice about it, not sure why it isn't working for you.

comment:7 Changed 10 years ago by Jared Jurkiewicz

I think the tab testing may have been a problem with waiting for the focus to settle down before checking positions. Anyway, I'll relook at testing it. I uploaded an updated set that has updates based on your e-mail comments + some more tests for ViewSource? (around the XSS/enable/disable options)

comment:8 Changed 10 years ago by Jared Jurkiewicz

Bill,

Are you generally okay with these plugins now? If you are, I'll start the work to get them fully committed in (merging the NLS to the main editor NLS file, merging the CSS, and merging the icons into the editor.gif).

comment:9 Changed 10 years ago by Jared Jurkiewicz

FYI: Becky looked at them from an AllY perspective and thought they looked good.

comment:10 Changed 10 years ago by bill

Description: modified (diff)

Hi Jared,

The plugin loosk generally good. Thanks for doing all the work on it.

See my comments in #9777 about removing the kludge in setEditor(). BTW the comment formatting for the button class variable is strange:

// buttonClass [protected]
//		Over-ride indicating the class of button to use, in this case a toggle.
buttonClass: dijit.form.ToggleButton,

(for both new plugins) but if you simplify the _initButton()/setEditor() code then you don't even need that attribute.

There are some other attributes with strange formatting too though (actually most of them are missing types and the colon), so please fix those.

Some specific comments about FullSizePlugin:

About _resizeEditor:

_resizeEditor: function(){
	// summary:
	//		Function to handle resizing the editor as the viewport 
	//		resizes (window scaled)
	// tags:
	//		private
	var vp = dijit.getViewport();
	dojo.style(this.editor.domNode, {
		width: vp.w + "px",
		height: vp.h + "px"
	});
},

Doesn't that make editor.domNode too big? On most browsers it's setting the content-box size to match the viewport, which means that padding, border, etc won't fit. I thought dojo.marginBox() is more appropriate. Maybe there is no border or padding.

About _toggleFullScreen(), I guess that should be called _setFullScreen() because it doesn't toggle per se, it sets according to the parameter.

The code in there also looks like it could be compressed a bit, in particular:

this._origState = {};
this._origiFrameState = {};

// Store the basic editor state we have to restore later.
// Not using dojo.style here, had problems, didn't 
// give me stuff like 100%, gave me pixel calculated values.
// Need the exact original values.
if(ed.domNode && ed.domNode.style && ed.domNode.style.width){
	this._origState.width = ed.domNode.style.width;
}else{
	this._origState.width = "";
}
if(ed.domNode && ed.domNode.style && ed.domNode.style.height){
	this._origState.height = ed.domNode.style.height;
}else{
	this._origState.height = "";
}
var position = dojo.style(ed.domNode, "position");
this._origState.position = position?position:"static";
var top = dojo.style(ed.domNode, "top");
this._origState.top = top?top:"";
var left= dojo.style(ed.domNode, "left");
this._origState.left = left?left:"";

// Store the iframe state we have to restore later.
// Not using dojo.style here, had problems, didn't 
// give me stuff like 100%, gave me pixel calculated values.
// Need the exact original values.
var bc = dojo.style(ed.iframe, "backgroundColor");
this._origiFrameState.backgroundColor = bc?bc:"transparent";

if(ed.iframe && ed.iframe.style && ed.iframe.style.width){
	this._origiFrameState.width = ed.iframe.style.width;
}else{
	this._origiFrameState.width = "auto";
}
if(ed.iframe && ed.iframe.style && ed.iframe.style.height){
	this._origiFrameState.height = ed.iframe.style.height;
}else{
	this._origiFrameState.height = "auto";
}
if(ed.iframe && ed.iframe.style && ed.iframe.style.zIndex){
	this._origiFrameState.zIndex = ed.iframe.style.zIndex;
}else{
	this._origiFrameState.zIndex = "";
}

Could be something like:

var domNode = ed.domNode,
	domStyle = domNode && domNode.style;
this._origState = {
	width: domStyle.width || "",
	height: domStyle.height || "",
	top: dojo.style(domNode, "top") || "",
	left: dojo.style(domNode, "left") || "",
	position: dojo.style(domNode, "position") || "static"
};

var iframe = ed.iframe,
	iframeStyle = iframe && iframe.style;
this._origiFrameState = {
	backgroundColor: dojo.style(iframe, "backgroundColor") || "transparent",
	width: iframeStyle.width || "auto",
	height: iframeStyle.width || "auto",
	zIndex: iframeStyle.zIndex || ""
}

I'm not sure how much of the object chain testing is necessary, but I see a lot of that in the code. Something like:

ed.iframe && ed.iframe.style && ed.iframe.style.zIndex

could be done via

dojo.getObject("iframe.style.zIndex", ed)

Although alternately, probably better, the above

iframeStyle = iframe && iframe.style;

could be changed to

iframeStyle = iframe && iframe.style || {};

and then iframeStyle.zIndex will never get an exception.

comment:11 Changed 10 years ago by bill

BTW thanks for writing the robot tests for all of these. OK, here are some comments on ViewSource plugin.

  1. I think this code could be simplified to one call to dojo.create() or maybe dojo.place():
this._ieFixNode = dojo.doc.createElement("div");
dojo.style(this._ieFixNode, {
	opacity: "0",
	zIndex: "-1000",
	position: "absolute",
	top: "-1000px"
});
dojo.body().appendChild(this._ieFixNode);
  1. _toggleSource() seems misnamed since it doesn't toggle per se. It does have one piece of code that seems to do that:
    // First check that we were in source view before toggling back.
    if(!ed._sourceQueryCommandEnabled){
    	return;
    }
    

but in general it just works off of a flag. Actually it might be better to have two separate functions to make the code easier to follow.

  1. would the filters be better applied as standard editor filters run whenever attr("value") or attr("value", ...) are called?
  1. some of the code can be reduced by using the array functions, for example
this._pluginList = [];
for(i = 0; i < edPlugins.length; i++){
	p = edPlugins[i];

	// Capture the initial state for restore later.
	this._pluginList.push({
		plugin: p, 
		state: p.button?p.button.attr("disabled"):false
	});
	if(!(p instanceof dijit._editor.plugins.ViewSource)){
		if(p.button && !p.useDefaultCommand){
			// check to see if we had a previous state for this plugin
			p.button.attr('disabled', true);
		}
	}
}

can be changed to

this._pluginList = dojo.map(edPlugins, function(p){
	return {
		plugin: p, 
		state: p.button && p.button.attr("disabled")
	};
});
dojo.forEach(edPlugins, function(p){
	if(!(p instanceof dijit._editor.plugins.ViewSource)){
		if(p.button && !p.useDefaultCommand){
			p.button.attr('disabled', true);
		}
	}
});

or better, just keep track of which plugins you disabled:

this._disabledPlugins = dojo.filter(edPlugins, function(p){
	if(p.button && !p.button.attr("disabled")){
		p.button.attr("disabled", true);
		return true;
	}
});
  1. I'm unclear on needing to override queryCommandEnabled()... in the above code you are manually disabling buttons etc, what's the need for overriding queryCommandEnabled() in addition to that?

comment:12 Changed 10 years ago by Jared Jurkiewicz

I'll look at applying some of the comments

You have to over-ride queryCommandEnabled while ViewSource? is active to stop the buildin editor commands from re-enabling which they will do whenever the editor does a recheck on enabling them (and it does to periodic checks). It makes sure that while in ViewSource?, the *only* active command you have is to toggle the source view off, no matter what. Which is necessary since in source view none of the other commands even apply.

comment:13 Changed 10 years ago by liucougar

I think there is no need to save _pluginList: after the editor is re-enabled, call onNormalizedDisplayChanged to trigger updating of all buttons on the toolbar

Changed 10 years ago by Jared Jurkiewicz

Attachment: FullScreen_ViewSource.zip added

Updated plugins based on Bill's comments.

comment:14 Changed 10 years ago by Jared Jurkiewicz

Okay, new set of plugins provided with changes suggested by Bill. Comments addressed below:
FullScreen:

_resizeEditor: - I was unaware of that distinction, plugin updated to use dojo.marginBox.

_toggleFullscreen - Renamed to _setFullscreen Agree with comments on its name.

Code reduction in style preservation - Great suggestions. Applied. Sections that preserve style are much smaller and easier to understand.

Also applied:
Reworked the _initButton as per suggestions in #9777. Much cleaner, no kludging.


ViewSource:


_ieFixNode: Reduced using dojo.create. Good suggestion!

_toggleSource - Renamed to _showSource, since it operates off a flag. The reason for that extra check in the code looking for the over-ride was I saw in testing a weird corner-case where it got called with false before it had ever been displayed. Because of that, it failed. The check basically keeps it from trying to hide something that hadn't been created yet. I don't see splitting up that function as extremely valuable.

Plugin list - Reduced it based on Bill's suggestion. I like keeping this list because I'm of the opinion that the exact state should always be restored when swapping out. I understand Liu's comment ... but in this case I'd like to keep the plugin being a 'good citizen' and resoring it exactly the way it left it.

filters - In this case I don't think they should be in the general filter list editor uses. Reason: I always want them to be the last filter applied when swapping into source view and the first filter applied before content sent to the editor. Since the general filters are an array populated by plugins as they load ... I have no way to guarantee where the filters would be in the ordering since I can't control what other plugins may do. By keeping them outside that array, I can control when they're invoked. Make sense?

Over-riding queryCommandEnabled: This is done because we have no public way to set a plugin as disabled, regardless of what state it may think it should be using. To be able to get rid of that over-ride, we need to add some public API to the plugin model that lets you programmatically disable a plugin completely. The only way I found to do that currently is what this plugin does. Over-ride the base queryCommandEnabled function and flag all buttons as disabled. If we can get consensus on a simple api for disabling/enabling plugins programmatically, I believe this can be safely removed.

If we're in general agreement now on the basics of these plugins, I'll get them merged and committed in. Once they're committed in, why don't we discuss a public disable API for plugins. Once in place, I can clean up the over-ride in ViewSource?.

-- Jared

comment:15 Changed 10 years ago by Jared Jurkiewicz

FYI: I also did the initButton cleanup for ViewSource?.

comment:16 Changed 10 years ago by liucougar

Plugin list - if you really want to preserve the state of the editor, I suggest perserving the cursor position in the editor, and then restore it, and call onNormalizedDisplayChanged (which will update buttons)

if the cursor position is not preserved and restored, the preserved states of all the buttons are irrelevant: they are not applicable to the current cursor position any more.

(perserving the cursor position is a bit more complex than save/restore bookmark: the original node may have been removed. One way of overcoming this is to use dijit.range.getIndex)

to conclude: I suggest either we don't bother with preserving the states, or preserve the cursor position (which is what the user really cares, IMO)

comment:17 Changed 10 years ago by bill

I see the point about caret position, although it will often be impossible to restore caret position because the user has modified the HTML around where the caret was. So given that the caret has likely changed position when edit mode is closed, we do need to call onNormalizedDisplayChanged() to reset each plugin.

Having said that, onNormalizedDisplayChanged() doesn't seem sufficient though, because some plugins (for example, ToggleDir and Print) always have enabled buttons, and thus updateState() for those plugins will do nothing. If we didn't reenable those buttons explicitly they would stay disabled forever.

comment:18 Changed 10 years ago by Jared Jurkiewicz

Resolution: fixed
Status: newclosed

(In [20070]) Adding in FullScreen? and ViewSource? with merged icons into the editor\*.gifs, + testcases. \!strict fixes #9745

comment:19 Changed 10 years ago by Jared Jurkiewicz

(In [20071]) Corner case fix, undefined plugin leaves null in plugin list, since we iterate it, need to avoid those. \!strict refs #9745

comment:20 Changed 10 years ago by Jared Jurkiewicz

(In [20084]) Programmatic setting of zIndex wasn't working, so fixed it. Also removed unecessary over-ride of useDefaultCommand. refs #9745

comment:21 Changed 10 years ago by Jared Jurkiewicz

(In [20127]) Fixed a focus issue, mainly caused isues on webkit. Fixed some inner size issues when editor had border/margin. Tested on: IE7, IE8, FF2, Google Chrome 2 and 3. \!strict refs #9745

comment:22 Changed 10 years ago by Jared Jurkiewicz

(In [20179]) Fixing minor webkit bug with focus changes. Without fix, bold, italic, etc break. \!strict refs #9745

comment:23 Changed 10 years ago by Jared Jurkiewicz

(In [20197]) Improving script filter a bit. \!strict refs #9745

comment:24 Changed 10 years ago by Jared Jurkiewicz

(In [20253]) Don't need to requery value, we already have it. \!strict refs #9745

comment:25 Changed 10 years ago by Jared Jurkiewicz

(In [20308]) Speeding up ViewSource? tests a bit, simplifying use of robot. refs #9745

comment:26 Changed 10 years ago by Jared Jurkiewicz

(In [20309]) Tba cleanup. refs #9745

comment:27 Changed 8 years ago by bill

(In [25257]) Simplify setting initial size of source view <textarea>. I think the old logic was wrong, but it didn't matter because _resize() was called right afterwards.

Also, since dojo.position(ed.domNode) returns the border-box size, we should not be looking at ed.domNode's margin size. Refs #9745, #11622 !strict.

comment:28 Changed 6 years ago by bill

In [30693]:

In setSourceAreaCaret(), use feature detection rather than browser sniffing, and avoid using deprecated dojo/_base/window::global attribute. The code to set the caret (both before and after my change) doesn't seem to work on chrome though. Also, it's hardly ever called; only when the editor is programatically focused. Refs #9745 !strict.

comment:29 Changed 6 years ago by dylan

Doesn't it make sense to do all feature detection tests as has() tests... that way static has features can be used in the build system to strip out unneeded code as desired, and also, we can hopefully move to sharing common feature tests, rather than recreating feature tests across many modules?

comment:30 Changed 6 years ago by bill

Theoretically yes, but it's tricky with IE because the iframe can have different features than the main page, see [28090].

comment:31 Changed 6 years ago by dylan

Right, but shouldn't we just create a new feature name for this (that matches this condition being checked) and the same thing with #28090?

Otherwise it seems like we're increasing the build size for mobile-optimized apps, since you can no longer use static-has-features to exclude these blocks of code?

comment:32 Changed 6 years ago by bill

Conceptually yes, but you need to remember that Editor doesn't run on mobile. Also, creating an iframe in a has() feature test is likely impossible, since they load asynchronously, and impractical, due to performance.

Note: See TracTickets for help on using tickets.