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

Improve handling of html markup in Navigation Label #34399

Closed
3 tasks
spacedmonkey opened this issue Aug 30, 2021 · 7 comments
Closed
3 tasks

Improve handling of html markup in Navigation Label #34399

spacedmonkey opened this issue Aug 30, 2021 · 7 comments

Comments

@spacedmonkey
Copy link
Member

spacedmonkey commented Aug 30, 2021

What problem does this address?

The new navigation editor allows to text in navigation label to add markup, like strong and italic tags.

Screenshot 2021-08-30 at 23 33 12

However, when switching back to current menu screens, this markup is not rendered correctly.

Menu screen

Screenshot 2021-08-30 at 23 35 05

Menus in customizer

Screenshot 2021-08-30 at 23 34 52

Firstly, we should handle this use case, better. Secondly, there maybe some themes with custom walkers, that to not support markup in menu item label ( title ). For example a plugin like dropdown menu or this example where a menu is used to generate a select / dropdown menu.

What is your proposed solution?

  • Add a Boolean filter to allow developers to opt in/out of formatting (ie: HTML markup) in Nav menu items.
  • Conditionally disable the formatting toolbar in the editor depending on filter value.
  • Conditionally remove HTML formatting from Nav Menu output (rendering) depending on filter value.
@getdave
Copy link
Contributor

getdave commented Sep 9, 2021

Thanks for spotting this @spacedmonkey. It's really important we consider this sort of thing.

A few questions:

we should handle this use case, better

Did you have a vision for what you would consider "better" in mind? I've been thinking but I can't think of an optimal soluton that covers all use cases.

The Nav Editor saves data to the same CPT as the Menus screen. Therefore anything we strip out for the purposes of the Menus screen will also get stripped out for the Nav Editor.

Secondly, there maybe some themes with custom walkers, that to not support markup in menu item label ( title ). For example a plugin like dropdown menu or this example where a menu is used to generate a select / dropdown menu.

I'd like to understand if this is a new problem.

My instinct is that it's fine if a custom walker doesn't support HTML in the nav menu items. The walker should correctly strip HTML prior to rendering. The ability to add HTML to your menu items is already part of the classic Menus screen so I assume developers should already account for that.

The issue is perhaps that the tools in the Nav Editor exposes the ability to set HTML.

Look forward to discussion further to refine this towards a solution. Thanks again 🙇‍♂️

@spacedmonkey
Copy link
Member Author

I'd like to understand if this is a new problem.

It is not a new problem. The current nav menu screen allows you to input text / html and it is not stripped out. But the average user will likely not do because, 1. they do not know html. 2. do not know that it was possible.

The nav editor screen brings this functionality to the surface. Most custom walkers, require very strict html markup / match css selectors. Not sure what effect just putting random html in as a label will do. It is impossible to know, as the walker can be whatever the developers wants. Which is why I provided the example of dropdown, as having html in label here, would break the dropdown. You would hope that developers would have escaped output, but you never know.

I don't know the solution here. May current recommendation is this.

  • Test popular themes with custom walkers, see how big the problem is.
  • Make sure that develops know this change is coming.
  • Give a filter / option, for developers to opt out of "rich html" labels. If this filter is used, then bold / italic options are hidden in this editor.

I don't want to strip html on labels either on save or render, they maybe html in them for a reason. Giving developers tools to fix they own problems, will be the key.

@adamziel
Copy link
Contributor

If I understand the problem correctly, it is a matter of “can I upgrade to the new editor without breaking my current setup?", right? Walkers are there at all times, it's just that the new editor makes the use of HTML tags transparent for the user.

As long as things are opt-in, we have a lot of space to find if and how things break, +1 to what @spacedmonkey suggested here:

I don't know the solution here. May current recommendation is this.

  • Test popular themes with custom walkers, see how big the problem is.
  • Make sure that develops know this change is coming.
  • Give a filter / option, for developers to opt out of "rich html" labels. If this filter is used, then bold / italic options are hidden in this editor.

@getdave
Copy link
Contributor

getdave commented Sep 10, 2021

Test popular themes with custom walkers

Can folks list some of these here so we can run some tests?

Give a filter / option, for developers to opt out of "rich html" labels. If this filter is used, then bold / italic options are hidden in this editor.

So would this be an option in the new Editor which disables the rich formatting toolbar if/when rendering using a Walker?

We could also introduce a filter on the current Menus screen which would allow devs to opt out of HTML in labels. That way if their Plugin breaks they simply activate the filter in PHP to strip any HTML in the labels.

Make sure that develops know this change is coming.

We need a place to list known potential backwards compatibility issues. I'll set one up on the Tracking Issue.

@getdave
Copy link
Contributor

getdave commented Sep 21, 2021

@spacedmonkey I collated the proposed solutions under What is your proposed solution? above. Do you agree with them?

@spacedmonkey
Copy link
Member Author

A filter is a the right solution here. Where would this filter come from? What is be controlled at the REST API level? Would be strip html at the API level if posted to the REST API?

@kathrynwp
Copy link

Closing this issue due to the Navigation Screen project being moved to an inactive status on the feature projects page in coordination with the project leads. (The developer documentation in the initial post are no longer accessible)

If this work is picked back up, issues can be revisited and reopened as needed. Thanks for pitching in on this early feature so the wider WordPress project could benefit from the lessons learned!

@kathrynwp kathrynwp closed this as not planned Won't fix, can't repro, duplicate, stale Sep 21, 2022
Navigation editor automation moved this from Inbox to Done Sep 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

No branches or pull requests

4 participants