Material-ui: Fix W3C validation issues

[sambhav-gore]

• [x] I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior

When using material-ui with Server Side Rendering -
There should be no validation issues shown by https://validator.w3.org

Current Behavior

There are many validation issues thrown by https://validator.w3.org when using Material-UI with server side rendering

Steps to Reproduce (for bugs)

1. Go to https://validator.w3.org
2. Enter https://material-ui-next.com/ in address Bar (since material-ui-next is exactly the use-case which is Server-Side-Rendering with Material-UI
3. There are many error generated, some of them quite easy to fix.
4. Enter URLs of other pages from components demo and there are some similar errors and some other errors as well

Context

I want to create an application which validates with W3C validator.

Your Environment

| Tech | Version |
|--------------|---------|
| Material-UI | 1.0.0-beta.27 |
| React | 16.2.0 |
| browser | |
| etc | |

[mbrookes]

Wow, that's quite some list! 😳

We'd be grateful if you were to submit a PR (many PRs?! 😅 ) for the low-hanging fruit.

[sambhav-gore]

@mbrookes I will surely try to submit some PRs to fix the errors of validation. Warnings may be ignored for now.
Actually having my app rendered as server-side exposes these errors.
I am not sure of how beneficial it is to have 100% validated markup, but in my case it is a requirement from the stakeholders.

[oliviertassinari]

It might require some fixes in Next.js side.

I am not sure of how beneficial it is to have 100% validated markup

It will help with Material-UI credibility.

[oliviertassinari]

I have been fixing some of the issues. I'm more or less stuck with the homepage. Ideally, I would also scan all the pages.

[sambhav-gore]

@oliviertassinari Your changes have taken care of many errors. I have submitted a small change now as well.

If you fix the duplicate ids issue and the _target property used on div (also on every page) then Most of it would be closed already.

BTW, thank you for the quick turnaround time for fixing most of the issues.

[oliviertassinari]

@sambhav-gore I'm wondering if we shouldn't close the issue now. I believe the last issues are Next.js relative.

[sambhav-gore]

@oliviertassinari I still see some issues, I will list it all down here in a comment and then you can take a call about whether this can be closed.

[sambhav-gore]

Following are the issues which IMO are related to the core styles of components. I am listing here the demo page name (under components demo) and the issue reported by w3c and my reasoning of it

• [x] [Autocomplete] :
  Error: CSS: “appearance”: “textfield” is not a “appearance” value in “textfield” in “.jss220”.
  Reason: JSS only adds the browser prefix -webkit-appearance and -moz-appearance based on the browser used. W3C is also expecting a fallback appearance property value
• [x] [Badge] :
  Error: Element “div” not allowed as child of element “span” in this context.
  Reason: Badge is is a div placed inside a Button
• [x] [Button/Typography]:
  Error: Element “h3” not allowed as child of element “span” in this context.
  Reason: Typography creates a span to wrap h1, h2 and other headings. this is not allowed by validator
• [x] [Dividers]:
  Error: Element “hr” not allowed as child of element “ul” in this context.
  Reason: hr is used as a direct chid of ul (I am not 100% sure if this is in component source or demo/docs only)
• [x] [Lists]:
  Error: Bad value “button” for attribute “role” on element “li”
  Reason: May be demo error only.
• [x] [Progress]:
  Error: CSS: “stroke-dasharray”: only “0” can be a “unit”. You must put a unit after your number in “1,200”
  Reason: W3C is expecting the values to have a unit like px or pc or % but it is not mentioned in the specs. However, I spent some time looking at it but I was not sure if the numbers were meant to be percentages. If they are indeed percentage I think adding a unit is best. See this file src/Progress/CircularProgress.js
• [x] [Selection Controls]:
  Error: Attribute “required” not allowed on element “fieldset” at this point
  Reason: required is used on fieldset
• [x] [Stepper]:
  Error: Element “div” not allowed as child of element “button”
  Reason: Horizontal Non Linear Stepper wraps div inside button
• [x] [Tables]:
  Error: Table columns in range 4…1000 established by element “td” have no cells
  Reason: I am not sure about this one, haven't checked yet
• [x] And following are docs/demo specific on every page:
  Error: Attribute “target” not allowed on element “div” at this point
  Reason: target used on div for icon buttons of github, codesandbox and link to file
• [x] Error: Duplicate ID {demo-github|demo-codesandbox|demo-source}
  Reason: the ids when there are more than one demos on a page.

Finally, As long as users can generate valid markup by using components slightly differently tahn in demo it is okay. My main concern is when an error is generated from core styles.

[oliviertassinari]

@sambhav-gore Wow, thanks for the detailed list!

My main concern is when an error is generated from core styles.

We can split the work then. I will focus on the documentation issues :).

[sambhav-gore]

Can you confirm about the CircularProgress listed above in the list ? It is a percentage value ?

[oliviertassinari]

It should be px.

[sambhav-gore]

@oliviertassinari I have created a PR to fix all of the issues from the list above except for the first one - appearance as it is hard to find out where the magic or vendor prefix happening, may be that lib has a config to fix it. anyway since it is a build time issue it is not related to the source code.

Please let me know if my changes cause any issues as I am still quite new to material-ui

[sambhav-gore]

@oliviertassinari after digging a lot more I found the root cause of 1st point in my list the appearance: textfield error. It seems that the JSS generated on SSR is different than that of in-browser:

SSR

.jss220 {
    appearance: textfield;
}

In-Browser

.jss273 {
    -webkit-appearance: textfield;
}

The SSR one is wrong since spec doesn't have textfield value for appearance yet. Now to me it looks like a jss issue. right ?

[sambhav-gore]

On second thoughts, following can be changed to have vendor prefixes for webkit and moz since textfield is not a valid value for appearance anyway.

https://github.com/mui-org/material-ui/blob/d89cad0f80835615591036b03dd68b3ca1c3e34c/src/Input/Input.js#L198

[oliviertassinari]

It seems that the JSS generated on SSR is different than that of in-browser:

JSS doesn't vendor prefix on the server. It's up to the users, at least, it's what @kof is suggesting. I haven't tried yet.

[kof]

As of right now, yes, we should change this though.

[oliviertassinari]

[Selection Control]
Error: Attribute “required” not allowed on element “fieldset” at this point
Reason: required is used on fieldset

I'm working on it.

[oliviertassinari]

[Lists]:
Error: Bad value “button” for attribute “role” on element “li”
Reason: May be demo error only.

I'm working on it.

[oliviertassinari]

I'm working on the last appearance issue.

[oliviertassinari]

@sambhav-gore We should be good now. Feel free to report and fix the w3c issues that we have missed :). Thanks a lot for raising the concern. It's important for the vision of the project that goes beyond Material Design.

[talentedandrew]

IMO and a suggestion, fixing w3c issues using https://validator.w3.org will not help much when using modern web technologies. In my project it gave me an error Element style not allowed as child of element body in this context. (Suppressing further errors from this subtree.) .It wants the <style> tag in the <head>, not in a <div> or anything inside the <body>. This explains this issue. For apps that SSR, following tools are much more helpful.
google speed Insight,
gmetrix for speed & server tech