wrap img tags in a tags

Canop / miaou

A chat server with OAuth2 authentication, persistent and searchable history, video and audio, markdown formatting, private and public rooms, stars, votes, embedded games, and many other features

http://miaou.dystroy.org/

Other

539 stars 73 forks source link

wrap img tags in a tags #61

Closed 0e4ef622 closed 8 years ago

0e4ef622 commented 8 years ago

this should let you "click" on an image using cVim

tckmn commented 8 years ago

+1, <img href> is dubious at best (I've never even heard of it until seeing it in the Miaou source) and bad for accessibility, as well as not being compatible with extensions such as Pentadactyl for Firefox or cVim for Chromium.

tckmn commented 8 years ago

... taking a closer look, href isn't even valid on img, and the clicking is handled with JavaScript. No point in relying on intercepting click with JS when you can simply wrap with <a>, which is far more semantically accurate.

Canop commented 8 years ago

Reminder:

it's best to discuss on Miaou before writing a PR
there's a test suite (run ./test.sh, I'll work on making travis aware of it), it must be updated when you change code making it fail

Canop commented 8 years ago

Right now, this has many unwanted side effects:

broken room description on home page
clicking the image in the room description opens the image instead of the room
clicking the image in the chat unexpectedly opens in the same tab (remember, this is a chat application, not a web site)
clicking the image in notable messages or search results opens the image instead of going to the message (sometimes loading it along the messages around, sometimes directly scrolling to it)
clicking the image in watch drawer opens the image instead of going to the relevant room+message

That's why I removed the a tag initially. I'm not sure of what would be the best solution for cvim compatibility (and other accessibility problems related to not having a a element).

0e4ef622 commented 8 years ago

Hmm... I could add target="_blank" to make it open in a new tab. I'm not sure what to do for the other problems though...

Canop commented 8 years ago

Obviously we need the JS handling part. I don't know if cvim is OK with an image in an href element whose behavior is changed by an event handler. In that case it might be possible to combine both.