Skip to content

Revert "Remove ignoreEditableText param"#12766

Merged
michaelDCurran merged 2 commits intomasterfrom
revertIgnoreEditableText
Sep 24, 2021
Merged

Revert "Remove ignoreEditableText param"#12766
michaelDCurran merged 2 commits intomasterfrom
revertIgnoreEditableText

Conversation

@michaelDCurran
Copy link
Copy Markdown
Member

This reverts commit 6262596.

In issue #12746 it was reported that moving by paragraph in Google Docs in Google Chrome would sometimes result in NVDA saying "blank" for certain paragraphs.
Similarly although not noted in an issue yet, Some lines in Thunderbird message composition windows would also say blank.

These regressions were found to be caused by the merging of pr #12500

Further investigation found that the specific commit in the pr that caused this was 6262596

I'm not entirely sure on why this commit causes the regression, however my understanding is that this commit was only to improve readability / understanding of logic along the way and that the actual feature introduced by the pr had no dependency on this commit as such.

Thus, this pr reverts that single commit, and therefore fixes #12746.

Perhaps the logic of this commit can be better investigated at a later stage, but for now it is better to return to what we know works.

@michaelDCurran michaelDCurran requested a review from a team as a code owner August 23, 2021 23:01
@AppVeyorBot
Copy link
Copy Markdown

See test results for failed build of commit cd95ffa236

@feerrenrut
Copy link
Copy Markdown
Contributor

I suspect this bug has something to do with the "controlStack" parameter and whether it should hold None or not. This code is quite hard to reason about. If we really can't find a way to adjust this code to fix this issue, then I'm ok to revert. On the other hand, if we don't work through issues like this, it can result in "not safe to touch" code, no one knows what the requirements are so it can never be modified.

@michaelDCurran
Copy link
Copy Markdown
Member Author

michaelDCurran commented Aug 24, 2021 via email

@feerrenrut
Copy link
Copy Markdown
Contributor

Yes, to fix this on alpha while we are working on investigating it, let's revert. But could you also add a system test to cover this case? Then when we are working on it we can be sure we don't re-introduce this specific problem.

Co-authored-by: Sean Budd <sean@nvaccess.org>
@AppVeyorBot
Copy link
Copy Markdown

See test results for failed build of commit bab14c34bf

@michaelDCurran michaelDCurran merged commit 5b821c6 into master Sep 24, 2021
@michaelDCurran michaelDCurran deleted the revertIgnoreEditableText branch September 24, 2021 03:06
@nvaccessAuto nvaccessAuto added this to the 2021.3 milestone Sep 24, 2021
@feerrenrut feerrenrut mentioned this pull request Oct 20, 2021
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Focus issues reading by paragrph in Google docs involving blank lines

5 participants