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

Skip blank lines in browse mode, WIP fixes #4904 #6704

Closed
wants to merge 13 commits into from

Conversation

derekriemer
Copy link
Collaborator

Allow skipping blank lines in browse mode. @jcsteh @michaelDCurran @feerrenrut If possible at some point, can I chat with someone who's familiar with this code to ask how I can implement one last UX fix and see if this approach is scalable?

derekriemer added 5 commits January 6, 2017 02:05
This is implemented by a cursor manager tweek to the caret helper, and a new method on treeInterceptors/cursor managers called shouldSkipBlankLines taking an text info.
Don't skip blank lines for pre tags (Works in all browsers but edge
Don't skip blank lines in word.
If clickable and not only line, skipperuni.
 quicnav blank skipping, so that the arrows end up at the first line of text.
…econdly, if control+home or control+end, don't land on a blank at any cost
@derekriemer
Copy link
Collaborator Author

Um, the lines that disagree in config are strange. it looks very much like a white space issue.

derekriemer added 5 commits March 23, 2017 12:09
Conflicts:
	source/config/__init__.py
Do to config spec upgrade, the config addition was placed into config/configSpec
@derekriemer
Copy link
Collaborator Author

I can't help but laugh at the fact that my last two commits were about the significance of blank lines, in a PR about omitting them.

@derekriemer
Copy link
Collaborator Author

@michaelDCurran Is it possible in creators yet to get access to the field name, I.E. pre in edge?

@josephsl
Copy link
Collaborator

Hi,

Fails in Edge (build 14393). Traceback says:

ERROR - scriptHandler.executeScript (12:43:20):
error executing script: <bound method EdgeHTMLTreeInterceptor.script_moveByLine_forward of <NVDAObjects.UIA.edge.EdgeHTMLTreeInterceptor object at 0x05DAB970>> with gesture u'down arrow'
Traceback (most recent call last):
File "scriptHandler.py", line 187, in executeScript
script(gesture)
File "cursorManager.py", line 233, in script_moveByLine_forward
self._caretMovementScriptHelper(gesture,textInfos.UNIT_LINE,1)
File "cursorManager.py", line 150, in _caretMovementScriptHelper
if self._shouldSkipBlankLines(blankInfo) and self._isBlankHelper(blankInfo):
File "browseMode.py", line 1076, in _shouldSkipBlankLines
controlFields = [field for field in blankInfo.getTextWithFields() if not isinstance(field, basestring) and field.command == "controlStart" and not field.field["isHidden"]]
KeyError: 'isHidden'

This was observed when I visited google.com with Edge and tried to use down arrow to move through lines in browse mode. Thanks.

@derekriemer
Copy link
Collaborator Author

@michaelDCurran Is there a way in edge to get if something is hidden? If not, I'll just remove that check.

@derekriemer
Copy link
Collaborator Author

I could also just do and field.field.get("isHidden", False)

@derekriemer
Copy link
Collaborator Author

@josephsl Can you try again with that change?

@josephsl
Copy link
Collaborator

Hi,

Resolved in 14393. I'll get back to you once I run tests in 15063 and in 10586 (Version 1511). Thanks.

@josephsl
Copy link
Collaborator

Hi,

Works in builds 10586 and 15063. Thanks.

@michaelDCurran
Copy link
Member

michaelDCurran commented Mar 23, 2017 via email

derekriemer added 2 commits April 12, 2017 02:28
Base this for updating the cursorManager unit tests.
… and for simply using cursor managers with blank line skipping disabled.
@derekriemer
Copy link
Collaborator Author

fixes #4904

@derekriemer
Copy link
Collaborator Author

I'll fix the conflicts and then update the users gui8de (Forgot to)

@derekriemer derekriemer changed the title Skip blank lines in browse mode, WIP #4904 Skip blank lines in browse mode, WIP fixes #4904 Jul 3, 2017
@derekriemer
Copy link
Collaborator Author

refactoring to put the blank skip code in the move part of the textinfo for a cursor manager, because @jcsteh correctly pointed out that my method doesn't work for braille.

@LeonarddeR
Copy link
Collaborator

@derekriemer: Just wondering, how is the refactoring going?

@derekriemer
Copy link
Collaborator Author

haven't had time to work out a new design at all. If you have any ideas, I'm happy to discuss.

@Adriani90
Copy link
Collaborator

Hey folks,

this feature would be really really nice. Thank you for your great work.

I hope it does not cause any issues in MS Excel when navigating empty rows in browse mode.

@feerrenrut
Copy link
Contributor

Unless there are any objections, I will close this pull request and suggest that if anyone is interested that they can take over. Thanks @derekriemer for providing the initial implementation!

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

Successfully merging this pull request may close these issues.

None yet

6 participants