[Modal] Fix persisting aria-hidden #16392

eps1lon · 2019-06-27T08:48:32Z

~~Not a fix yet just a failing test case.~~ Opening this since I have a question: Why do we export ariaHidden from the ModalManager and use it in Modal? Isn't the point of ModalManager that it manages this stuff in which case it shouldn't be exported?

I suspected that the issue was with a rogue parent aria-hidden which is why I added a custom matcher (aria-hidden is inherited but not overwriteable).

Closes #16391

/cc @oliviertassinari

eps1lon · 2019-06-27T08:50:44Z

test/utils/init.js

+  chai.Assertion.addProperty('ariaHidden', function elementIsAccessible() {
+    const element = utils.flag(this, 'object');
+
+    // "An element is considered hidden if it, or any of its ancestors are not


For integration test we could implement a custom matcher for hidden content as well. It won't catch everything (since it requires layout) but a crude implementation could just join ariaHiden || !displayed || !visible.

mui-pr-bot · 2019-06-27T08:55:27Z

Details of bundle changes.

Comparing: a0f0843...0a9c506

bundle	parsed diff	gzip diff	prev parsed	current parsed	prev gzip	current gzip
@material-ui/core	0.00%	0.00%	320,047	320,047	88,362	88,363
@material-ui/core/Paper	0.00%	0.00%	68,290	68,290	20,366	20,366
@material-ui/core/Paper.esm	0.00%	0.00%	61,573	61,573	19,161	19,161
@material-ui/core/Popper	0.00%	0.00%	28,945	28,945	10,395	10,395
@material-ui/core/Textarea	0.00%	0.00%	5,524	5,524	2,378	2,378
@material-ui/core/TrapFocus	0.00%	0.00%	3,753	3,753	1,578	1,578
@material-ui/core/styles/createMuiTheme	0.00%	0.00%	16,009	16,009	5,791	5,791
@material-ui/core/useMediaQuery	0.00%	0.00%	2,597	2,597	1,102	1,102
@material-ui/lab	0.00%	0.00%	140,095	140,095	43,397	43,397
@material-ui/styles	0.00%	0.00%	51,698	51,698	15,348	15,348
@material-ui/system	0.00%	0.00%	15,420	15,420	4,391	4,391
Button	0.00%	0.00%	84,316	84,316	25,722	25,722
Modal	0.00%	0.00%	14,427	14,427	5,087	5,087
Portal	0.00%	0.00%	3,473	3,473	1,572	1,572
Slider	0.00%	0.00%	74,775	74,775	23,246	23,246
colorManipulator	0.00%	0.00%	3,904	3,904	1,544	1,544
docs.landing	0.00%	0.00%	54,843	54,843	13,901	13,901
docs.main	0.00%	-0.00%	647,121	647,121	203,996	203,994
packages/material-ui/build/umd/material-ui.production.min.js	0.00%	0.00%	293,253	293,253	84,247	84,247

Generated by 🚫 dangerJS against 0a9c506

test/utils/init.js

oliviertassinari

Why do we export ariaHidden from the ModalManager and use it in Modal?

The modal manager handles the active modals. In this case, the aria-hidden is used on a modal that is mounted in the DOM, and that will likely be active in the future. Removing the line breaks this test:

<Modal keepMounted open={false}

oliviertassinari · 2019-06-27T09:22:29Z

packages/material-ui/src/Modal/ModalManager.test.js

@@ -161,17 +161,17 @@ describe('ModalManager', () => {
      const modal2 = {};
      modalManager.add(modal1, container3);
      modalManager.mount(modal1);
-      assert.strictEqual(container3.children[0].getAttribute('aria-hidden'), 'true');
+      expect(container3.children[0]).to.be.ariaHidden;


What do you think of minimizing the API surface people have to know for contributing? Where does .ariaHidden come from? Would it be simpler with the native DOM API?

Suggested change

expect(container3.children[0]).to.be.ariaHidden;

expect(container3.children[0].getAttribute('aria-hidden')).to.equal('true');

Ok, I have read the rest of the pull request. The native DOM API is not a good idea. I was confused by the magic behind the API.

What do you think of importing the aria-hidden helper function instead?

Doesn't scale very well (need to import all the matchers) and doesn't address the concern (knowledge is required in both cases). It's common to rely on injected matcher methods. Autocomplete should work (needs other PR) which helps with discoverability and teaching.

Oh and yeah it helps debugging failing test. A simple boolean helper would not.

need to import all the matchers

I would hope a file test doesn't need to import all the matchers. For instance, will we need the aria-hidden helper outside of the modal context?

knowledge is required in both cases

It depends on the case. When writing the test case, knowledge is required in both cases. When reading the test case, the import can be followed. Considering we spend most of our time reading the test case, I find that value proposing interesting.

Oh and yeah it helps debugging failing test. A simple boolean helper would not.

Yeah, that wasn't a good idea from my part. However, does it invalidate the import helper point? Can we write the helper to run the assertions?

This is the fundamental issue. When something fails you need to understand how the code works not your test.

Yeah, I wish it was true, it's a great target!

Now, I haven't experienced that in practice in this codebase nor in any other. I have found myself needed to understand what was the initial situation, what was the actions and what was the assertions.

And tests are not always regression test.

Should they all be regression tests (i.e remove the ones that are not)?

There's a reason why we say "test" and "regression test". One being more specific and less common therefore having the longer word. Regression test make little sense for behavior that's not observable to the user (e.g. component snapshot test, asserting instance fields).

I have done a quick search on the topic. It seems that people define "regression test" as "when it fails, it tells you that the application no longer behaves the way it used to".
I was using the term in a broader context, closer to "acceptance test": "when it fails, it tells you that the application is not doing what the customer expects it to do".

exactly. And these tests next to our components are acceptance test that describe what's expected. The more declarative the better.

The only regression test I'm aware of that is valuable is the screenshot tests. I would be cautious with snapshot tests, it can quickly end-up like this: they take too much time to look, we accept the changes blindly.

accessibility check would include perceivability which requires layout

eps1lon added bug 🐛 Something doesn't work component: modal This is the name of the generic UI component, not the React module! labels Jun 27, 2019

eps1lon commented Jun 27, 2019

View reviewed changes

test/utils/init.js Show resolved Hide resolved

eps1lon commented Jun 27, 2019

View reviewed changes

test/utils/init.js Show resolved Hide resolved

oliviertassinari reviewed Jun 27, 2019

View reviewed changes

eps1lon marked this pull request as ready for review June 27, 2019 12:03

eps1lon added 5 commits June 27, 2019 14:05

[test] Use a11y matcher in ModalManger test

88ac00a

accessible -> ariaHidden

165ed7d

accessibility check would include perceivability which requires layout

Add failing test for persisting aria-hidden

1a025fa

Fix aria-hidden not being removed

5a0f9e8

Document custom matchers

4f605a1

eps1lon force-pushed the fix/modal-aria-hidden branch from 6c5660e to 4f605a1 Compare June 27, 2019 12:15

oliviertassinari approved these changes Jun 27, 2019

View reviewed changes

oliviertassinari added the PR: good for merge label Jun 27, 2019

Improve code docs

0a9c506

eps1lon merged commit e36f5d0 into mui:master Jun 28, 2019

eps1lon deleted the fix/modal-aria-hidden branch June 28, 2019 06:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Modal] Fix persisting aria-hidden #16392

[Modal] Fix persisting aria-hidden #16392

eps1lon commented Jun 27, 2019 •

edited

Loading

eps1lon Jun 27, 2019

mui-pr-bot commented Jun 27, 2019 •

edited

Loading

oliviertassinari left a comment

oliviertassinari Jun 27, 2019

oliviertassinari Jun 27, 2019 •

edited

Loading

eps1lon Jun 27, 2019

eps1lon Jun 27, 2019

oliviertassinari Jun 27, 2019 •

edited

Loading

oliviertassinari Jun 27, 2019 •

edited

Loading

eps1lon Jun 27, 2019

oliviertassinari Jun 27, 2019

eps1lon Jun 27, 2019

oliviertassinari Jun 27, 2019

	expect(container3.children[0]).to.be.ariaHidden;
	expect(container3.children[0].getAttribute('aria-hidden')).to.equal('true');

[Modal] Fix persisting aria-hidden #16392

[Modal] Fix persisting aria-hidden #16392

Conversation

eps1lon commented Jun 27, 2019 • edited Loading

Choose a reason for hiding this comment

mui-pr-bot commented Jun 27, 2019 • edited Loading

oliviertassinari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Jun 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Jun 27, 2019 • edited Loading

Choose a reason for hiding this comment

oliviertassinari Jun 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eps1lon commented Jun 27, 2019 •

edited

Loading

mui-pr-bot commented Jun 27, 2019 •

edited

Loading

oliviertassinari Jun 27, 2019 •

edited

Loading

oliviertassinari Jun 27, 2019 •

edited

Loading

oliviertassinari Jun 27, 2019 •

edited

Loading