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

Significantly speed up heading quick navigation in Microsoft Edge by walking heading Text elements rather than looking at all paragraphs #7343

Merged
merged 2 commits into from Jul 25, 2017

Conversation

Projects
None yet
5 participants
@michaelDCurran
Contributor

michaelDCurran commented Jun 30, 2017

Summary of the issue:

Currently navigating by heading in Microsoft Edge is extremely slow on large pages.
For example, when on the World War I wikipedia article in Edge, pressing 1 (to jump to the next heading level 1) can take up to 20 seconds to report there are no more heading ones.
Similarly, jumping heading by heading through the page with h can sometimes take up to 5 seconds locating the next heading.

The current way this is implemented is that NVDA moves through the document by paragraph and checks the text attributes to see if this paragraph is a heading.

Description of how this pull request fixes the issue:

As of the Windows Anniversary update, Edge started exposing an real UIA element for each heading. It can be identified with a controlType of text, and a level between 1 and 6.
Therefore, this pull request walks the UIA tree requesting only these elements. When one is found however, it still double checks the text attributes as sometimes the level on the element can be wrong.
Even so, this significantly speeds up heading navigation. Searching for a heading 1 in the World War I article now takes about 1 to 2 seconds rather than 20.

Change log entry:

Bug fixes:

  • Performing quick navigation to headings in Microsoft Edge is now significantly faster.
Significantly speed up heading quick navigation in Microsoft Edge but…
… walking heading Text elements rather than looking at all paragraphs.

@michaelDCurran michaelDCurran requested a review from feerrenrut Jun 30, 2017

@leonardder

This comment has been minimized.

Show comment
Hide comment
@leonardder

leonardder Jun 30, 2017

Collaborator
Collaborator

leonardder commented Jun 30, 2017

@derekriemer

This comment has been minimized.

Show comment
Hide comment
@derekriemer

derekriemer Jul 1, 2017

Collaborator
Collaborator

derekriemer commented Jul 1, 2017

def isChild(self,parent):
return self.level>parent.level
def EdgeHeadingQuicknavIterator(itemType,document,position,direction="next"):

This comment has been minimized.

@feerrenrut

feerrenrut Jul 10, 2017

Contributor

Could you add a doc string here? This isn't an inherited interface is it?

@feerrenrut

feerrenrut Jul 10, 2017

Contributor

Could you add a doc string here? This isn't an inherited interface is it?

This comment has been minimized.

@michaelDCurran

michaelDCurran Jul 10, 2017

Contributor

The docstring can be found on browseMode.QuickNavItem.isChild

@michaelDCurran

michaelDCurran Jul 10, 2017

Contributor

The docstring can be found on browseMode.QuickNavItem.isChild

This comment has been minimized.

@feerrenrut

feerrenrut Jul 11, 2017

Contributor

Sorry I meant on EdgeHeadingQuicknavIterator. As an aside, how would you feel about adding doc strings on derived class functions to say something like @see parentclass.methodname. This would make it clearer that this is an override, and where to find the documentation for it.

@feerrenrut

feerrenrut Jul 11, 2017

Contributor

Sorry I meant on EdgeHeadingQuicknavIterator. As an aside, how would you feel about adding doc strings on derived class functions to say something like @see parentclass.methodname. This would make it clearer that this is an override, and where to find the documentation for it.

levelString=itemType[7:]
for item in UIAControlQuicknavIterator(itemType,document,position,condition,direction=direction,itemClass=EdgeHeadingQuickNavItem):
# Verify this is the correct heading level via text attributes
if item.level and (not levelString or levelString==str(item.level)):

This comment has been minimized.

@feerrenrut

feerrenrut Jul 10, 2017

Contributor

Is this to catch mistakes in what Edge returns? Do we have any examples of where this is required?

@feerrenrut

feerrenrut Jul 10, 2017

Contributor

Is this to catch mistakes in what Edge returns? Do we have any examples of where this is required?

This comment has been minimized.

@michaelDCurran

michaelDCurran Jul 10, 2017

Contributor

The UIA search condition searches for an element with a controlType of text, and some kind of level. Edge itself does not provide anything more specific to search for. Thus, once we do have an element and have placed it in a headingQuickNavItem we then need to verify it is actually a heading by checking its level property. HeadingQuicknavItem.level is only valid if the element is in deed a valid heading (I.e. it also checks some text attributes).

@michaelDCurran

michaelDCurran Jul 10, 2017

Contributor

The UIA search condition searches for an element with a controlType of text, and some kind of level. Edge itself does not provide anything more specific to search for. Thus, once we do have an element and have placed it in a headingQuickNavItem we then need to verify it is actually a heading by checking its level property. HeadingQuicknavItem.level is only valid if the element is in deed a valid heading (I.e. it also checks some text attributes).

@derekriemer

This comment has been minimized.

Show comment
Hide comment
@derekriemer

derekriemer Jul 11, 2017

Collaborator

what about this invalid, but still seen in the wild code? Will this PR break heading nav to these?

<div role="heading">I don't like doing things correctly because, well I don't</div>

I see this at least once a month, and it is incorrect, but it is nice to be able to move by heading with them still.

Collaborator

derekriemer commented Jul 11, 2017

what about this invalid, but still seen in the wild code? Will this PR break heading nav to these?

<div role="heading">I don't like doing things correctly because, well I don't</div>

I see this at least once a month, and it is incorrect, but it is nice to be able to move by heading with them still.

@michaelDCurran

This comment has been minimized.

Show comment
Hide comment
@michaelDCurran

michaelDCurran Jul 11, 2017

Contributor

@derekriemer: Edge currently assigns these a level of 1, so they will be picked up.

Contributor

michaelDCurran commented Jul 11, 2017

@derekriemer: Edge currently assigns these a level of 1, so they will be picked up.

michaelDCurran added a commit that referenced this pull request Jul 12, 2017

@michaelDCurran michaelDCurran merged commit c2bc803 into master Jul 25, 2017

@nvaccessAuto nvaccessAuto added this to the 2017.3 milestone Jul 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment