fix(dialogBehavior): move id generation to component #1449

layershifter · 2019-06-04T12:19:30Z

Problem 1

IDs in dialogBehavior are generated on each render. If we will try to have some state there to persist there IDs it will break the idea that all accessibility behaviors are pure functions.

Problem 2

Abstraction with slots leaked to components because we accessed there header['id'] and content['id'].

Solution

Move IDs generation to component and keep them in state, IDs will be stored for each slot separately (headerId, contentId) so Stardust slots will not leak.

codecov · 2019-06-04T13:09:52Z

Codecov Report

Merging #1449 into master will decrease coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1449      +/-   ##
==========================================
- Coverage   73.67%   73.63%   -0.04%     
==========================================
  Files         807      808       +1     
  Lines        6089     6092       +3     
  Branches     1772     1774       +2     
==========================================
  Hits         4486     4486              
- Misses       1598     1601       +3     
  Partials        5        5

Impacted Files	Coverage Δ
...ages/react/test/specs/behaviors/testDefinitions.ts	`98.24% <ø> (-0.06%)`	⬇️
packages/react/src/components/Dialog/Dialog.tsx	`44.68% <100%> (+8.56%)`	⬆️
...b/accessibility/Behaviors/Dialog/dialogBehavior.ts	`100% <100%> (ø)`	⬆️
packages/react/test/utils/findIntrinsicElement.ts	`100% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 92f09e0...b328e5f. Read the comment docs.

…b.com/stardust-ui/react into fix/omit-id-generation

layershifter · 2019-06-04T13:12:01Z

packages/react/src/lib/AutoControlledComponent.tsx

  }

  static getDerivedStateFromProps(props, state) {
-    const { autoControlledProps } = state
+    const { autoControlledProps, getAutoControlledStateFromProps } = state


The single downside that I can't access actual getAutoControlledStateFromProps() by referencing this because it's static. Has anyone better idea than keep it in the state?

I got confused, where this method comes from? Why are we extracting it form the state?

Let me check this part again: https://codesandbox.io/s/elastic-framework-w3kfk

I expect that this should work properly...

React calls it without this, that's why it's not working:
https://github.com/facebook/react/blob/2b93d686e359c7afa299e2ec5cf63160a32a1155/packages/react-test-renderer/src/ReactShallowRenderer.js#L560

layershifter · 2019-06-04T13:12:59Z

packages/react/src/lib/accessibility/Behaviors/Dialog/dialogBehavior.ts

@@ -15,8 +14,6 @@ import * as _ from 'lodash'
 * Adds attribute 'aria-labelledby' based on the property 'aria-labelledby' to 'popup' component's part.
 * Adds attribute 'aria-describedby' based on the property 'aria-describedby' to 'popup' component's part.
 * Adds attribute 'role=dialog' to 'popup' component's part.
- * Generates unique ID and adds it as attribute 'id' to the 'header' component's part if it has not been provided by the user.
- * Generates unique ID and adds it as attribute 'id' to the 'content' component's part if it has not been provided by the user.


This definitely should be replaced. @silviuavram @sophieH29 can you help me with rules?

Maybe you can refactor the Dialog-test.tsx to check that IDs are generated there if they are not provided. I think you can remove the other tests (label, labelledby etc) since they should be checked in the behavior.

In dialogBehavior-test.tsx you can add individual tests for the generation. Pass header as a shorthand with id and check it's added as aria-labelledby. Similar to content. The other case when they are applied directly from the aria-labelledby / aria-describedby should already be tested by a rule.

After looking at this file at 'aria-labelledby': defaultAriaLabelledBy || props['aria-labelledby'], probably you can remove the || props['aria-labelledby'] and do the whole logic in the method, which you can rename without Default: getAriaDescribedBy or something. Where you return it as the prop value, if passed, as header['id] if header exists, or nothing.

what do you think @sophieH29 ?

Updated UTs 🔍

CHANGELOG.md

mnajdova · 2019-06-10T12:14:45Z

packages/react/src/lib/AutoControlledComponent.tsx

+      ...state,
+      ...initialAutoControlledState,
+      autoControlledProps,
+      getAutoControlledStateFromProps,


Isn't this a method? Should we invoke it to get the object containing props?

#1449 (comment)

It's an initial state 🤔 We store it in state and will be able to access it inside of static.

Let's describe from user perspective how these methods should be defined and used in the PR description. I am a bit confused as why we are introducing this method, and how it should be used.

Update PR description

…b.com/stardust-ui/react into fix/omit-id-generation # Conflicts: # packages/react/src/lib/accessibility/Behaviors/Dialog/dialogBehavior.ts

…b.com/stardust-ui/react into fix/omit-id-generation # Conflicts: # packages/react/src/lib/AutoControlledComponent.tsx # packages/react/src/lib/accessibility/Behaviors/Dialog/dialogBehavior.ts

mnajdova · 2019-06-11T13:43:01Z

packages/react/src/components/Dialog/Dialog.tsx

@@ -142,7 +144,21 @@ class Dialog extends AutoControlledComponent<WithAsProp<DialogProps>, DialogStat
  triggerRef = React.createRef<HTMLElement>()

  getInitialAutoControlledState(): DialogState {
-    return { open: false }
+    return {
+      contentId: _.uniqueId('dialog-content-'),


Can we store this strings to some statics, or reuse the classnames if they exists?

I don't there a big benefit because we don't want to expose them to clients

mnajdova · 2019-06-11T13:45:07Z

packages/react/src/components/Dialog/Dialog.tsx

@@ -88,6 +88,8 @@ export interface DialogProps
 }

 export interface DialogState {
+  contentId?: string


Shouldn't we use these as an id of the elements, or they are used only for the aria-* attributes?

I decided to keep responsibility for this on accessibility behaviors: https://github.com/stardust-ui/react/pull/1449/files#diff-7d8c4d117a2402a4e3fbb997e3c6693fL32

mnajdova · 2019-06-11T13:46:53Z

packages/react/src/components/Dialog/Dialog.tsx

+    return {
+      contentId: (props.content && props.content['id']) || state.contentId,
+      headerId: (props.header && props.header['id']) || state.headerId,
+    }


I really like the changes. It would be great if we can extract this logic, as I think it may be reused in other components as well, of course not as part of this PR.

Yep, we should inspect other cases with leaked React abstractions

kuzhelov · 2019-06-12T09:12:18Z

packages/react/src/lib/accessibility/Behaviors/Dialog/dialogBehavior.ts

-  content?: {
-    id?: string
-  }
+  header?: string | object


are header and content props used anywhere in the behavior's logic? I mean, even from semantical point of view as a client I wouldn't expect them to be necessary to calculate accessibility attributes - am I missing something?

kuzhelov · 2019-06-12T09:27:04Z

packages/react/src/components/Dialog/Dialog.tsx

+    state: DialogState,
+  ): Partial<DialogState> {
+    return {
+      contentId: (props.content && props.content['id']) || state.contentId,


props.content && props.content['id']

what if React element is passed to any of these slots, with id provided? As I see, in that case we will silently override client's ID value - this is definitely what client expects in this situation.

As we've committed to handle the props calculation for React elements provided to slots, we should handle this case as well.

Good catch, will rework it

…b.com/stardust-ui/react into fix/omit-id-generation

kuzhelov · 2019-06-12T11:55:09Z

packages/react/src/components/Dialog/Dialog.tsx

@@ -23,6 +23,26 @@ import Header from '../Header/Header'
 import Portal from '../Portal/Portal'
 import Flex from '../Flex/Flex'

+const getOrGenerateIdFromShorthand = (


👍 probably, in future we will need to move this function to lib, as the logic of peek if client had specified some prop for shorthand value seems to be a quite common case

may just suggest to cover React element's case for shorthand slot in tests

Was done in e816e3c

layershifter added 3 commits June 4, 2019 14:17

fix(dialogBehavior): move id generation to component

78a88a8

add changelog entry

0e8a336

update ACC prop

9727226

Merge branches 'fix/omit-id-generation' and 'master' of https://githu…

acc8348

…b.com/stardust-ui/react into fix/omit-id-generation

layershifter commented Jun 4, 2019

View reviewed changes

layershifter marked this pull request as ready for review June 4, 2019 13:13

layershifter requested review from dvdzkwsk, kuzhelov, levithomason, miroslavstastny and mnajdova as code owners June 4, 2019 13:13

layershifter added 🧰 fix Introduces fix for broken behavior. 🚀 ready for review labels Jun 4, 2019

mnajdova reviewed Jun 10, 2019

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

mnajdova reviewed Jun 10, 2019

View reviewed changes

layershifter added 3 commits June 10, 2019 14:37

Merge branches 'fix/omit-id-generation' and 'master' of https://githu…

ac76e05

…b.com/stardust-ui/react into fix/omit-id-generation # Conflicts: # packages/react/src/lib/accessibility/Behaviors/Dialog/dialogBehavior.ts

update CHANGELOG

74f58da

update UTs

286abcf

layershifter mentioned this pull request Jun 10, 2019

feat(AutoControlledComponent): add getAutoControlledStateFromProps() #1483

Merged

layershifter added 2 commits June 11, 2019 15:34

Merge branches 'fix/omit-id-generation' and 'master' of https://githu…

c71242d

…b.com/stardust-ui/react into fix/omit-id-generation # Conflicts: # packages/react/src/lib/AutoControlledComponent.tsx # packages/react/src/lib/accessibility/Behaviors/Dialog/dialogBehavior.ts

update changelog

74d4dc2

mnajdova reviewed Jun 11, 2019

View reviewed changes

mnajdova approved these changes Jun 11, 2019

View reviewed changes

Merge branch 'master' into fix/omit-id-generation

898a0ec

kuzhelov reviewed Jun 12, 2019

View reviewed changes

layershifter added 5 commits June 12, 2019 13:26

refactor to support all shorthands

0108569

Merge branches 'fix/omit-id-generation' and 'master' of https://githu…

df207ed

…b.com/stardust-ui/react into fix/omit-id-generation

fix typo

39ae7aa

remove only

8051f0c

add UT

e816e3c

kuzhelov reviewed Jun 12, 2019

View reviewed changes

kuzhelov approved these changes Jun 12, 2019

View reviewed changes

silviuaavram approved these changes Jun 12, 2019

View reviewed changes

layershifter added 2 commits June 12, 2019 14:40

Merge branch 'master' into fix/omit-id-generation

f08b8a9

Update CHANGELOG.md

b328e5f

layershifter merged commit 921d4e5 into master Jun 12, 2019

delete-merged-branch bot deleted the fix/omit-id-generation branch June 12, 2019 13:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(dialogBehavior): move id generation to component #1449

fix(dialogBehavior): move id generation to component #1449

layershifter commented Jun 4, 2019 •

edited

codecov bot commented Jun 4, 2019 •

edited

layershifter Jun 4, 2019

mnajdova Jun 10, 2019

layershifter Jun 10, 2019

layershifter Jun 10, 2019

layershifter Jun 4, 2019

silviuaavram Jun 10, 2019

silviuaavram Jun 10, 2019

layershifter Jun 10, 2019

mnajdova Jun 10, 2019

layershifter Jun 10, 2019

mnajdova Jun 10, 2019

layershifter Jun 10, 2019

mnajdova Jun 11, 2019

layershifter Jun 11, 2019

mnajdova Jun 11, 2019

layershifter Jun 11, 2019

mnajdova Jun 11, 2019

layershifter Jun 11, 2019

kuzhelov Jun 12, 2019

kuzhelov Jun 12, 2019

layershifter Jun 12, 2019

kuzhelov Jun 12, 2019

kuzhelov Jun 12, 2019

layershifter Jun 12, 2019

fix(dialogBehavior): move id generation to component #1449

fix(dialogBehavior): move id generation to component #1449

Conversation

layershifter commented Jun 4, 2019 • edited

Problem 1

Problem 2

Solution

codecov bot commented Jun 4, 2019 • edited

Codecov Report

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

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

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

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

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

layershifter commented Jun 4, 2019 •

edited

codecov bot commented Jun 4, 2019 •

edited