Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Compound TextInfos: Don't set the name attribute on control fields fo…
…r links, as this cause double speaking for quick nav. Use the content attribute on control fields for graphics, as this works well for both speech and braille.
  • Loading branch information
jcsteh committed Dec 13, 2016
commit a3a0f4ea4ef3dbd6bca51e43a6d5c7cbe19e0444
7 changes: 5 additions & 2 deletions source/NVDAObjects/IAccessible/ia2TextMozilla.py
Original file line number Diff line number Diff line change
Expand Up @@ -201,14 +201,17 @@ def _iterRecursiveText(self, ti, controlStack, formatConfig):
yield item
elif isinstance(item, int): # Embedded object.
embedded = _getEmbedded(ti.obj, item)
notText = _getRawTextInfo(embedded) is NVDAObjectTextInfo
if controlStack is not None:
controlField = self._getControlFieldForObject(embedded)
controlStack.append(controlField)
if controlField:
if notText:
controlField["content"] = embedded.name
controlField["_startOfNode"] = True
yield textInfos.FieldCommand("controlStart", controlField)
if _getRawTextInfo(embedded) is NVDAObjectTextInfo: # No text
yield embedded.basicText
if notText:
yield u" "

@feerrenrut feerrenrut May 7, 2020 •

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.

@jcsteh I'm trying to debug an issue where "space" in focus mode after an image in a content editable. The speech ends up as "graphic blah space" in both firefox and chrome. A space character in the textWithFields while in getTextInfoSpeech is converted to the word "space" which originates here.

I don't understand why the space is included, is there anything you can remember about this? Even test cases or something this supports that might help me avoiding a regression while fixing this?

The word "space" comes from spelling the character called from:

spellingSequence = list(getSpellingSpeech(

I'm considering whether it makes sense in this block (in speech/__init__.py) to yield the spelling sequence after yield speechSequence. The conditions above this are really specific to moving by one character, so if there is content in the initialFields then we don't need the space?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If I recall correctly, the reason for the space is this:

For contentEditable, graphics, etc. essentially act like characters. That is, they occupy one stop with right/left arrow. However, they have no text, so we need some character as a placeholder. A space is generally what we use in this case. You can also see this in virtual buffers. For example:

data:text/html,before<hr>after

If you repeatedly move through that with the right arrow key, you'll eventually hear "separator space". That's not ideal - it should probably just say "separator" - but we need some character in the stream.

One way to fix this that I vaguely recall considering is to use some other character (e.g. U+FFFC or U+FFFD) and explicitly "silence" that in speech and braille.

I'm considering whether it makes sense in this block (in speech/init.py) to yield the spelling sequence after yield speechSequence. The conditions above this are really specific to moving by one character, so if there is content in the initialFields then we don't need the space?

If I understand correctly, the problem with this is that it would prevent reading of spaces, say, at the start of links. For example:

data:text/html,before<pre><a href="https://nvaccess.org/"> test</a>

Right arrow to the link. You should hear "link space"; the space is real and the user should know about it. If you make this change, you might only hear "link".

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Here's a more realistic example of where we don't want to lose the space:

data:text/html,<pre>This is my<ins> lovely</ins> dog.</pre>

When you right arrow to the insertion, you should hear "insertion space". It's really important in this case that the user knows the space is part of the insertion.

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.

Great! Thanks for the example!

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.

Turns out either removing this space or changing it to something else (eg to "testing") does not seem to have any impact on those examples, but does fix the bug I'm seeing with:

data:text/html,<div contenteditable="true"><span>First</span><span><img src="https://picsum.photos/200" alt="an alt"></span><span>last</span></div>

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.

This image has the alt text "an alt", apologies that this is more confusing than I intended.

Steps to repro:

  • In focus mode
  • From the start of the line use right arrow to move through the word "first" then hear "graphic an alt space"

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.

I tested removing the space, which in particular causes issues for routing in braille. I also tested using the object replacement character (0xFFFC), which then gets displayed in braille. Although we use the object replacement character in a few places in NVDA, generally I think it's a mistake when we know what it is acting as a stand in for. Instead it would be preferable to use type like TextInfos.ControlField to represent it and handle it's replacement at the presentation layer (braille or speech). I'm actually not sure why ControlFields don't already handle this. Also, object replacement characters could come from the content itself. Using them as a stand in can cause:

  • inability to differentiate internal obj replacement chars from external.
  • inability to provide different presentations for internal obj replacement chars based on what they stand in for.

I think I'm going to have to leave this behavior as it is. I'll create a new issue, and add some comments to the code.

Ideally we would replace this space with an NVDA internal object replacement representation object, or customize ControlField to handle this. everywhere that we process field commands.

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.

See issue ""space" announced after image" #11291

else:
for subItem in self._iterRecursiveText(self._makeRawTextInfo(embedded, textInfos.POSITION_ALL), controlStack, formatConfig):
yield subItem
Expand Down
3 changes: 1 addition & 2 deletions source/compoundDocuments.py
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,6 @@ def _getControlFieldForObject(self, obj, ignoreEditableText=True):
states.discard(controlTypes.STATE_MULTILINE)
states.discard(controlTypes.STATE_FOCUSED)
field["states"] = states
field["name"] = obj.name
field["_childcount"] = obj.childCount
field["level"] = obj.positionInfo.get("level")
if role == controlTypes.ROLE_TABLE:
Expand Down Expand Up @@ -259,7 +258,7 @@ def getTextWithFields(self, formatConfig=None):
embedIndex += 1
field = ti.obj.getChild(embedIndex)
controlField = self._getControlFieldForObject(field, ignoreEditableText=False)
controlField["alwaysReportName"] = True
controlField["content"] = field.name
fields.extend((textInfos.FieldCommand("controlStart", controlField),
u"\uFFFC",
textInfos.FieldCommand("controlEnd", None)))
Expand Down