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

Fix problem input [FC-0076] #1646

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 38 additions & 20 deletions src/editors/sharedComponents/TinyMceWidget/hooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,40 @@ export const getImageResizeHandler = ({ editor, imagesRef, setImage }) => () =>
});
};

/**
* Fix TinyMCE editors used in Paragon modals, by re-parenting their modal <div>
* from the body to the Paragon modal container.
*
* This fixes a problem where clicking on any modal/popup within TinyMCE (e.g.
* the emoji inserter, the link inserter, the floating format toolbar -
* quickbars, etc.) would cause the parent Paragon modal to close, because
* Paragon sees it as a "click outside" event. Also fixes some hover effects by
* ensuring the layering of the divs is correct.
*
* This could potentially cause problems if there are TinyMCE editors being used
* both on the parent page and inside a Paragon modal popup, but I don't think
* we have that situation.
Comment on lines +161 to +163
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this concern be resolved by passing editor.bodyElement to this function, and using the .closest() selector like you do below, instead of searching across the whole document?

To be clear, I didn't find any problems with using document in the problem editor, this is just a suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure, but I suspect that wouldn't help, because the various DIVs created to hold modals are always appended to the end of the body element and there's no way to distinguish between the ones that are for editors outside the modal and the ones that are for editors inside the modal.

What we really need is a TinyMCE API to get its modal DIV(s) given a reference to the editor object, but I looked at the entire API and the code and played around with the editor object in the JS console, and it doesn't seem to expose that publicly :/

*
* Note: we can't just do this on init, because the quickbars plugin used by
* ExpandableTextEditors creates its modal DIVs later. Ideally we could listen
* for some kind of "modal open" event, but I haven't been able to find anything
* like that so for now we do this quite frequently, every time there is a
* "selectionchange" event (which is pretty often).
*/
export const reparentTinyMceModals = /* istanbul ignore next */ () => {
const modalLayer = document.querySelector('.pgn__modal-layer');
if (!modalLayer) {
return;
}
const tinymceAuxDivs = document.querySelectorAll('.tox.tox-tinymce-aux');
for (const tinymceAux of tinymceAuxDivs) {
if (tinymceAux.parentElement !== modalLayer) {
// Move this tinyMCE modal div into the paragon modal layer.
modalLayer.appendChild(tinymceAux);
}
}
};

export const setupCustomBehavior = ({
updateContent,
openImgModal,
Expand Down Expand Up @@ -221,30 +255,14 @@ export const setupCustomBehavior = ({
}

editor.on('init', /* istanbul ignore next */ () => {
// Moving TinyMce aux modal inside the Editor modal
// if the editor is on modal mode.
// This is to avoid issues using the aux modal:
// * Avoid close aux modal when clicking the content inside.
// * When the user opens the `Edit Source Code` modal, this adds `data-focus-on-hidden`
// to the TinyMce aux modal, making it unusable.
const modalLayer = document.querySelector('.pgn__modal-layer');
const tinymceAux = document.querySelector('.tox.tox-tinymce-aux');

if (modalLayer && tinymceAux) {
modalLayer.appendChild(tinymceAux);
if (editor.bodyElement?.closest('.pgn__modal')) {
// This editor is inside a Paragon modal. Use this hack to avoid interference with TinyMCE's own modal popups:
reparentTinyMceModals();
editor.on('selectionchange', reparentTinyMceModals);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The only editing issue I had here was when one of my problem's answers is an image:

image.as.answer.mp4

There's no way to select the embedded alt text to re-edit that image. But this was happening before, and is unrelated to this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Weird. I wonder why it renders only the alt text and doesn't just show the (selectable) image. Would you mind opening that as a separate issue on this repo?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure thing: #1654

});

editor.on('ExecCommand', /* istanbul ignore next */ (e) => {
// Remove `data-focus-on-hidden` and `aria-hidden` on TinyMce aux modal used on emoticons, formulas, etc.
// When using the Editor in modal mode, it may happen that the editor modal is rendered
// before the TinyMce aux modal, which adds these attributes, making the TinyMce aux modal unusable.
const modalElement = document.querySelector('.tox.tox-silver-sink.tox-tinymce-aux');
if (modalElement) {
modalElement.removeAttribute('data-focus-on-hidden');
modalElement.removeAttribute('aria-hidden');
}

if (editorType === 'text' && e.command === 'mceFocus') {
const initialContent = editor.getContent();
const newContent = module.replaceStaticWithAsset({
Expand Down
13 changes: 3 additions & 10 deletions src/editors/sharedComponents/TinyMceWidget/pluginConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,16 +64,9 @@ const pluginConfig = ({ placeholder, editorType, enableImageUpload }) => {
[editImageSettings],
]),
quickbarsInsertToolbar: toolbar ? false : mapToolbars([
[buttons.undo, buttons.redo],
[buttons.formatSelect],
[buttons.bold, buttons.italic, buttons.underline, buttons.foreColor],
[
buttons.align.justify,
buttons.bullist,
buttons.numlist,
],
[imageUploadButton, buttons.blockQuote, buttons.codeBlock],
[buttons.table, buttons.emoticons, buttons.charmap, buttons.removeFormat, buttons.a11ycheck],
// To keep from blocking the whole text input field when it's empty, this "insert" toolbar
// used with ExpandableTextArea is kept as minimal as we can.
[imageUploadButton, buttons.table],
]),
quickbarsSelectionToolbar: toolbar ? false : mapToolbars([
[buttons.undo, buttons.redo],
Expand Down
4 changes: 1 addition & 3 deletions src/header/Header.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import { getConfig } from '@edx/frontend-platform';
import { useIntl } from '@edx/frontend-platform/i18n';
import { StudioHeader } from '@edx/frontend-component-header';
import { type Container, useToggle } from '@openedx/paragon';
import { generatePath, useHref } from 'react-router-dom';

import { getWaffleFlags } from '../data/selectors';
import { SearchModal } from '../search-modal';
Expand Down Expand Up @@ -32,7 +31,6 @@ const Header = ({
containerProps = {},
}: HeaderProps) => {
const intl = useIntl();
const libraryHref = useHref('/library/:libraryId');
const waffleFlags = useSelector(getWaffleFlags);

const [isShowSearchModalOpen, openSearchModal, closeSearchModal] = useToggle(false);
Expand Down Expand Up @@ -63,7 +61,7 @@ const Header = ({

const getOutlineLink = () => {
if (isLibrary) {
return generatePath(libraryHref, { libraryId: contextId });
return `/library/${contextId}`;
}
return waffleFlags.useNewCourseOutlinePage ? `/course/${contextId}` : `${studioBaseUrl}/course/${contextId}`;
};
Expand Down