Make WordPress Core

Opened 5 years ago

Closed 3 weeks ago

#46658 closed defect (bug) (fixed)

Twenty Nineteen: Navigation menu is messy with RTL language

Reported by: manooweb's profile manooweb Owned by: karmatosed's profile karmatosed
Milestone: 6.7 Priority: normal
Severity: normal Version:
Component: Bundled Theme Keywords: has-patch has-screenshots needs-testing commit
Focuses: rtl Cc:

Description

Hello,

For our languages switcher, we can add it in primary navigation menu with an options for adding country flag for each language.

With a rtl language like arabic, we have a strange behaviour because of the first li tag does not have the behaviour expected.

See first screenshot attached

Unlike in twenty seventeen

display: inline;

is used instead of

display: inline-block;

When I corrected to

display: inline-block;

it works well again in this case.

See second screenshot attached

Regards

Attachments (6)

messy-menu-wp.jpg (330.5 KB) - added by manooweb 5 years ago.
first screenshot
not-messy-menu-wp.jpg (333.1 KB) - added by manooweb 5 years ago.
second screenshot
correct-menu-rtl-twenty-nineteen.diff (449 bytes) - added by manooweb 5 years ago.
patch file
2019-inline-flags-mixup-ltr-english-chrome.png (16.7 KB) - added by sabernhardt 5 years ago.
Two RTL languages in switcher, on English page in Chrome
2019-inline-block-flags-fixed-ltr-english-chrome.png (16.6 KB) - added by sabernhardt 5 years ago.
Two RTL languages in primary menu list items set to display: inline-block (on English page, in Chrome)
46658.diff (1.3 KB) - added by sabernhardt 5 years ago.
Primary menu list items set to display: inline-block for both LTR and RTL stylesheets, plus SCSS

Download all attachments as: .zip

Change History (17)

@manooweb
5 years ago

first screenshot

@manooweb
5 years ago

second screenshot

#1 @audrasjb
5 years ago

  • Component changed from Themes to Bundled Theme
  • Version trunk deleted

#2 @SergeyBiryukov
5 years ago

  • Summary changed from [twenty nineteen]Navigation menu is messy with rtl language to Twenty Nineteen: Navigation menu is messy with RTL language

#3 @SergeyBiryukov
5 years ago

  • Milestone changed from Awaiting Review to 5.3
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

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


5 years ago

#5 @davidbaumwald
5 years ago

@SergeyBiryukov This one was brought up in the bug scrub today. Is this still on your list for 5.3?

#6 @marybaum
5 years ago

  • Keywords needs-testing added

@sabernhardt
5 years ago

Two RTL languages in switcher, on English page in Chrome

@sabernhardt
5 years ago

Two RTL languages in primary menu list items set to display: inline-block (on English page, in Chrome)

#7 @sabernhardt
5 years ago

  • Keywords needs-refresh added

I'll still test more by tomorrow to see if I can find a problem with using display: inline-block for the primary menu items, but a change to one stylesheet would need to be made for both language directions.

A second, adjacent RTL language link on a left-to-right language's page mixes up the two flags visually in Chrome, Edge and Internet Explorer (Firefox was fine). Setting the list items to inline-block in styles.css as well would take care of that.

(testing Twenty Nineteen 1.4 with Polylang 2.6.5 and WordPress 5.3 beta 3 on Windows 10)

@sabernhardt
5 years ago

Primary menu list items set to display: inline-block for both LTR and RTL stylesheets, plus SCSS

#8 @sabernhardt
5 years ago

  • Keywords needs-refresh removed

Setting to display: inline-block works for me:

  1. with or without the flags,
  2. either single-level or multi-level, and
  3. with both LTR and RTL text directions.

So the new patch includes the two stylesheets plus the related SCSS partial.

Windows browsers used:

  • Google Chrome 77.0.3865.120
  • Mozilla Firefox 69.0.3
  • Microsoft Edge 44.18362.387.0
  • Internet Explorer 11

#9 @davidbaumwald
5 years ago

  • Milestone changed from 5.3 to Future Release

With 5.3 RC1 releasing today, this is being moved to Future Release. If any committer feels this can be worked in quickly for 5.3 or can assume ownership in 5.4, feel free to move it back up.

#10 @karmatosed
3 weeks ago

  • Keywords commit added
  • Milestone changed from Future Release to 6.7
  • Owner changed from SergeyBiryukov to karmatosed

I am going to test this to look at getting this into commit as it seems like that was all needed to be done. Thanks everyone.

#11 @karmatosed
3 weeks ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 58582:

Twenty Nineteen: Fixes messy navigation with RTL language.

The first tag wasn't switching correctly. This resolves it for an adjacent RTL language link setting list items to inline-block.

Props manooweb, audrasjb, SergeyBiryukov, davidbaumwald, marybaum, sabernhardt.
Fixes #46658.

Note: See TracTickets for help on using tickets.