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.

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

Sanitize Content Handler #1077

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

ggalileo
Copy link
Contributor

@ggalileo ggalileo commented Feb 6, 2014

This pull request adds the ability to configure and run the Sanitize Content Handler by editable element tag name, in addition to element ID and class attribute functionality that exists today.

Additionally, the Sanitize Content Handler initialization and setup has been refactored to cache Sanitizers per config. Also, the relevant section in the plugin_contenthandler guide page has been cleanup up to match released implementation plus this enhancement.

This code has been tested across browsers by our QE.

  1. obey coding guidelines
  2. write JSLint compliant code
  3. write JSDoc for every method you touched during your implementation
  4. write a human-readable :) Changelog entry that describes your change
  5. add a qunit test (if it makes sense) and/or document the steps to manually test it (in a separate guides page)
  6. test your changes in Internet Explorer 7, 8, 9 and the latest Firefox and latest Chrome
  7. document your changes in a guides page
  8. include this list in a your pull request

@Jotschi
Copy link
Contributor

Jotschi commented Feb 7, 2014

The diff sanitizecontenthandler.js is a little big. I think you changed the indentation of the whole file. It would be good to avoid such steps when doing changes. Only modify the place you are changing. It is okay to have another pull request or commit that deals with the indentation.

@deliminator
Copy link
Contributor

@Jotschi adding ?w=1 to the url has the same effect as git diff -w.
It's ok to change indentation, if -w shows the actual diff.

@ggalileo
Copy link
Contributor Author

Thanks for reviewing. The enhancement necessitated some code reorganization. If you diff with ?w=1 you will see that most of the change is more than just whitespace.


function initSanitize (configAllows) {
var
filter = [ 'restricted', 'basic', 'relaxed' ],
Copy link
Contributor

Choose a reason for hiding this comment

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

There was functionality in place to allow Aloha.settings.contentHandler.sanitize to be either 'restricted', 'basic', or 'relaxed'. This change would remove this functionality. Is that intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This goal of this enhancement is to support both ways, the existing restricted, basic, or relaxed as well as sanitization config by editable element tag name, id, or class.

In the initDefault function, we look for what is configured in Aloha.settings.contentHandler.santitize and use that for default config. This will maintain backward compatibility with existing configs.

        var initDefault = function() {
            var config = Aloha.defaults.sanitize.relaxed;

            if (Aloha.settings.contentHandler) {
                if (Aloha.settings.contentHandler.allows) {
                    config = Aloha.settings.contentHandler.allows;
                } else if (Aloha.settings.contentHandler.sanitize && Aloha.defaults.sanitize[Aloha.settings.contentHandler.sanitize]) {
                    config = Aloha.defaults.sanitize[Aloha.settings.contentHandler.sanitize];
                }
            }

Then in the initEditableSpecific function we initialize any editable specific config that will take priority over the default restricted, basic , or relaxed if editable specific config is provided and it matches the current editable.

        var initEditableSpecific = function() {
            if (Aloha.settings.contentHandler &&
                Aloha.settings.contentHandler.handler &&
                Aloha.settings.contentHandler.handler.sanitize) {
                var config, editableSelector;
                for (editableSelector in Aloha.settings.contentHandler.handler.sanitize) {
                    if (Aloha.settings.contentHandler.handler.sanitize.hasOwnProperty(editableSelector)) {
                        config = Aloha.settings.contentHandler.handler.sanitize[editableSelector];
                        config.filters = filters;
                        editableSelector = normalizeTagName(editableSelector);
                        editableSelector = /^[\.#]/.test(editableSelector) ? editableSelector : editableSelector.toLowerCase();
                        map[editableSelector] = new Sanitize(config, jQuery);
                    }
                }
            }
        };

I tried to convey this concept in the guide page.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks I get it now. The functionality is still there, except the inArray(Aloha.settings.contentHandler.sanitize, filter) check was removed because it's unnecessary.

@GenticsDev
Copy link
Contributor

Can one of the admins verify this patch?

@deliminator
Copy link
Contributor

@GenticsDev waiting for feedback.

@Jotschi
Copy link
Contributor

Jotschi commented Feb 19, 2014

Admin please build this request

@GenticsDev
Copy link
Contributor

Test FAILed.
Refer to this link for build results: http://jenkins.office:8080/job/alohaeditor-upstream-pullrequests/8/

@Jotschi
Copy link
Contributor

Jotschi commented Feb 19, 2014

Admin please build this request

@GenticsDev
Copy link
Contributor

Test PASSed.
Refer to this link for build results: http://jenkins.office:8080/job/alohaeditor-upstream-pullrequests/20/

@ggalileo
Copy link
Contributor Author

ggalileo commented Mar 4, 2014

@deliminator Per your comment, I updated this PR to use the editable passed to handleContent() rather than Aloha.activeEditable.

@Jotschi
Copy link
Contributor

Jotschi commented Mar 5, 2014

Admin please build this request

@GenticsDev
Copy link
Contributor

✔ Test passed.
Refer to this link for build results: http://jenkins.office:8080/job/alohaeditor-upstream-pullrequests/46/

@GenticsDev
Copy link
Contributor

Can one of the admins verify this patch?

1 similar comment
@GenticsDev
Copy link
Contributor

Can one of the admins verify this patch?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants