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

fix(react-dialog): make aria-* properties reassignable #25092

Conversation

bsunderhus
Copy link
Contributor

@bsunderhus bsunderhus commented Oct 5, 2022

Current Behavior

aria-describedby or aria-labelledby properties provided by the user for DialogSurface will be overridden to internal values.

New Behavior

  1. aria-* properties will be properly reassigned by the user, this ensure removing aria-describedby by assigning it to undefined on scenarios where we don't want screen readers to read the body of the dialog.
  2. Improves stories around form scenarios where removing aria-describeby is recommended

@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 5, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 9c0eed5:

Sandbox Source
@fluentui/react 8 starter Configuration
@fluentui/react-components 9 starter Configuration

@fabricteam
Copy link
Collaborator

fabricteam commented Oct 5, 2022

📊 Bundle size report

Unchanged fixtures
Package & Exports Size (minified/GZIP)
react-components
react-components: Accordion, Button, FluentProvider, Image, Menu, Popover
188.815 kB
52.287 kB
react-components
react-components: FluentProvider & webLightTheme
33.4 kB
11.008 kB
react-dialog
Dialog (including children components)
82.465 kB
24.447 kB
react-portal-compat
PortalCompatProvider
5.857 kB
1.978 kB
🤖 This report was generated against ff55fc0e1abd7e61331bbcba18e066b6c449ba79

@size-auditor
Copy link

size-auditor bot commented Oct 5, 2022

Asset size changes

Size Auditor did not detect a change in bundle size for any component!

Baseline commit: ff55fc0e1abd7e61331bbcba18e066b6c449ba79 (build)

@fabricteam
Copy link
Collaborator

fabricteam commented Oct 5, 2022

Perf Analysis (@fluentui/react-components)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 1266 1268 5000
Button mount 919 911 5000
FluentProvider mount 1476 1482 5000
FluentProviderWithTheme mount 580 580 10
FluentProviderWithTheme virtual-rerender 536 543 10
FluentProviderWithTheme virtual-rerender-with-unmount 576 573 10
MakeStyles mount 1956 1962 50000
SpinButton mount 2329 2350 5000

@bsunderhus bsunderhus marked this pull request as ready for review October 5, 2022 13:31
@bsunderhus bsunderhus requested a review from a team as a code owner October 5, 2022 13:31
@bsunderhus bsunderhus changed the title fix(react-dialog): aria-* properties should be reassignable fix(react-dialog): make aria-* properties reassignable Oct 5, 2022
@bsunderhus bsunderhus closed this Oct 5, 2022
@bsunderhus bsunderhus reopened this Oct 5, 2022
@bsunderhus bsunderhus force-pushed the react-dialog/fix--aria-properties-should-be-reassignable branch from 8ca6f89 to 9c0eed5 Compare October 5, 2022 15:50
@bsunderhus bsunderhus enabled auto-merge (squash) October 5, 2022 16:55
@ling1726 ling1726 closed this Oct 5, 2022
auto-merge was automatically disabled October 5, 2022 16:59

Pull request was closed

@ling1726 ling1726 reopened this Oct 5, 2022
@@ -1,7 +1,7 @@
import * as React from 'react';
import { useModalAttributes } from '@fluentui/react-tabster';
import { applyTriggerPropsToChildren, getTriggerChild, useEventCallback } from '@fluentui/react-utilities';
import { DialogTriggerProps, DialogTriggerState } from './DialogTrigger.types';
import type { DialogTriggerProps, DialogTriggerState } from './DialogTrigger.types';
import { useDialogContext_unstable, useDialogSurfaceContext_unstable } from '../../contexts';
import { useARIAButtonProps } from '@fluentui/react-aria';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import { useARIAButtonProps } from '@fluentui/react-aria';
import type { useARIAButtonProps } from '@fluentui/react-aria';

@bsunderhus bsunderhus merged commit b48083b into microsoft:master Oct 6, 2022
@bsunderhus bsunderhus deleted the react-dialog/fix--aria-properties-should-be-reassignable branch October 6, 2022 09:45
marcosmoura added a commit to marcosmoura/fluentui that referenced this pull request Oct 10, 2022
* master: (23 commits)
  Revert "chore: screener-run workflow should report to PR (microsoft#25144)" (microsoft#25145)
  chore: screener-run workflow should report to PR (microsoft#25144)
  applying package updates
  fix: The Tooltip should not hide if it gets keyboard focus (microsoft#25138)
  fix: Tooltip should not hide if an element inside it gets focused (microsoft#25140)
  Create react-migration-v8-v9 with shims and stories (microsoft#25121)
  fix: CheckboxField to set a generated ID on the input, to match the label's htmlFor (microsoft#25079)
  feat: Overflow menu should be registered in overflowManager (microsoft#25091)
  fix: version-bump generator removes beachball disallowedChangeType config (microsoft#25130)
  fix: new overflow items should only be enqueued while observing (microsoft#25122)
  fix(script): allow runPublished call from CLI (microsoft#25127)
  feat: add vertical variation for toolbar (microsoft#24940)
  ProgressField implementation and stories (microsoft#25103)
  fix: Dropdown icon layout with no placeholder/value (microsoft#25049)
  chore: add a bundle size fixture (Button, Provider & theme) (microsoft#25113)
  feat: Adding subtle transition between states on Button components (microsoft#25106)
  Revert "chore: screener-run workflow should report to PR (microsoft#25096)" (microsoft#25115)
  chore: screener-run workflow should report to PR (microsoft#25096)
  fix(react-dialog): aria-* properties should be reassignable (microsoft#25092)
  fix(scripts): don't run publish if web-components are affected (microsoft#25095)
  ...
NotWoods pushed a commit to NotWoods/fluentui that referenced this pull request Nov 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants