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

Extract image alternate text from keydef linktext #2752

Closed
raducoravu opened this issue Jul 18, 2017 · 8 comments
Closed

Extract image alternate text from keydef linktext #2752

raducoravu opened this issue Jul 18, 2017 · 8 comments
Labels
bug dita standard Something does not comply with DITA specification preprocess/keyref priority/medium Medium (or unknown) priority issue
Milestone

Comments

@raducoravu
Copy link
Member

raducoravu commented Jul 18, 2017

According to this email exchange on the DITA TC list, according to DITA 1.3 spec:

For empty <image> elements, effective content is used as alternate text, equivalent to creating an <alt> sub-element to hold that content.

If in my DITA Map I have a key definition:

<keydef keys="myImage" href="images/Chrysanthemums.jpg" format="jpg">
  <topicmeta>
    <linktext>Alternate text</linktext>
  </topicmeta>
</keydef>

and in a DITA topic I have a keyref to it:

<image keyref="myImage" id="image_tq4_4lc_p1b"/>

the image <alt> element should be computed automatically from the <linktext> set on the keydef <topicmeta>.

@robander
Copy link
Member

I wondered about this when working on #2719 but knew I wouldn't have time to add it in. I think the final refactoring @jelovirt did before merging that item should actually make fixing this easier, but I haven't tried yet. Would be nice to get it into 2.5.2 if we can.

@robander robander added dita standard Something does not comply with DITA specification priority/medium Medium (or unknown) priority issue preprocess/keyref bug labels Jul 18, 2017
@robander
Copy link
Member

Looking into this one, the current code inserts the alternate text directly inside <image> when the image does not already have alternate text. Of course that's not valid, but it's what I'd expect from the current code. It doesn't show up anywhere because X/HTML and PDF all explicitly look for <alt> and process that.

Gets a bit trickier here because technically, the deprecated @alt on images should probably also override anything specified in the key definition -- currently every test for "link text" is based on element content, not attribute content.

@robander
Copy link
Member

robander commented Oct 14, 2017

This one was bugging me so I kept thinking about it. My eventual logic may be partly related to the fact that all the keyref link text processing is in the endElement method so no easy access to test if @alt was specified. But ignoring that..

  1. The @alt attribute was deprecated in favor of <alt> way back in DITA 1.1 (2007).
  2. Keys were not even defined until DITA 1.2.
  3. The key resolution here, that explicitly leads to <alt>, wasn't fully clarified until DITA 1.3 (2015).
  4. Now, 10 years after it was deprecated, I don't think we should go out of our way to support @alt as a way to override the key reference feature that was added more recently.

With that in mind -- @jelovirt or @raducoravu any thoughts on this commit as a fix for the current bug?
robander@8046803

It currently breaks the integration tests because we've got a sample in there that changes, but I could fix that in a pull request.

This one has been tested with key definitions that are referenced by an image, where the definition:

  • sets <linktext>
  • sets <navtitle>
  • sets @navtitle
  • sets <keyword> (does not resolve text per the spec)
  • sets no link text

Each of those 5 conditions tested with an image with no alt text, with <alt>, and with @alt.

@jelovirt
Copy link
Member

I'm ok with that fix.

@raducoravu
Copy link
Member Author

👍 There is a little bit of duplicate code between the two "writeAlt" methods, maybe a "writeAlt" which could received two parameters, the content as Element and the content as String, and depending on which one is not null, output that one...
One question, how about if there is an element derived from DITA image? Maybe it no longer has an alt element or maybe its "alt" element is not called "alt" anymore... Should we treat in the fix only DITA "image" elements (having the tag name "image") instead of matching the class attribute value?

@robander
Copy link
Member

For the duplicate code -- I've just used the same pattern we already have for writeLinktext, so I don't really want to create a new pattern here. Can change both later if we decide it's better to do as one method.

For specializations - we should not worry about that just for this case. The same theoretical issue comes up in almost any DITA processing that changes markup, so we shouldn't do something special here. For example - almost any DITA keyword/term/link/xref element could be specialized with EMPTY content model, but we still insert link text for those as well. Similarly, you could specialize a topic that doesn't allow <related-links> but we'll still generate map based linking. None of these break anything because we are not enforcing schema validity at this point, and if special processing exists for these conditions they'll just ignore the generated content.

@raducoravu
Copy link
Member Author

@robander Ok.

jelovirt added a commit that referenced this issue Oct 19, 2017
Fix for #2752 pull alt text from key definition
@infotexture infotexture added this to the 3.0 milestone Oct 20, 2017
@infotexture
Copy link
Member

Fixed in #2814.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug dita standard Something does not comply with DITA specification preprocess/keyref priority/medium Medium (or unknown) priority issue
Projects
None yet
Development

No branches or pull requests

4 participants