MozillaCompoundTextInfo: Don't skip ignored objects when determining whether the range covers the end of any ancestors of endObj.#9477
Merged
Conversation
…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.
Collaborator
|
Does this supersede #9382? |
LeonarddeR
reviewed
Apr 10, 2019
LeonarddeR
left a comment
Collaborator
There was a problem hiding this 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.
Contributor
Author
dc50a70 to
8b48838
Compare
feerrenrut
reviewed
Apr 11, 2019
Contributor
Author
|
Sorry; I did my best to explain this in the PR description, but it seems I didn't quite manage it. :( We choose to ignore paragraphs because they aren't useful; they're essentially presentational for our purposes. So, we don't generate a control field for them. This is where field will be None. However, just because we choose to ignore it in terms of control fields doesn't change the fact that there's an object for it. So, we still need to check whether we're at the end of that object. If we are, we won't do anything here because there's no field to annotate. However, if this is *not* the end, we will still break out of the loop, since we're not at the end of any of its ancestors. This is the key change here. Previously, we wouldn't break out of the ancestor loop for a paragraph (because no field), even if we were at the end of that paragraph. It's essential that we break out of the loop here if we're at the end of any object, regardless of whether we chose to generate a field for it.
|
Contributor
|
Ok, thanks for the further explanation. |
feerrenrut
approved these changes
Apr 12, 2019
feerrenrut
added a commit
that referenced
this pull request
Apr 12, 2019
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)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
data:text/html,<div contenteditable role="textbox" aria-multiline="true"><ul><li><p>test</p></li></ul></div>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.