Opened 2 years ago
Last modified 3 months ago
#52294 new enhancement
add_editor_style: Allow replacing a style with an RTL version
Reported by: |
|
Owned by: | |
---|---|---|---|
Milestone: | Future Release | Priority: | normal |
Severity: | normal | Version: | 5.6 |
Component: | Bundled Theme | Keywords: | has-patch needs-testing has-unit-tests |
Focuses: | rtl | Cc: |
Description (last modified by )
The editor-styles in Twenty Twenty-One do not have an RTL version.
This causes major style issues in the editor when in RTL mode.
For example, this is how a nested ordered list looks like:
Notice all items are on the same horizontal level.
This is how it looks with the theme editor styles turned off:
Details of the problem
Currently add_editor_style
RTL support is pretty limited. It will, if the file exists, load a style-editor-rtl.css
file, in addition to the main style-editor.css
file (similarly to how rtl.css is loaded for themes).
The problem with this method is that we don't currently have the tooling to generate that addon rtl file, which only overwrites the necessary CSS declarations.
With the complexity of Twenty Twenty-One style-editor.css
file (~2500 lines, SASS generated), creating a manual addon file is not reasonable.
Possible solutions
Any solution should start by generating a fully mirrored style-editor-rtl.css
file automatically, using rtlcss.
I considered a couple of possible solutions - and I'd be happy to hear opinions on this:
- We could use the Twenty Twenty method in
twentytwenty_block_editor_styles
- it doesn't useadd_editor_style
and instead callswp_enqueue_style
, and then useswp_style_add_data
to define that the RTL version should replace the ltr version.
- We could update
add_editor_style
to support loading an additional parameter. I'm attaching an example of how this could be done.
Attachments (3)
Change History (41)
#4
follow-up:
↓ 5
@
2 years ago
Another alternative is for the stylesheet to contain both LTR and RTL styles. I do this in my theme. It makes it much easier to keep the styles consistent when they are together.
#5
in reply to:
↑ 4
@
2 years ago
Replying to joyously:
Another alternative is for the stylesheet to contain both LTR and RTL styles. I do this in my theme. It makes it much easier to keep the styles consistent when they are together.
That works (by targeting the .rtl body class), but do you have a solution to automate the generation of the RTL CSS in the main stylesheet?
#6
@
2 years ago
I like the idea of making the new parameter available for all themes.
I searched Trac to see if there were any duplicates. I did not find any but
perhaps it could be paired with #42645, Support passing version number to add_editor_style().
This ticket was mentioned in Slack in #themereview by poena. View the logs.
2 years ago
#8
@
20 months ago
- Keywords has-patch needs-testing added
- Milestone changed from Awaiting Review to 5.9
#9
@
20 months ago
- Summary changed from Twenty-Twentyone: editor styles are broken in RTL to Twenty Twenty-One: editor styles are broken in RTL
This ticket was mentioned in Slack in #themereview by kafleg. View the logs.
20 months ago
This ticket was mentioned in Slack in #core by chaion07. View the logs.
19 months ago
This ticket was mentioned in Slack in #core-test by hellofromtonya. View the logs.
19 months ago
#13
@
19 months ago
Hi @yoavf! Thank you so much for reporting this. We discussed this ticket during a recent bug-scrub and based on the discussion, 'Testing Instructions' are to be added. It could also use the keyword 'Add_Editor_Style' (which is currently in proposal stage for the keyword to be officially added) instead of 'Theme'. Additionally, it would be a good idea to add this to the agenda considering the manual test session that the Test Team is scheduled to host tomorrow. After the test report is available, this should be in an ideal position to 'Commit'. Cheers!
Props: @hellofromTonya & @poena
This ticket was mentioned in Slack in #core-test by boniu91. View the logs.
19 months ago
This ticket was mentioned in PR #1815 on WordPress/wordpress-develop by pbearne.
19 months ago
#15
- Keywords has-unit-tests added
Added support for replacing a style sheet with a RTL version in the add_editor_style function
Added rtl editor styles sheets for twentytwentytOne theme
Added tests for add_editor_style
Trac ticket: https://core.trac.wordpress.org/ticket/52294
#16
@
19 months ago
- Keywords has-patch needs-testing has-unit-tests removed
Hi All,
I had to change the code to get the add_editor_style function to get it to work
I have tested that the rtl style sheet loads in the classic editor (tinyMCE)
Good to go
Paul
This ticket was mentioned in Slack in #core-test by pbearne. View the logs.
19 months ago
#18
@
19 months ago
- Keywords has-patch needs-testing added
- Milestone changed from 5.9 to 6.0
With 5.9 Beta 1 release happening in a few hours, moving this ticket to 6.0. However, if it can get tested and there's consensus on the patch before the release party, move it back into the milestone.
#19
@
17 months ago
I have not been able to reproduce the original example with the list block missing indentations.
When I try to use npm run build
on the pull request, I am getting warnings that prevents the build.
Perhaps it needs to be rebased?
JustinyAhin commented on PR #1815:
17 months ago
#20
Hello @pbearne, could you add some tests instructions to the PR please?
17 months ago
#21
testing:
install a right to left lang
open the editor and note the lay does match the front end
Apply patch and the layout should match as we are not now loading the ltr CSS just the rtl CSS
This ticket was mentioned in Slack in #core by mike. View the logs.
14 months ago
This ticket was mentioned in Slack in #core-editor by mike. View the logs.
14 months ago
This ticket was mentioned in Slack in #core-themes by mike. View the logs.
14 months ago
#25
@
14 months ago
We looked at this ticket during a bug scrub today.
I checked, and the PR's patch still applies.
I reached out to #core-editor and #core-themes for feedback.
#26
@
13 months ago
- Keywords has-unit-tests added
6.0's last beta is happening in < 15 minutes and tomorrow is RC1. Though really close to being ready, more testing and review are needed. As the issue was not introduced in 6.0 and it's not quite ready yet for commit, moving it to the next cycle. Thank you everyone for your contributions!
#27
@
13 months ago
- Milestone changed from 6.0 to 6.1
A shame that this is so close but didn't get attention this cycle. While it's been through several cycles, as it's close to resolution, I'm moving this ticket to the 6.1 milestone. Let's try to land this one in 6.1!
#28
@
12 months ago
I have been able to build the PR now.
The testing instructions are a bit vague still:
When I add a nested list block like in the issue description, the block looks correct in both LTR and RTL: This layout does not change when I apply the PR.
Instead I made a more limited test where I added a red border style to the theme's assets/css/style-editor-rtl.css file. With this style I was able to confirm that the styles are loading in the editor.
I can also confirm that the generation of the file works correctly (left is replaced with right, the npm commands work well).
MacOs 12.4
Chrome 02.0.5005.115
WordPress 6.0
#30
@
4 months ago
Test Report
Patch tested: NO Patch
Environment
- OS: macOS 13.0.1
- Web Server: Nginx
- PHP: 7.4.33
- WordPress: 6.2-alpha-54642-src
- Browser: Chrome 109.0.5414.125
- Theme: Twenty Twenty Twenty-One
- Active Plugins:
- RTL Tester
Actual Results
RTL Backend: https://d.pr/i/5VDLl6
RTL Frontend: https://d.pr/i/pnHmES
LTR Backend: https://d.pr/i/vBkpYS
LRT Frontend: https://d.pr/i/yr63Ma
- Issue resolved without the patch.
#32
@
4 months ago
- Keywords needs-testing-info removed
@pbearne and @costdev Are all the requested changes on the PR resolved?
If they are, I think this is ready for commit?
The theme CSS files might need to be rebuilt,
I believe the implementation is working well, it is just that the original issue with the list in Twenty Twenty-One can not be reproduced.
#34
@
4 months ago
Yes, I see nested list issue is not reproducible.
Unable to reproduce.
Screenshots:
RTL Editor: https://d.pr/i/EGJpIF
RTL Frontend: https://d.pr/i/lwAugw
#35
@
4 months ago
- Summary changed from Twenty Twenty-One: editor styles are broken in RTL to add_editor_style: Allow replacing a style with an RTL version
This ticket was mentioned in Slack in #core by costdev. View the logs.
3 months ago
#37
@
3 months ago
@poena @pbearne @costdev @robinwpdeveloper While this ticket originally stems from what is somewhat a broken experience, I would disagree with the current ticket type used here: Technically this is clearly an enhancement, as it adds a new parameter to a function that is commonly used by themes. There is also no particular urgency to addressing it in 6.2, as it was not introduced in this release, in fact has been like this for years.
While I completely agree that this should be addressed, to me this is not something we should commit at this late point in the 6.2 release cycle, so I would strongly advise to punt this to 6.3, and commit it for that release.
adding a new param to add_editor_style to allow replacing the css file with RTL version