Skip to content

Conversation

@rajatofficial
Copy link
Contributor

@rajatofficial rajatofficial commented Mar 6, 2024

JIRA for the test Cases: https://jira.corp.adobe.com/browse/FORMS-13508

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes and the overall coverage did not decrease.
  • All unit tests pass on CircleCi.
  • I ran all tests locally and they pass.

@adobe-bot
Copy link

Lighthouse scores (desktop)

Performance Accessibility Best-Practices SEO
Scores 100 100 100 73

@adobe-bot
Copy link

Lighthouse scores (mobile)

Performance Accessibility Best-Practices SEO
Scores 92 100 100 73

@adobe-bot
Copy link

Accessibility Violations Found

Id Impact
focus-order-semantics minor
label-title-only serious
landmark-one-main moderate
region moderate
target-size serious

2 similar comments
@adobe-bot
Copy link

Accessibility Violations Found

Id Impact
focus-order-semantics minor
label-title-only serious
landmark-one-main moderate
region moderate
target-size serious

@adobe-bot
Copy link

Accessibility Violations Found

Id Impact
focus-order-semantics minor
label-title-only serious
landmark-one-main moderate
region moderate
target-size serious

@codecov
Copy link

codecov bot commented Mar 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.82%. Comparing base (e39a679) to head (fe263fc).

Additional details and impacted files
@@            Coverage Diff            @@
##                dev    #1112   +/-   ##
=========================================
  Coverage     80.82%   80.82%           
  Complexity      774      774           
=========================================
  Files            92       92           
  Lines          2117     2117           
  Branches        285      285           
=========================================
  Hits           1711     1711           
  Misses          252      252           
  Partials        154      154           

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

@adobe-bot
Copy link

Lighthouse scores (desktop)

Performance Accessibility Best-Practices SEO
Scores 100 100 100 73

@adobe-bot
Copy link

Lighthouse scores (mobile)

Performance Accessibility Best-Practices SEO
Scores 87 100 100 73

@adobe-bot
Copy link

Accessibility Violations Found

Id Impact
focus-order-semantics minor
label-title-only serious
landmark-one-main moderate
region moderate
target-size serious

2 similar comments
@adobe-bot
Copy link

Accessibility Violations Found

Id Impact
focus-order-semantics minor
label-title-only serious
landmark-one-main moderate
region moderate
target-size serious

@adobe-bot
Copy link

Accessibility Violations Found

Id Impact
focus-order-semantics minor
label-title-only serious
landmark-one-main moderate
region moderate
target-size serious

Copy link
Contributor

@sakshi-arora1 sakshi-arora1 left a comment

Choose a reason for hiding this comment

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

You will also need to honour this in core component container (guidecontainer) component.

@rismehta
Copy link
Collaborator

rismehta commented Mar 7, 2024

You will also need to honour this in core component container (guidecontainer) component.

The role="main" attribute is typically used for the main content area of a web page, not specifically for form. Why did we add this in container ? Ideally, it should not be present.

@adobe adobe deleted a comment from sakshi-arora1 Mar 7, 2024
@rismehta
Copy link
Collaborator

rismehta commented Mar 7, 2024

@sakshi-arora1, kindly refrain from mentioning internal JIRA IDs here. The issue you provided pertains to a page rather than a form. Ideally, the attribute should be included in the page component, as mentioned previously. This attribute is specifically utilized for the main content area and not for the HTML elements of the form.

@sakshi-arora1
Copy link
Contributor

@sakshi-arora1, kindly refrain from mentioning internal JIRA IDs here. The issue you provided pertains to a page rather than a form. Ideally, the attribute should be included in the page component, as mentioned previously. This attribute is specifically utilized for the main content area and not for the HTML elements of the form.

For a standalone form and for form embedded (within iframe), form is the main content of the page, hence role main needs to be added to the container. For form embedded within a div on sites page, role main will not apply. So the fix is to conditionally add. role main.

@rismehta
Copy link
Collaborator

rismehta commented Mar 7, 2024

For a standalone form and for form embedded (within iframe), form is the main content of the page, hence role main needs to be added to the container.

I acknowledge the point, but the responsibility of including role="main" in the HTML should lie with the page component rather than the form container. Otherwise, resorting to workarounds like removeRoleMain introduces complexity to the API surface and compromises code readability.

@sakshi-arora1
Copy link
Contributor

For a standalone form and for form embedded (within iframe), form is the main content of the page, hence role main needs to be added to the container.

I acknowledge the point, but the responsibility of including role="main" in the HTML should lie with the page component rather than the form container. Otherwise, resorting to workarounds like removeRoleMain introduces complexity to the API surface and compromises code readability.

Page component will not get an opportunity to add role main. Also this is reported in our accessibility report under "Best Practice" -> landmark-one-main

@rismehta
Copy link
Collaborator

rismehta commented Mar 7, 2024

For a standalone form and for form embedded (within iframe), form is the main content of the page, hence role main needs to be added to the container.

I acknowledge the point, but the responsibility of including role="main" in the HTML should lie with the page component rather than the form container. Otherwise, resorting to workarounds like removeRoleMain introduces complexity to the API surface and compromises code readability.

Page component will not get an opportunity to add role main. Also this is reported in our accessibility report under "Best Practice" -> landmark-one-main

Page component will not get an opportunity to add role main.

Could you clarify your statement about the page component not getting an opportunity? While I acknowledge that this is an accessibility issue, I believe the proposed solution for addressing the problem is not accurate. It's acceptable to address this for foundation components in this manner, but for core components, a more refined solution is necessary. The current approach of introducing removeRoleMain appears very hacky and less readable.

@rismehta
Copy link
Collaborator

rismehta commented Mar 7, 2024

merging this since the change

For a standalone form and for form embedded (within iframe), form is the main content of the page, hence role main needs to be added to the container.

I acknowledge the point, but the responsibility of including role="main" in the HTML should lie with the page component rather than the form container. Otherwise, resorting to workarounds like removeRoleMain introduces complexity to the API surface and compromises code readability.

Page component will not get an opportunity to add role main. Also this is reported in our accessibility report under "Best Practice" -> landmark-one-main

For reference, HTML spec, https://html.spec.whatwg.org/multipage/grouping-content.html#the-main-element

ideally this is how the HTML structure should be

 <!-- Page Component -->
<html>
   <head>
   </head
   <body>
      <!-- Header Component -->
      <header>
          <!-- Header content goes here -->
      </header>

      <!-- Main Content Area with role="main" --> // we might need some hook to inject this when the page is just containing the entire form
      <main role="main">
          <!-- Other components/content on the page -->
  
          <!-- Adaptive Form Inclusion -->
  
          <!-- Other components/content on the page -->
      </main>

    <!-- Footer Component -->
    <footer>
        <!-- Footer content goes here -->
    </footer>
</div>

@sakshi-arora1
Copy link
Contributor

@rismehta still not merged

merging this since the change

For a standalone form and for form embedded (within iframe), form is the main content of the page, hence role main needs to be added to the container.

I acknowledge the point, but the responsibility of including role="main" in the HTML should lie with the page component rather than the form container. Otherwise, resorting to workarounds like removeRoleMain introduces complexity to the API surface and compromises code readability.

Page component will not get an opportunity to add role main. Also this is reported in our accessibility report under "Best Practice" -> landmark-one-main

For reference, HTML spec, https://html.spec.whatwg.org/multipage/grouping-content.html#the-main-element

ideally this is how the HTML structure should be

 <!-- Page Component -->
<html>
   <head>
   </head
   <body>
      <!-- Header Component -->
      <header>
          <!-- Header content goes here -->
      </header>

      <!-- Main Content Area with role="main" --> // we might need some hook to inject this when the page is just containing the entire form
      <main role="main">
          <!-- Other components/content on the page -->
  
          <!-- Adaptive Form Inclusion -->
  
          <!-- Other components/content on the page -->
      </main>

    <!-- Footer Component -->
    <footer>
        <!-- Footer content goes here -->
    </footer>
</div>

@rismehta still not merged

@rismehta rismehta merged commit ca9cb59 into dev Mar 11, 2024
@rismehta rismehta deleted the roleMainCheck branch March 11, 2024 06:37
github-actions bot pushed a commit that referenced this pull request Mar 18, 2024
* FORMS-13392 role main check added

* FORMS-13392 role main check added I

---------

Co-authored-by: Rajat Khurana <rajatkhurana@adobe.com>
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.

6 participants