Skip to content

Commit

Permalink
Fixes part of #9749: Migrate few instances of angular-html-bind (#14263)
Browse files Browse the repository at this point in the history 
* Revert "Revert "Fix part of #9749: Create a component that shows rte-output-display and use it instead of angular-html-bind. (#13229)" (#13361)"

This reverts commit 792de45.

* Fix issues

* Fix spelling mistakes in comment

* Merge with upstream/develop

* Address review comments:
  1. Move oppia-rte-output.spec from the list of ignored files for innerHTML check in eslintrc and move to inline disables.
  2. Change the CODEOWNER listing from oppia-rte-parser.service.ts to oppia-rte-parser.service*.ts to include the spec file.
  3. Add refernce to the note at the top of file for all value.replace ops.
  4. Miscellaneous changes.
  • Loading branch information
@srijanreddy98 @seanlip
srijanreddy98 authored and seanlip committed Nov 27, 2021
1 parent 0958ade commit 3996a61
Show file tree
Hide file tree
Showing 20 changed files with 775 additions and 39 deletions.
2 changes: 2 additions & 0 deletions .github/CODEOWNERS
View file
Original file line number Diff line number Diff line change
Expand Up @@ -565,12 +565,14 @@
/core/templates/services/autoplayed-videos.service*.ts @aks681
/core/templates/services/external-save.service*.ts @srijanreddy98
/core/templates/services/external-rte-save.service*.ts @srijanreddy98
/core/templates/services/oppia-rte-parser.service*.ts @srijanreddy98 @aks681
/core/templates/services/image-local-storage*.ts @DubeySandeep
/core/templates/services/rte-helper.service*.ts @aks681
/core/templates/services/rte-helper-modal.controller*.ts @aks681
/core/domain/image_validation_services*.py @DubeySandeep
/core/domain/rte_component_registry*.py @vojtechjelinek
/extensions/ckeditor_plugins/ @aks681
/extensions/rich_text_components/rte-output-display.component* @srijanreddy98 @aks681
/extensions/rich_text_components/ @aks681
/assets/rich_text_components_definitions.ts @aks681

Expand Down
51 changes: 47 additions & 4 deletions core/templates/components/ck-editor-helpers/ck-editor-4-rte.component.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,26 @@

/**
* @fileoverview Directive for CK Editor.
* NOTE: The way we show rich text components in CKEditor is by using Web
* components. We don't create an angular view inside ckeditor. In our case,
* the web components can't have the same selector as the angular component even
* though they are literally the same component and use the same class. This is
* because using the same selector is causing issues in the angular view as
* angular creates a component instance and adds it to the view. When adding to
* the view, it will also create a node with the selector we have specified.
* Usually, this has no effect as there is no element in the web-browser
* registered by the selector. But in our case, we did it to show rte components
* in the ck-editor view.
*
* In order to overcome this situation, ck-editor uses the same component but we
* register it with a different selector. The selector prefix is now
* oppia-noninteractive-ckeditor-* instead of oppia-noninteractive we have for
* the angular counterpart. This just an internal representation and the value
* emitted to the parent component doesn't have oppia-noninteractive-ckeditor-*
* tags, They have the normal oppia-noninteractive tags in them. Similarly, for
* the value that's passed in, we don't expect oppia-noninteractive-ckeditor-*
* tags. We expect the normal angular version of our tags and that is converted
* on the fly.
*/

import { AfterViewInit, Component, ElementRef, EventEmitter, Input, OnChanges, OnDestroy, Output, SimpleChanges, OnInit } from '@angular/core';
Expand Down Expand Up @@ -99,6 +119,18 @@ export class CkEditor4RteComponent implements AfterViewInit, OnChanges,
// components may change without re-rendering each of the components,
// in such cases, it is sufficient to update the ckeditor instance manually
// with the latest value.
let value = this.value;
// Refer to the note at the top of the file for the reason behind replace.
value = value.replace(
/<oppia-noninteractive-/g,
'<oppia-noninteractive-ckeditor-'
);
// Refer to the note at the top of the file for the reason behind replace.
value = value.replace(
/<\/oppia-noninteractive-/g,
'</oppia-noninteractive-ckeditor-'
);
this.value = value;
if (this.ck && this.ck.status === 'ready' && changes.value) {
this.ck.setData(this.wrapComponents(this.value));
}
Expand Down Expand Up @@ -261,7 +293,7 @@ export class CkEditor4RteComponent implements AfterViewInit, OnChanges,
*/
// Whitelist the component tags with any attributes and classes.
var componentRule = names.map((name) => {
return 'oppia-noninteractive-' + name;
return 'oppia-noninteractive-ckeditor-' + name;
}).join(' ') + '(*)[*];';
// Whitelist the inline component wrapper, which is a
// span with a "type" attribute.
Expand Down Expand Up @@ -437,9 +469,20 @@ export class CkEditor4RteComponent implements AfterViewInit, OnChanges,
break;
}
}
this.valueChange.emit(elt.html());
this.value = elt.html();
this.currentValue = this.value;
let html = elt.html();
this.value = html;
// Refer to the note at the top of the file for the reason behind replace.
html = html.replace(
/<oppia-noninteractive-ckeditor-/g,
'<oppia-noninteractive-'
);
// Refer to the note at the top of the file for the reason behind replace.
html = html.replace(
/<\/oppia-noninteractive-ckeditor-/g,
'</oppia-noninteractive-'
);
this.valueChange.emit(html);
this.currentValue = html;
});
ck.setData(this.value);
this.ck = ck;
Expand Down
2 changes: 1 addition & 1 deletion core/templates/components/ck-editor-helpers/ck-editor-4-widgets.initializer.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ export class CkEditorInitializerService {
if (CKEDITOR.plugins.registered[ckName] !== undefined) {
return;
}
var tagName = 'oppia-noninteractive-' + componentDefn.id;
var tagName = 'oppia-noninteractive-ckeditor-' + componentDefn.id;
var customizationArgSpecs = componentDefn.customizationArgSpecs;
var isInline = rteHelperService.isInlineComponent(componentDefn.id);

Expand Down
8 changes: 4 additions & 4 deletions core/templates/components/ck-editor-helpers/ck-editor-copy-content.service.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,10 +112,10 @@ describe('Ck editor copy content service', () => {
service.toggleCopyMode();
expect(service.copyModeActive).toBe(true);
const imageWidgetElement = generateContent(
'<oppia-noninteractive-image alt-with-value="&amp;quot;&amp;quot;" capt' +
'ion-with-value="&amp;quot;Banana&amp;quot;" filepath-with-value="&amp;' +
'quot;img_20200630_114637_c2ek92uvb8_height_326_width_490.png&amp;quot;' +
'"></oppia-noninteractive-image>');
'<oppia-noninteractive-image ng-reflect-alt-with-value="&amp;" alt-with' +
'-value="&amp;quot;&amp;quot;" caption-with-value="&amp;quot;Banana&amp' +
';quot;" filepath-with-value="&amp;quot;img_20200630_114637_c2ek92uvb8_' +
'height_326_width_490.png&amp;quot;"></oppia-noninteractive-image>');

service.bindPasteHandler(ckEditorStub);
service.broadcastCopy(imageWidgetElement);
Expand Down
25 changes: 20 additions & 5 deletions core/templates/components/ck-editor-helpers/ck-editor-copy-content.service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ interface CkEditorCopyEvent {
})
export class CkEditorCopyContentService {
private readonly OUTPUT_VIEW_TAG_NAME = 'ANGULAR-HTML-BIND';
private readonly OUTPUT_NG_TAG_NAME = 'OPPIA-RTE-OUTPUT-DISPLAY';
private readonly NON_INTERACTIVE_TAG = '-noninteractive-';
private readonly ALLOWLISTED_WIDGETS = new Set([
'oppia-noninteractive-collapsible',
Expand Down Expand Up @@ -73,7 +74,11 @@ export class CkEditorCopyContentService {
containedWidgetTagName = currentTagName;
break;
}
if (parentElement.tagName === this.OUTPUT_VIEW_TAG_NAME) {

if (
parentElement.tagName === this.OUTPUT_VIEW_TAG_NAME ||
parentElement.tagName === this.OUTPUT_NG_TAG_NAME
) {
break;
}

Expand Down Expand Up @@ -122,26 +127,36 @@ export class CkEditorCopyContentService {
let elementTagName = (
containedWidgetTagName || element.tagName.toLowerCase());
let html = element.outerHTML;

html = html.replace(/<!--[^>]*-->/g, '').trim();
if (!containedWidgetTagName) {
editor.insertHtml(html);
} else {
const widgetName = elementTagName.replace('-noninteractive-', '');

// Look for x-with-value="y" to extract x and y.
// Group 1 (\w+): Any word containing [a-zA-Z0-9_] characters. This is
// the name of the property.
// Group 1 ([\w-]+): Any word containing [a-zA-Z0-9_-] characters. This
// is the name of the property. (Note that this will also catch
// ng-reflect-* which is added by angular).
// Group 2 (-with-value="): Matches characters literally.
// Group 3 ([^"]+): Matches any characters excluding ". This is the
// value of the property.
// Group 4 ("): Matches " literally.
const valueMatcher = /(\w+)(-with-value=")([^"]+)(")/g;
const valueMatcher = /([\w-]+)(-with-value=")([^"]+)(")/g;

let match;
let startupData: {[id: string]: string} = {};

while ((match = valueMatcher.exec(html)) !== null) {
const key = match[1];
// Angular adds a new attribute for each @Input() variable. The
// attribute starts with ng-reflect and the full attribute is
// ng-reflect-attribute-name. ng-reflect has char limit on how much
// is added to the dom. So if these are not ignored we will get data
// parsing errors in the components we are trying to copy over to the
// editor.
if (key.startsWith('ng-reflect')) {
continue;
}
// Must replace & for html escaper to properly work- html escaper
// service depends on & already escaped.
const value = match[3].replace(/&amp;/g, '&');
Expand Down
2 changes: 1 addition & 1 deletion core/templates/components/oppia-angular-root.component.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ export class OppiaAngularRootComponent implements AfterViewInit {
componentMap[rteKey].component_class,
{injector: this.injector});
customElements.define(
'oppia-noninteractive-' +
'oppia-noninteractive-ckeditor-' +
ServicesConstants.RTE_COMPONENT_SPECS[rteKey].frontend_id,
rteElement
);
Expand Down
6 changes: 3 additions & 3 deletions ...emplates/components/state-editor/state-content-editor/state-content-editor.directive.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,9 @@
<[getStateContentPlaceholder()]>
</span>
<span>
<angular-html-bind html-data="StateContentService.savedMemento.html"
class="oppia-rte-editor protractor-test-state-content-display">
</angular-html-bind>
<oppia-rte-output-display [rte-string]="StateContentService.savedMemento.html"
class="oppia-rte-editor protractor-test-state-content-display">
</oppia-rte-output-display>
</span>
</div>
<!-- This is a dummy div created to mask the contents when the user hovers over the content. -->
Expand Down
18 changes: 9 additions & 9 deletions ...mplates/pages/contributor-dashboard-page/modal-templates/translation-modal.component.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,12 @@
</div>
<div *ngIf="!loadingData && !uploadingTranslation" class="oppia-text-to-translate-container">
<!-- textToTranslate contains the html content of an exploration state -->
<angular-html-bind-wrapper *ngIf="activeDataFormat === 'html' || activeDataFormat === 'unicode'"
[ngClass]="{'oppia-rte-editor-focused': isCopyModeActive()}"
(click)="onContentClick($event)"
[htmlData]="textToTranslate"
class="oppia-rte-editor">
</angular-html-bind-wrapper>
<oppia-rte-output-display *ngIf="activeDataFormat === 'html' || activeDataFormat === 'unicode'"
[ngClass]="{'oppia-rte-editor-focused': isCopyModeActive()}"
(click)="onContentClick($event)"
[rteString]="textToTranslate"
class="oppia-rte-editor">
</oppia-rte-output-display>
<div *ngIf="isSetOfStringDataFormat()">
<strong>{{ activeRuleDescription }}</strong>
<ul class="oppia-option-list oppia-translate-rule-list" role="tablist">
Expand Down Expand Up @@ -87,9 +87,9 @@
</div>
<strong *ngIf="isSubmitted()">Submitted translation:</strong>
<div *ngIf="isSubmitted()" class="oppia-text-to-translate-container">
<angular-html-bind-wrapper [htmlData]="activeWrittenTranslation"
class="oppia-rte-editor">
</angular-html-bind-wrapper>
<oppia-rte-output-display [rteString]="activeWrittenTranslation"
class="oppia-rte-editor">
</oppia-rte-output-display>
</div>
<div *ngIf="TRANSLATION_TIPS[activeLanguageCode]" class="oppia-translation-tips-container">
<ul>
Expand Down
2 changes: 2 additions & 0 deletions ...utor-dashboard-page/translation-opportunities/translation-opportunities.component.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import { LazyLoadingComponent } from 'components/common-layout-directives/common
import { SchemaBasedEditorDirective } from 'components/forms/schema-based-editors/schema-based-editor.directive';
import { AngularHtmlBindWrapperDirective } from 'components/angular-html-bind/angular-html-bind-wrapper.directive';
import { CkEditorCopyToolbarComponent } from 'components/ck-editor-helpers/ck-editor-copy-toolbar/ck-editor-copy-toolbar.component';
import { NO_ERRORS_SCHEMA } from '@angular/core';
import { EventEmitter } from '@angular/core';

describe('Translation opportunities component', () => {
Expand Down Expand Up @@ -78,6 +79,7 @@ describe('Translation opportunities component', () => {
NgbModal,
NgbActiveModal
],
schemas: [NO_ERRORS_SCHEMA]
}).compileComponents();
translationModal = TestBed.createComponent(
TranslationModalComponent) as unknown as NgbModalRef;
Expand Down
10 changes: 5 additions & 5 deletions ...xploration-editor-page/translation-tab/state-translation/state-translation.component.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,11 +57,11 @@
<[getEmptyContentMessage()]>
</span>
<span>
<angular-html-bind ng-class="{'oppia-rte-editor-focused': isCopyModeActive()}"
ng-click="onContentClick($event)"
html-data="getRequiredHtml(stateContent)"
class="oppia-rte-editor">
</angular-html-bind>
<oppia-rte-output-display [rte-string]="getRequiredHtml(stateContent)"
ng-class="{'oppia-rte-editor-focused': isCopyModeActive()}"
ng-click="onContentClick($event)"
class="oppia-rte-editor">
</oppia-rte-output-display>
</span>
</span>
</div>
Expand Down
6 changes: 3 additions & 3 deletions core/templates/pages/exploration-player-page/learner-experience/tutor-card.component.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@
[ngClass]="getContentAudioHighlightClass()"
[oppiaFocusOn]="getContentFocusLabel($index)">
<div tabindex="1">
<angular-html-bind-wrapper classStr="protractor-test-conversation-content"
[htmlData]="displayedCard.getContentHtml()">
</angular-html-bind-wrapper>
<oppia-rte-output-display [rteString]="displayedCard.getContentHtml()"
class="protractor-test-conversation-content">
</oppia-rte-output-display>
</div>
<div *ngIf="isContentAudioTranslationAvailable()"
class="conversation-skin-audio-controls">
Expand Down
2 changes: 2 additions & 0 deletions core/templates/services/angular-services.index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -370,6 +370,7 @@ import { ExplorationPlayerStateService } from 'pages/exploration-player-page/ser
import { TopicEditorRoutingService } from 'pages/topic-editor-page/services/topic-editor-routing.service';
import { SubtopicValidationService } from 'pages/topic-editor-page/services/subtopic-validation.service';
import { NavigationService } from './navigation.service';
import { OppiaRteParserService } from './oppia-rte-parser.service';
import { TopicEditorStateService } from 'pages/topic-editor-page/services/topic-editor-state.service';
import { ExplorationTagsService } from 'pages/exploration-editor-page/services/exploration-tags.service';
import { ExplorationLanguageCodeService } from 'pages/exploration-editor-page/services/exploration-language-code.service';
Expand Down Expand Up @@ -613,6 +614,7 @@ export const angularServices: [string, Type<{}>][] = [
['NumericInputRulesService', NumericInputRulesService],
['NumericInputValidationService', NumericInputValidationService],
['OutcomeObjectFactory', OutcomeObjectFactory],
['OppiaRteParserService', OppiaRteParserService],
['PageHeadService', PageHeadService],
['PageTitleService', PageTitleService],
['ParamChangeObjectFactory', ParamChangeObjectFactory],
Expand Down
Loading

0 comments on commit 3996a61

Please sign in to comment.