Editor: PostTitle: Decode entities for display #20887
Conversation
aduth
added
[Type] Bug
aduth
requested review from
ajitbohra,
chrisvanpatten,
jorgefilipecosta and
youknowriad
as code owners
|
It would probably be good to add an end-to-end test case which verifies some of the behaviors here:
|
|
Size Change: +159 B (0%) Total Size: 857 kB
ℹ️ View Unchanged
|
| @@ -695,6 +695,12 @@ | |||
| "markdown_source": "../packages/components/src/date-time/README.md", | |||
| "parent": "components" | |||
| }, | |||
| { | |||
| "title": "DetectFocusOutside", | |||
There was a problem hiding this comment.
This seems useful for the Gallery block. I know we merged a similar thing to unselect images lately cc @nosolosw
There was a problem hiding this comment.
Oh, nice. Yeah, it looks like the same use case (link for reference). Note that I didn't extract this logic to a reusable component there, so we can refactor gallery to use this when the PR lands.
There was a problem hiding this comment.
@nosolosw FWIW, there is the withFocusOutside higher-order component already available today:
This pull request "discourages" it, but it will expect to continue to exist I think. Maybe still worth waiting, in case we want to consolidate to DetectFocusOutside as the encouraged implementation.
| @@ -0,0 +1,36 @@ | |||
| # DetectFocusOutside | |||
There was a problem hiding this comment.
Lately, I have been wondering if it's better to avoid components for these "behavior-based" components and instead write hooks that accept "refs" or return elements (like useResizeAware). The advantage being to avoid the extra wrapper.
There was a problem hiding this comment.
Hm, I'm not sure exactly how that would look in this case. useResizeAware returns an element which is expected to be rendered as a child of a component. We need to capture events bubbling from within, so it would at least need to be a wrapping element, which would basically be the same as what this implementation is already.
I do think it would be ideal if we could at least flatten the DOM hierarchy, since currently this component must add a div wrapper. We don't strictly need the div, aside from being able to capture bubbling events. I recall having some hope that in the future, React might support event handlers to be assigned to Fragment, since their synthetic events don't use the DOM anyways and it can technically be possible.
In fact, they still note this in their documentation:
keyis the only attribute that can be passed toFragment. In the future, we may add support for additional attributes, such as event handlers.
https://reactjs.org/docs/fragments.html#keyed-fragments
I think I saw later that they might be avoiding that option though (perhaps part of their new "Fire" effort).
So maybe it's not worth placing our bets on this. The alternatives might have some concerning implication on the ergonomics of the component:
- We could have a hook which returns an object of props for the event handlers. But how do we handle a case where both the hook and the original component each need to have an e.g.
onFocusevent handler? - We could have a hook which returns a
ref, and then the hook attaches DOM event handlers to therefinstance. But should we have any concern that those are DOM events, not React events, which behave (and bubble) slightly differently? We've had issues with this in the past for things likeSlot/FillbubblesVirtually.
One possible advantage of using the DOM events is that we could use the actual blur event, where the distinction between blur and focusout is basically the point of what this component is trying to implement. As far as I can recall, React counter-intuitively uses the focusout event when assigning onBlur (see facebook/react#6410, specifically facebook/react#6410 (comment)).
There was a problem hiding this comment.
I'm not sure if a good pattern or not but on previous occasions I've wrote hooks like that:
useSomething = ( ref ) => {
useEffect(() => {
// do something with ref.current.
// useEffect ensures the ref is defined
});
}
There was a problem hiding this comment.
I believe it would be good if we find a pattern here and follow it for future hooks relying on refs.
There was a problem hiding this comment.
I think the harder question is if and when we "rely on refs", including whether it makes sense here.
| @@ -100,7 +96,6 @@ function Layout() { | |||
| editorSidebarOpened || pluginSidebarOpened || publishSidebarOpened; | |||
| const className = classnames( 'edit-post-layout', 'is-mode-' + mode, { | |||
| 'is-sidebar-opened': sidebarIsOpened, | |||
| 'has-fixed-toolbar': hasFixedToolbar, | |||
There was a problem hiding this comment.
Is this something that you think is worth mentioning on a dev note?
There was a problem hiding this comment.
Hm, I saw this as largely a clean-up of classes which were no longer used in the editor, but never removed. To answer your question, I guess it depends on a combination of (a) are plugins using it and (b) are there use-cases for it? I don't have a huge concern of reverting this specific change, aside from the fact that it becomes dead code (maintenance burden, larger bundle sizes).
I do see a couple results here: https://wpdirectory.net/search/01E3J9B3K378P5PV2104804D06
But it's hard to tell if the usage is intended for has-fixed-toolbar of the layout component, vs. has-fixed-toolbar of the block.
One exception to clarify: If a user types |
|
There's some additional prior art to consider at #17994 and #17789 (cc @ellatrix, @dmsnell), particularly in regards to the intentionality of inputs provided by the user: if a user types "&", they most likely expect to see an ampersand in the editor and on the front-end, meaning that it should be escaped as In some initial iterations, I had also explored the feasibility of decoding the entities once at initial load, and then not transforming the input value in any way thereafter. The problem with this approach is that it's hard to know when that "once" should be. The underlying post data can be updated for any number of reasons programmatically. For example, when a user saves their post and the server-side escapes ampersands and other characters, the post title data is updated to reflect the latest value. Should we re-run that "once" behavior? Because otherwise, we're back to the original issue. How can we know that those title updates came as a result of a save, as opposed to the post title input itself? |
One helpful way I think about it is whether the editor gets in the way of that goal or not. If someone wants to enter unescaped content into the text view that WordPress will mess up on the server I don't see it as a bad thing to fix it right there in the client before going to the server. If we change what someone writes and the effect is that they become aware of it sooner before publishing their document then I consider that a win.
I concur with your assessment. In my proposal I know it felt a little overboard but this is just a bit of an ugly job. What makes most sense to me is providing the low-level inputs into Gutenberg where we declare "the output should match what the person thinks they are typing" like the normal Somewhere probably in a lot of grunt work is finding the range of things that are valid HTML but which WordPress will nonetheless mangle. I think those things are the ones we should abstract away from block developers. |
|
Why are we not using the |
|
It should definitely be a block. I think soon we can try using the block that already exists for FSE. |
|
RichText (and, more broadly, a title block) is likely the better path forward here, so I'm going to close this attempt. As mentioned in the original comment, there's a lot of supporting changes which could still be salvaged into unrelated improvements. |
Closes #20490
Related: #18616, #19955, https://core.trac.wordpress.org/ticket/11311
This pull request seeks to improve the behavior for title character encoding for users who do not have
unfiltered_htmlcapability. Without these changes, a contributing user to the site would see escape sequences for characters in the title after they have saved a post (i.e.&becomes&, shown literally in the editor's title field).Notably, this implementation must make quite a few accommodations:
core/data) should still report a raw title value including entities, if applicable. The implementation here could have transformed the values at the editor level. However, since this is largely a concern of display, it was decided to implement this at the level of the title component.&,&a,&am, etc., the title should not suddenly update to decode that input. This is accounted for by preemptively encoding an ampersand input.Current Status:
This is largely complete. However, as may be evidenced by the size of the changes, a fair bit of unrelated refactoring was included. This could be included as part of review of these changes, or separated out to separate pull requests and rebased on this branch to make this smaller.
These include:
Testing Instructions:
Repeat Steps to Reproduce from #20490, both as a user with and without
unfiltered_htmlcapability (i.e. adminstrators and contributors). Note that it's not necessary to use a multisite installation. This can be tested in single-site as well.Verify also all of the above accommodations.
Specifically, be on the lookout for edge cases concerning: