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

w3c validator warning: The type attribute is unnecessary for JavaScript resources. #4135

Closed
OBreidenbach opened this issue Oct 11, 2018 · 22 comments
Assignees
Labels
Resolution: Wontfix Issue will not be fixed due to technical limitations workaround A workaround has been provided

Comments

@OBreidenbach
Copy link
Contributor

OBreidenbach commented Oct 11, 2018

1) Environment

2) Expected behavior

No warning by the w3c validator.

3) Actual behavior

If Html 5 is used, the validator is complaining that the attribute 'type="text/javascript"' is unnecessary.

4) Steps to reproduce

Let the validator validate the primefaces showcase.
https://validator.w3.org/nu/?doc=https%3A%2F%2Fwww.primefaces.org%2Fshowcase%2F

I can provide a PR, but I do not know what would be the best solution.

  • Adding a config parameter to en-/disable rendering the type attribute.
  • Suppress rendering of type attribute if Html 5 is used. Is there a way to figure this out?
  • Is there a central place to handle it? ResponceWriter?
@tandraschko
Copy link
Member

tandraschko commented Oct 11, 2018

  1. there are only ~5-10 places in PF which renders the type, we can of course change them but script includes are rendered by the JSF impl (if you don't use MOVE_SCRIPTS_TO_BOTTOM)

  2. i don't think there is a easy way, i would just check the config

  3. -1, we can easily change these 5-10 places

@tandraschko tandraschko added the enhancement Additional functionality to current component label Oct 11, 2018
@cnsgithub
Copy link
Contributor

cnsgithub commented Oct 12, 2018

I like where you are going, PrimeFaces should definetely render valid markup in order for users to be able to include W3C QA validation icons on their websites for example.

But shouldn't we first take care of the errors:

image

Since script type is allowed in HTML5 as well, I wouldn't spend too much effort on this.

@tandraschko
Copy link
Member

+1 @cnsgithub

@melloware
Copy link
Member

+1. @tandraschko do you have an idea of where those errors are coming from? if not I can help investigate if you want.

@tandraschko
Copy link
Member

no idea, sry

@melloware
Copy link
Member

understood. I will investigate and see what I can find out.

@melloware
Copy link
Member

OK the errors are weird. The W3C validator is reporting type="button" is in the DOM twice but I when inspect the DOM in Chrome or any other tool its only there once. So I have no idea why its reporting this an an error and twice unless the JS is doing something to the DOM after it loads the W3C only looks at the original DOM sent from the server and not the JS manipulated DOM?

@cnsgithub
Copy link
Contributor

@melloware Just right-click and view the page source, then you'll find them twice.

@melloware
Copy link
Member

Ahh interesting. Chrome and Firefox in the Inspector filter this out but View Source shows it. The issue is due to this..

<p:button type="button" value="GET STARTED" icon="fa fa-download" outcome="/getstarted"/>

So its just bad markup in the showcase as that type="button" is not necessary.

@cnsgithub
Copy link
Contributor

Ok, it's rather a showcase issue then...

@melloware
Copy link
Member

Yep I will submit a Showcase PR.

@OBreidenbach
Copy link
Contributor Author

As a solution for my problem, do not render the type attribute for the script element, I did write my own response writer. This solves the problem for primefaces and the standard jsf components.

@melloware
Copy link
Member

@OBreidenbach Do you care to share your response writer here please?

@melloware melloware added Resolution: Wontfix Issue will not be fixed due to technical limitations workaround A workaround has been provided and removed enhancement Additional functionality to current component labels Jan 28, 2021
@OBreidenbach
Copy link
Contributor Author

@melloware sure, no big deal. Why are you coming back to this ticket after over 2 years?

public class Html5ResponseWriter extends ResponseWriterWrapper {
    private boolean inScriptStartTag;
    public Html5ResponseWriter(ResponseWriter wrapped) {
        super(wrapped);
    }

    @Override
    public void startElement(String name, UIComponent component) throws IOException {
        super.startElement(name, component);
        inScriptStartTag = "script".equals(name);
    }

    @Override
    public void endElement(String name) throws IOException {
        super.endElement(name);
        if ("script".equals(name)) {
            inScriptStartTag = false;
        }
    }

    @Override
    public void writeAttribute(String name, Object value, String property) throws IOException {
        if (inScriptStartTag && "text/javascript".equals(value)) {
            return;
        }
        super.writeAttribute(name, value, property);
    }
}
public class MyFacesContext extends FacesContextWrapper {

    public MyFacesContext(FacesContext context) {
        super(context);
    }

    @Override
    public void setResponseWriter(ResponseWriter responseWriter) {
        super.setResponseWriter(new Html5ResponseWriter(responseWriter));
    }
}
public class MyFacesContextFactory extends FacesContextFactoryImpl {

    private final FacesContextFactory wrapped;

    public MyFacesContextFactory(FacesContextFactory facesContextFactory) {
        wrapped = facesContextFactory;
    }

    @Override
    public FacesContext getFacesContext(Object context, Object request, Object response, Lifecycle lifecycle) throws FacesException {
        FacesContext wrappedContext = this.wrapped.getFacesContext(context, request, response, lifecycle);
        return wrappedContext instanceof MyFacesContext ? wrappedContext : new MyFacesContext(wrappedContext);
    }
}
<faces-config>
    <factory>
        <faces-context-factory>my.package.MyFacesContextFactory</faces-context-factory>
    </factory>
</faces-config>

@melloware
Copy link
Member

@OBreidenbach I think I might add this code in PFE for people to be able to enable with just...

<faces-config>
    <factory>
        <faces-context-factory>org.primefaces.extensions.Html5ContextFactory</faces-context-factory>
    </factory>
</faces-config>

My client is running our HTML through WCAG and is seeing this and asking "WHY?" and they don't like my answer. :)

@OBreidenbach
Copy link
Contributor Author

OBreidenbach commented Jan 29, 2021

Cool, good idea.

@tandraschko
Copy link
Member

tandraschko commented Jan 29, 2021

just to clarify as i dont understand this 2 different factories?

JSF/Java Server Faces API/Specs = javax:*
Jakarta Faces API/Specs = jakarta.*

Mojarra 2.3 = JSF impl
Mojarra 3.0 = Jakarta Faces Impl

MyFaces 2.3 = JSF Impl
MyFaces 3.0 = Jakarta Faces Impl

@tandraschko
Copy link
Member

Therefor if we need to 2 different factories, they should be called Mojarra/MyFaces, not MyFaces/Jakarta, that doesnt make any sense.

Its even better to only have 1 factory and add if-else statements for myfaces/mojarra

@tandraschko
Copy link
Member

And yes, the factory doenst need to extend a mojarra or myfaces class actually? Just check PrimeFaces, we have FactoryContextFactory there AFAIR

@melloware
Copy link
Member

Let me look I assumed i needed to extend the factory.

@melloware
Copy link
Member

Got it modeled after PrimeContextFactory now its just one class Html5ContextFactory.

<faces-config>
    <factory>
        <faces-context-factory>org.primefaces.extensions.application.Html5ContextFactory</faces-context-factory>
    </factory>
</faces-config>

@melloware
Copy link
Member

This is now a first class citizen in Faces 4.0: jakartaee/faces#1568

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Wontfix Issue will not be fixed due to technical limitations workaround A workaround has been provided
Projects
None yet
Development

No branches or pull requests

4 participants