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(FormItem):As a subcomponent of FormItem, the aria-label prop of the Select component should correspond to the label prop of the FormItem. #45184

Closed
wants to merge 7 commits into from

Conversation

liangkuku
Copy link

@liangkuku liangkuku commented Oct 5, 2023

[中文版模板 / Chinese template]

🤔 This is a ...

  • New feature
  • Bug fix
  • Site / documentation update
  • Demo update
  • Component style update
  • TypeScript definition update
  • Bundle size optimization
  • Performance optimization
  • Enhancement feature
  • Internationalization
  • Refactoring
  • Code style optimization
  • Test Case
  • Branch merge
  • Workflow
  • Other (about what?)

🔗 Related issue link

close #45168

💡 Background and solution

📝 Changelog

Language Changelog
🇺🇸 English fix(FormItem):As a subcomponent of FormItem, the aria-label prop of the Select component should correspond to the label prop of the FormItem.
🇨🇳 Chinese 修复 FormItem 的 bug,Select 作为 FormItem 的子组件,其 aria-label 属性应该对应于 FormItem 的 label 属性。

☑️ Self-Check before Merge

⚠️ Please check all items below before requesting a reviewing. ⚠️

  • Doc is updated/provided or not needed
  • Demo is updated/provided or not needed
  • TypeScript definition is updated/provided or not needed
  • Changelog is provided or not needed

🚀 Summary

Generated by Copilot at dcf5ef
Assign the label prop of the FormItem to the aria-label prop of the child component.

🔍 Walkthrough

Assign the label prop of the FormItem to the aria-label prop of the child component.
To override Select default aria-label values

@stackblitz
Copy link

stackblitz bot commented Oct 5, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 5, 2023

Preview Is ready

@argos-ci
Copy link

argos-ci bot commented Oct 5, 2023

The latest updates on your projects. Learn more about Argos notifications ↗︎

Build Status Details Updated (UTC)
default (Inspect) 🧿 Changes detected (Review) 1 change Oct 10, 2023, 3:13 PM

@codecov
Copy link

codecov bot commented Oct 5, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (940a1a5) 100.00% compared to head (66bfe51) 99.99%.
Report is 20 commits behind head on master.

❗ Current head 66bfe51 differs from pull request most recent head f73b6e7. Consider uploading reports for the commit f73b6e7 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##            master   #45184      +/-   ##
===========================================
- Coverage   100.00%   99.99%   -0.01%     
===========================================
  Files          692      692              
  Lines        11663    11677      +14     
  Branches      3117     3125       +8     
===========================================
+ Hits         11663    11676      +13     
- Misses           0        1       +1     
Files Coverage Δ
components/form/FormItem/index.tsx 100.00% <100.00%> (ø)
components/form/util.ts 97.05% <87.50%> (-2.95%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@yoyo837
Copy link
Contributor

yoyo837 commented Oct 5, 2023

@khalibloo Is this what you expected?

@khalibloo
Copy link
Contributor

@yoyo837 I just had a look in the preview of the Form component. It works correctly now. Thanks

@@ -778,7 +800,7 @@ describe('Form', () => {
);

// if getByLabelText can get element, then it is a validate field with form control and label
expect(screen.queryByLabelText('test')).not.toBeInTheDocument();
expect(screen.queryByLabelText('test')).toBeInTheDocument();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why change this?

Copy link
Author

Choose a reason for hiding this comment

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

The Form.Item has an prop label 'test', the input element has an aria-label attribute also set to 'test' and it can locate based on the input's aria-label attribute by screen.queryByLabelText('test').

Copy link
Contributor

Choose a reason for hiding this comment

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

@zombieJ Please help to check this.

@@ -7532,6 +7588,7 @@ exports[`renders components/form/demo/label-debug.tsx extend context correctly 1
class="ant-form-item-control-input-content"
>
<input
aria-label="[object Object]"
Copy link
Member

Choose a reason for hiding this comment

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

This should be a wrong aria-label.

Copy link
Author

Choose a reason for hiding this comment

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

@yoyo837 @afc163 @zombieJ Sorry,it was my negligence.The problem 'aria-label="[object Object]' arises because the label property of FormItem is a component.if the prop label of FormItem is component , then the aria-label of its child components should be what?
1696648319017

Copy link
Contributor

Choose a reason for hiding this comment

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

renderToString?

Copy link
Author

Choose a reason for hiding this comment

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

@yoyo837 Extracting text from a component? In the example,regard 'longtextlongtext...' as input aria-label? What if label is an empty tag for example <div></div>?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe other members have good suggestions... 🤔

Copy link
Author

Choose a reason for hiding this comment

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

I have an idea. if label is a component, the children property of props is searched recursively until children are of type String.If not , the default 'Search ' is used as the value of the aria-label property .

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I understand completely, but how about domElement.innerText? That should handle both string and component children.

Copy link
Author

Choose a reason for hiding this comment

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

I know what you mean.But label is React.ReactNode not HTMLElement.What I want to do is as follows.

function getTextFromElement(element) {
  let text = '';

  React.Children.map(element.props.children, child => {
    if (React.isValidElement(child)) {
      text += getTextFromElement(child);
    } else if (typeof child === 'string') {
      text += child;
    }
  });

  return text;
}

const A = (
  <div>
    Hello <span>world</span>!
  </div>
);

const text = getTextFromElement(A);
console.log(text); // Output: "Hello world!"

Copy link
Author

Choose a reason for hiding this comment

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

@yoyo837 @afc163 @zombieJ I fix wrong aria-label. If you have time, please review Pr.

/**
* Get element inner text
*/
export function getTextFromElement(element: React.ReactElement): string {
Copy link
Member

Choose a reason for hiding this comment

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

This will not work as expect since React support HOC:

const My = () => 'Hello World';

getTextFromElement will get nothing.

Copy link
Member

Choose a reason for hiding this comment

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

And this will also break in some locale case:

const Counter = ({ unit }) => {
  const counter = useContext(parentContext);
  return `${counter} ${unit}s missing`;
};

get unit instead of 3 units missing

Copy link
Member

Choose a reason for hiding this comment

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

Another case is customize component use another prop instead of children:

<Badge status="success" text="Success" />

@@ -692,6 +692,7 @@ exports[`renders components/auto-complete/demo/form-debug.tsx extend context cor
class="ant-form-item-control-input-content"
>
<div
aria-label="单独 AutoComplete"
Copy link
Member

Choose a reason for hiding this comment

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

为啥会出现中英文混合的内容,感觉不太好,demo 里面还是用纯英文吧

Copy link
Contributor

Choose a reason for hiding this comment

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

这个应该是demo的自己的锅

Copy link
Member

Choose a reason for hiding this comment

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

感觉可以改一下 demo

@zombieJ
Copy link
Member

zombieJ commented Oct 10, 2023

Could you help to check using aria-labelledby instead?

@liangkuku
Copy link
Author

Could you help to check using aria-labelledby instead?

Sure,but I did some research and found that aria-labelledby requires the element's id as an identifier.If the element does not have an id, it can pose a problem when trying to identify it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Select has unnecessary aria-label
6 participants