search

yourhead / s3

public info about s3
8 stars 7 forks source link

hyphen in link attribute not respected #811

Closed marathia closed 6 years ago

marathia commented 6 years ago

When adding an attribute to a link as described on http://api.yourhead.com/templates/link/ (bottom of the page) and the attribute contains a hyphen, the hyphen and the part before it are omitted in the output. Example:

Info.plist:

<dict>
    <key>title</key>
    <string>link</string>
    <key>default</key>
    <string>https://google.com</string>
    <key>type</key>
    <string>link</string>
    <key>id</key>
    <string>linkID</string>
</dict>
<dict>
    <key>title</key>
    <string>attribute</string>
    <key>default</key>
    <string>default aria-label text</string>
    <key>type</key>
    <string>input</string>
    <key>id</key>
    <string>labelID</string>
</dict>
<dict>
    <key>title</key>
    <string>link text</string>
    <key>default</key>
    <string>ckick here</string>
    <key>type</key>
    <string>input</string>
    <key>id</key>
    <string>textID</string>
</dict>

template.html: %id=linkID +aria-label="%id=labelID%"%%id=textID%</a>

expected output: <a href="https://google.com" aria-label="default aria-label text">click here</a>

actual output: <a href="https://google.com" label="default aria-label text">click here</a>

yourhead commented 6 years ago

I very nearly marked this as unfixable. And it may still be.

Allowing a dash in operands might be more than my little parser can handle. I have managed to add this allowance, but I am quite concerned that this will have undesirable effects when parsing a dash in completely different circumstances. And unfortunately there isn't a way (currently) for me to segregate link parsing from other sorts of parsing.

I'm going to release this beta build to the developers, but I think there's a very significant likelihood that I'll end up pulling it out. In that case we'll have to look for other possible workarounds.

So... what I'm saying is... please help me test this one out with some other stacks -- and if it does't work, hang tight, we'll try to be a bit more clever. We will fix this. I do not want to give up accessibility and aria-labels. 3.6.0 will absolutely make these work somehow.

marathia commented 6 years ago

Thanks Isaiah, I just hoped some escaping or encoding could make it work…

I did some testing in RW7 and found the folowing: it works now with both aria-label and data-* – to some extend:

%id=linkID +aria-label="%id=labelID%"%%id=textID%</a> and %id=linkID +data-color="%id=dataValue%"%%id=textID%</a> both work.

%id=linkID +data-%id=dataID%="red"%%id=textID%</a> and %id=linkID +data-%id=dataID%="%id=dataValue%"%%id=textID%</a> fail.

When using more then one property, only the last one is parsed, so: %id=linkID +class="test" +id="someID"%%id=textID%</a> becomes <a href="https://google.com" id="someID">click here</a>

In case you're interested: you can download a simple stack with examples here: https://we.tl/BKsCw4UfTJ

yourhead commented 6 years ago

I'm going to do a bit more testing. I think it might be that there are some corner cases that are impacted by this change. What that means: If I can't find an solution with zero impact we'll have to roll it back.

Give me another day to finish testing and adjust the parsing. If I can't find any solutions there we'll fall back to some special characters to give the parsing a hand.

And if all else fails I'll just create a one-off special case for this. It would definitely have a negative impact on performance overall though -- which is something I'm trying hard to avoid.

But overall I'm unwilling to compromise when it comes to supporting accessibility features. This is a 100% must-fix immediately bug as far as I'm concerned. So we WILL find a solution. Hopefully soon.

joeworkman commented 6 years ago

I have seem other libraries convert camel case to dashes. So ariaLabel => aria-label

yourhead commented 6 years ago

i was thinking this exact thing. i’m going to see what that implementation would take. if it’s not too bad i think that’s the direction i’ll head. it’s a pretty straightforward solution and won’t come with any mystery side effects of trying to parse out those dashes in the middle of identifiers.

yourhead commented 6 years ago

this was added in 3.6.0

related #819