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

Fix key resolution text #2014 and more #2719

Merged
merged 6 commits into from Jun 20, 2017

Conversation

Projects
None yet
2 participants
@robander
Member

robander commented Jun 9, 2017

The DITA 1.3 spec topic about generated text based on key references is clear that when the primary rules do not work, all elements with @keyref should look to <linktext>. This is not working today for elements without @href, as reported in #2014.

After starting with @raducoravu 's suggested fix, I extended my test case and found quite a few more edge cases that are either broken (wrong output) or bad (no output when we could provide something). This pull request covers all of the following updates:

  • In rule number 2, the spec topic says non-linking elements should look for <keyword> inside of <keywords> and consider that as matching content. Today: broken for all elements when keyword appears outside of keywords. The code looks for <keyword> anywhere within the key definition. So if I have <linktext><keyword>book</keyword> title</linktext>, matching text for all elements becomes "book" instead of "book title".
  • The spec explicitly says that for linking elements, if there are multiple keyword elements in <keywords>, all should be used (rule 3 has an example). Today: broken, we only use the first. This pull request takes them all, though I consider this an edge case - the rule carried over from DITA 1.2.
  • Getting to rule 4, this request adds support for the full content of <linktext> as the next fallback for all elements. Today: broken for all non-linking elements, broken for all elements if keyword appears in linktext.
  • Based on rule 5 in the spec topic, linking elements today fall back to @navtitle. Today: broken for navtitle element, sort of working for navtitle attribute on linking elements, and gives null result for non-linking elements. There are a couple of problems here:
    • If using navtitle, it should prefer <navtitle> (today that is ignored), so I've added that.
    • Non linking elements never get here, and give bad output. Having reached the end of the specification algorithm, with no good output, I think it's foolish to ignore the available navtitle, so I've added that as another fallback for non-linking elements.
    • If the target is a local DITA topic, we should not pull the navtitle unless that title is locked, because the true title will be pulled from that topic later
  • Additionally in rule 5, the specification says that "normal rules apply", which is clearly implementation specific. In links without keys, we fall back to the URI as the last resort, so I've added that as well. Again this is only for non-DITA or non-local content, because local DITA should wait and retrieve link text from the target.

Basically, the focus of this report is on

  1. Getting linktext to work so that key definitions have a universal fallback (this is specifically described in the spec as a best practice, and is broken today.
  2. Fixing output that today is broken (or just bad), and can easily be made correct (or at least reasonable).

robander added some commits Jun 9, 2017

Update fallback text resolution for key reference elements
Signed-off-by: Robert D Anderson <robander@us.ibm.com>
Correct test case for xref with two keywords
Signed-off-by: Robert D Anderson <robander@us.ibm.com>
Extend fallback to use href if nothing else found
Signed-off-by: Robert D Anderson <robander@us.ibm.com>
Update test cases for href fallback when no other key text
Signed-off-by: Robert D Anderson <robander@us.ibm.com>
New test cases for keyref text resolution
Signed-off-by: Robert D Anderson <robander@us.ibm.com>

@robander robander changed the title from Fix key resolution text #2104 and more to Fix key resolution text #2014 and more Jun 9, 2017

@robander robander requested a review from jelovirt Jun 9, 2017

@robander

This comment has been minimized.

Show comment
Hide comment
@robander

robander Jun 9, 2017

Member

Looks like this also addresses #1590.

Member

robander commented Jun 9, 2017

Looks like this also addresses #1590.

@robander robander added this to the 2.5.1 milestone Jun 15, 2017

@jelovirt

This comment has been minimized.

Show comment
Hide comment
@jelovirt

jelovirt Jun 20, 2017

Member

Converted for-loop with index to stream API, intent easier to read.

Extracted writeLinktext methods to separate methods, also for readability.

Member

jelovirt commented Jun 20, 2017

Converted for-loop with index to stream API, intent easier to read.

Extracted writeLinktext methods to separate methods, also for readability.

@jelovirt jelovirt merged commit 9c79077 into dita-ot:hotfix/2.5.1 Jun 20, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@robander robander deleted the robander:hotfix/keyreftext branch Jun 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment