Properly implement default background element (#1798)

* Split `duplicatePage` from `createPage` and support new props

* Implemented, documented and tested new default background behavior

* Changed story reducer to new background element structure

* Fix allowed properties per page and element

* Added new migration for default background elements

* Updated all references to background element ids

* Fixed new background element rules in combineElements and added test of that

* Update selection after merge

* Fix some missing test coverage

* Fix test comment

* Updated logic around dragging directly from library to match creating element and then dragging

* Fix test comment

Author: Morten Barklund

assets/src/edit-story/app/story/effects/usePageBackgrounds.js

@@ -29,7 +29,6 @@ import useLoadStory from './effects/useLoadStory';
 import useSaveStory from './actions/useSaveStory';
 import useHistoryEntry from './effects/useHistoryEntry';
 import useHistoryReplay from './effects/useHistoryReplay';
-import usePageBackgrounds from './effects/usePageBackgrounds';
 import useStoryReducer from './useStoryReducer';
 import useDeleteStory from './actions/useDeleteStory';
 import useAutoSave from './actions/useAutoSave';
@@ -93,13 +92,6 @@ function StoryProvider({ storyId, children }) {
   useHistoryEntry({ pages, current, selection, story, capabilities });
   useHistoryReplay({ restore });
 
-  // Ensure all pages have a background element at all times
-  usePageBackgrounds({
-    currentPage,
-    setBackgroundElement: api.setBackgroundElement,
-    addElement: api.addElement,
-  });
-
   // This action allows the user to save the story
   // (and it will have side-effects because saving can update url and status,
   //  thus the need for `updateStory`)

assets/src/edit-story/app/story/storyProvider.js

@@ -86,10 +86,10 @@ const updateSelectedElements = (dispatch) => ({ properties }) =>
     payload: { elementIds: null, properties },
   });
 
-const combineElements = (dispatch) => ({ firstId, secondId }) =>
+const combineElements = (dispatch) => ({ firstId, firstElement, secondId }) =>
   dispatch({
     type: types.COMBINE_ELEMENTS,
-    payload: { firstId, secondId },
+    payload: { firstId, firstElement, secondId },
   });
 
 const setBackgroundElement = (dispatch) => ({ elementId }) =>

assets/src/edit-story/app/story/useStoryReducer/actions.js

@@ -27,6 +27,8 @@ import { isInsideRange } from './utils';
  *
  * Current page will be updated to point to the newly inserted page.
  *
+ * New page must have at least one element, the default background element.
+ *
  * Selection is cleared.
  *
  * @param {Object} state Current state
@@ -45,10 +47,12 @@ function addPage(state, { page, position }) {
 
   const { id } = page;
 
-  // Ensure new page has elements array and background id
+  // Ensure new page has at least one element
+  if (!page.elements?.length >= 1) {
+    return state;
+  }
+
   const newPage = {
-    elements: [],
-    backgroundElementId: null,
     backgroundOverlay: OverlayType.NONE,
     ...page,
   };

assets/src/edit-story/app/story/useStoryReducer/reducers/addPage.js

@@ -62,20 +62,20 @@ function arrangeElement(state, { elementId, position }) {
 
   const page = state.pages[pageIndex];
 
-  // Abort if there's less than two elements (nothing to rearrange)
-  if (page.elements.length < 2) {
+  // Abort if there's less than three elements (nothing to rearrange as first is bg)
+  if (page.elements.length < 3) {
     return state;
   }
 
   const currentPosition = page.elements.findIndex(
     ({ id }) => id === idToArrange
   );
 
-  if (currentPosition === -1 || page.backgroundElementId === idToArrange) {
+  if (currentPosition === -1 || page.elements[0].id === idToArrange) {
     return state;
   }
 
-  const minPosition = page.backgroundElementId ? 1 : 0;
+  const minPosition = 1;
   const maxPosition = page.elements.length - 1;
   const newPosition = getAbsolutePosition({
     currentPosition,

assets/src/edit-story/app/story/useStoryReducer/reducers/arrangeElement.js

@@ -15,53 +15,124 @@
  */
 
 /**
- * Internal dependencies
+ * External dependencies
  */
-import deleteElements from './deleteElements';
-import updateElements from './updateElements';
+import { v4 as uuidv4 } from 'uuid';
 
 /**
  * Combine elements by taking properties from a first item and
- * adding them to the given second item
+ * adding them to the given second item, then remove first item.
+ *
+ *
+ * First item must either to be the id of an existing element with
+ * a resource (some media element, or be given as the properties of
+ * an element (with a resource) that doesn't exist but whose properties
+ * should be merged into the target element.
+ *
+ * If second item is background element, copy some extra properties not
+ * relevant while background, but relevant if unsetting as background.
+ *
+ * If the second item is the default background element,
+ * save a copy of the old element as appropriate and remove flag.
+ *
+ * Updates selection to only include the second item after merge.
  *
  * @param {Object} state Current state
  * @param {Object} payload Action payload
  * @param {string} payload.firstId Element to take properties from
+ * @param {string} payload.firstElement Element with properties to merge
  * @param {string} payload.secondId Element to add properties to
  * @return {Object} New state
  */
-function combineElements(state, { firstId, secondId }) {
-  if (!firstId || !secondId) {
+function combineElements(state, { firstId, firstElement, secondId }) {
+  if ((!firstId && !firstElement) || !secondId) {
     return state;
   }
 
-  const page = state.pages.find(({ id }) => id === state.current);
-  const element = page.elements.find(({ id }) => id === firstId);
+  const pageIndex = state.pages.findIndex(({ id }) => id === state.current);
+  const page = state.pages[pageIndex];
+  const elementPosition = page.elements.findIndex(({ id }) => id === firstId);
+  const element =
+    elementPosition > -1 ? page.elements[elementPosition] : firstElement;
+
+  const secondElementPosition = page.elements.findIndex(
+    ({ id }) => id === secondId
+  );
+  const secondElement = page.elements[secondElementPosition];
 
-  if (!element || !element.resource) {
+  if (!element || !element.resource || !secondElement) {
     return state;
   }
 
+  const newPageProps = secondElement.isDefaultBackground
+    ? {
+        defaultBackgroundElement: {
+          ...secondElement,
+          // But generate a new ID for this temp background element
+          id: uuidv4(),
+        },
+      }
+    : {};
+
   const {
     resource,
     scale = 100,
     focalX = 50,
     focalY = 50,
     isFill = false,
+    width,
+    height,
+    x,
+    y,
   } = element;
 
-  const updatedState = updateElements(state, {
-    elementIds: [secondId],
-    properties: {
-      resource,
-      type: resource.type,
-      scale,
-      focalX,
-      focalY,
-      isFill,
-    },
-  });
-  return deleteElements(updatedState, { elementIds: [firstId] });
+  const newElement = {
+    ...secondElement,
+    resource,
+    type: resource.type,
+    scale,
+    focalX,
+    focalY,
+    isFill,
+    // Only remember these for backgrounds, as they're ignored while being background
+    ...(secondElement.isBackground
+      ? {
+          width,
+          height,
+          x,
+          y,
+        }
+      : {}),
+    // Only unset if it was set
+    ...(secondElement.isDefaultBackground
+      ? { isDefaultBackground: false }
+      : {}),
+  };
+
+  // Elements are now
+  const elements = page.elements
+    // Remove first element if combining from existing id
+    .filter(({ id }) => id !== firstId)
+    // Update reference to second element
+    .map((el) => (el.id === secondId ? newElement : el));
+
+  const newPage = {
+    ...page,
+    elements,
+    ...newPageProps,
+  };
+
+  const newPages = [
+    ...state.pages.slice(0, pageIndex),
+    newPage,
+    ...state.pages.slice(pageIndex + 1),
+  ];
+
+  return {
+    ...state,
+    pages: newPages,
+    selection: [secondId],
+  };
 }
 
 export default combineElements;

assets/src/edit-story/app/story/useStoryReducer/reducers/combineElements.js

@@ -17,7 +17,6 @@
 /**
  * Internal dependencies
  */
-import { OverlayType } from '../../../../utils/backgroundOverlay';
 import { intersect } from './utils';
 
 /**
@@ -30,7 +29,8 @@ import { intersect } from './utils';
  *
  * If an empty list or a list of only unknown ids is given, state is unchanged.
  *
- * If any id to delete is the current background element, background element will be unset for the page.
+ * If any id to delete is the current background element, background element will be unset for the page,
+ * and default background element will be restored. Default background element cannot be removed.
  *
  * If any id to delete is in current selection, deleted ids are removed from selection.
  * Otherwise selection is unchanged.
@@ -53,41 +53,50 @@ function deleteElements(state, { elementIds }) {
 
   const oldPage = state.pages[pageIndex];
   const pageElementIds = oldPage.elements.map(({ id }) => id);
+  const backgroundElement = oldPage.elements[0];
+
+  const isDeletingBackground = idsToDelete.some(
+    (id) => id === backgroundElement.id
+  );
+  const backgroundIsDefault = backgroundElement.isDefaultBackground;
+
+  const validDeletionIds =
+    isDeletingBackground && backgroundIsDefault
+      ? idsToDelete.filter((id) => id !== backgroundElement.id)
+      : idsToDelete;
 
   // Nothing to delete?
-  const hasAnythingToDelete = intersect(pageElementIds, idsToDelete).length > 0;
+  const hasAnythingToDelete =
+    intersect(pageElementIds, validDeletionIds).length > 0;
   if (!hasAnythingToDelete) {
     return state;
   }
 
-  const filteredElements = oldPage.elements.filter(
-    (element) => !idsToDelete.includes(element.id)
+  let newElements = oldPage.elements.filter(
+    (element) => !validDeletionIds.includes(element.id)
   );
 
+  // Restore default background if non-default bg has been deleted.
+  if (isDeletingBackground && !backgroundIsDefault) {
+    newElements = [oldPage.defaultBackgroundElement, ...newElements];
+  }
+
   const newPage = {
     ...oldPage,
-    elements: filteredElements,
+    elements: newElements,
   };
 
-  // Clear background if it has been deleted.
-  if (
-    Boolean(oldPage.backgroundElementId) &&
-    idsToDelete.includes(oldPage.backgroundElementId)
-  ) {
-    newPage.backgroundElementId = null;
-    newPage.backgroundOverlay = OverlayType.NONE;
-  }
-
   const newPages = [
     ...state.pages.slice(0, pageIndex),
     newPage,
     ...state.pages.slice(pageIndex + 1),
   ];
 
   // This check is to make sure not to modify the selection array if no update is necessary.
-  const wasAnySelected = intersect(state.selection, idsToDelete).length > 0;
+  const wasAnySelected =
+    intersect(state.selection, validDeletionIds).length > 0;
   const newSelection = wasAnySelected
-    ? state.selection.filter((id) => !idsToDelete.includes(id))
+    ? state.selection.filter((id) => !validDeletionIds.includes(id))
     : state.selection;
 
   return {

assets/src/edit-story/app/story/useStoryReducer/reducers/deleteElements.js

@@ -30,15 +30,15 @@ function selectElement(state, { elementId }) {
   }
 
   const currentPage = state.pages.find(({ id }) => id === state.current);
-  const isBackgroundElement = currentPage.backgroundElementId === elementId;
+  const isBackgroundElement = currentPage.elements[0].id === elementId;
   const hasExistingSelection = state.selection.length > 0;
   // The bg element can't be added to non-empty selection
   if (isBackgroundElement && hasExistingSelection) {
     return state;
   }
 
   // If bg element was already the (only) selection, set selection to new element only
-  if (state.selection.includes(currentPage.backgroundElementId)) {
+  if (state.selection.includes(currentPage.elements[0].id)) {
     return {
       ...state,
       selection: [elementId],