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

Kindle 1.19 support #6638

Merged
merged 14 commits into from Jan 23, 2017
Merged

Kindle 1.19 support #6638

merged 14 commits into from Jan 23, 2017

Conversation

@jcsteh
Copy link
Contributor

@jcsteh jcsteh commented Dec 13, 2016

This adds support for new Kindle functionality introduced in Kindle 1.19, as well as other improvements to Kindle support. It also adds documentation for Kindle support to the User Guide.

Features:

  • Reporting of links and graphics and activation of links. The IA2 embedded object model is used for this, so we use MozillaCompoundTextInfo.
  • Reporting of notes and highlights.
  • Selection of text and access to selection options (including creating highlights and notes).
  • Single letter navigation to links (including footnotes) and graphics.

Fixes/improvements:

  • Fix failure to detect focus change when opening a book from the Kindle library.
  • Handle page changes not triggered by NVDA (e.g. using the mouse, using a keyboard command not intercepted by NVDA, via the Table of Contents, resizing the window, etc.).
  • Better workaround for focus being inaccurately thrown into the Table of Contents when turning pages. Kindle still fires incorrect events, but we can now use the focused state on the book area to accurately determine whether these events are incorrect.
  • The find commands aren't currently supported, so report this fact rather than just throwing an exception.
  • Remove various hacks that are no longer needed: waiting in a loop for a pageChange event after turnPage (since the action is now synchronous), forcing shouldAllowIAccessibleFocusEvent for BookPageView (since Kindle now sets the focused state correctly).

This work required quite a few fixes and changes scattered throughout the code base. Because some of these changes will be hard to understand without context, I've split these into smaller commits. When it comes time to merge to master, this should be done as a standard merge, rather than our usual squash.

What's New entry: in New Features:

- Support for reading and navigating books in Kindle for PC version 1.19, including access to links, footnotes, graphics, highlighted text and user notes. Please see the Kindle for PC section of the NVDA User Guide for further information. (#6247, #6638)
jcsteh added 10 commits Nov 3, 2016
…e library.

IAccessibleHandler.OrderedWinEventLimiter: Don't drop focus events if we have a foreground event with the same parameters. Otherwise, we might drop a valid focus event even though the foreground event is invalid. With this change, pumpAll rejects the invalid foreground event and falls back to the valid focus event.
This will cause us to end up with more focus events in the limiter, so bump maxFocusItems from 3 to 4 to compensate.
…n't offset based.

The previous code assumed that the TextInfo implementation allowed moving the end before the start. OffsetsTextInfo handles this and normalises the result so that end > start. However, TextInfos such as MozillaCompoundTextInfo and Microsoft Word don't handle this.
This fixes backwards selection in Kindle and Microsoft Word browse mode.
… offset if the attributes string is empty, as an empty attributes string can mean no attributes which is a valid case such as in Kindle.
…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.
The previous hack of using caret doesn't work for Kindle, which doesn't have a real caret.
…mplementation doesn't have a caret (such as with Kindle).
* Fix CompoundTextInfo.__eq__ when comparing against TextInfos of other types.
* Fix MozillaCompoundTextInfo.move for backwards movement.
* Fix off-by-one error in CompoundTextInfo which broke CursorManager selection across embedded objects.
…ll as other improvements to Kindle support.

Features:
* Reporting of links and graphics and activation of links. The IA2 embedded object model is used for this, so we use MozillaCompoundTextInfo.
* Reporting of notes and highlights.
* Selection of text and access to selection options (including creating highlights and notes).
* Single letter navigation to links (including footnotes) and graphics.

Fixes/improvements:
* Handle page changes not triggered by NVDA (e.g. using the mouse, using a keyboard command not intercepted by NVDA, via the Table of Contents, resizing the window, etc.).
* Better workaround for focus being inaccurately thrown into the Table of Contents when turning pages. Kindle still fires incorrect events, but we can now use the focused state on the book area to accurately determine whether these events are incorrect.
* The find commands aren't currently supported, so report this fact rather than just throwing an exception.
* Remove various hacks that are no longer needed: waiting in a loop for a pageChange event after turnPage (since the action is now synchronous), forcing shouldAllowIAccessibleFocusEvent for BookPageView (since Kindle now sets the focused state correctly).
@jcsteh jcsteh requested a review from michaelDCurran Dec 13, 2016
@jcsteh jcsteh added this to the 2017.1 milestone Dec 13, 2016
@jcsteh jcsteh added the p2 label Dec 13, 2016
@jcsteh jcsteh force-pushed the kindle1.19 branch from 0cd994d to 364b566 Dec 13, 2016

+++ User Notes +++
You can add a note regarding a word or passage of text.
To do this, select the relevant text as described above and then choose Add Note.

This comment has been minimized.

@michaelDCurran

michaelDCurran Dec 14, 2016
Member

I assume you mean select the text and then press applications key or shift+f10 right? I.e. choose add note from the menu thingy. I see this as more than "selecting text" though I logically see what you're saying... some won't I feel.

jcsteh added a commit that referenced this pull request Dec 15, 2016
jcsteh added a commit that referenced this pull request Jan 11, 2017
…e which imports the gui module.

This was due to the circular import of speech from textInfos and treeInterceptorHandler. We now import late to avoid this.
Fixes #6749.
jcsteh added a commit that referenced this pull request Jan 19, 2017
@jcsteh
Copy link
Contributor Author

@jcsteh jcsteh commented Jan 19, 2017

@michaelDCurran reviewed and approved that last commit out-of-band. :)

We're going to merge to master on Monday, since we really need to get this into master for wider testing and I think this change is small enough that we don't need to reincubate for the whole time.

@jcsteh jcsteh merged commit 47d1fb0 into master Jan 23, 2017
jcsteh added a commit that referenced this pull request Jan 23, 2017
…ll as other improvements to Kindle support. (#6638)
@jcsteh
Copy link
Contributor Author

@jcsteh jcsteh commented Jan 23, 2017

I accidentally squash merged this when I meant to full merge, so I quickly force pushed a full merge. The correct merge commit is 7fefb3a.

if _getRawTextInfo(embedded) is NVDAObjectTextInfo: # No text
yield embedded.basicText
if notText:
yield u" "

This comment has been minimized.

@feerrenrut

feerrenrut May 7, 2020
Member

@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?

This comment has been minimized.

@jcsteh

jcsteh May 7, 2020
Author Contributor

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".

This comment has been minimized.

@jcsteh

jcsteh May 7, 2020
Author Contributor

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.

This comment has been minimized.

@feerrenrut

feerrenrut Jun 16, 2020
Member

Great! Thanks for the example!

This comment has been minimized.

@feerrenrut

feerrenrut Jun 16, 2020
Member

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>

This comment has been minimized.

@feerrenrut

feerrenrut Jun 17, 2020
Member

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"

This comment has been minimized.

@feerrenrut

feerrenrut Jun 22, 2020
Member

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.

This comment has been minimized.

@feerrenrut

feerrenrut Jun 22, 2020
Member

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

@jcsteh
Copy link
Contributor Author

@jcsteh jcsteh commented May 7, 2020

@jcsteh
Copy link
Contributor Author

@jcsteh jcsteh commented Jun 22, 2020

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants