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

feat(get-element-stack): performant api to replace document.elementsFromPoint #1842

Merged
merged 15 commits into from Nov 18, 2019

Conversation

straker
Copy link
Contributor

@straker straker commented Oct 9, 2019

Just making a pr with the new api for now since it's quite big in terms of code size. Once this is merged I'll make another pr to use it.

The code essentially does the following:

  1. It creates a 2d grid of the page and adds every element to every cell of the grid it belongs to. If the element creates a scroll region, then a new grid is created for that element and all its children
  2. As it adds elements to the grid, it calculates the stack order of the node using an array of numbers. Each entry in the array is a new stacking context and the number represents the z-index of the element at that level.
  3. When the stack is requested for a node, it starts at the grid the node belongs to and calculates the row/col of the center point of the element. It then gets all nodes that are part of that row/col and performs a collision detection against the element and the point. If the grid is a sub-grid, it then goes to the parent grid and gets the stack at that level
  4. Finally it sorts all the returned nodes based on their visual order

I also added cache functions to virtual node as we do these lookups a ton throughout the codebase.

In terms of performance. Running this new function on every node on https://developer.mozilla.org/en-US/docs/Web/HTML/Element/template compared to document.elementsFromPoint and our color.getRectStack function (which is what uses document.elmentsFromPoint under the hood for color-contrast checks), the average of 5 runs are:

  • color.getRectStack: 143.47900002030656ms
  • dom.getElementStack: 53.457000001799314ms

Running it on https://web.archive.org/web/20190613132353/https://giveawaylisting.com/ (have to use web archive result now that the site is going down) results in:

  • color.getRectStack: 143147.4000000162ms
  • dom.getElementStack: 2190.5150000122376ms

NOTE: Jason's suggestion saved ~20% time (500ms on giveawaylistings.com)

Reviewer checks

Required fields, to be filled out by PR reviewer(s)

  • Follows the commit message policy, appropriate for next version
  • Code is reviewed for security

@straker straker requested a review from a team as a code owner October 9, 2019 23:11
Copy link
Member

@stephenmathieson stephenmathieson left a comment

Choose a reason for hiding this comment

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

Nitpicky stuff. Will review in depth at a later date.

lib/commons/dom/get-element-stack.js Outdated Show resolved Hide resolved
lib/commons/dom/get-element-stack.js Outdated Show resolved Hide resolved
lib/commons/dom/get-element-stack.js Outdated Show resolved Hide resolved
Copy link
Member

@scurker scurker left a comment

Choose a reason for hiding this comment

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

Some minor nitpicks and micro performance suggestions. Overall, good work here!

lib/commons/dom/get-element-stack.js Outdated Show resolved Hide resolved
lib/commons/dom/get-element-stack.js Outdated Show resolved Hide resolved
lib/commons/dom/get-element-stack.js Outdated Show resolved Hide resolved
lib/commons/dom/get-element-stack.js Outdated Show resolved Hide resolved
lib/commons/dom/get-element-stack.js Outdated Show resolved Hide resolved
lib/commons/dom/get-element-stack.js Outdated Show resolved Hide resolved
@WilcoFiers WilcoFiers added this to the Axe-core 3.5 milestone Oct 17, 2019
Copy link
Contributor

@WilcoFiers WilcoFiers left a comment

Choose a reason for hiding this comment

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

Half a review

lib/commons/dom/get-element-stack.js Outdated Show resolved Hide resolved
lib/core/base/virtual-node/virtual-node.js Outdated Show resolved Hide resolved
lib/commons/dom/get-element-stack.js Outdated Show resolved Hide resolved
lib/commons/dom/get-element-stack.js Outdated Show resolved Hide resolved
lib/commons/dom/get-element-stack.js Outdated Show resolved Hide resolved
lib/commons/dom/get-element-stack.js Outdated Show resolved Hide resolved
lib/commons/dom/get-element-stack.js Outdated Show resolved Hide resolved
dylanb
dylanb previously requested changes Oct 22, 2019
Copy link
Contributor

@dylanb dylanb left a comment

Choose a reason for hiding this comment

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

Need to add support for the other type of replaced objects - see comment

@straker
Copy link
Contributor Author

straker commented Oct 29, 2019

We decided to hold off on supporting CSS rotation for now and add it as a separate PR / function.

lib/commons/dom/get-element-stack.js Outdated Show resolved Hide resolved
lib/commons/dom/get-element-stack.js Outdated Show resolved Hide resolved
lib/commons/dom/get-element-stack.js Outdated Show resolved Hide resolved
lib/commons/dom/get-element-stack.js Outdated Show resolved Hide resolved
lib/commons/dom/get-element-stack.js Show resolved Hide resolved
lib/commons/dom/get-element-stack.js Outdated Show resolved Hide resolved
test/commons/dom/get-element-stack.js Outdated Show resolved Hide resolved
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

7 participants