Skip to content

Stop using UnidentifiedEdit in Fast Log Entry edit field#11488

Merged
feerrenrut merged 4 commits into
nvaccess:masterfrom
lukaszgo1:I8996
Sep 9, 2020
Merged

Stop using UnidentifiedEdit in Fast Log Entry edit field#11488
feerrenrut merged 4 commits into
nvaccess:masterfrom
lukaszgo1:I8996

Conversation

@lukaszgo1

@lukaszgo1 lukaszgo1 commented Aug 12, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

Link to issue number:

Fixes #8996

Summary of the issue:

In #8165 we started using Unidentifiededit for editable text fields in which there is a caret and which seems to support Edit API. While both of these criterion's are true for Fast Log Entry edit fields their WindowText contains complete garbage and UnidentifiedEdit relies on Windowtext being correct.

Description of how this pull request fixes the issue:

  1. For edit field in Fast Log Entry DisplayModelEditableText is used similar to the pre Try to use window.edit.Edit when an editable window supports it #8165
  2. DisplayModelEditableText no longer inherits from EditableText rather from EditableTextWithoutAutoSelectDetection which makes selection announced when the color used for highlight in a particular app is the default Windows highlight color - This has been suggested by @jcsteh in textpad - selection done using shift + arrow keys is not read #4535.

Testing performed:

Ensured that main edit field in Fast Log Entry is once again readable and that selection is announced in most cases.
The try build has also been tested by the original reporter and the issue is fixed for him as well.

Known issues with pull request:

Change log entry:

Section: Bug fixes

  • NVDA once again works correctly in Fast Log Entry edit field.
  • NVDA should announce text selection in more cases.

@lukaszgo1

lukaszgo1 commented Aug 12, 2020

Copy link
Copy Markdown
Contributor Author

@LeonarddeR You may be interested in reviewing this.

@feerrenrut

feerrenrut commented Aug 13, 2020

Copy link
Copy Markdown
Contributor

What else uses DisplayModelEditableText? Changing it's behavior runs the risk of introducing regressions elsewhere, unless those usages have been checked, and will benefit from the change.

@LeonarddeR

LeonarddeR commented Aug 13, 2020

Copy link
Copy Markdown
Collaborator

DisplayModelEditableText is still used in quite a few places in the wild. Having said that, I think it makes sense to test this and see whether any regressions are reported.

LeonarddeR
LeonarddeR suggested changes Aug 13, 2020
View reviewed changes

@LeonarddeR LeonarddeR left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This appModule shares logic in thechooseNVDAObjectOverlayClasses and event_NVDAObject_init methods. Therefore, I prefer this to be changed in such a way that you create a new overlay class that's added in chooseNVDAObjectOverlayClasses, which inherrits both from DisplayModelEditableText and EditableTextWithoutAutoSelectDetection. It can then override name to return None. I think this causes the least chance of regression possible so it also addresses @feerrenrut 's concern

@lukaszgo1

lukaszgo1 commented Aug 13, 2020

Copy link
Copy Markdown
Contributor Author

@feerrenrut wrote:

What else uses DisplayModelEditableText?

It is used in various editable text cases when NVDA cannot access text otherwise.

Changing it's behavior runs the risk of introducing regressions elsewhere, unless those usages have been checked, and will benefit from the change.

First of all EditableTextWithoutAutoSelectDetection extends EditableText the only difference lies in the fact that EditableText does not support selection changes. and most, (probably all) edit fields using DisplayModelEditableText allows for text selection. IMHO using EditableText for DisplayModelEditableText was a mistake in the first place. In addition I've worked with a custom appmodule for Becky! Internet Mail with this change for for half a year now and there was no problems for DisplayModelEditableText cases and it allows for selection to be announced. Even if we decide not to change this in this PR it is something which has to be changed eventually - using class which has no support for selection for controls which allows for it makes no sense.

@LeonarddeR

LeonarddeR commented Aug 13, 2020

Copy link
Copy Markdown
Collaborator

@lukaszgo1 commented on Aug 13, 2020, 2:59 PM GMT+2:

First of all EditableTextWithoutAutoSelectDetection extends EditableText the only difference lies in the fact that EditableText does not support selection changes. and most, (probably all) edit fields using DisplayModelEditableText allows for text selection.

While this is true, theoretically it might be the case that selection is reported erroneously in controls using this.
Would be interested to know what @michaelDCurran thinks. I have no opinion, there are good reasons for both decisions.

@lukaszgo1

lukaszgo1 commented Aug 13, 2020

Copy link
Copy Markdown
Contributor Author

@LeonarddeR wrote:

This appModule shares logic in thechooseNVDAObjectOverlayClasses and event_NVDAObject_init methods. Therefore, I prefer this to be changed in such a way that you create a new overlay class that's added in chooseNVDAObjectOverlayClasses,...

Done

which inherrits both from DisplayModelEditableText and EditableTextWithoutAutoSelectDetection. It can then override name to return None. I think this causes the least chance of regression possible so it also addresses @feerrenrut 's concern

While we are avoiding possible regressions this way we then have to either do that for every DisplayModelEditableText for which it gives better results or do that for Fast Log Entry and then, in a subsequent PR, make DisplayModelEditableText inherit from EditableTextWithoutAutoSelectDetection.

If we consider #8996 serious enough to include a fix for it into 2020.3 then I agree option 2 is the proper fix and I'll implement it. If the fix can wait until 2020.4 however, I'd rather leave this pr as is and ask @feerrenrut or @michaelDCurran to merge it early during 2020.4 development cycle and let the change from EditableText to EditableTextWithoutAutoSelectDetection have proper testing in Alpha and prove itself.

Also regarding this:

While this is true, theoretically it might be the case that selection is reported erroneously in controls using this.

I agree that it is theoretically possible, however:

  • for now selection in DisplayModelEditableText cannot be reported at all
  • I don't see any possibility to prove that this is causing spurious selection announcements other than having this changed in Alpha for a while and reacting to user reports.

@LeonarddeR

LeonarddeR commented Aug 14, 2020

Copy link
Copy Markdown
Collaborator

If the fix can wait until 2020.4 however, I'd rather leave this pr as is and ask @feerrenrut or @michaelDCurran to merge it early during 2020.4 development cycle and let the change from EditableText to EditableTextWithoutAutoSelectDetection have proper testing in Alpha and prove itself.

This makes sense to me. I believe the merge window for 2020.3 is now closed.

@LeonarddeR

LeonarddeR commented Aug 14, 2020

Copy link
Copy Markdown
Collaborator

Looking at this again, It occurs to me that TSynMemo is a Delphi control that is likely to be used much more generally. See https://github.com/SynEdit/SynEdit/
I'd also agree if you apply this change more generally.
You can add it to the IAccessible.delphi module. Note that you can simply override name and set windowTextLineCount to 0 there, it ensures that later on, DisplayModelEditableText is added.
I agree it is a bit of a change, though. Sorry for not coming across this earilier.

@lukaszgo1

lukaszgo1 commented Aug 14, 2020

Copy link
Copy Markdown
Contributor Author

@LeonarddeR wrote:

Looking at this again, It occurs to me that TSynMemo is a Delphi control that is likely to be used much more generally. See https://github.com/SynEdit/SynEdit/
I'd also agree if you apply this change more generally.

I don't think this is a good idea. This control is used, for example, in Dev-C++ and there WindowText contains the real text of the control.

@LeonarddeR

LeonarddeR commented Aug 14, 2020

Copy link
Copy Markdown
Collaborator

Ah, that's great.

LeonarddeR
LeonarddeR previously approved these changes Aug 14, 2020
View reviewed changes
@feerrenrut feerrenrut added this to the 2020.4 milestone Aug 14, 2020
feerrenrut
feerrenrut previously approved these changes Sep 9, 2020
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 @lukaszgo1 looks good to me.

@feerrenrut feerrenrut dismissed stale reviews from LeonarddeR and themself via 1b8b283 September 9, 2020 14:49
@feerrenrut feerrenrut merged commit 213a3c2 into nvaccess:master Sep 9, 2020
@lukaszgo1 lukaszgo1 deleted the I8996 branch September 9, 2020 16:18
dawidpieper pushed a commit to dawidpieper/nvda that referenced this pull request Sep 13, 2020
@lukaszgo1 @dawidpieper
Use DisplayModelEditableText in Fast Log Entry edit field (PR nvacces…
ec1e72d 
…s#11488)

Fixes nvaccess#8996

In nvaccess#8165 we started using Unidentifiededit for editable text fields in which there is a caret and which seems to support Edit API. While both of these criterion's are true for Fast Log Entry edit fields their WindowText contains complete garbage and UnidentifiedEdit relies on Windowtext being correct.

Instead:
1. For edit field in Fast Log Entry DisplayModelEditableText is used similar to the pre nvaccess#8165 
2. DisplayModelEditableText no longer inherits from EditableText rather from EditableTextWithoutAutoSelectDetection which makes selection announced when the color used for highlight in a particular app is the default Windows highlight color  - This has been suggested by @jcsteh in nvaccess#4535
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@feerrenrut feerrenrut feerrenrut approved these changes

@LeonarddeR LeonarddeR LeonarddeR left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2020.4

Development

Successfully merging this pull request may close these issues.

NVDA does not read in the Fast Log Entry

3 participants

@lukaszgo1 @feerrenrut @LeonarddeR