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

Remove invalid rel attribute from <img> tag #2019

Closed
wants to merge 1 commit into from

Conversation

daveharris
Copy link
Contributor

Hi,
Can we please remove the rel attribute from <img> tags as it produces invalid html.

Currently it produces html like:

<img src="/system/images/BAhbB1sHOgZmSSItMjAxMi8xMS8xMi8wOV81N18wN182MzZfQXZhdGFyX3NtYWxsLmpwZwY6BkVUWwg6BnA6CnRodW1iSSINMjI1eDI1NT4GOwZG/Avatar-small.jpg" title="Avatar Small" alt="Avatar Small" rel="225x255" width="225" height="150" />

This is invalid html according http://validator.w3.org/, see our page at http://validator.w3.org/check?uri=http%3A%2F%2Fbeta.n%20atlib.govt.nz

According to http://dev.w3.org/html5/spec-author-view/the-img-element.html#the-img-element the only valid attributes are the global attributes and:

  • alt
  • src
  • crossorigin
  • usemap
  • ismap
  • width
  • height

Tested this locally and it works fine, specs are passing with no changes. I wasn't sure what I needed to do to in terms of testing as I had just removed an attribute, but if you could point me in the right direction I could add them if necessary.

Dave

@parndt
Copy link
Member

parndt commented Nov 12, 2012

Is the attribute causing pain somehow besides it validating as "invalid"?

Would need to check the image dialogue JS code as it looks like it
probably uses this for the image resize option. Not sure if we still
use it now that we can inspect the Dragonfly URI hash information?

If it is indeed still used, could it be a data attribute instead? For
example data-img-resize="255x255"

@daveharris
Copy link
Contributor Author

Hi,
Thanks for the quick response!
Yeah, our product owner requires the page to be valid html (government client).

When I tested it locally, I didn't have any javascript errors, so I don't think it's used anymore.

I don't know anything about how to inspect the Dragonfly URL, are you able to take it from here?

I couldn't quite tell what the rel attribute contained. It looks like the maximum dimensions of that image size. Why would that be needed?

Dave

@parndt
Copy link
Member

parndt commented Nov 14, 2012

@danielfone
Copy link
Contributor

I believe if you changed the definition of the wymeditor rel tag from REL: 'rel', to (e.g.) REL: 'data-rel', you could retain the functionality and generate valid html with only a 5 byte change. :)

@parndt
Copy link
Member

parndt commented Nov 24, 2012

@danielfone good point. @daveharris would this work for you?

@daveharris
Copy link
Contributor Author

Yes that will work great for me! Thanks @danielfone

@parndt
Copy link
Member

parndt commented Nov 25, 2012

@daveharris would you care to modify your commit to incorporate this feedback?

Locally, just change the files appropriately and use this:

git add .
git commit --amend
# editor will open, change commit message then save and close editor.
git push origin master --force
# yes, you're force pushing against your master branch
# usually this is a custom branch name which would seem less scary

Let me know if you can't or don't want to :-)

Thanks again,
Phil

@daveharris
Copy link
Contributor Author

Yes, I will update it. I am currently at work so will hopefully get some time in-between other client work!

amrit added a commit to amrit/refinerycms that referenced this pull request Dec 1, 2012
@parndt parndt closed this in 1d99abc Dec 3, 2012
ugisozols added a commit that referenced this pull request Dec 3, 2012
@daveharris
Copy link
Contributor Author

Yay!
Thanks for the commit @aayalur I have been crazy busy on other client work

@amrit
Copy link
Contributor

amrit commented Dec 3, 2012

No problem -- it was one of the few issues somewhat within the reach of my limited coding chops!

@parndt
Copy link
Member

parndt commented Dec 3, 2012

Appreciate you helping :)

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.

4 participants