Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MozillaCompoundTextInfo: Don't skip ignored objects when determining whether the range covers the end of any ancestors of endObj. #9477

Merged
merged 3 commits into from Apr 12, 2019

Conversation

@jcsteh
Copy link
Contributor

commented Apr 10, 2019

Link to issue number:

None.

Summary of the issue:

In Google Docs (and other web-based editors), braille incorrectly displays "lst end" before the cursor in a list in some cases, even if the cursor is in the middle of the list item.

Description of how this pull request fixes the issue:

In order to display markers at the end of containers in braille (e.g. "lst end" at the end of lists), MozillaCompoundTextInfo needs to check the object at the end of the range (endObj) and its ancestors. If the range is at the end of any of these objects, we mark the associated control field as being at the end of its node. For example, if the range is at the end of a list, we mark the list's control field as being at end of node, which causes braille to display "lst end". However, if the range is not at the end of one of these objects, the range also can't be at the end of any of its ancestors. For example, if the range ends in the middle of a list item, it obviously doesn't cover the end of the list, even if the list item (as a whole) is the last item in the list. In this case, we don't check the remaining ancestors.

Previously, this was failing where there was a single paragraph inside the last list item in a list. The list would be marked as end of node, even if the range ended in the middle of the paragraph, resulting in a spurious "lst end" indicator before the cursor in braille.

This happened because we ignore paragraphs as irrelevant and thus don't generate control fields for them. However, the code which checks whether the range is at the end of endObj or its ancestors only ran if there was a control field. Because the paragraph had no control field, we'd skip it completely, not determining that the range ended in the middle of it and thus aborting the search. We'd then check the next ancestor, and since the paragraph was the last child of the list item which was in turn the last child of the list, we'd mark all of these as end of node, including the list.

To fix this, we now run this check regardless of whether we generated a control field for an object; i.e. regardless of whether we ignored a paragraph.

Testing performed:

  1. Opened this test case:
    data:text/html,<div contenteditable role="textbox" aria-multiline="true"><ul><li><p>test</p></li></ul></div>
  2. Focused the text box.
  3. Pressed home. Observed that braille displays "lst • test lst end".
  4. Pressed right arrow.
    • Result after the patch (expected result): Braille displayed "lst • test lst end"
    • Result before the patch (incorrect result): Braille displayed "lst • t lst end est lst end"

Also tested the same scenario in Google Docs and confirmed that it works as expected.

Known issues with pull request:

None known.

Change log entry:

Bug Fixes:

- In Google Docs (and other web-based editors), braille no longer sometimes incorrectly shows "lst end" before the cursor in the middle of a list item.

MozillaCompoundTextInfo: Don't skip ignored objects when determining …
…whether the range covers the end of any ancestors of endObj.

In order to display markers at the end of containers in braille (e.g. "lst end" at the end of lists), MozillaCompoundTextInfo needs to check the object at the end of the range (endObj) and its ancestors.
If the range is at the end of any of these objects, we mark the associated control field as being at the end of its node.
For example, if the range is at the end of a list, we mark the list's control field as being at end of node, which causes braille to display "lst end".
However, if the range is *not* at the end of one of these objects, the range also can't be at the end of any of its ancestors.
For example, if the range ends in the middle of a list item, it obviously doesn't cover the end of the list, even if the list item (as a whole) is the last item in the list.
In this case, we don't check the remaining ancestors.

Previously, this was failing where there was a single paragraph inside the last list item in a list.
The list would be marked as end of node, even if the range ended in the middle of the paragraph, resulting in a spurious "lst end" indicator before the cursor in braille.

This happened because we ignore paragraphs as irrelevant and thus don't generate control fields for them.
However, the code which checks whether the range is at the end of endObj or its ancestors only ran if there was a control field.
Because the paragraph had no control field, we'd skip it completely, not determining that the range ended in the middle of it and thus aborting the search.
We'd then check the next ancestor, and since the paragraph was the last child of the list item which was in turn the last child of the list, we'd mark all of these as end of node, including the list.

To fix this, we now run this check regardless of whether we generated a control field for an object; i.e. regardless of whether we ignored a paragraph.

@jcsteh jcsteh requested a review from feerrenrut Apr 10, 2019

@leonardder

This comment has been minimized.

Copy link
Collaborator

commented Apr 10, 2019

Does this supersede #9382?

@leonardder
Copy link
Collaborator

left a comment

Just if you're unaware, Mozilla Corporation is not yet in the contributors.txt. I guess it makes sense to be added, since you're marking them as copyright holder of your code.

@jcsteh

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

Does this supersede #9382?

No. They might seem related, but they're different issues and quite separate from a code perspective, though both fixes are certainly needed. Also, this one is needed for both Firefox and Chrome, whereas #9382 only manifests in Chrome.

@jcsteh jcsteh force-pushed the jcsteh:ia2TextMozillaEndOfNodeFix branch from dc50a70 to 8b48838 Apr 10, 2019

@jcsteh

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

@feerrenrut

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2019

Ok, thanks for the further explanation.

@feerrenrut feerrenrut merged commit f876635 into nvaccess:master Apr 12, 2019

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@nvaccessAuto nvaccessAuto added this to the 2019.2 milestone Apr 12, 2019

feerrenrut added a commit that referenced this pull request Apr 12, 2019
Update changes file for PR #9477
In Google Docs (and other web-based editors), braille no longer sometimes incorrectly shows "lst end" before the cursor in the middle of a list item. (#9477)

@jcsteh jcsteh deleted the jcsteh:ia2TextMozillaEndOfNodeFix branch Sep 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.