You can't have a <div> (block element) inside a <span> (inline element), but that's exactly what the markup produced by this module does. This is one of the few remaining problems causing my latest site to not validate as XHTML strict.

The span is coming since you're not using the hook_link() API properly -- you're returning the full-blown link already formatted in the 'title' attribute, and putting nothing in the 'href'. Therefore, core is getting confused and trying to wrap everything in a span for you, since it assumes there's no link since 'href' isn't defined. If you refactored the (rather convoluted) code that's messing with the links to use the intended API, I think you'd avoid this extra span, and then the markup would be valid XHTML...

I'd offer to write the code myself, but I tried once before to get all this to work more sanely, and ran into troubles, since I'm not a full-fledged jQuery hacker (yet), and some of the tricks are beyond what I could understand. So, I was basically just working in the dark. Someone more familiar with this stuff could probably do a quicker/better job of it. Hence, unassigned. ;)

I hope someone volunteers. I'd be happy to review and test patches, and potentially work on them if no one else steps forward...

Thanks,
-Derek

Comments

greenskin’s picture

greenskin commented
Status: Active » Closed (works as designed)

The use of hook_link() is valid according to the api docs for it:

The second format can be used for non-links. Leaving out the href index will select this format:

  • title: Required. The text or HTML code to display.
  • html: Optional. If not set to true, check_plain() will be run on the title before it is displayed.

The hook_link() thus should not wrap the contents inside a span tag if it is truly to allow valid XHTML code and not limit the elements allowed.

dww’s picture

dww
we/he/they
commented
Status: Closed (works as designed) » Active

There are other problems created by the current approach, in addition to the invalid markup. It makes it very difficult to do anything with share *except* put it in the node links. If you want to move it around anywhere else on the page from your theme, use a different icon without CSS (so there can be alt/title tags, etc), etc, etc, you're basically doomed. Good luck if you don't happen to want it inside an li. ;) I spent a few hours just trying to cobble together something that works, and it's still not what I'd like -- without actually parsing the "title" share gives back and fixing the markup myself, there's basically nothing you can do with this monolithic link.

And, just because you can use the API this way to accomplish a works-in-some-cases effect, doesn't mean that's the proper use of the API. You're saying "I have a 'link' with no href, but please don't mess with the title since I already know it's been escaped properly". That's not the situation here at all. You really have a link, but it's already formatted as a href and that's crammed into the 'title'.

Anyway, if you're set on keeping things this way, I won't keep pushing. But, as someone who'd otherwise be a happy customer, this is a pretty difficult hurdle for users of your module that want to do anything with it beyond the single case you're expecting.

Thanks for your consideration,
-Derek

greenskin’s picture

greenskin commented

The reason for providing straight HTML in hook_link() instead of letting it generate the link itself is so the popup HTML is provided as well. Now, I could provide the popup HTML inside the node content but who's to say a theme is going to display it. I could append the popup HTML to the body teaser but then the code could be loaded even when not needed, say in a View list.

The latest code should allow for showing the Share link in the node itself and not in with the links, in this case it will not be in 'li' tags.

The code currently is a second rendition of what I was trying to provide, and concerning your comment above about the code being rather convoluted, I would have to agree but it is better than the first endeavor. I was in the process of simplifying my version 2's method of providing the popup when I decided that yet again I would rewrite it, so I refrained from continuing the cleanup. This is why version 2 is in the Alpha stage instead of a Beta stage.

The reason I have decided on pursuing another rewrite is to provide the ability for other modules to easily hook into the popup as individual tabs. Share has received an issue requesting the Send module be incorporated like the Forward module is. Creating a hook method for providing tabs will allow for this.

I appreciate the insight you've provided and if you have interest in helping in the development of Share, please let me know. I understand you don't feel comfortable with the javascript/jQuery, but any PHP coding help is welcomed.

dww’s picture

dww
we/he/they
commented

Great, thanks. Yeah, I'm happy to help. I'm using share on a site now (check out any story on http://socialistworker.org), and anything I use I start trying to help + improve. ;) And, I'm not totally illiterate when it comes to jQuery, I'm just not a full blown JS hacker yet.

A hook for other modules to expose their own tabs sounds like a lovely idea.

I understand that it must be difficult to get the hidden divs to appear in the links without the tricks you're playing. Perhaps a compromise would be to refactor the code internally a little better so that if another module or theme wanted to, it could cleanly get the separate parts (popup markup, URL, link title, etc) and render them as appropriate. If you don't do that, and just put Share in the node links, you get what you have now. That way, it still works in the "simple" case, but anyone trying to do anything custom can still get what they need without reimplementing huge chunks of your code.

I'll submit some separate issues about smaller refactorings you should consider that I found made my life easier when trying to customize what's there.

Cheers,
-Derek

mfer’s picture

mfer commented

@greenSkin I've been looking this module over and am about to implement it on a site. I'd be willing to test your rewrite, code review it, or even help with the jQuery (something I'm pretty comfortable with).

scottrigby’s picture

scottrigby
he/him
Primary language English
Location Brooklyn, NY
commented

Hi Guys,
I got a different markup validation error. 

Validation Output: 1 Error
 Line 114, Column 155: there is no attribute "language".
…="share_share_this"><script language='javascript' type='text/javascript'>

I didn't see this in the issue queue... then again, I think I'm using the dev version, to try to avoid the above error...

Hope this helps...

but also, would like to know if there's any obvious way to fix this so my site can be valid in the meantime?

Cheers!
:) Scott

greenskin’s picture

greenskin commented

You can remove the "language='javascript'" out of line 114 to get a quick fix.

scottrigby’s picture

scottrigby
he/him
Primary language English
Location Brooklyn, NY
commented

Hi greenSkin – I don't really know lots about javascript so didn't want to mess with it... but it works to just remove this... thanks!
btw, in the dev version it's in two places... (I just did a search)
:) Scott

greenskin’s picture

greenskin commented
Status: Active » Closed (fixed)

A work around for the

inside issue has been implemented in the latest Alpha release.