Message is produced in contradiction of documentation

[benkasminbullock]

The following command:

./tidy --alt-text "moo" --show-body-only yes --quiet yes

run on this input

 <img src='http://example.com/example.png'>

produces output

line 1 column 1 - Warning: <img> lacks "alt" attribute
<img src='http://example.com/example.png' alt="moo">

This contradicts the documentation:

https://github.com/htacg/tidy-html5/blob/0ef4493ae8bdc4b8fec125fecf4e1dcc95c30fb1/src/localize.c#L405

which says

"Use with care, as this feature suppresses further accessibility warnings. "

The code clearly doesn't check for the option before issuing the warning:

The warning is issued here:

tidy-html5/src/tags.c

Line 858 in 0ef4493

TY_(ReportMissingAttr)( doc, node, "alt" );

which is before the check for the accessibility text:

tidy-html5/src/tags.c

Line 861 in 0ef4493

if ( cfgStr(doc, TidyAltText) )

The fix is obvious, but I have no idea whether the documentation is incorrect, or the code is incorrect.

[balthisar]

Hmmm.... that's interesting and should provoke a discussion about expected behavior. That's classic Tidy behavior, and is probably the result of accessibility-check being in its default state of 0 (Tidy Classic, where the missing alt is reported as a TidyWarning. As soon as I change accessibility-check to something else (e.g., 1 (Priority 1 Checks)), the TidyWarning message goes away and I get a TidyAccess message instead: "[1.1.1.1] <img> missing 'alt' text."

However, it's still not squelched when I add quiet, and in fact I'm getting the remaining accessibility errors along with it.

I would suggest that the wording is right when using the default accessibility-check, but could be clarified to point out that accessibility warnings only apply when accessibility checking is specifically turned on.

However it's certainly inaccurate in cases where accessibility checking is enabled.

[geoffmcl]

@benkasminbullock I agree with @balthisar that this is classic tidy behaviour - to report a warning even though it provided a fix, in this case with user option assistance.

Not that I am particularly in love with this action as it is. I think the warning should stay, but maybe tidy should add something like which I have fixed!. That is it would be good if it distinguished between the items fixed, and those remaining that the user needs to fix... but that is a little off topic here...

I think I was the one that introduced this option all those years back, but I remember it not being universally liked, in that it allows a user to add some cheap, rubbish alt to an image, rather than adding a proper description to aid with accessability.

So I guess the sense I was trying to convey in the message was more take care user, you are creating an attribute in the tidied document that will suppress this accessibilty warning. Not that it would suppress accessability warnings in this document, Tidy will always yap about that, as everybody eventually learns...

Hmmmm, that's very interesting. Somewhere along the road my original CVS message was changed from a strong, perhaps clearer message -

  {TidyAltText,
   "This option specifies the default \"alt=\" text Tidy uses for "
   "<IMG> attributes. This feature is dangerous as it suppresses "
   "further accessibility warnings. You are responsible for making your "
   "documents accessible to people who can not see the images! "
  },

Maybe I could have added "as it suppresses further accessibility warnings in the tidied document" to make it 100% clear!

Oops, @balthiar it may have been you? Documentation beautification underway

Maybe a message somewhere in between might still be beautiful ;=))

[balthisar]

@geoffmcl, that was me. The previous wording seemed a bit hyperbolic. I'll find a happy medium and post a PR.

[balthisar]

Potential fix in 933fc3d.

New wording for option description:

This option specifies the default alt= text Tidy uses for <img> attributes when the alt= attribute is missing.

Use with care, as it is your responsibility to make your documents accessible to people who cannot see the images.

Output 1:

./tidy --alt-text "moo" --show-body-only yes --quiet yes foo.html
line 1 column 1 - Warning: <img> inserting "alt" attribute using value "moo"
<img src='http://example.com/example.png' alt="moo">

Output 2:

./tidy --show-body-only yes --quiet yes foo.html
line 1 column 1 - Warning: <img> lacks "alt" attribute
<img src='http://example.com/example.png'>

I'll issue a PR against its branch pending review.

[geoffmcl]

@balthisar have looked at this, and it seems all fine as is...

I like the new, more exact message. You did mark the message with an /* Error */ comment, when I think it should be a /* Warning */, but that comment is a bit erronious...

But since you did request a review, here goes ;=))

src/message.h - Question: Generally, when adding new message values should we try to find the best place for it, and like now, potentially change a number of values before we reach a hole?

Now the fact that the list does contain numeric holes, and did group them under comment headings, does suggest the original designer envisaged that. But to what aim? Where is that used, ranged checked, to provide additional information? And who is to guess how big a hole needs to be between such groups?

This is a purely internal to the library value. These values are not exposed in the library user API so do not feature in the API docs... so can be anything...

Further they added a comment to PREVIOUS_LOCATION of /* last */! But it is not really last in that some values lesser are written/given after it! And this is not an enumeration where it is sometimes good the have a maximum value as the last. It is a set of defines only.

The fetching of the GetFormatFromCode() format string, is also grouped under headings, but again to what aim? Is anything gained by this? The loop code that finds the format is going to be fast, and is only used in a warning/error situation, so does not really need to be optimised.

I remember from the very first time I wanted to add a warning/error message being very confused by the arrangement of this list! And welcome discussing on it at long last.

Is this the time to do something about that? And make the adding a message process simpler for the next developer?

Initially I would put them back in pure numeric order, with the higest number last, leaving the current holes, remove that last comment, and put in a simple comment at the end of the list, like -

   /* end of message values - define a new warning/error message above here 
      using the next numeric value. Ignore holes and groups. */

An alternative would be the extra work to make it an enum msgVals, maybe with a 1 start, and a true last, say LAST_MESSAGE_VALUE. And then remove the #if SUPPORT_ACCESSIBILITY_CHECKS - always include these - then the GetFormatFromCode(msgVals code) could be -

static ctmbstr GetFormatFromCode(msgVals code)
{
    if (code && (code < LAST_MESSAGE_VALUE))
        return return msgFormat[code].fmt;
    return NULL;
}

That would allow us to even remove the code from the struct _msgfmt, but I do like in a search for a value in the code that I 'see' the message associated with the value... so would keep this... seem a small space cost for developer convenience, knowledge... and helps getting the format variable count correct...

Anyway, maybe just discussion at this point. As stated, what you have done is fine.

src/tags.c: Question: When should a value be put in a variable rather than just repeatedly fetched?

The new code uses cfgStr(doc, TidyAltText) three times. Is this sufficient reason to get this value once and use it 3 times? Like say -

    if ( !HasAlt )
    {
        ctmbstr alttxt = cfgStr(doc, TidyAltText);
        if ( ( cfg(doc, TidyAccessibilityCheckLevel ) == 0) && !alttxt )
        {
            doc->badAccess |= BA_MISSING_IMAGE_ALT;
            TY_(ReportMissingAttr)( doc, node, "alt" );
        }

        if ( alttxt ) {
            AttVal *attval = TY_(AddAttribute)( doc, node, "alt", alttxt );
            TY_(ReportAttrError)( doc, node, attval, INSERTING_AUTO_ATTRIBUTE);
        }
     }

Again, not really that important, and what you have is fine.

Would welcome other ideas, comments...

And the new TidyAltText message is fine by me. Go for it ;=))

[balthisar]

When I saw the long list of defines my gut wrenched a little and my instinct was to rework it into an enum or a lot of exported constants (probably the enum). But like so many things, expediency often wins the day!

The string assignment's fine; I shouldn't assume that every compiler will optimize such function calls.

I'll pull the fixes a little later and bump the version up a bit, unless you've beat me to it.

Adding on to this, and it's a broader topic for another day, I've always been a bit dissatisfied on how localize.c works in the first place. Unlike modern applications tidy doesn't adapt to its locale. While we don't currently have any language localizations, the only possible way to localize is to build a language specific version of Tidy.

There are a couple of different options, but they either involve increasing the libtidy size (strictly speaking, only if we add localizations), or they break the simplicity of having a single library, or they break cross-platform compatibility. It's something I keep telling myself I'll have a look at. Right now my current thought is to move all the strings into exported constants, perhaps wrapped with a macro indicating the ISO language code, e.g.,
#def lang_es_mx(INSERTING_AUTO_ATTRIBUTE) "%s insertando\"%s\" atributo usando el valor \"%s\""

That's probably one justification for keeping GetFormatFromCode and not using the macro directly.

And of course we have a language option that's been in the API for who knows how long, but still remains without function.

[balthisar]

I'll close this as it's in master, although @geoffmcl points out there's a different bug exposed with show-body-only.