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.

Sign up for GitHub

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

Jump to bottom

fix: Scroll state had top and left properties flipped #1469

Merged
merged 2 commits into from
Apr 26, 2019
Merged

fix: Scroll state had top and left properties flipped #1469

merged 2 commits into from
Apr 26, 2019

Conversation

mislav
Copy link
Contributor

@mislav mislav commented Apr 2, 2019 •
edited by WilcoFiers
Loading

The arguments for window.scroll() are x-axis, y-axis; meaning that "top" value (vertical scrolling) was supposed to be passed as 2nd argument.

Reviewer checks

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

  • Follows the commit message policy, appropriate for next version
  • Has documentation updated, a DU ticket, or requires no documentation change
  • Includes new tests, or was unnecessary
  • Code is reviewed for security by: @WilcoFiers

@mislav
Fix preserving window scroll state
e1e8a59 
The arguments for `window.scroll()` are x-axis, y-axis; meaning that "top" value
(vertical scrolling) was supposed to be passed as 2nd argument.

https://developer.mozilla.org/en-US/docs/Web/API/Window/scroll
@mislav mislav requested a review from a team as a code owner April 2, 2019 09:46
@CLAassistant
Copy link

CLAassistant commented Apr 2, 2019 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

WilcoFiers
WilcoFiers requested changes Apr 10, 2019
View reviewed changes
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.

Yikes! Good catch. Do you have a test case to go with this though?

@mislav
Copy link
Contributor Author

mislav commented Apr 11, 2019 •
edited
Loading

@WilcoFiers There was already a test case, but the test case involved a stub of window.scroll that was set up in the wrong way. This PR corrects the stub, therefore we do have a test case going forward that ensures this won't regress.

A "proper" test case would be to invoke the non-stubbed version of window.scroll and verify the scroll position changes (or, at least, stays the same as before running the checks) by inspecting window.scrollY. However, I'm not sure if it's a good idea for the test suite to change the scroll position of the document executing the test suite. I think the author of the initial test felt this as well; that's why they created a stub in the first place.

Do you have an idea for a better test?

@stephenmathieson
Copy link
Member

@WilcoFiers thoughts on the above? I think @mislav has a good point.

@WilcoFiers WilcoFiers changed the title Fix preserving window scroll state fix: Scroll state had top and left properties flipped Apr 26, 2019
@WilcoFiers WilcoFiers merged commit 2ba83d3 into dequelabs:develop Apr 26, 2019
@WilcoFiers WilcoFiers added this to the Axe-core 3.3 milestone May 1, 2019
stephenmathieson added a commit to mohanraj-r/axe-core that referenced this pull request May 10, 2019
@stephenmathieson
Merge branch 'develop' into patch-1
7ce3859 
* develop: (50 commits)
  docs: add jsdom example and tests (dequelabs#1530)
  fix(aria-valid-attr-value): allow aria-owns to pass when element is not in the DOM (dequelabs#1526)
  fix(isSkipLink): cache first page link (dequelabs#1525)
  chore: update Babel dependencies (dequelabs#1527)
  chore(package): Update karma to version 4.1.0 (dequelabs#1528)
  feat: Improve perf of axe.run [WWD-1821] (dequelabs#1503)
  fix(prettier): ignore generated api doc files (dequelabs#1522)
  fix(skip-link,region): Allow multiple skiplinks at page top (dequelabs#1496)
  fix(raw-reporter): do not output `DqElement`s (dequelabs#1513)
  fix: Scroll state had top and left properties flipped (dequelabs#1469)
  refactor: commons.color.getBackgroundColor method (dequelabs#1451)
  fix(aria-valid-attr-value): allow aria-controls to pass when element is not in the DOM
  chore: Update husky to the latest version 🚀 (dequelabs#1514)
  style: format HTML files with Prettier (dequelabs#1508)
  test: Fix invalid test html (dequelabs#1502)
  feat(rule): Inline text spacing must be adjustable with custom stylesheets (dequelabs#1446)
  chore: Remove version number from axe.d.ts (dequelabs#1499)
  chore: Update make-dir to the latest version 🚀 (dequelabs#1465)
  fix: Exclude  iframe for html-has-lang rule (Issue 1424) (dequelabs#1430)
  feat(utils): add support for complex CSS selectors (dequelabs#1494)
  ...
@WilcoFiers WilcoFiers added fix Bug fixes commons Issues in the common code (lib/commons) labels Jun 11, 2019
@mislav mislav deleted the patch-1 branch June 26, 2019 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WilcoFiers WilcoFiers WilcoFiers approved these changes

Assignees
No one assigned
Labels
commons Issues in the common code (lib/commons) fix Bug fixes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mislav @CLAassistant @stephenmathieson @WilcoFiers