Comment 1 by jteh on 2015-04-20 22:44
It's not by design, no, but I guess i can understand why it would occur. I guess we'll need to maintain some state as to whether a space was appended in order to separate fields.

Comment 2 by dkager on 2015-04-21 18:48
Wehre is this concatenation currently handled? I tend to get lost in this component of the screen reader. :)

There are some more things that would go a long way to making browse mode more efficient for braille readers. One example is optionally moving the element indicators to the end of the text ("Test h1" instead of "h1 Test"). There are already some older tickets with feature requests. It would be great if a system could be devised that makes it easy to implement such changes.

While I can't really contribute any code right now I would still like to become more familiar with how all this braille rendering works. It's small things like this ticket that make a big difference.

Comment 3 by jteh (in reply to comment 2) on 2015-04-21 23:31
Replying to dkager:

Wehre is this concatenation currently handled? I tend to get lost in this component of the screen reader. :)

Probably braille.TextInfoRegion._addFieldText or braille.TextInfoRegion._addTextWithFields.

There are some more things that would go a long way to making browse mode more efficient for braille readers. One example is optionally moving the element indicators to the end of the text ("Test h1" instead of "h1 Test").

Similar requests have been made for speech (#1772). For the most part, the same practical and technical concerns apply. I suggest we move that discussin there, since although that ticket is specific to speech, the principles/technical implementation issues are similar.

Comment 4 by dkager (in reply to comment 3) on 2015-04-22 07:04
Replying to jteh:

Replying to dkager:

There are some more things that would go a long way to making browse mode more efficient for braille readers. One example is optionally moving the element indicators to the end of the text ("Test h1" instead of "h1 Test").

Similar requests have been made for speech (#1772). For the most part, the same practical and technical concerns apply. I suggest we move that discussin there, since although that ticket is specific to speech, the principles/technical implementation issues are similar.

There is also #1956 for braille. I believe there are strong arguments to support such an option, so I'll definitely read up on the technical issues soon.

Comment 5 by dkager (in reply to comment 3) on 2015-04-29 14:59
Replying to jteh:

Replying to dkager:

Wehre is this concatenation currently handled? I tend to get lost in this component of the screen reader. :)

Probably braille.TextInfoRegion._addFieldText or braille.TextInfoRegion._addTextWithFields.

Yeah, in _addFieldText, which adds either a space after the field or a space before '''and''' after the field. The straightforward fix should be to check if the previous text (in the rawText attribute) ends in a space and to only prepend one if it doesn't. Or, seeing that a space is appended for all fields, never prepend a space. However, if the rawText is a mix of actual content and fields this might lead to problems. Only the first field after some actual content should have a space prepended.

Maybe an alternative solution would be to build a string of all HTML element names, join them with spaces, then append that entire string to the rawText? If I understand the code correctly this is how Windows GUI meta data is built up, which also explains why that information isn't double-spaced.

Finally, another short question. The _addTextWithFields function uses _addFieldText everywhere except for the case where command is "controlEnd". In that case the _addFieldText function is copied over with only one difference:
self._rawToContentPos.extend((self._currentContentPos - 1,) * textLen)
Why the minus one? Would this not overlap with existing text? Obviously the control end indicator isn't part of the actual text content, but why then does the "controlStart" case not do the same? Needless to say the double-space problem would be a little easier to tackle if the same code wasn't defined in two places. I also note that neither the "controlStart" nor the "controlEnd" case update self._currentContentPos, so am generally a bit confused. :)

Comment 6 by jteh (in reply to comment 5) on 2015-04-30 03:14
Replying to dkager:

Yeah, in _addFieldText, which adds either a space after the field or a space before '''and''' after the field. The straightforward fix should be to check if the previous text (in the rawText attribute) ends in a space and to only prepend one if it doesn't.

rawText is indeed a mix of content and fields, which is why we prepend a space. That's why I suggested we'd probably need some flag somewhere.

Maybe an alternative solution would be to build a string of all HTML element names, join them with spaces, then append that entire string to the rawText?

That would be tricky because fields and text are all handled in the same loop, so you'd have to buffer all your fields and then spit them out when you see text. It's certainly possible, but means you have to handle the edge case at the end of the loop, etc.

If I understand the code correctly this is how Windows GUI meta data is built up, which also explains why that information isn't double-spaced.

Not sure what you mean. That's how the information for a single object is built up, but we're dealing with multiple objects (plus text) here.

Finally, another short question. The _addTextWithFields function uses _addFieldText everywhere except for the case where command is "controlEnd". In that case the _addFieldText function is copied over with only one difference:

self._rawToContentPos.extend((self._currentContentPos - 1,) * textLen)

Why the minus one? Would this not overlap with existing text?

self._currentContentPos keeps track of the real position in the content (excluding field information). The content at that position hasn't actually been handled yet. (Perhaps a better name might have been _nextContentPos.) In this case, you want to map the meta information to the end of that field's content; i.e. _currentContentPos - 1. For example, if you press a routing key anywhere in "lst end", it should route you to the last character of content in the list, not the next character of content (which would be after the list).

Obviously the control end indicator isn't part of the actual text content, but why then does the "controlStart" case not do the same?

Because for controlStart, you want it to map to the start of the content for that field, not the end of the previous content (which is before the field).

Needless to say the double-space problem would be a little easier to tackle if the same code wasn't defined in two places.

We could probably unify that code and pass in a variable indicating which content position to map to. I didn't do this originally because controlEnd was the exceptional case.

I also note that neither the "controlStart" nor the "controlEnd" case update self._currentContentPos, so am generally a bit confused. :)

As above, _currentContentPos maps to the position in the real content. Field text is meta information, not real content. We need some way to map from text which includes field content to the original text which doesn't.

Comment 7 by jteh (in reply to comment 6) on 2015-04-30 03:22
Replying to jteh:

rawText is indeed a mix of content and fields, which is why we prepend a space. That's why I suggested we'd probably need some flag somewhere.

On further thought, maybe this doesn't matter. I was originally thinking that if there was a space at the end of some content, we still want to insert a space before the field (resulting in a double space) so the user is aware of the real space at the end of the content. However, maybe this just isn't important. If the user really cares, they can figure this out by seeing where the cursor lands as they move through the text. If it lands on the space, it's a real space. If it doesn't, it's just separating meta information. The only time this would be confusing is if the user actually inserted a space just before a field. In that case, the display wouldn't change at all, which might make it seem as if the space hadn't been inserted.

Comment 8 by dkager (in reply to comment 6) on 2015-04-30 18:39
Replying to jteh:

Replying to dkager:

If I understand the code correctly this is how Windows GUI meta data is built up, which also explains why that information isn't double-spaced.

Not sure what you mean. That's how the information for a single object is built up, but we're dealing with multiple objects (plus text) here.

What I meant is that if some control has a role and a state (such as checked or selected), then these two are joined with precisely one space. Of course that's for a single object, whereas nested HTML elements lead to multiple objects.

Needless to say the double-space problem would be a little easier to tackle if the same code wasn't defined in two places.

We could probably unify that code and pass in a variable indicating which content position to map to. I didn't do this originally because controlEnd was the exceptional case.

Ah, something to try. Even if it doesn't simplify the problem it would slim down the code a bit.

As for comment:7, the conclusion sounds serious enough to me to avoid that fix if at all possible:
"In that case, the display wouldn't change at all, which might make it seem as if the space hadn't been inserted."

Attachment braille_double_spaces.html added by dkager on 2015-05-02 11:41
Description:

Comment 9 by dkager on 2015-05-03 15:06
The double spaces should be fixed, though I need to test this some more.
While experimenting I ran into another space-related issue. If the text to be displayed fills all braille cells, it is possible to pan to the right. For text that is exactly as long as the display width, this results in only empty cells. The relevant code is in TextInfoRegion.update:

        # Strip line ending characters, but add a space in case the cursor is at the end of the reading unit.
        self.rawText = self.rawText.rstrip("\r\n\0\v\f") + " "
        self._rawToContentPos.append(self._currentContentPos)
        rawTextLen = len(self.rawText)
        del self.rawTextTypeforms[- 1:](rawTextLen)
        self.rawTextTypeforms.append(louis.plain_text)

This is of course a corner case. I do have a question, though. Why is self._rawToContentPos extended (to accoount for that extra space) whereas self.rawTextTypeforms is kept to its original length? If I'm reading this correctly the last element of self.rawTextTypeforms is replaced by louis.plain_text. Doesn't this mean the extra space has no typeform assigned to it?
EDIT: On further studying this kind of deletion it looks like the code is trying to account for line ending characters that were stripped. But in that case the question arises why the same isn't done for self._rawToContentPos.

Comment 10 by dkager on 2015-05-09 13:56
Fix is in this branch. I first tried to determine if a space had to be appended by looking at TextInfoRegion._rawToContentPos, but that didn't work well. So I added an attribute to TextInfoRegion to track if the last item processed was a field. The code now adds spaces based on that and on whether TextInfoRegion.rawText is empty. This also fixes the "space at end of line" problem described in comment:9. I tested for regressions in Internet Explorer (browse mode), Notepad, Excel and Word, and couldn't find any issues.

The only thing I'm not entirely sure about is the handling of fields of type ControlField.PRESCAT_MARKER. I don't see how this fix could cause problems, but I don't know where to find a real-world example of such a field.

Comment 11 by jteh (in reply to comment 9) on 2015-05-10 02:56
Replying to dkager:

If the text to be displayed fills all braille cells, it is possible to pan to the right. For text that is exactly as long as the display width, this results in only empty cells.

As noted in the comment, we "add a space in case the cursor is at the end of the reading unit". Without this, the user wouldn't be able to get to the end of the line to insert a character after the last character on the line. Thus, the fact that your fix in comment:10 removes this very much concerns me. :)

EDIT: On further studying this kind of deletion it looks like the code is trying to account for line ending characters that were stripped. But in that case the question arises why the same isn't done for self._rawToContentPos.

Yeah, it probably should be. I'd say that was an oversight on my part. The deletion is definitely correct.

Comment 12 by dkager (in reply to comment 11) on 2015-05-10 09:23
Replying to jteh:

Replying to dkager:

If the text to be displayed fills all braille cells, it is possible to pan to the right. For text that is exactly as long as the display width, this results in only empty cells.

As noted in the comment, we "add a space in case the cursor is at the end of the reading unit". Without this, the user wouldn't be able to get to the end of the line to insert a character after the last character on the line. Thus, the fact that your fix in comment:10 removes this very much concerns me. :)

As well it should, haha. Some clarification. There should be a space at the end of editable content. This is how other screen readers work, too. However, there shouldn't be a space at the end of immutable text (i.e. controlEnd fields such as "lst end"). My fix checks for this and only adds a space to editable content. This avoids the rare case of text in browse mode exactly filling the display width and the user still being able to pan to the right. For example, this would happen on a 20-cell display with the following text:

10. Testing. lst end

EDIT: On further studying this kind of deletion it looks like the code is trying to account for line ending characters that were stripped. But in that case the question arises why the same isn't done for self._rawToContentPos.

Yeah, it probably should be. I'd say that was an oversight on my part. The deletion is definitely correct.

This is tricky because if you trim off stuff then TextInfoRegion._currentContentPos will also end up being inaccurate. Should I add a patch to resolve this? Curiously, the current code isn't causing any issues, possibly because of error handling being done elsewhere.

Comment 13 by dkager on 2015-05-10 12:31
Just pushed another commit to the topic branch that should resolve the problem of TextInfoRegion._currentContentPos becoming out-of-sync. Also fixes an oversight of my own. This probably needs some scrutinizing since it's very easy to make an off-by-one-mistake here.

Comment 15 by dkager on 2015-06-10 11:48
Two more small commits, one refactor (yeah I'm fussing) and one fix for something that might not actually be a problem (class versus instance attribute). Apart from that this branch seems to work for me.

Comment 16 by jteh on 2015-06-29 07:35
Okay. Review time. :) In general, this looks very nice; thanks!

Regarding your last two tweak commits:

1. cursor/selection shape: While I'm all for readability fixes (though I'm not really convinced this improves readability enough to be worthwhile), this isn't related to this ticket at all and the diff for this ticket is tricky enough as it is. If you really think this is important, please file a separate ticket. :)
2. It's true that pendingCaretUpdate is manipulated as an instance attribute. However, because it's immutable, there's no reason it can't be initialised as a class attribute and doing so technically saves a few calls and some memory. That said, if you found this a bit confusing, I'd accept this as a good enough reason to change it. Again, let's address it elsewhere if you want to. I'd even accept both of these refactors in one ticket, just not in this one. :)

I think endsWithField should be marked private (_endsWithField), since it's only useful within the update method. That also means you can just initialise it in update as we do with other such variables, rather than in the constructor.

When _addFieldText is called with self._currentContentPos, please add this comment above the call:

# Map this field text to the start of the field's content.

and similarly, when it's used for self._currentContentPos - 1:

# Map this field text to the end of the field's content.

I want to make sure we don't forget the reason for the difference. :)

At around line 816, is there any reason you reversed the shouldMoveCursorToFirstContent and isSelection and self._selectionStart is None checks? I don't think it'll make any difference, but changes make me nervous when I don't understand their purpose. :)

The value of _currentContentPos isn't used outside of the update method, which is why it never caused any problems. Despite comment:13, I've realised that the reason I didn't bother trimming _rawToContentPos is that it doesn't matter; we'll only ever ask for indexes valid in rawText. This means you can happily get away without updating _rawToContentPos and _currentContentPos at this point. Updating them is more elegant, but essentially amounts to wasted calls. Still, if we're going to rely on this, there should be a comment to that effect.

Comment 17 by dkager on 2015-06-29 15:26

cursor/selection shape

I'll put this in a cursor-specific ticket if that proves useful, e.g. the idea of a hotkey to turn the cursor/highlighting off interests me quite a bit.

It's true that pendingCaretUpdate is manipulated as an instance attribute. However, because it's immutable

Aren't "manipulated" and "immutable" somewhat mutually exclusive? E.g. on line 1537:

region.pendingCaretUpdate=True
Does this not change the attribute for all regions, assuming there can even be more than one at any given time?

At around line 816, is there any reason you reversed the ... checks?

No, other than that it had me worried that the order held some subtle difference, so I reversed them to match code elsewhere in that function. This way it's clear (at least it was to me) that it is repeated code that checks the same as the code around line 772.

I pushed some rebased history to the branch which fixes the other open issues. _rawToContentPos is no longer trimmed but _currentContentPos is still used to make routing to the cell after the last rawText character work as expected.

I wonder if we shouldn't take the performance penalty as it is and keep _rawToContentPos accurate. It's unlikely that more than 2 characters will be trimmed, and truncating a list by 2 items seems light enough. On the other hand I guess this happens for pretty much all lines of text in a multi-line field, so I agree that would be a waste. Maybe another solution is to compile some liblouis table entries on-the-fly that specify that \r, \n, etc should simply be discarded. This removes the need for trimming altogether, but I don't know how solid that solution is.

Comment 18 by jteh (in reply to comment 17) on 2015-06-30 03:27
Thanks. Haven't looked at the updated code yet, but:

Replying to dkager:

Aren't "manipulated" and "immutable" somewhat mutually exclusive?

Of course you can change the value of the variable, but a bool object itself is immutable. So, if this were a list (which is is a mutable) object, we couldn't use this class attribute default trick, as modifying it would then modify it for all instances.

E.g. on line 1537:

region.pendingCaretUpdate=True

Does this not change the attribute for all regions

No. Doing this assigns a value to a new instance attribute. When you read from an instance, Python checks for an instance attribute first and then falls back to class attributes. When you assign a value to an instance attribute, you always assign to an instance attribute. If you wanted to change the value for all instances, you'd have to do:
TextInfoRegion.pendingCaretUpdate = True

assuming there can even be more than one at any given time?

There's usually only one, but this shouldn't be relied upon.

At around line 816, is there any reason you reversed the ... checks?

No, other than that it had me worried that the order held some subtle difference, so I reversed them to match code elsewhere in that function. This way it's clear (at least it was to me) that it is repeated code that checks the same as the code around line 772.

Fair enough. I'm worried about the change for the same reason, but if you've tested it and it works, that's good enough.

I wonder if we shouldn't take the performance penalty as it is and keep _rawToContentPos accurate. It's unlikely that more than 2 characters will be trimmed, and truncating a list by 2 items seems light enough. On the other hand I guess this happens for pretty much all lines of text in a multi-line field, so I agree that would be a waste.

My feeling is to leave it untrimmed, but comment to the effect that this is an optimisation so we know why it's untrimmed when the other lists aren't.

Maybe another solution is to compile some liblouis table entries on-the-fly that specify that \r, \n, etc should simply be discarded. This removes the need for trimming altogether, but I don't know how solid that solution is.

I'd rather not. We have enough trouble with liblouis input/output position mapping as it is. :(

Btw, you asked about PRESCAT_MARKER. The important use case for this part of the braille code is math. That will require you to have MathPlayer installed. You'll want ot test both block and inline MathML, ensuring that the cursor lands on the start of the math content instead of after or before it. I can test this if this is going to be too tedious for you.

Comment 19 by James Teh <jamie@... on 2015-06-30 06:02
In [da798c4]:

Merge branch 't5043' into next

Incubates #5043.

Changes:
Added labels: incubating

Comment 20 by jteh on 2015-06-30 06:06
All looks good. Thanks! I rebased the branch significantly, as some of the history isn't useful, but it's still good to see the difference between refactors and actual implementation. If you do need to make further changes, please do them based on the NV Access t5043 branch.

Btw, well done for finding your way around that code. I did my best not to make it too ugly, but it's fairly complex and you can only do so much to make complex code readable. :)
Changes:
Milestone changed from None to 2015.3

Comment 21 by dkager (in reply to comment 18) on 2015-06-30 13:54
Replying to jteh:

Replying to dkager:

E.g. on line 1537:

`````` region.pendingCaretUpdate=TrueDoes this not change the attribute for all regions No. Doing this assigns a value to a new instance attribute. When you read from an instance, Python checks for an instance attribute first and then falls back to class attributes. When you assign a value to an instance attribute, you always assign to an instance attribute. If you wanted to change the value for all instances, you'd have to do:TextInfoRegion.pendingCaretUpdate = True```

Aha. This is one bit of Python that I (and many with me) have never fully understood. I did know to watch out for these kinds of constructs, though. :)

Btw, you asked about PRESCAT_MARKER. The important use case for this part of the braille code is math. That will require you to have MathPlayer installed. You'll want ot test both block and inline MathML, ensuring that the cursor lands on the start of the math content instead of after or before it. I can test this if this is going to be too tedious for you.

I remember looking up PRESCAT_MARKER and I believe I found that it is also used for footnotes in Word. I tested that and saw no regressions. The cursor moves into the field and always stays on the first character, but you can route to the field from any of its characters using cursor routing. If this is the intended behavior, nothing broke. I'll test the math later this week, so let's say no news is good news. :)

Comment 22 by jteh on 2015-06-30 23:12
I neglected to mention that I tested math before I merged to next. All is fine.

Comment 23 by James Teh <jamie@... on 2015-07-14 06:01
In [4a7a8bf]:

In braille, extraneous spaces are no longer displayed between or after indicators for controls and formatting.

Fixes #5043.

Changes:
Removed labels: incubating
State: closed