feat: deprecate and replace dom.isVisible, utils.isHidden, and dom.isHiddenWithCss #3351

straker · 2022-01-14T18:33:32Z

A precursor to fixing #2806 and #3532.

WilcoFiers

The direction looks fine. I left some suggestions on refactoring this to make the logic of which method does what a little more explicit. Obviously I'd like to see tests added to this.

lib/commons/dom/is-visible.js

Co-authored-by: Wilco Fiers <WilcoFiers@users.noreply.github.com>

Resolved

lib/commons/dom/is-visible.js

Co-authored-by: Wilco Fiers <WilcoFiers@users.noreply.github.com>

Resolved

lib/commons/dom/is-visible.js

Co-authored-by: Wilco Fiers <WilcoFiers@users.noreply.github.com>

straker · 2022-09-08T21:48:18Z

All comments about changing things outside of the refactor captured here #3655

Resolved

straker · 2022-09-08T23:18:54Z

So although I converted the functions to use options, I don't think memoize can memoize options. Given the code:

foo = axe.utils.memoize(function(a, { b }) {
  console.log('here', {a, b})
  return true
})

foo(1, {b: 2}) // here {a: 1, b: 2}
foo(1, {b: 2}) // here {a: 1, b: 2}

Which shows that the memoized function is not be cached as the options object is not strictly equal for the cache to trigger. We may not be able to do options after all.

lib/commons/dom/is-offscreen.js

WilcoFiers · 2022-09-13T14:06:10Z

lib/commons/dom/visibility-functions.js

@@ -0,0 +1,171 @@
+import {


Disagree. I'm uncomfortable when we do it just for pulling a method into its own file. With whole new methods I think we need to make the effort. These are easy tests to write. Let's just get them done.

WilcoFiers · 2022-09-13T14:19:49Z

Also remember to open an issue for those questionable uses of isVisible.

Resolved

.eslintrc.js

WilcoFiers

Couple minor points

lib/commons/dom/is-offscreen.js

WilcoFiers · 2022-09-20T10:23:34Z

lib/core/core.js

+axe._thisWillBeDeletedDoNotUse.commons =
+  axe._thisWillBeDeletedDoNotUse.commons || {};
+axe._thisWillBeDeletedDoNotUse.commons.dom =
+  axe._thisWillBeDeletedDoNotUse.commons.dom || {};
+axe._thisWillBeDeletedDoNotUse.commons.dom.nativelyHidden = nativelyHidden;
+axe._thisWillBeDeletedDoNotUse.commons.dom.displayHidden = displayHidden;
+axe._thisWillBeDeletedDoNotUse.commons.dom.visibilityHidden = visibilityHidden;
+axe._thisWillBeDeletedDoNotUse.commons.dom.ariaHidden = ariaHidden;
+axe._thisWillBeDeletedDoNotUse.commons.dom.opacityHidden = opacityHidden;
+axe._thisWillBeDeletedDoNotUse.commons.dom.scrollHidden = scrollHidden;
+axe._thisWillBeDeletedDoNotUse.commons.dom.overflowHidden = overflowHidden;
+axe._thisWillBeDeletedDoNotUse.commons.dom.clipHidden = clipHidden;
+axe._thisWillBeDeletedDoNotUse.commons.dom.areaHidden = areaHidden;


This is getting messy. Lets pull this into a separate file.

Added to the tech debt ticket #3655 (cleaning it up properly is out of scope for this pr as this object is in multiple places throughout the lib code base)

Your PR is increasing tech debt. That's not out of scope. But fine, under protest.

.eslintrc.js

Resolved

WilcoFiers · 2022-09-21T10:55:04Z

lib/core/core.js

+axe._thisWillBeDeletedDoNotUse.commons =
+  axe._thisWillBeDeletedDoNotUse.commons || {};
+axe._thisWillBeDeletedDoNotUse.commons.dom =
+  axe._thisWillBeDeletedDoNotUse.commons.dom || {};
+axe._thisWillBeDeletedDoNotUse.commons.dom.nativelyHidden = nativelyHidden;
+axe._thisWillBeDeletedDoNotUse.commons.dom.displayHidden = displayHidden;
+axe._thisWillBeDeletedDoNotUse.commons.dom.visibilityHidden = visibilityHidden;
+axe._thisWillBeDeletedDoNotUse.commons.dom.ariaHidden = ariaHidden;
+axe._thisWillBeDeletedDoNotUse.commons.dom.opacityHidden = opacityHidden;
+axe._thisWillBeDeletedDoNotUse.commons.dom.scrollHidden = scrollHidden;
+axe._thisWillBeDeletedDoNotUse.commons.dom.overflowHidden = overflowHidden;
+axe._thisWillBeDeletedDoNotUse.commons.dom.clipHidden = clipHidden;
+axe._thisWillBeDeletedDoNotUse.commons.dom.areaHidden = areaHidden;


Your PR is increasing tech debt. That's not out of scope. But fine, under protest.

straker added 2 commits January 14, 2022 11:30

chore: refactor isVisible

6693b99

comment

68f30a2

straker requested a review from a team as a code owner January 14, 2022 18:33

fix firefox

c3f9f39

WilcoFiers previously requested changes Jan 17, 2022

View reviewed changes

WilcoFiers reviewed Jan 17, 2022

View reviewed changes

lib/commons/dom/is-visible.js Outdated Show resolved Hide resolved

straker and others added 2 commits January 17, 2022 08:40

Update lib/commons/dom/is-visible.js

c87d1e8

Co-authored-by: Wilco Fiers <WilcoFiers@users.noreply.github.com>

update per suggestions

7a9dcef

WilcoFiers previously requested changes Jan 19, 2022

View reviewed changes

straker and others added 2 commits January 21, 2022 10:35

Update lib/commons/dom/is-visible.js

3d668f0

Co-authored-by: Wilco Fiers <WilcoFiers@users.noreply.github.com>

Update lib/commons/dom/is-visible.js

0e8892a

Co-authored-by: Wilco Fiers <WilcoFiers@users.noreply.github.com>

straker changed the title ~~chore: refactor isVisible~~ [WIP] chore: refactor isVisible Feb 11, 2022

straker added 2 commits July 25, 2022 15:24

Merge branch 'refs/heads/develop' into refactor-visible

2134df8

fixes

cd7871b

straker changed the title ~~[WIP] chore: refactor isVisible~~ chore: refactor isVisible Jul 28, 2022

straker commented Jul 28, 2022

View reviewed changes

lib/commons/dom/is-visible.js Outdated Show resolved Hide resolved

straker changed the title ~~chore: refactor isVisible~~ refactor(isVisible): clean up function and put different visible checks into their own function Jul 28, 2022

WilcoFiers previously requested changes Aug 18, 2022

View reviewed changes

lib/commons/dom/is-visible.js Outdated Show resolved Hide resolved

lib/commons/dom/is-visible.js Outdated Show resolved Hide resolved

lib/commons/dom/is-visible.js Outdated Show resolved Hide resolved

straker changed the title ~~refactor(isVisible): clean up function and put different visible checks into their own function~~ [WIP] refactor(isVisible): clean up function and put different visible checks into their own function Aug 22, 2022

This was referenced Aug 24, 2022

Refactor multiple visibility functions #3617

Closed

[WIP] fix: do not run axe against elements inside closed details #3562

Closed

revert changes

db3006d

straker changed the title ~~[WIP] refactor(isVisible): clean up function and put different visible checks into their own function~~ refactor(isVisible): clean up function and put different visible checks into their own function Aug 25, 2022

straker marked this pull request as draft August 25, 2022 22:04

straker added 2 commits August 26, 2022 11:31

is-hidden-for-everyone

d5f74c8

functions done

b9ad968

straker dismissed a stale review via b9ad968 August 31, 2022 21:52

straker changed the title ~~refactor(isVisible): clean up function and put different visible checks into their own function~~ feat: deprecate isVisible, isHidden, and isHiddenWithCss Use isVisibleOnScreen, isVisibleForScreenreader, and isHiddenForEveryone (respectively) Aug 31, 2022

straker and others added 3 commits September 8, 2022 13:59

Update lib/commons/dom/is-modal-open.js

5d4aca6

Co-authored-by: Wilco Fiers <WilcoFiers@users.noreply.github.com>

suggestions

c3371e8

Update lib/commons/dom/visibility-functions.js

04e8c2a

Co-authored-by: Wilco Fiers <WilcoFiers@users.noreply.github.com>

straker mentioned this pull request Sep 8, 2022

Code quality improvements from isVisible refactor #3655

Closed

9 tasks

straker added 4 commits September 8, 2022 15:48

suggestions

d63581f

merge develop

fe8cdff

merge develop

432cacc

suggestions

86b9051

straker added 2 commits September 9, 2022 11:22

memoize correctly

f8dadac

memoize for real this time

f514f5f

WilcoFiers previously requested changes Sep 13, 2022

View reviewed changes

straker added 2 commits September 19, 2022 08:49

uni tests

b825fbb

guard

41328e6

straker commented Sep 19, 2022

View reviewed changes

.eslintrc.js Outdated Show resolved Hide resolved

straker added 3 commits September 19, 2022 15:25

reset eslintrc

3c5899c

Merge branch 'develop' into refactor-visible

c59d8b6

merge

fdddacf

WilcoFiers previously requested changes Sep 20, 2022

View reviewed changes

changes

992613d

fix lint

fff04f1

WilcoFiers approved these changes Sep 21, 2022

View reviewed changes

straker merged commit 1ae2ac0 into develop Sep 21, 2022

straker deleted the refactor-visible branch September 21, 2022 15:09

Shubhabrata08 mentioned this pull request Jan 13, 2024

Investigate the accessibility rules configured in the jest-axe library. microsoft/fluentui-charting-contrib#28

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: deprecate and replace dom.isVisible, utils.isHidden, and dom.isHiddenWithCss #3351

feat: deprecate and replace dom.isVisible, utils.isHidden, and dom.isHiddenWithCss #3351

straker commented Jan 14, 2022 •

edited

Loading

WilcoFiers left a comment

straker commented Sep 8, 2022

straker commented Sep 8, 2022

WilcoFiers Sep 13, 2022

WilcoFiers commented Sep 13, 2022

WilcoFiers left a comment

WilcoFiers Sep 20, 2022

straker Sep 20, 2022 •

edited

Loading

WilcoFiers Sep 21, 2022

WilcoFiers Sep 21, 2022

feat: deprecate and replace dom.isVisible, utils.isHidden, and dom.isHiddenWithCss #3351

feat: deprecate and replace dom.isVisible, utils.isHidden, and dom.isHiddenWithCss #3351

Conversation

straker commented Jan 14, 2022 • edited Loading

WilcoFiers left a comment

Choose a reason for hiding this comment

straker commented Sep 8, 2022

straker commented Sep 8, 2022

WilcoFiers Sep 13, 2022

Choose a reason for hiding this comment

WilcoFiers commented Sep 13, 2022

WilcoFiers left a comment

Choose a reason for hiding this comment

WilcoFiers Sep 20, 2022

Choose a reason for hiding this comment

straker Sep 20, 2022 • edited Loading

Choose a reason for hiding this comment

WilcoFiers Sep 21, 2022

Choose a reason for hiding this comment

WilcoFiers Sep 21, 2022

Choose a reason for hiding this comment

straker commented Jan 14, 2022 •

edited

Loading

straker Sep 20, 2022 •

edited

Loading