From 449c25459c917b546cdfd786dbfd56dd3cb9a43a Mon Sep 17 00:00:00 2001
From: Andrew Duthie <andrew@andrewduthie.com>
Date: Mon, 13 May 2019 11:48:20 -0400
Subject: [PATCH] Rich Text: Set applied format as active in applyFormats
 (#15573)

* Rich Text: Set applied format as active in applyFormats

* Rich Text: Update toggleFormat tests per activeFormats revision

* Rich Text: Replace existing active format in applied

* Rich Text: Remove active format in all cases for removeFormat

* Rich Text: Update toggleFormat tests per removeFormats revision

* Testing: Update E2E test case to verify link display regression

* Rich Text: Verify by test of activeFormats format replacement
---
 .../specs/__snapshots__/links.test.js.snap    |  6 +++
 packages/e2e-tests/specs/links.test.js        | 43 +++++++++++++++++++
 packages/rich-text/src/apply-format.js        | 23 +++++-----
 packages/rich-text/src/remove-format.js       | 11 +++--
 packages/rich-text/src/test/apply-format.js   | 11 +++++
 packages/rich-text/src/test/remove-format.js  |  2 +
 packages/rich-text/src/test/toggle-format.js  |  2 +
 7 files changed, 82 insertions(+), 16 deletions(-)

diff --git a/packages/e2e-tests/specs/__snapshots__/links.test.js.snap b/packages/e2e-tests/specs/__snapshots__/links.test.js.snap
index d63515e5d2ee34..292ac9e5781a92 100644
--- a/packages/e2e-tests/specs/__snapshots__/links.test.js.snap
+++ b/packages/e2e-tests/specs/__snapshots__/links.test.js.snap
@@ -47,3 +47,9 @@ exports[`Links should contain a label when it should open in a new tab 1`] = `
 <p>This is <a href=\\"http://w.org\\" target=\\"_blank\\" rel=\\"noreferrer noopener\\" aria-label=\\"WordPress (opens in a new tab)\\">WordPress</a></p>
 <!-- /wp:paragraph -->"
 `;
+
+exports[`Links should contain a label when it should open in a new tab 2`] = `
+"<!-- wp:paragraph -->
+<p>This is <a href=\\"http://wordpress.org\\">WordPress</a></p>
+<!-- /wp:paragraph -->"
+`;
diff --git a/packages/e2e-tests/specs/links.test.js b/packages/e2e-tests/specs/links.test.js
index 84563375547b2e..a1d1dd3a46934b 100644
--- a/packages/e2e-tests/specs/links.test.js
+++ b/packages/e2e-tests/specs/links.test.js
@@ -503,5 +503,48 @@ describe( 'Links', () => {
 		await page.keyboard.press( 'Enter' );
 
 		expect( await getEditedPostContent() ).toMatchSnapshot();
+
+		// Regression Test: This verifies that the UI is updated according to
+		// the expected changed values, where previously the value could have
+		// fallen out of sync with how the UI is displayed (specifically for
+		// collapsed selections).
+		//
+		// See: https://github.com/WordPress/gutenberg/pull/15573
+
+		// Collapse selection.
+		await page.keyboard.press( 'ArrowLeft' );
+		await page.keyboard.press( 'ArrowRight' );
+		// Edit link.
+		await pressKeyWithModifier( 'primary', 'k' );
+		await waitForAutoFocus();
+		await pressKeyWithModifier( 'primary', 'a' );
+		await page.keyboard.type( 'wordpress.org' );
+		// Navigate to the settings toggle.
+		await page.keyboard.press( 'Tab' );
+		await page.keyboard.press( 'Tab' );
+		// Open settings.
+		await page.keyboard.press( 'Space' );
+		// Navigate to the "Open in New Tab" checkbox.
+		await page.keyboard.press( 'Tab' );
+		// Uncheck the checkbox.
+		await page.keyboard.press( 'Space' );
+		// Navigate back to the input field.
+		await page.keyboard.press( 'Tab' );
+		// Submit the form.
+		await page.keyboard.press( 'Enter' );
+
+		// Navigate back to inputs to verify appears as changed.
+		await pressKeyWithModifier( 'primary', 'k' );
+		await waitForAutoFocus();
+		const link = await page.evaluate( () => document.activeElement.value );
+		expect( link ).toBe( 'http://wordpress.org' );
+		await page.keyboard.press( 'Tab' );
+		await page.keyboard.press( 'Tab' );
+		await page.keyboard.press( 'Space' );
+		await page.keyboard.press( 'Tab' );
+		const isChecked = await page.evaluate( () => document.activeElement.checked );
+		expect( isChecked ).toBe( false );
+
+		expect( await getEditedPostContent() ).toMatchSnapshot();
 	} );
 } );
diff --git a/packages/rich-text/src/apply-format.js b/packages/rich-text/src/apply-format.js
index c2b96af10d6002..03a9ddc9f94d5c 100644
--- a/packages/rich-text/src/apply-format.js
+++ b/packages/rich-text/src/apply-format.js
@@ -2,7 +2,7 @@
  * External dependencies
  */
 
-import { find } from 'lodash';
+import { find, reject } from 'lodash';
 
 /**
  * Internal dependencies
@@ -34,7 +34,7 @@ export function applyFormat(
 	startIndex = value.start,
 	endIndex = value.end
 ) {
-	const { formats, activeFormats = [] } = value;
+	const { formats, activeFormats } = value;
 	const newFormats = formats.slice();
 
 	// The selection is collapsed.
@@ -59,13 +59,6 @@ export function applyFormat(
 					replace( newFormats[ endIndex ], index, format );
 				endIndex++;
 			}
-		// Otherwise, insert a placeholder with the format so new input appears
-		// with the format applied.
-		} else {
-			return {
-				...value,
-				activeFormats: [ ...activeFormats, format ],
-			};
 		}
 	} else {
 		// Determine the highest position the new format can be inserted at.
@@ -92,5 +85,15 @@ export function applyFormat(
 		}
 	}
 
-	return normaliseFormats( { ...value, formats: newFormats } );
+	return normaliseFormats( {
+		...value,
+		formats: newFormats,
+		// Always revise active formats. This serves as a placeholder for new
+		// inputs with the format so new input appears with the format applied,
+		// and ensures a format of the same type uses the latest values.
+		activeFormats: [
+			...reject( activeFormats, { type: format.type } ),
+			format,
+		],
+	} );
 }
diff --git a/packages/rich-text/src/remove-format.js b/packages/rich-text/src/remove-format.js
index 4a4c9c820f9008..5ce2fcd7ce8333 100644
--- a/packages/rich-text/src/remove-format.js
+++ b/packages/rich-text/src/remove-format.js
@@ -48,11 +48,6 @@ export function removeFormat(
 				filterFormats( newFormats, endIndex, formatType );
 				endIndex++;
 			}
-		} else {
-			return {
-				...value,
-				activeFormats: reject( activeFormats, { type: formatType } ),
-			};
 		}
 	} else {
 		for ( let i = startIndex; i < endIndex; i++ ) {
@@ -62,7 +57,11 @@ export function removeFormat(
 		}
 	}
 
-	return normaliseFormats( { ...value, formats: newFormats } );
+	return normaliseFormats( {
+		...value,
+		formats: newFormats,
+		activeFormats: reject( activeFormats, { type: formatType } ),
+	} );
 }
 
 function filterFormats( formats, index, formatType ) {
diff --git a/packages/rich-text/src/test/apply-format.js b/packages/rich-text/src/test/apply-format.js
index 67d2c3ff082971..2c74bc1d9aa269 100644
--- a/packages/rich-text/src/test/apply-format.js
+++ b/packages/rich-text/src/test/apply-format.js
@@ -23,6 +23,7 @@ describe( 'applyFormat', () => {
 		};
 		const expected = {
 			...record,
+			activeFormats: [ em ],
 			formats: [ [ em ], [ em ], [ em ], [ em ] ],
 		};
 		const result = applyFormat( deepFreeze( record ), em, 0, 4 );
@@ -39,6 +40,7 @@ describe( 'applyFormat', () => {
 		};
 		const expected = {
 			...record,
+			activeFormats: [ em ],
 			formats: [ [ strong, em ], [ strong, em ], [ strong, em ], [ strong, em ] ],
 		};
 		const result = applyFormat( deepFreeze( record ), em, 0, 4 );
@@ -55,6 +57,7 @@ describe( 'applyFormat', () => {
 		};
 		const expected = {
 			...record,
+			activeFormats: [ em ],
 			formats: [ [ strong, em ], [ strong, em ], [ strong, em ], [ strong, em ] ],
 		};
 		const result = applyFormat( deepFreeze( record ), em, 0, 4 );
@@ -71,6 +74,7 @@ describe( 'applyFormat', () => {
 		};
 		const expected = {
 			...record,
+			activeFormats: [ strong ],
 			formats: [ [ strong ], [ strong, em ], [ strong, em ], [ strong ] ],
 		};
 		const result = applyFormat( deepFreeze( record ), strong, 0, 4 );
@@ -87,6 +91,7 @@ describe( 'applyFormat', () => {
 		};
 		const expected = {
 			...record,
+			activeFormats: [ strong ],
 			formats: [ [ strong ], [ strong, em ], [ strong, em ], , ],
 		};
 		const result = applyFormat( deepFreeze( record ), strong, 0, 3 );
@@ -103,6 +108,7 @@ describe( 'applyFormat', () => {
 		};
 		const expected = {
 			...record,
+			activeFormats: [ strong ],
 			formats: [ , [ strong, em ], [ strong, em ], [ strong ] ],
 		};
 		const result = applyFormat( deepFreeze( record ), strong, 1, 4 );
@@ -119,6 +125,7 @@ describe( 'applyFormat', () => {
 		};
 		const expected = {
 			...record,
+			activeFormats: [ strong ],
 			formats: [ , [ strong, em ], [ strong ], [ strong, em ] ],
 		};
 		const result = applyFormat( deepFreeze( record ), strong, 1, 4 );
@@ -134,6 +141,7 @@ describe( 'applyFormat', () => {
 			text: 'one two three',
 		};
 		const expected = {
+			activeFormats: [ strong ],
 			formats: [ , , , [ strong ], [ strong, em ], [ strong, em ], [ em ], , , , , , , ],
 			text: 'one two three',
 		};
@@ -152,6 +160,7 @@ describe( 'applyFormat', () => {
 			end: 6,
 		};
 		const expected = {
+			activeFormats: [ strong ],
 			formats: [ , , , [ strong ], [ strong, em ], [ strong, em ], [ em ], , , , , , , ],
 			text: 'one two three',
 			start: 3,
@@ -184,12 +193,14 @@ describe( 'applyFormat', () => {
 
 	it( 'should apply format on existing format if selection is collapsed', () => {
 		const record = {
+			activeFormats: [ a ],
 			formats: [ , , , , [ a ], [ a ], [ a ], , , , , , , ],
 			text: 'one two three',
 			start: 4,
 			end: 4,
 		};
 		const expected = {
+			activeFormats: [ a2 ],
 			formats: [ , , , , [ a2 ], [ a2 ], [ a2 ], , , , , , , ],
 			text: 'one two three',
 			start: 4,
diff --git a/packages/rich-text/src/test/remove-format.js b/packages/rich-text/src/test/remove-format.js
index 4d6b10f0d27fbe..343eb8c47e011f 100644
--- a/packages/rich-text/src/test/remove-format.js
+++ b/packages/rich-text/src/test/remove-format.js
@@ -21,6 +21,7 @@ describe( 'removeFormat', () => {
 		};
 		const expected = {
 			formats: [ , , , , [ em ], [ em ], [ em ], , , , , , , ],
+			activeFormats: [],
 			text: 'one two three',
 		};
 		const result = removeFormat( deepFreeze( record ), 'strong', 3, 6 );
@@ -37,6 +38,7 @@ describe( 'removeFormat', () => {
 		};
 		const expected = {
 			formats: [ , , , , [ em ], [ em ], [ em ], , , , , , , ],
+			activeFormats: [],
 			text: 'one two three',
 		};
 		const result = removeFormat( deepFreeze( record ), 'strong', 4, 4 );
diff --git a/packages/rich-text/src/test/toggle-format.js b/packages/rich-text/src/test/toggle-format.js
index 28762a37b1603c..9613e970d40a4e 100644
--- a/packages/rich-text/src/test/toggle-format.js
+++ b/packages/rich-text/src/test/toggle-format.js
@@ -23,6 +23,7 @@ describe( 'toggleFormat', () => {
 		};
 		const expected = {
 			formats: [ , , , , [ em ], [ em ], [ em ], , , , , , , ],
+			activeFormats: [],
 			text: 'one two three',
 			start: 3,
 			end: 6,
@@ -43,6 +44,7 @@ describe( 'toggleFormat', () => {
 		};
 		const expected = {
 			formats: [ , , , [ strong ], [ strong, em ], [ strong, em ], [ em ], , , , , , , ],
+			activeFormats: [ strong ],
 			text: 'one two three',
 			start: 3,
 			end: 6,