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

wrap new att lists in <ul> (where they were not so wrapped) #641

Merged
merged 6 commits into from
Nov 8, 2023

Conversation

sydb
Copy link
Member

@sydb sydb commented Oct 28, 2023

  • template showAttClasses is called from 6 different places (all in common_tagdocs);
  • generated debugging code for each of those places;
  • found which 2 were causing problem (li element as child of td instead of child of ul);
  • wrapped them in ul element.
  • Note: deliberately left debugging PIs in place, but they should maybe be removed before merging into dev.

 * template showAttClasses is called from 6 different places (all in common_tagdocs);
 * generated debugging code for each of those places;
 * found which 2 were causing problem (li element as child of td instead of child of ul);
 * wrapped them in ul element.
 * Note: deliberately left debugging PIs in place, but they should maybe be removed before merging into dev.
@sydb sydb added the priority: releaseBlocker All tickets with this priority must be cleared in advance of the freeze before a release. label Oct 28, 2023
@sydb sydb added this to the Release 7.56.0 milestone Oct 28, 2023
Copy link
Member

@ebeshero ebeshero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm looking for any trouble with @sydb 's repair in the output Guidelines. I see no problems with specs in:
[x] - Epub (inspected several element specs)
[x] - Guidelines PDF build
[x] - Guidelines HTML

Locally, the following tests pass:
[x] - TEI repo (no diffs from expected results)

Stylesheets Test 1 does not pass, breaks on

make[1]: *** [Makefile:583: test-oddity] Error 1
make[1]: Leaving directory '/stylesheet/Test'
make: *** [Makefile:106: test] Error 2

Stylesheets Test 2 passes on my Mac.
It's failing for me ONLY in Docker (and I suspect a Docker error?) on

  • build.xml: 550
  • build_docx.xml: 32, 43, and 64

@ebeshero
Copy link
Member

ebeshero commented Oct 29, 2023

Summarizing how things went by the end of the day on Saturday 10/29 (EST). @sydb notes (from Slack):

Every test in Test/ now passes except test-epub, for which 13 of the output files get the error “This file should declare in the OPF the property: scripted”.

In order to get the XSLT to run, the top of tei-to-epub3.xsl looks like:

  <xsl:import href="../html/html.xsl"/>
  <xsl:import href="../odds/teiodds.xsl"/>   <!-- only included for $autoGlobal param, which ../odds/classatts.xsl relies on -->
  <xsl:import href="../odds/classatts.xsl"/> <!-- only included for ATTCLASSES key -->
  <xsl:import href="../epub/epub-common.xsl"/>
  <xsl:import href="../epub/epub-preflight.xsl"/>
  <xsl:import href="../html/html_oddprocessing.xsl"/> <!-- only included for schemaOut template, which ../odds/teiodds.xsl seems to need -->

Syd notes: We need to figure out the correct file for the "schemaOut" template (there are 5 of them, all different).
All that work (i.e., importing 3 files which have a total of 4,390 elements and attributes) is being done just so we can execute this line in common_tagdocs.xsl :
<xsl:variable name="thisClassSpec" select="key('ATTCLASSES', @key)" as="element(tei:classSpec)?"/>

Perhaps this is better than using key():

<xsl:variable name="key" select="@key"/>
<!-- note: following line does not use "key('ATTCLASSES', @key)" because ATTCLASSES is not defined in this or any imported file, and importing it requires importing other files, too. -->
<xsl:variable name="thisClassSpec" select="//tei:classSpec[ @ident eq $key]" as="element(tei:classSpec)?"/>

@joeytakeda
Copy link
Contributor

I've now tested this (adding the all of those <xsl:import>s as described above) and get the same validation problem with the epub:

Validating against EPUB version 3.0
ERROR: actual-results/test_3.epub/OPS/index.html: This file should declare in the OPF the property: scripted
ERROR: actual-results/test_3.epub/OPS/index-front.1_div.1.html: This file should declare in the OPF the property: scripted
ERROR: actual-results/test_3.epub/OPS/index-front.1_div.2.html: This file should declare in the OPF the property: scripted
[... plus many more ....]

I believe this is caused by an import, which is adding all of the HTML javascript into the epubs but shouldn't be there — or, at least, isn't there in test_3.epub from the last Stylesheets release's actual-results folder .

Adding the following to the bottom of epub3/tei-to-epub3.xsl allows make test-epub to succeed:

  <!--Add empty javascript hook to remove JS-->
  <xsl:template name="javascriptHook"/>

The suggestion of replacing key() with a variable (and not adding a bunch of imports) is probably the right call; however, when I did that, different parts of Test failed, so some more investigation would be needed there to see if there are different and unexpected repercussions (sigh)

@sydb
Copy link
Member Author

sydb commented Nov 4, 2023

Implemented change I suggested above and which @joeytakeda thought was the way to go in 7ed5111. Has passed test of building the Guidelines. Running Stylesheet tests now.

@sydb
Copy link
Member Author

sydb commented Nov 4, 2023

After removing <xsl:import href="../odds/classatts.xsl"/> from epub3/tei-to-epub3.xsl (which I forgot to do in previous commit, now done in 05231d4), this branch passes Test2/ but fails Test/ with a diff error (albeit one we may eventually to decide is OK).[1]

In Test/ the file actual-results/test15.odd.html has 4 extraneous empty <li class="classSpecItem" itemprop="item"> elements. Should we be worrying about that?

Note
[1] This is a bit troublesome because it means Test2/ is missing something. But that is not the subject of this PR.

@joeytakeda
Copy link
Contributor

In Test/ the file actual-results/test15.odd.html has 4 extraneous empty <li class="classSpecItem" itemprop="item"> elements. Should we be worrying about that?

@sydb: Do we know what is it that's causing them? At the moment, it produces an output that has (unsurprisingly) empty additional list items (below screenshot of output of actual-results/test15.odd.html), so I think we do need to worry about that:

Screenshot 2023-11-06 at 12 41 57 PM

I wonder if we should reverse course and take the brute-force approach of adding the imports and the empty javascript hook? While this is certainly more overhead, it does allow both test suites to pass (and I don't imagine would cause problems in other transformations, unlike changes to common)

@sydb
Copy link
Member Author

sydb commented Nov 7, 2023

No, I do not really know what is causing them.

As for going the “adding the imports and the empty javascript hook” route, do we know what this test file looks like with that system?

@sydb
Copy link
Member Author

sydb commented Nov 8, 2023

I found the bug, I think: the //classSpec that I used instead of the ATTCLASSES key needed to look at only <classSpec>s with an @type of "atts". We added the needed predicate. @martindholmes is pushing up the results now.

Copy link
Contributor

@martindholmes martindholmes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This appears to be working. Ready for merge as soon as the tests all complete.

@martindholmes martindholmes merged commit 8e0242d into dev Nov 8, 2023
4 checks passed
@raffazizzi raffazizzi added this to the Release 7.56.0 milestone Nov 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: releaseBlocker All tickets with this priority must be cleared in advance of the freeze before a release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants