Skip to content

Do not report "baseline" on formatting information request#11897

Merged
feerrenrut merged 19 commits into
nvaccess:masterfrom
CyrilleB79:noBaselineInFormattingInfo2
Feb 15, 2022
Merged

Do not report "baseline" on formatting information request#11897
feerrenrut merged 19 commits into
nvaccess:masterfrom
CyrilleB79:noBaselineInFormattingInfo2

Conversation

@CyrilleB79

@CyrilleB79 CyrilleB79 commented Dec 3, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

Link to issue number:

Fixes #11815
Fixes #11078.

Summary of the issue:

When fetching formatting information with NVDA+F (once or twice) in a Firefox page, "baseline" is reported.
Only subscript or superscript should be reported, not "baseline" since "baseline" should be considered the default text position.
Also, when "Report superscript and subscript" option is checked, navigating in a Firefox page cause spurious reporting of 'baseline'. This is due to transitions between elements having no formatting information such as button or graphics and baseline text.

Description of how this pull request fixes the issue:

Updated design after review comments in this PR:

  • Ensured that all object generating the textInfo format field provide only standard values for 'text-position' field; superscript, subscript, baseline and unspecified.
  • Unspecified is used in the following cases:
    • the API does not provide formatting info (e.g. buttons in Firefox)
    • mixed text-position information, e.g. an Excel cell containing baseline and superscript.

Alternative solution

The old design, initially implemented in this issue before review comments, was the following:

  • No modification was done on the values produced by the various API.
  • In getFormatFieldSpeechin speech\__init__.py, when getting 'text-position' attribute, replace any attribute that do not match 'sub' or 'super' with None.
  • This allows not to report "baseline" when requesting formatting information, but also not to report "baseline" when the cursor moves from a web element that do not have any formatting attribute available.

Testing performed:

For the folling APIs/files, the following test were made:

  • Check that NVDA+F is able to report superscript and subscript
  • Check that NVDA+F does not report text position information for baseline text or unspecified text position (if applicable)
  • Activate "Report superscript and subscript" option and navigate whith the caret to test all possible transitions between the 3 or 4 possible text position (superscript, subscript, baseline and unspecified).

API/files tested:

  • NVDAObjects/IAccessible/__init__.py: tested with Chrome
  • NVDAObjects/UIA/__init__.py: tested with Word (UIA)
  • NVDAObjects/window/edit.py, class ITextDocumentTextInfo: Tested with Wordpad.
  • NVDAObjects/window/edit.py, class EditTextInfo: I do not know where I can test it natively, but I have used the suggestion made in Do not report "baseline" on formatting information request #11897 (comment) to test (successfully) in WordPad.
  • NVDAObjects/window/excel.py: tested with Excel, cell navigation.
    Note: When editing the cell's content, no formatting can be reported (neither before this PR nor with this PR).
  • NVDAObjects/window/winword.py: tested with Word '16.0.5188.1000' (UIA not checked)
  • appModules/powerpnt.py: tested with PowerPoint slide writing mode (formatting not reported in slideshow mode, neither before this PR nor after).
  • appModules/soffice.py: tested with Libre Office Writer 7.1
  • virtualBuffers/MSHTML.py: Tested with IE, with CSS vertical-align property.
    Note: <sub> / <super> tags do not work, neither with this PR nor before.

API/files not (yet) tested:

  • NVDAObjects/JAB/__init__.py
    In which control/program could I test?

Other tests:

  • Firefox: With "Report superscript and subscript" option checked, navigate with the virtual cursor on a GitHub issue page around buttons and graphics. No spurious "baseline" can be heard.
  • Excel: While editing cell content and pressing NVDA+F, "No formatting information" is now reported, since it cannot be retrieved. Before this PR, "baseline" was reported whatever the content of the cell.

Known issues with pull request:

Two APIs have not been tested (cf Test section).
Reviewers: please help me complete the test coverage.

Also reviewer, please check that I haven't forgotten any API.

Change log entry:

Changes
`'Baseline' is not reported anymore upon text formatting request (NVDA+F). (#11815)``

Bug fixes
'Baseline' is not frequently spuriously reported anymore whild reading some webpages when report superscript and subscript option is active. (#11078)

@CyrilleB79
When requesting text formatting information, "baseline" is not report…
69e252d 
…ed anymore since it is considered as the default text position.

It is still reported on cursor movement if report superscript and subscript option is activated in document formatting options.
@CyrilleB79 CyrilleB79 mentioned this pull request Dec 3, 2020
Closed
@CyrilleB79 CyrilleB79 mentioned this pull request Jan 27, 2021
Closed
feerrenrut
feerrenrut suggested changes Mar 25, 2021
View reviewed changes

@feerrenrut feerrenrut left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally this looks good, thanks @CyrilleB79. Add in that comment and I think this is good to go.

Comment thread source/speech/__init__.py Outdated
@CyrilleB79

CyrilleB79 commented Apr 1, 2021

Copy link
Copy Markdown
Contributor Author

Regarding IA2TextTextInfo, the function normalizeIA2TextFormatField seems to perform some sort of normalization on the attribute but superscript / subscript seems not to be considered there.

What I can suggest for this PR is at least:

  • remove the capability to produce 'baseline' in UIATextInfo and in SymphonyTextInfo. Indeed, this PR considers in getFormatFieldSpeech that the text is baseline if attrs["text-position"] == 'baseline' or if the attribute attrs["text-position"] does not exists at all.
  • log a warning in if attrs["text-position"] is something else than 'sub' or 'super', in order to fix this unexpected value in a future PR

Optionally I may also normalize the text-position attribute of IA2TextTextInfo to 'sub' or 'super' only, depending to your comments on this topic.
On this page, I have seen that only sub, super and baseline values are allowed. Since the unexpected values ('auto', 'middle', '50%'.) come from IE, maybe it corresponds to an older spec or maybe IE does not fully support the spec? Or do you have another link regarding this spec with more info? I am not sure to get the information at the correct place.

At last, in future PRs, I may fix some other issues encountered thanks to the warning I will have added in this PR.
I have also seen some other potential issues while digging for this PR (WordPad, Excel cells). I will investigate and open new issues/PR for this if required.

@michaelDCurran

michaelDCurran commented Apr 6, 2021

Copy link
Copy Markdown
Member

To clarify, Internet Explorer does not use IAccessible2. Rather we get the value for text-position directly from the CSS vertical-align property: see https://developer.mozilla.org/en-US/docs/Web/CSS/vertical-align for a list of all possible vertical-align values.
I would suggest for IE that we filter out anything that is not super or sub.
This can be done in virtualBuffers.mshtml.mshtmlTextInfo._normalizeFormatField.
In general though, I would a gree the best approach is for all APIs to expose text-position as either only super, sub, or not at all.

@CyrilleB79

CyrilleB79 commented Apr 6, 2021

Copy link
Copy Markdown
Contributor Author

I have addressed the majority of the comments.

Two points / questions are still pending:

  1. @feerrenrut, you suggested in a line comment:

It would be nice if each of the providers for this info normalized the values to a some Enum value.
As seen with MSHTML (Internet Explorer) virtual buffer, there are some case where the values are passed directly from the API. The enum will not be used there. Is there a risk to mix-up values from the enum and values produced directly as strings?

  1. I hope to have considered all the possible APIs that may produce a 'text-position' attribute in a formatted text to speak. Reviewer, please double check it; I do not have the full picture of all the APIs and what they may generate.
    Searching for "text-position" in the code should be quite exhaustive however.
    In any case, if there is an unexpected value, it will be signaled by the warning.

feerrenrut
feerrenrut reviewed Apr 7, 2021
View reviewed changes
Comment thread source/virtualBuffers/MSHTML.py Outdated
@CyrilleB79 CyrilleB79 requested a review from a team as a code owner May 1, 2021 14:45
@CyrilleB79 CyrilleB79 requested a review from seanbudd May 1, 2021 14:45
@CyrilleB79 CyrilleB79 marked this pull request as draft May 19, 2021 10:09
@lukaszgo1

lukaszgo1 commented Sep 8, 2021

Copy link
Copy Markdown
Contributor

Hello @CyrilleB79 I was pretty excited when I saw this fix since #11078 is pretty annoying for me however it seems this PR is not progressing. Is it just a matter of finding time to work on this for you or have you encountered any technical difficulties. Is there anything I can help with to progress this work?

@CyrilleB79

CyrilleB79 commented Sep 8, 2021

Copy link
Copy Markdown
Contributor Author

Hello

Sorry for the delay. And thanks to ping me.
This PR requires a little time to be updated to the last code base and for testing all the cases and I have postponed it to handle more straight forward issues.
I will try to dedicate time to it and ask for help if needed (maybe for testing the various applications/interfaces).
There should not be technical issue either.

@CyrilleB79

CyrilleB79 commented Sep 26, 2021

Copy link
Copy Markdown
Contributor Author

Re-testing after the upmerges and fixes pushed on 22nd September 2021:
The tests are done with report superscript and subscript activated:

Work in progress:

  1. Firefox does not report spuriously 'baseline' when navigating in this GitHub PR page (as previously described in report subscripts and superscripts #10919: exclude reporting on elements in browsers such as buttons etc. #11078).

  2. Checked the transition between superscript, subscript, baseline (and unspecified if applicable) and checked that NVDA+F does not report baseline anymore:

    • Excel '16.0.14326.20404': transitions between cells: 4 types of cell (4th type = cell with mixed text position)
    • Word '16.0.14326.20404' (thus with UIA now): 3 text position types
    • PowerPoint '16.0.14326.20404': 3 text position types (slide edition ; slideshow does not seem to support such report)
    • WordPad (thus IAccessible): 3 text position types

@CyrilleB79 CyrilleB79 marked this pull request as ready for review October 1, 2021 22:00
@CyrilleB79

CyrilleB79 commented Oct 1, 2021

Copy link
Copy Markdown
Contributor Author

Hello

I have done the rework to generate standard text-position values at each API level and have tried to test all possible cases.
I have updated the initial description according to the new design and with the many additional manual tests done.

@feerrenrut you may review it again since you had asked to use the Enum. Note that @seanbudd was automatically requested as a reviewer when I have switched back this PR from draft to ready state.

@lukaszgo1 if you want to add your review comments or additional test coverage, you are welcome.

At last, sorry for the delay to address this PR review comments. I hope to have it finalized soon now!

@lukaszgo1

lukaszgo1 commented Oct 2, 2021

Copy link
Copy Markdown
Contributor

API/files not (yet) tested:

* `NVDAObjects/JAB/__init__.py`

It would be pretty tough to find a example for this - the only known JAB object which exposed formatting info was editor in Open Office before they switched to IA2.

* `NVDAObjects/window/edit.py`, class `EditTextInfo`
  In which control/program could I test?

You can always modify _get_TextInfo on Edit class to unconditionally return EditTextInfo and test in wordpad.

@CyrilleB79

CyrilleB79 commented Oct 2, 2021

Copy link
Copy Markdown
Contributor Author

It would be pretty tough to find a example for this - the only known JAB object which exposed formatting info was editor in Open Office before they switched to IA2.

Do you have the version number, and even a portable version to download?

You can always modify _get_TextInfo on Edit class to unconditionally return EditTextInfo and test in wordpad.

Thanks for the trick. I have been able to test this part of code successfully and have updated the initial description accordingly.

@lukaszgo1

lukaszgo1 commented Oct 2, 2021

Copy link
Copy Markdown
Contributor

Do you have the version number, and even a portable version to download?

It looks like version 3.3.0 still required JAB these builds can be downloaded from http://archive.apache.org/dist/incubator/ooo/stable/3.3.0/. It looks like portable versions weren't officially available.

@CyrilleB79

CyrilleB79 commented Oct 5, 2021

Copy link
Copy Markdown
Contributor Author

Many thanks Lukasz for your help.

After thinking again to it, Open Office v3.3 is already ten year old. And I do not know any other application using this interface and featuring text with superscript / subscript.
Maybe an attentive review of NVDAObjects/JAB/__init__.py without test would be enough; the code is not complex.
@feerrenrut just let us know if you expect to have this part of code tested or not.

@feerrenrut feerrenrut requested review from feerrenrut and removed request for seanbudd October 7, 2021 03:08
@lukaszgo1

lukaszgo1 commented Oct 8, 2021

Copy link
Copy Markdown
Contributor

It turns out that while OO 3.3 indeed requires JAB to be accessible the version of Java it bundles is too old to work with the version of Access Bridge which is bundled in NVDA. It should probably be possible to work around this somehow, perhaps by forcing Open Office to use never JDK, but given age of this version I don't think trying to force it to work with NVDA is good use of anyone's time.

@CyrilleB79

CyrilleB79 commented Oct 8, 2021

Copy link
Copy Markdown
Contributor Author

It turns out that while OO 3.3 indeed requires JAB to be accessible the version of Java it bundles is too old to work with the version of Access Bridge which is bundled in NVDA. It should probably be possible to work around this somehow, perhaps by forcing Open Office to use never JDK, but given age of this version I don't think trying to force it to work with NVDA is good use of anyone's time.

And overall it would not correspond to any real life use case.

Thanks @lukaszgo1 for this investigation.

Let's just review the code and test it if one day we find another more modern application using this part of the code.

@CyrilleB79

CyrilleB79 commented Oct 21, 2021

Copy link
Copy Markdown
Contributor Author

@feerrenrut, given your recent work in #12969, you may be interested to review this one. It may be interesting to test it a while in alpha given the modifications in all the interfaces (even if minor).

Moreover if you do not plan to review it soon, could you add a milestone to keep it tracked? Thanks.

@CyrilleB79

CyrilleB79 commented Nov 23, 2021

Copy link
Copy Markdown
Contributor Author

@feerrenrut is this PR still on your radar?
Any chance to have it in 2022.1? If yes, please tag it with the milestone.

Having this review finalized would also allow me to submit another work regarding formatting review, using the same strategy.
Thanks.

@CyrilleB79

CyrilleB79 commented Jan 16, 2022

Copy link
Copy Markdown
Contributor Author

@feerrenrut now that 2021.3.1 is released, is there a chance to have this PR reviewed for 2022.1?
There has just been a request again on a French user mailing list on this topic.

@CyrilleB79

This comment was marked as off-topic.

Sign in to view
@seanbudd

seanbudd commented Feb 1, 2022

Copy link
Copy Markdown
Member

I haven't reviewed the code, I have just noticed that a file had a chmod operation, source/speech/__init__.py 100644 → 100755. Can this be removed?

@CyrilleB79

CyrilleB79 commented Feb 2, 2022

Copy link
Copy Markdown
Contributor Author

Thanks for this remark @seanbudd .

The following commits have changed the permissions on this file: b95743d and 57f2791. They do not belong to this PR, but come from a merge of master branch.
But strangely, the merge of the commits from master has not modified the permissions in my branch. I do not know why.

Anyway, it should be fixed now.

Regarding the targetted release, do you have any information?

@AppVeyorBot

This comment was marked as outdated.

Sign in to view
@CyrilleB79

CyrilleB79 commented Feb 7, 2022

Copy link
Copy Markdown
Contributor Author

I have hidden my previous comment (#11897 (comment)) regarding compatibility and have tagged it as off-topic. Indeed, it was irrelevant since there is actually no compatibility issue in this PR.
I have just pushed a commit removing the irrelevant code dealing with compatibility since it was not needed.
Sorry for the confusion.

@AppVeyorBot

This comment was marked as outdated.

Sign in to view
@AppVeyorBot

This comment was marked as outdated.

Sign in to view
feerrenrut
feerrenrut approved these changes Feb 15, 2022
View reviewed changes

@feerrenrut feerrenrut left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @CyrilleB79

@feerrenrut feerrenrut merged commit 60d564f into nvaccess:master Feb 15, 2022
@nvaccessAuto nvaccessAuto added this to the 2022.1 milestone Feb 15, 2022
@CyrilleB79 CyrilleB79 deleted the noBaselineInFormattingInfo2 branch February 15, 2022 14:21
@CyrilleB79 CyrilleB79 mentioned this pull request Apr 6, 2022
Merged
15 tasks
@CyrilleB79 CyrilleB79 mentioned this pull request Jul 24, 2022
Merged
5 tasks
@CyrilleB79 CyrilleB79 mentioned this pull request Jul 27, 2023
Merged
13 tasks
seanbudd pushed a commit that referenced this pull request Aug 29, 2023
@CyrilleB79
Text alignment reporting and enhancements for MS Excel cells and MS W…
6dd48c4 
…ord text (#15205)

Closes #15206
Closes #15220

Summary of the issue:
Various issues for alignment reporting:

In MS Excel horizontal or vertical text alignment is not always correctly reported.
In MS Word (legacy), text alignment is not correctly reported for "justify" and for "distrubute".
Description of user facing changes
Reporting text alignment in Word and Excel is now handled better in various situations. "Default" is not reported anymore since it does not give any clear indication of the position of the text.

In MS Excel (no UIA) the alignment is reported as per Excel's options:
for horizontal text alignment in cell: "General", "Centered across columns", "Repeat" and "Distribute" is now reported instead of "Default" or nothing
for vertical alignment: "bottom" is reported instead of "default"
In MS Word (no UIA):
"justify" is now reported instead of "Default"
"distribute" is now reported instead of nothing
Description of development approach
Standardized alignments returned by getFormatField* functions around a string enum as done in #11897. Thus, all the interfaces are impacted.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@feerrenrut feerrenrut feerrenrut approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2022.1

7 participants

@CyrilleB79 @michaelDCurran @lukaszgo1 @seanbudd @AppVeyorBot @feerrenrut @nvaccessAuto