Fix right-to-left layout direction issues #12181

seanbudd · 2021-03-16T04:14:40Z

Link to issue number:

Summary of the issue:

#8859

Right to left layouts caused several GUI elements to not render, or not render correctly.

A ticket was raised on wxWidgets, where support was removed for the behaviour we were using, and a work around was suggested.

Adding controls to the wx.StaticBoxSizer by adding them to the same parent causes undefined behaviour, specifically with Right-To-Left layouts. wxWidgets now requires that these items be added as children.

#638

wxWidgets did not respect the layout out of language when setting the language via NVDA as opposed through the system/Windows. This means languages with a right-to-left layout would render left-to-right unless the system locale also used a right-to-left layout

Description of how this pull request fixes the issue:

#8859

The suggested and implemented workaround is as follows:

Before:

sizer = new wx.StaticBoxSizer(wx.VERTICAL, self, "Test")
sizer.Add(wx.StaticText(self, wx.ID_ANY, "Where am I?"))
sizer.Add(wx.Button(self, wx.ID_ADD))

After:

sizer = new wx.StaticBoxSizer(wx.VERTICAL, self, "Test")
sizer.Add(wx.StaticText(sizer.GetStaticBox(), wx.ID_ANY, "Where am I?"))
sizer.Add(wx.Button(sizer.GetStaticBox(), wx.ID_ADD))

#638

The layout for the wx.App main frame is manually set using wx.Locale information.

Testing strategy:

Visually compare GUI features outlined in #8859 between an LTR language such as English and an RTL language such as Farsi.

Known issues with pull request:

Some GUI features still display differently for right-to-left languages

the right border of groupings clips with labels/controls.

Change log entry:

Bug fixes

- Fix graphical bugs such as missing elements when using NVDA with a right-to-left layout. (#12181)
   - known issue for right-to-left languages: the right border of groupings clips with labels/controls. 
- Respect the GUI layout direction based on the NVDA language, not the system locale. (#12181)

Code Review Checklist:

standardize LTRStaticBoxSizer usage standardize LTRStaticBoxSizer usage test wx RTL workaround use wxWidgets workaround commit c59e874 Author: buddsean <sean@nvaccess.org> Date: Tue Mar 16 14:09:26 2021 +1100 try parent instead of sibling method commit 11017de Author: buddsean <sean@nvaccess.org> Date: Tue Mar 16 13:25:59 2021 +1100 remove LTRStaticBoxSizer workaround commit e17de62 Author: buddsean <sean@nvaccess.org> Date: Tue Mar 16 13:14:39 2021 +1100 try parent instead of sibling method commit 45514f2 Author: buddsean <sean@nvaccess.org> Date: Tue Mar 16 11:48:24 2021 +1100 try parent instead of sibling method commit 9575740 Author: buddsean <sean@nvaccess.org> Date: Tue Mar 16 11:23:59 2021 +1100 try parent instead of sibling method commit 608fb66 Author: buddsean <sean@nvaccess.org> Date: Mon Mar 15 14:38:37 2021 +1100 test wx RTL workaround commit f1189bf Author: buddsean <sean@nvaccess.org> Date: Mon Mar 15 14:03:20 2021 +1100 test wx RTL workaround

AppVeyorBot · 2021-03-16T04:31:09Z

PR introduces Flake8 errors
Build (for testing PR)

See test results for failed build of commit de830fa79b

AppVeyorBot · 2021-03-16T04:54:37Z

System test failure
Build (for testing PR)

See test results for failed build of commit e773d661f1

CyrilleB79 · 2021-03-16T08:06:25Z

Testing strategy:

Visually compare GUI features outlined in #8859 between an LTR language such as English and an RTL language such as Farsi. This must be done by changing the system locale, so using a VM is suggested.

Why can't it just be checked by changing NVDA language to an RTL language?

seanbudd · 2021-03-16T22:46:31Z

Hi @CyrilleB79,

Currently this issue #638, but I think it's worth fixing in this PR just to make testing this easier. I think a major factor blocking #638 was the GUI issues fixed in this PR.

Going to investigate a fix for this and hopefully close out both tickets.

AppVeyorBot · 2021-03-17T00:07:08Z

PR introduces Flake8 errors
Build (for testing PR)

See test results for failed build of commit d668aae5e6

feerrenrut

In the changes entry for #638 I think it would be good to mention Right to Left languages.

Overall this looks good. Testing this myself I noticed that the right side of the static box outline is missing where there are labels or controls. It's still an improvement, but it might be worth mentioning this is known in the change log.

I tried "Arabic":

source/gui/__init__.py

source/core.py

source/gui/__init__.py

source/core.py

AppVeyorBot · 2021-03-17T06:38:12Z

Unit test failure
Build (for testing PR)

See test results for failed build of commit bd9f0f3e61

AppVeyorBot · 2021-03-17T06:55:27Z

PR introduces Flake8 errors
Build (for testing PR)

See test results for failed build of commit 6a62962912

source/core.py

feerrenrut

Looks good, just a reminder in my previous review I mentioned some considerations for the change log entries, happy for you to go ahead and merge.

CyrilleB79 · 2021-03-18T07:32:53Z

Hi @seanbudd

You have merged this branch in master, but you did not select "Squash merge", which is the usual way to do in NVDA project. This result in many commits just for this PR in the commit history of master. It seems it was not intended.
Please think to it next time.
Cc @feerrenrut FYI.

feerrenrut · 2021-03-18T07:38:07Z

I see what has happened. @CyrilleB79 it was actually squash merged, but the commit message includes the details of all the commits.

I suspect @seanbudd used the gh tool (GitHub-cli) to merge. Older versions of this did not let you override the squash commit message. Newer versions do, but it must be done carefully and the title can not yet be written

seanbudd · 2021-03-18T23:44:06Z

Ah no @feerrenrut , I did not see the convention that was used in commit messages, is it the changelog? I have just been using the default in previous squash merges.

Thanks @CyrilleB79 for raising this

Summary of the issue: The standard installation process crashes with the following traceback, caused by #12181 Description of how this pull request fixes the issue: Fix various crashing GUI bugs introduced to the installation process in #12181 Add smoke tests for the installation process

In #12181, the setting "Report formatting changes after the cursor" in "Document Formatting" was incorrectly moved to the wrong parent, the static box grouping above it, making it invisible to the user. It was readable using NVDA but not graphically visible to the user. This has been corrected.

Navigating through settings using tabbing does not visually scroll to the focused control. wxPython ScrolledPanels calculate the position to scroll to based on the relative position to a focus elements parent, rather than the relative position to the ScrolledPanel itself. When fixing right-to-left issues in #12181, another layer of nesting was introduced for controls in our settings panels, which caused the controls to no longer get scrolled to. A patched version of ScrolledPanel is created, which calculates relative position to the ScrolledPanel

seanbudd added this to the 2021.1 milestone Mar 16, 2021

seanbudd self-assigned this Mar 16, 2021

fix flake8 issues

79af2fb

seanbudd requested a review from feerrenrut March 16, 2021 04:36

seanbudd marked this pull request as draft March 16, 2021 22:46

seanbudd added 2 commits March 17, 2021 09:54

Merge remote-tracking branch 'origin/master' into fix-8859

538389e

set layout on mainframe for locale

4e29c7d

seanbudd linked an issue Mar 16, 2021 that may be closed by this pull request

When using rtl languages, the text in the interface should be right justified. #638

Closed

seanbudd changed the title ~~move to standard usage of wx.StaticBoxSizer~~ Fix right-to-left layout direction issues Mar 16, 2021

seanbudd marked this pull request as ready for review March 16, 2021 23:52

fix lint

9cfc2d7

feerrenrut suggested changes Mar 17, 2021

View reviewed changes

source/gui/__init__.py Outdated Show resolved Hide resolved

source/core.py Outdated Show resolved Hide resolved

source/core.py Outdated Show resolved Hide resolved

seanbudd added 4 commits March 17, 2021 15:02

Merge branch 'master' into fix-8859

b31a61f

move out wx import

f241a04

use better typing and naming

39ec7d1

remove unused variable

503675d

seanbudd requested a review from feerrenrut March 17, 2021 04:35

apply DRY

560c278

feerrenrut reviewed Mar 17, 2021

View reviewed changes

source/core.py Outdated Show resolved Hide resolved

feerrenrut reviewed Mar 17, 2021

View reviewed changes

source/core.py Show resolved Hide resolved

feerrenrut reviewed Mar 17, 2021

View reviewed changes

source/gui/__init__.py Outdated Show resolved Hide resolved

feerrenrut reviewed Mar 17, 2021

View reviewed changes

source/core.py Outdated Show resolved Hide resolved

seanbudd added 2 commits March 17, 2021 16:58

undo risky import change

16d7477

fix lint

ce2c5f2

undo risky import change, fix lint

68e62cb

fix nose typing read

3484766

feerrenrut reviewed Mar 17, 2021

View reviewed changes

source/core.py Outdated Show resolved Hide resolved

remove unneccessary typing import

45460ee

feerrenrut approved these changes Mar 18, 2021

View reviewed changes

seanbudd added 2 commits March 18, 2021 14:39

Merge remote-tracking branch 'origin/master' into fix-8859

304cc6c

update changes.t2t

b138918

seanbudd merged commit 073826a into master Mar 18, 2021

seanbudd deleted the fix-8859 branch March 18, 2021 04:02

This was referenced Mar 19, 2021

Installation process crashes due to StaticBoxSizer changes #12198

Closed

fix installation crash from StaticBoxSizer changes #12199

Merged

NVDA Alpha 22012: installation aborts unexpectedly #12196

Closed

feerrenrut mentioned this pull request Apr 14, 2021

wxAssertion error when closing braille or speech settings panels. #12220

Closed

This was referenced Apr 16, 2021

Ensure settings are scrolled to using tab navigation #12300

Merged

Move report formatting changes box to the correct parent #12307

Merged

Create tool for comparing GUI and UI elements in copies of NVDA #12308

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix right-to-left layout direction issues #12181

Fix right-to-left layout direction issues #12181

seanbudd commented Mar 16, 2021 •

edited

AppVeyorBot commented Mar 16, 2021

AppVeyorBot commented Mar 16, 2021

CyrilleB79 commented Mar 16, 2021

Testing strategy:

seanbudd commented Mar 16, 2021 •

edited

AppVeyorBot commented Mar 17, 2021

feerrenrut left a comment

AppVeyorBot commented Mar 17, 2021

AppVeyorBot commented Mar 17, 2021

feerrenrut left a comment

CyrilleB79 commented Mar 18, 2021

feerrenrut commented Mar 18, 2021

seanbudd commented Mar 18, 2021

Fix right-to-left layout direction issues #12181

Fix right-to-left layout direction issues #12181

Conversation

seanbudd commented Mar 16, 2021 • edited

Link to issue number:

Summary of the issue:

#8859

#638

Description of how this pull request fixes the issue:

#8859

#638

Testing strategy:

Known issues with pull request:

Change log entry:

Code Review Checklist:

AppVeyorBot commented Mar 16, 2021

AppVeyorBot commented Mar 16, 2021

CyrilleB79 commented Mar 16, 2021

Testing strategy:

seanbudd commented Mar 16, 2021 • edited

AppVeyorBot commented Mar 17, 2021

feerrenrut left a comment

Choose a reason for hiding this comment

AppVeyorBot commented Mar 17, 2021

AppVeyorBot commented Mar 17, 2021

feerrenrut left a comment

Choose a reason for hiding this comment

CyrilleB79 commented Mar 18, 2021

feerrenrut commented Mar 18, 2021

seanbudd commented Mar 18, 2021

seanbudd commented Mar 16, 2021 •

edited

seanbudd commented Mar 16, 2021 •

edited