Render preview in a modal with responsive sizes. #18385

tellthemachines · 2019-11-07T23:13:59Z

Description

Addresses #13203

Render post preview inside a modal with a resizable iframe.
Add toggles to change preview size between desktop, tablet and mobile.
Add link to preview in a new tab (current behaviour).

I removed the interstitial screen for now because the autosave now happens when the overlay is opened so opening new tab is quicker.

Further iterations/ things to consider:

It would be nice if initial preview size matched the size of user's device, so if post was being edited on a tablet preview would open up in tablet mode. Make sure this works when users are zoomed in on desktop.
Figure out what the best experience is to show previews for devices larger than what the user is on. If viewing on a phone, should desktop preview be really zoomed out and tiny, or have a horizontal scroll?
Should preview sizes be customisable by themes, or should we make a call on what the most standard device sizes are?
Is interstitial markup still necessary now we're rendering preview in a modal?
Anything else I've forgotten?

How has this been tested?

Tested in multiple browsers, keyboard navigation checked and also checked with VoiceOver and NVDA.

Updated unit and e2e tests.

Screenshots

Types of changes

Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.

talldan

This is working nicely. The code seems pretty tidy, all the comments should be easy to fix.

The only thing I noticed in testing is that on a slower connection the iframe content can take a little while to load, so I wonder if we want some kind of placeholder or loading spinner to be displayed.

talldan · 2019-12-04T09:01:58Z

packages/editor/src/components/post-preview-button/index.js

+					</DotTip>
+				</Button>
+				{ this.state.isPreviewOpen &&
+				<Modal


Indentation here doesn't look quite right. Usually these components are indented an extra tab when there's boolean logic.

talldan · 2019-12-04T09:02:40Z

packages/editor/src/components/post-preview-button/index.js

+				{ this.state.isPreviewOpen &&
+				<Modal
+					title={
+					// translators: Preview dialog title.


I think the comment should be indented at the same level as the __() function call below.

talldan · 2019-12-04T09:06:19Z

packages/editor/src/components/post-preview-button/index.js

+				>
+					{ _x( 'Preview', 'imperative verb' ) }
+
+					<DotTip tipId="core/editor.preview">


Just thought I'd double check that the DotTip should be here? My understanding is they've all been removed in master.

Uh, rebase gone wrong 😄

talldan · 2019-12-04T09:09:01Z

packages/editor/src/components/post-preview-button/index.js

+							target={ this.getWindowTarget() }
+							onClick={ this.openPreviewInNewTab }
+						>
+							{ _x( 'Open preview in new tab', 'imperative verb' ) }


Suggested change

{ _x( 'Open preview in new tab', 'imperative verb' ) }

{ __( 'Open preview in new tab' ) }

talldan · 2019-12-04T09:09:24Z

packages/editor/src/components/post-preview-button/index.js

+				<Modal
+					title={
+					// translators: Preview dialog title.
+						__( 'Preview' )


Suggested change

__( 'Preview' )

_x( 'Preview', 'imperative verb' )

Hmm maybe the modal title shouldn't be "preview"? It doesn't sound right to have a component title as an imperative. I'll change it to "Preview dialog" for now, open to better suggestions.

Oh right, I missed that it's the noun form. I think you can always change it to _x( 'Preview', 'noun' );

talldan · 2019-12-04T09:12:05Z

packages/editor/src/components/post-preview-button/index.js

+					shouldCloseOnClickOutside={ false }
+					className="editor-block-preview"
+				>
+					<div className="editor-block-preview__controls">


Suggested change

<div className="editor-block-preview__controls">

<div className="editor-post-preview__controls">

talldan · 2019-12-04T09:15:18Z

packages/editor/src/components/post-preview-button/index.js

+					onRequestClose={ this.closePreviewWindow }
+					// Needed so the Modal doesn't close when tabbing into the iframe.
+					shouldCloseOnClickOutside={ false }
+					className="editor-block-preview"


The classname should be made up of the package and component names. I think this would ideally be something like editor-post-preview-button__preview-modal. I notice the button that was here originally also had the wrong class name, it should be editor-post-preview-button.

talldan · 2019-12-04T09:16:54Z

packages/editor/src/components/post-preview-button/index.js

-		// Display a 'Generating preview' message in the Preview tab while we wait for the
-		// autosave to finish.
-		writeInterstitialMessage( this.previewWindow.document );
+	closePreviewWindow() {


I think this closes the modal, so the name of the function is a bit confusing.

talldan · 2019-12-04T09:18:07Z

packages/editor/src/components/post-preview-button/index.js

@@ -128,7 +67,24 @@ export class PostPreviewButton extends Component {
 		return `wp-preview-${ postId }`;
 	}

-	openPreviewWindow( event ) {
+	openPreviewOverlay() {


I think it'd be good to stick with the name Modal instead of window or overlay 😄

talldan · 2019-12-04T09:23:54Z

packages/editor/src/components/post-preview-button/index.js

@@ -138,59 +94,123 @@ export class PostPreviewButton extends Component {

 		// Open up a Preview tab if needed. This is where we'll show the preview.
 		if ( ! this.previewWindow || this.previewWindow.closed ) {
-			this.previewWindow = window.open( '', this.getWindowTarget() );
+			this.previewWindow = window.open( '', '_blank' );


Should it be _blank? It looks like getWindowTarget is still being used elsewhere.

This is a funny one. I found that using the window name returned from getWindowTarget caused window.open to focus the iframe in the modal, rather than opening a new window (this although the iframe doesn't have any name set). Switching to _blank solves that problem and if a preview tab is already open it will still be reused, so doesn't seem to make any difference.

getdave · 2019-12-04T12:50:41Z

I have similar questions in relation to adding Block controls which have different settings across viewports. There's a working effort in a PR here

#16730

I'd like to sync my work with yours.

tellthemachines · 2019-12-05T05:35:15Z

I'd like to sync my work with yours.

@getdave this particular PR is not very relevant to yours because here I'm implementing responsive views in Preview mode, not in the editor. The ticket this originated from does mention the possibility of responsive editor controls though, and I'm intending to have a play around with that and see what we can do, but in another PR. I think both options answer different needs. The editor mode won't be an easy problem to solve though 😅 , usability-wise if not technically.

jasmussen · 2019-12-05T07:49:58Z

GIF of ongoing work:

It is helpful to see responsive previews like this. However making it a dialog feels like it's optimizing for how the editor works today, at the expense of how we'd like the editor to work in the future:

The editing canvas itself is a preview
The editing canvas itself is responsive
The preview button is, potentially, a dropdown and/or edit mode selector that is extensible with additional controls such as "Preview AMP"

I much appreciate Dave reaching out to sync up, because feedback on the DimensionControl closely mirrors the challenge here. It recognizes that the editing canvas is currently neither responsive nor a proper preview, and tries to address that in a way that will arguably transform into technical debt one day when the editing canvas is responsive and a proper preview.

What could be an interim step? Making it not a dialog, but showing it in the canvas itself, as mocked up here. It may seem a subtle difference — showing it as a not-modal modal absolutely positioned layer over the editing canvas. But it is a treament that can transform into what it should be in the future. And given it shows the sidebar, it might even lend itself to an interim responsive preview control for block settings too.

afercia · 2019-12-05T07:51:35Z

packages/editor/src/components/post-preview-button/index.js

+					<Modal
+						title={
+							// translators: Preview dialog title.
+							__( 'Preview dialog' )


Please remove the word "dialog". That's already automatically announced by assistive technologies because of the role=dialog.

afercia · 2019-12-05T07:54:05Z

packages/editor/src/components/post-preview-button/index.js

+						<div className="editor-post-preview__frame-container">
+							<iframe
+								className="editor-post-preview__frame"
+								title={ __( 'Responsive preview frame' ) }


Please remove the word "frame", that's already announced by assistive technologies because of the element native role

afercia · 2019-12-05T10:04:15Z

Re: the aXe test failure [question asked on Slack](https://wordpress.slack.com/archives/C02RP4X03/p1575522453104800), it is possible that aXe doesn;t get the content within the iframe and it "sees" it empty. Regardless, the rule aims to make sure:

the scrollable region content can receive focus if it contains focusable elements
the scrollable region itself can be operated with a keyboard

In this specific implementation, the iframe container needs to be horizontally scrollable. Note that this will produce visible scrollbars on Windows (and also on macOS depending on user settings).

Since it's scrollable, the scroll mechanism needs to be operable also when using the keyboard. Browsers behavior differs on scrollable divs. To make them accessible and consistent, see:

Short note on improving usability of scrollable regions
https://developer.paciellogroup.com/blog/2016/02/short-note-on-improving-usability-of-scrollable-regions/

add tabindex=0
add role=region
add an aria-label with a meaningful, short, text e.g. "Preview"

Indication of focus would be a good addition.

It’s also a very good idea to provide a clear indication of focus on the scrollable region so keyboard users know where they are.

After that, the container height should be adjusted to not exceed the modal height otherwise, when it receives focus, the modal content will scroll.

Re: the modal size:
I'd consider to make the modal full screen. As it is now, the viewport of the document within the iframe is smaller than the viewport of the main document. Not sure this is the best experience for users. I guess they would like to see the preview take all the available space in their browser (and then they can use the preview buttons).

Other things noticed in this PR that need to be addressed:

Tabbing is not constrained within the modal: I guess the iframe breaks the withConstrainedTabbing / focus.tabbable implementation. Sounds like it doesn't get the "tabbables" within the iframe, as that's another document. I think the easier way to fix this is by adding a "Close" button after the iframe.
There's no indication of the active preview mode: add aria-pressed="true" on the active preview button and aria-pressed="false" on the inactive ones.
The "Preview" button in the Gutenberg top bar still says "(opens in a new tab)": this screen-reader-text should be removed, as the preview now opens in a modal dialog.

Lastly, worth noting that when tabbing to an iframe element, browsers behavior differs, for example:

with Chrome: focus goes from "Open preview in new tab" to the "Skip to toolbar" link within the iframe
with Firefox there's one more tab stop: focus goes from "Open preview in new tab" to the iframe document (the HTML element) and then to the "Skip to toolbar" link within the iframe.

Not sure this (historical) inconsistency can be addressed in any way as it's just a different vendor implementation.

tellthemachines · 2019-12-09T04:03:18Z

Thanks for your feedback @jasmussen ! It would be good to further discuss a few points:

It is helpful to see responsive previews like this. However making it a dialog feels like it's optimizing for how the editor works today, at the expense of how we'd like the editor to work in the future:

The editing canvas itself is a preview

The editing canvas itself is responsive

We can make the editing canvas responsive today, and that's likely what we need to do for @getdave's PR, but we can't make it a true preview, and I doubt we will ever be able to do so. Let me elaborate:

The markup for the editing canvas is necessarily different from the output markup. It's a lot more complex, and blocks are wrapped up in lots of layers needed to display the editing controls. Even if we assume that

we can import and apply in the editor all user-defined CSS overrides, such as the ones that can currently be added in the Customizer,
theme developers take the editor markup into account when defining their block styles, and
editor-specific styles don't override or otherwise impact theme-defined styles

it'll always be a brittle situation where any change to the editor can potentially cause theme styles to not render properly in it. Even if the editor gets really good at mirroring the output content, which no doubt we can accomplish, we can't trust it'll be exactly the same.

Bearing that in mind, I think it's useful to keep the distinction between (responsive) editor and preview.

The preview button is, potentially, a dropdown and/or edit mode selector that is extensible with additional controls such as "Preview AMP"

My concerns with making this a dropdown are that it adds an intermediate step to the flow, as you click to open the dropdown, and then have to click again before getting to the preview, and also that it makes it harder to toggle between the different screen sizes. With the modal all the controls are visible along with the preview, so we can just switch between them. We might even have custom views, such as the AMP view, render in the same iframe. I guess this could also be worked around by showing a toolbar instead of a dropdown though.

What could be an interim step? Making it not a dialog, but showing it in the canvas itself, as mocked up here. It may seem a subtle difference — showing it as a not-modal modal absolutely positioned layer over the editing canvas.

Positioning the preview over the canvas is technically possible but I don't think we should do it with a modal as that would make keyboard focus management very awkward. I also anticipate that it will be very confusing for users to see the content in the editor area and then not be able to edit it. When it appears as a modal, it's clearly a different interface so that confusion isn't possible.

Fwiw, I think we should implement both this preview and the responsive editor. I still have questions about how the responsiveness would work on a mobile device, though. I left a few thoughts about that here too.

talldan · 2019-12-09T04:11:40Z

What could be an interim step? Making it not a dialog, but showing it in the canvas itself, as mocked up here.

Showing the preview as it works currently in the editing canvas seems like it might be an option that bridges what we want in the future and what we have today.

The preview button is, potentially, a dropdown and/or edit mode selector that is extensible with additional controls such as "Preview AMP"

I might be misunderstanding how it works, but I do wonder if the dropdown is obvious enough. My first reaction might be "Where has the Preview button gone?" given that by default it might just say 'Full Page', and it would take some active searching from a user to locate the option.

Another question is whether the Code Editor would also become one of these modes. It currently works quite similarly in that it replaces the entire editing canvas and quite considerably changes the functionality in the editor.

folletto · 2019-12-09T10:27:59Z

Heya! I just saw this proposal and I think there's a lot that is worth discussion and exploration, and while I can add some feedback on the various points in this conversation... I think this isn't really addressing the problem stated in the issue referenced in #13203:

Users who are concerned about their site's mobile experience can't view the entire content area of Gutenberg in smaller screens easily to see how it all looks. Some of the user interviews from wporg showed this concern.

I'd suggest to review the approach here, in two parts:

First address mobile and responsiveness within the editor, iterate, iterate, iterate
Once that lands and is rock solid, explore previews

Notice also that the above approach will sidestep the more structural point raised by Matías in the other thread, addressing the issue raised without yet having to confront two other key points:

building preview into a modal goes against the direction of making the editor itself feel like the real site

and:

there will always be a case for having a "preview externally" that just lets you open the live page.

—

On the specifics, I agree with @talldan:

I do wonder if the dropdown is obvious enough

jasmussen · 2019-12-09T10:38:08Z

Thank you for walking through the thought process. The heart is in the right place.

we can't make it a true preview, and I doubt we will ever be able to do so

There are profound technical hurdles. But on the horizon are also technologies that will enable us to get almost all the way to a perfect preview:

Shadow DOM can protect editor UI, like the block toolbar and 3rd party created controls, from inheriting editor styles.
PRs like Lighter block DOM: contextual toolbar in popover #18779 and Lighter block DOM: remove extra div wrapper #19010 will substantially reduce the amount of extraneous UI nodes in the DOM.
Properties like display: contents; can make some unavoidable additional nodes "disappear" as far as editor styles are concerned.

Even with the clunkiness of editor styles today, we can get very close to a 1:1 preview; TwentyNineteen and TwentyTwenty are examples of this.

It may be worth checking in with @ellatrix, she's been fighting to get the editor closer to the front-end, and is better positioned than myself to suggest how close to parity we can get.

Bearing that in mind, I think it's useful to keep the distinction between (responsive) editor and preview.

What we want to avoid is reinforce the idea that the editor itself is not a preview, because it ideally should be, same as WordPress page builders, Squarespace, Wix and Weebly. But to Dan's point, maybe the dropdown idea needs more time in the oven. @folletto notes that it's worth addressing the editor responsiveness first; doing so would definitely help inform how to design a better preview.

As an interim solution I do worry that adding this preview dialog now will just end up creating vestigial features.

ellatrix · 2019-12-09T17:17:46Z

We're definitely working towards an editor that will 100% be the same as the front-end.

The markup for the editing canvas is necessarily different from the output markup.

The markup will be much simpler in the near future. Block controls will eventually be removed from the content's DOM, and all block wrapper elements will go away. Instead, we'll render the controls in floating popovers. When all this is done, theme authors won't need to add extra editor styles.

tellthemachines · 2019-12-12T01:25:15Z

@folletto @jasmussen @ellatrix thanks for your feedback and additional information! Based on that, I have made a new PR to try out resizing the editing canvas: #19082
There's still quite a few bits and pieces to work out but it would be great to know what you think!

Also, thanks @talldan and @afercia for your comprehensive reviews. I'll leave this PR open for now until we reach a decision on the best way forward.

afercia · 2020-02-09T15:09:45Z

Block controls will eventually be removed from the content's DOM, and all block wrapper elements will go away. Instead, we'll render the controls in floating popovers.

Any GitHub issue where this is discussed and worked on? Rendering the Block controls UI far in the DOM will significantly worsen the experience for all non-mouse / non-touch users and goes against what the accessibility team always recommended. That is: render the controls in close proximity to the edited block, in a logical, predictable, order.

tellthemachines · 2020-02-12T00:32:48Z

Any GitHub issue where this is discussed and worked on?

@afercia I don't know of any single issue for this work, but there have been a few (already merged) PRs working towards it:
#17607
#19010
#19593

There have been changes to how keyboard navigation works too: #19235

I hope that answers your question!

I'm closing this PR now because the alternative approach in #19082 has been merged.

tellthemachines mentioned this pull request Nov 8, 2019

Responsive View Controls in Gutenberg #13203

Closed

tellthemachines force-pushed the try/Responsive-view branch from 24ecf7e to 991c7d7 Compare November 28, 2019 04:38

tellthemachines marked this pull request as ready for review December 4, 2019 06:31

tellthemachines requested review from ajitbohra, nerrad, ntwb and talldan as code owners December 4, 2019 06:31

tellthemachines added 6 commits December 4, 2019 17:34

Render preview in a modal with responsive sizes.

71d867b

Base styling.

1eed768

Add open in new tab button

c210afa

Fix for opening in new tab.

43127d4

Updated unit tests.

f0b892e

Updated e2e tests.

00b9f49

tellthemachines force-pushed the try/Responsive-view branch from dff34cb to 00b9f49 Compare December 4, 2019 06:35

talldan reviewed Dec 4, 2019

View reviewed changes

talldan added [Type] Enhancement A suggestion for improvement. General Interface Parts of the UI which don't fall neatly under other labels. labels Dec 4, 2019

Remove DotTip

713cbe7

tellthemachines added the Needs Design Feedback Needs general design feedback. label Dec 4, 2019

Address PR feedback and update tests

68a7162

afercia reviewed Dec 5, 2019

View reviewed changes

tellthemachines added 2 commits December 9, 2019 16:07

Labelling and keyboard focus improvements

fe836cd

Fix scrollbar on frame container

c66fc1d

Update snapshot

f3b7e6f

tellthemachines mentioned this pull request Dec 18, 2019

Resizable editor experiment #19082

Merged

6 tasks

tellthemachines closed this Feb 12, 2020

aristath deleted the try/Responsive-view branch November 10, 2020 14:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Render preview in a modal with responsive sizes. #18385

Render preview in a modal with responsive sizes. #18385

tellthemachines commented Nov 7, 2019 •

edited

Loading

talldan left a comment

talldan Dec 4, 2019 •

edited

Loading

talldan Dec 4, 2019 •

edited

Loading

talldan Dec 4, 2019

tellthemachines Dec 4, 2019

talldan Dec 4, 2019

talldan Dec 4, 2019

tellthemachines Dec 4, 2019 •

edited

Loading

talldan Dec 5, 2019

talldan Dec 4, 2019

talldan Dec 4, 2019

talldan Dec 4, 2019

talldan Dec 4, 2019

talldan Dec 4, 2019

tellthemachines Dec 5, 2019

getdave commented Dec 4, 2019

tellthemachines commented Dec 5, 2019

jasmussen commented Dec 5, 2019

afercia Dec 5, 2019

afercia Dec 5, 2019

afercia commented Dec 5, 2019 •

edited

Loading

tellthemachines commented Dec 9, 2019 •

edited

Loading

talldan commented Dec 9, 2019

folletto commented Dec 9, 2019 •

edited

Loading

jasmussen commented Dec 9, 2019

ellatrix commented Dec 9, 2019

tellthemachines commented Dec 12, 2019

afercia commented Feb 9, 2020

tellthemachines commented Feb 12, 2020

	{ _x( 'Open preview in new tab', 'imperative verb' ) }
	{ __( 'Open preview in new tab' ) }

	<div className="editor-block-preview__controls">
	<div className="editor-post-preview__controls">

Render preview in a modal with responsive sizes. #18385

Render preview in a modal with responsive sizes. #18385

Conversation

tellthemachines commented Nov 7, 2019 • edited Loading

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

talldan left a comment

Choose a reason for hiding this comment

talldan Dec 4, 2019 • edited Loading

Choose a reason for hiding this comment

talldan Dec 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tellthemachines Dec 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

getdave commented Dec 4, 2019

tellthemachines commented Dec 5, 2019

jasmussen commented Dec 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afercia commented Dec 5, 2019 • edited Loading

tellthemachines commented Dec 9, 2019 • edited Loading

talldan commented Dec 9, 2019

folletto commented Dec 9, 2019 • edited Loading

jasmussen commented Dec 9, 2019

ellatrix commented Dec 9, 2019

tellthemachines commented Dec 12, 2019

afercia commented Feb 9, 2020

tellthemachines commented Feb 12, 2020

tellthemachines commented Nov 7, 2019 •

edited

Loading

talldan Dec 4, 2019 •

edited

Loading

talldan Dec 4, 2019 •

edited

Loading

tellthemachines Dec 4, 2019 •

edited

Loading

afercia commented Dec 5, 2019 •

edited

Loading

tellthemachines commented Dec 9, 2019 •

edited

Loading

folletto commented Dec 9, 2019 •

edited

Loading