WordPress.org

Make WordPress Core

Opened 5 months ago

Closed 5 months ago

Last modified 5 months ago

#50449 closed defect (bug) (fixed)

Sitemap style for RTL sites

Reported by: joyously Owned by: whyisjake
Milestone: 5.5 Priority: normal
Severity: normal Version:
Component: Sitemaps Keywords: has-screenshots has-patch commit
Focuses: rtl Cc:

Description

There is a style for
#sitemap__table tr th {text-align: left;}
even though I put my site in RTL mode, which at first glance seems wrong, but when we were building Twenty Twenty, I asked a RTL native user to test and he said that URLs should always be LTR. So maybe the table heading is correct and the rest of the table needs direction: ltr.

Please ask someone to test in a RTL language.

Attachments (6)

XML Sitemap-50.jpg (70.7 KB) - added by joyously 5 months ago.
Sitemap in RTL mode. (My browser default color is yellow.)
sitemap-rtl.png (26.0 KB) - added by pbiron 5 months ago.
wp-ms-sites-list-table-rtl.png (21.0 KB) - added by pbiron 5 months ago.
Screen showing the WP_MS_Sites_List_Table with RTL language
50449.2.diff (2.3 KB) - added by pbiron 5 months ago.
50449.3.diff (1.9 KB) - added by ramiy 5 months ago.
50449.4.diff (2.2 KB) - added by pbiron 5 months ago.

Download all attachments as: .zip

Change History (24)

@joyously
5 months ago

Sitemap in RTL mode. (My browser default color is yellow.)

#1 in reply to: ↑ description ; follow-up: @SergeyBiryukov
5 months ago

  • Focuses rtl added

Replying to joyously:

when we were building Twenty Twenty, I asked a RTL native user to test and he said that URLs should always be LTR. So maybe the table heading is correct and the rest of the table needs direction: ltr.

URLs should indeed be displayed in LTR per #16834 and #49949, but I think that only applies to UI.

Not sure if this also applies to machine-readable content like sitemaps. Adding rtl focus so that native RTL users could chime in.

@pbiron
5 months ago

#2 @pbiron
5 months ago

  • Keywords has-screenshots added

sitemap-rtl.png is a proposed change to how the sitemaps should be rendered for RTL languages.

It was generated with a patch I have locally. Want to make sure that's the correct behavior before uploading the patch.

Note that the Last Modified column was added via a plugin, and is there to test the alignment of any non-URL columns.

#3 @pbiron
5 months ago

Related question: I notice that WP_MS_Sites_List_Table does not left-align the "URL" column. Should it?

What is displayed in that column is not a "full" URL (i.e., doesn't display the URL schema/protocol), so maybe that's why it doesn't. Just asking...

@pbiron
5 months ago

Screen showing the WP_MS_Sites_List_Table with RTL language

#4 @swissspidy
5 months ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 5.5

@pbiron
5 months ago

#5 @pbiron
5 months ago

  • Keywords has-patch added; needs-patch removed

50449.2.diff modifies the CSS so that:

  1. the text direction and alignment of the URL column (both header and body) is always ltr and left
  2. all other column headers are aligned based on is_rtl() (to override browser defaults to center column headers)
    • the text direction of these columns (both header and body) is already determined by the dir attribute on the html element

#6 @apedog
5 months ago

When handling RTL URL's - mixed RTL/LTR URLs need be considered.
The domain (as well as scheme/protocol) is LTR, while some of the slugs can be RTL. So URL strings must be LTR for readability. However the URL cells alignment can be RTL.

I have a personal preference for the cells being LTR (as per sitemaps-rtl.png above - having the http://example.com align helps readability). But RTL is also common (as per wp-ms-sites-list-table-rtl.png), and I believe this is the current standard for most RLT interfaces (gmail, browsers, etc.). ie. even if a column's strings are LTR, the cells and column will be aligned RTL.

As for that column header's direction - I think consistency with the rest of the headers (RTL) is the way to go, even if the column cells end up aligned to the left. Especially if the latin-charactered "URL" header will get translated to an actual word. Having an RTL word aligned to the left is weird.

#7 in reply to: ↑ 1 @ramiy
5 months ago

Replying to SergeyBiryukov:

Replying to joyously:

when we were building Twenty Twenty, I asked a RTL native user to test and he said that URLs should always be LTR. So maybe the table heading is correct and the rest of the table needs direction: ltr.

URLs should indeed be displayed in LTR per #16834 and #49949, but I think that only applies to UI.

Not sure if this also applies to machine-readable content like sitemaps. Adding rtl focus so that native RTL users could chime in.

As mentioned in the linked tickets, the URL should be always displayed in LTR, both in LRT and RTL languages. This applies on sitemap too as the sitemap is not only machine-readable, humans use sitemaps too.

#8 follow-up: @swissspidy
5 months ago

  • Keywords needs-testing added

@ramiy Could you perhaps test 50449.2.diff and provide feedback?

This ticket was mentioned in Slack in #core-sitemaps by swissspidy. View the logs.


5 months ago

This ticket was mentioned in Slack in #core by pbiron. View the logs.


5 months ago

#11 in reply to: ↑ 8 @ramiy
5 months ago

  • Keywords needs-testing removed

Replying to swissspidy:

@ramiy Could you perhaps test 50449.2.diff and provide feedback?

RTL tested. Not good. the th is aligned to left, should be aligned to right.

@ramiy
5 months ago

#12 @ramiy
5 months ago

The third patch has minor fix for the second patch.

This ticket was mentioned in Slack in #core-sitemaps by ramiy. View the logs.


5 months ago

@pbiron
5 months ago

#14 follow-up: @pbiron
5 months ago

50449.4.diff produces the same results as 50449.3.diff but with simpler CSS.

@ramiy can you please verify?

#15 in reply to: ↑ 14 @ramiy
5 months ago

Replying to pbiron:

50449.4.diff produces the same results as 50449.3.diff but with simpler CSS.

@ramiy can you please verify?

Tested. Looks good!

#16 @pbiron
5 months ago

  • Keywords commit added

Thank you for your help!!!

#17 @whyisjake
5 months ago

  • Owner set to whyisjake
  • Resolution set to fixed
  • Status changed from new to closed

In 48414:

Sitemaps: Add better support for RTL sites.

While the URLs are intended to be machine readable, they should always be LTR, while other data would be RTL in the sitemap.

Fixes #50449.

Props joyously, SergeyBiryukov, pbiron. apedog, ramiy.

#18 @SergeyBiryukov
5 months ago

In 48425:

Sitemaps: Remove some extra space from WP_Sitemaps_Stylesheet::get_stylesheet_css().

Follow-up to [48414].

See #50449.

Note: See TracTickets for help on using tickets.