Skip to content

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

Merged
michaelDCurran merged 2 commits into
masterfrom
fasterEdgeHeadingQuickNav
Jul 25, 2017
Merged

Significantly speed up heading quick navigation in Microsoft Edge by walking heading Text elements rather than looking at all paragraphs#7343
michaelDCurran merged 2 commits into
masterfrom
fasterEdgeHeadingQuickNav

Conversation

@michaelDCurran

@michaelDCurran michaelDCurran commented Jun 30, 2017

Copy link
Copy Markdown
Member

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.

… walking heading Text elements rather than looking at all paragraphs.
@LeonarddeR

LeonarddeR commented Jun 30, 2017 via email

Copy link
Copy Markdown
Collaborator

@derekriemer

derekriemer commented Jul 1, 2017 via email

Copy link
Copy Markdown
Collaborator

def isChild(self,parent):
return self.level>parent.level

def EdgeHeadingQuicknavIterator(itemType,document,position,direction="next"):

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.

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The docstring can be found on browseMode.QuickNavItem.isChild

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.

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)):

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.

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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

derekriemer commented Jul 11, 2017

Copy link
Copy Markdown
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.

@michaelDCurran

Copy link
Copy Markdown
Member Author

@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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants