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.

Sign up for GitHub

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

Jump to bottom

Various fixes to EPUB 3 to DAISY 2.02 #153

Closed
wants to merge 9 commits into from
Closed

Various fixes to EPUB 3 to DAISY 2.02 #153

wants to merge 9 commits into from

Conversation

bertfrees
Copy link
Member

Addresses the issues #86 and #92.

@bertfrees bertfrees added this to the v1.12.0 milestone Feb 8, 2019
This was referenced Feb 8, 2019
Closed
Closed
@josteinaj
Copy link
Member

I tried running the test book from the nordic migrator through (C00000.epub), and the result is not valid when I use the Pipeline 1 DAISY 2.02 DTB Validator. I get similar results for our other books. I'm attaching the XML report from Pipeline 1: report.xml.txt

(Also, the "validation" option should probably be a boolean rather than a string option, but that's not really related to this PR.)

@bertfrees
Copy link
Member Author

Thanks for testing. How have you tested it? In your report I'm seeing all the errors that this PR should have fixed.

Why a boolean? It is currently a "choice":

 <choice>
     <value>off</value>
     <a:documentation xml:lang="en">No validation</a:documentation>
     <value>report</value>
     <a:documentation xml:lang="en">Report validation issues</a:documentation>
     <value>abort</value>
     <a:documentation xml:lang="en">Abort on validation issues</a:documentation>
 </choice>

@josteinaj
Copy link
Member

josteinaj commented Feb 19, 2019 •
edited
Loading

Oh, maybe I tested it the wrong way then.

  • I used the master branch of the super project and created a new local branch
  • then I ran git subrepo pull modules/scripts --branch super/epub3-to-daisy202
  • then I changed the scripts-bom version to 1.11.3-SNAPSHOT in the assembly
  • to make the system build and run I also changed the gui version to 1.2.0 in the assembly, and changed the java version requirement from 11 to 10 in …/bin/pipeline2
  • then I used make run to start the engine.

boolean/choice: oh, ok. It was a text input field with "off" as the default so I assumed it it would be "on" or "off". Maybe it has an incorrect option type.

@bertfrees
Copy link
Member Author

OK let me push a branch on which you can test it...

Regarding the "validation" option: I'm also seeing this in the web UI now. It looks like the data type is not exposed. Will investigate.

rdeltour
rdeltour reviewed Feb 19, 2019
View reviewed changes
Copy link
Member

@rdeltour rdeltour left a comment

Choose a reason for hiding this comment

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

Looks generally OK to me! (with a couple nitpicking details in inline comments)

epub3-to-daisy202/src/main/resources/xml/xslt/augment-smil.xsl Show resolved Hide resolved
epub3-to-daisy202/src/main/resources/xml/xslt/augment-smil.xsl Show resolved Hide resolved
epub3-to-daisy202/src/main/resources/xml/xslt/augment-smil.xsl Show resolved Hide resolved
epub3-to-daisy202/src/main/resources/xml/xslt/augment-smil.xsl
self::html:h4 or
self::html:h5 or
self::html:h6 or
self::html:span[matches(@class,'(^|\s)page-(front|normal|special)(\s|$)')]]">
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if we should also accept element with ARIA role="heading"…

Copy link
Member Author

Choose a reason for hiding this comment

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

But shouldn't we first convert that to a h then in the HTML downgrading?

Copy link
Member

Choose a reason for hiding this comment

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

You're right, this should rather be converted to a native heading element.

epub3-to-daisy202/src/main/resources/xml/xslt/augment-smil.xsl Show resolved Hide resolved
epub3-to-daisy202/src/main/resources/xml/xslt/augment-smil.xsl
The SMIL already references a containing element. FIXME: Unlikely to
happen for headings, but less unlikely for page numbers.
-->
<xsl:message terminate="yes">FIXME</xsl:message>
Copy link
Member

Choose a reason for hiding this comment

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

can we not terminate here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess so. But we would be creating an invalid SMIL?

epub3-to-daisy202/src/main/resources/xml/xslt/augment-smil.xsl Show resolved Hide resolved
@bertfrees
epub3-to-daisy202: fix bug in NCC: body element was missing
11e5dfd 
see #92
@bertfrees
epub3-to-daisy202: fix links in NCC
454b739 
see #92
@bertfrees
epub3-to-daisy202: should also have SMIL files when text-only
1e03a11 
SMIL files with references to all headings are generated when
needed. This is needed for the NCC to be valid.

Some things have been left out for now:
- support merging of existing par elements when they reference
  segments within a heading
- page numbers are left out from the NCC

See also issue #86.
@bertfrees
Add some info and debug messages
7a1b623
@bertfrees
test_epub3-to-daisy202-and-validate.xprocspec: store result and repor…
7afe8ea 
…t to disk

so that they can be manually inspected.
@bertfrees
Fix validation issue "ncc.html: element h1 missing required attribute…
2133726 
… id"
@bertfrees
Fix validation issue "ncc.html: value of attribute "name" is invalid"…
5fa670f 
… (meta elements)
@bertfrees
Make augment-smil.xsl more robust
bf6d60f
@bertfrees
Include page numbers in NCC
4010052
@bertfrees
Copy link
Member Author

@josteinaj Check the epub3-to-daisy202 branch in the super project.

@josteinaj
Copy link
Member

I tried the new branch. Still getting errors, but some different errors:
report-20190222.xml.txt

@bertfrees
Copy link
Member Author

bertfrees commented Feb 22, 2019 •
edited
Loading

Thanks. I think the validator from Pipeline 1 is more strict than the one in Pipeline 2. I wonder why. These are the validation issues:

  • Attribute ... must be declared for element type 'smil'
  • Attribute ... must be declared for element type 'html'
  • Invalid pseudo-function (from CSS validation)
  • File not found: C00000-2-toc.html (in file:/C:/Users/jostein/Desktop/C00000/C00000-01-cover.html)
  • required elements missing (in smil files)
  • required attributes missing (in smil files)
  • bad value for attribute 'name' (in ncc)
  • bad value for attribute 'content' (in ncc)
  • Could not compare calculated duration to stated duration since this information is missing in the NCC
  • NCC schematron: ncc:charset seems not to be in ncc exactly once
  • NCC schematron: ncc:pageFront seems not to be in ncc exactly once
  • NCC schematron: ncc:pageNormal seems not to be in ncc exactly once
  • NCC schematron: dc:identifier seems not to be in ncc exactly once
  • NCC schematron: dc:title seems not to be in ncc exactly once
  • NCC schematron: ncc:tocItems seems not to be in ncc exactly once
  • NCC schematron: ncc:totaltime seems not to be in ncc exactly once

@bertfrees
Copy link
Member Author

File not found: C00000-2-toc.html (in file:/C:/Users/jostein/Desktop/C00000/C00000-01-cover.html)

This one is solved. It was an error in the input EPUB. @rdeltour epubcheck didn't pick this up. See w3c/epubcheck#975

@bertfrees
Copy link
Member Author

@rdeltour Do you think we should make the validator more strict?

@bertfrees bertfrees modified the milestones: v1.12.0, Next Mar 7, 2019
bertfrees added a commit to daisy/pipeline-modules that referenced this pull request Apr 19, 2019
@bertfrees
Some changes in augment-smil.xsl requested by rdeltour
9f60176 
- correct computation of HTML base URLs
- support xml:base in SMIL documents
- normalize URLs before comparing
- add comment in augment-smil.xsl about add-missing-ids.xsl

see daisy/pipeline-scripts#153
bertfrees added a commit to daisy/pipeline-modules that referenced this pull request Apr 19, 2019
@bertfrees
Various fixes to EPUB 3 to DAISY 2.02
f7e2af2 
see daisy/pipeline-scripts#153
@bertfrees bertfrees mentioned this pull request Apr 19, 2019
Open
@bertfrees
Copy link
Member Author

bertfrees commented Apr 19, 2019 •
edited
Loading

Merged in daisy/pipeline-modules@98f901d and addressed most of @rdeltour's comments.

bertfrees added a commit to daisy/pipeline-modules that referenced this pull request Apr 19, 2019
@bertfrees
Some changes in augment-smil.xsl requested by rdeltour
3183b80 
- correct computation of HTML base URLs
- support xml:base in SMIL documents
- normalize URLs before comparing
- add comment in augment-smil.xsl about add-missing-ids.xsl

see daisy/pipeline-scripts#153
bertfrees added a commit to daisy/pipeline-modules that referenced this pull request Apr 19, 2019
@bertfrees
Various fixes to EPUB 3 to DAISY 2.02
98f901d 
see daisy/pipeline-scripts#153
josteinaj
josteinaj requested changes Apr 23, 2019
View reviewed changes
Copy link
Member

@josteinaj josteinaj left a comment

Choose a reason for hiding this comment

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

Code looks good. The validation errors given by Pipeline 1 should be fixed in my opinion: #153 (comment)

@bertfrees
Copy link
Member Author

bertfrees commented Apr 23, 2019 •
edited
Loading

Yes, this will be the subject of a next PR. This one addresses #86 and #92.

@bertfrees bertfrees closed this May 9, 2019
@bertfrees bertfrees deleted the super/epub3-to-daisy202 branch May 9, 2019 09:52
@bertfrees bertfrees modified the milestones: Next, v1.12.2 Sep 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rdeltour rdeltour rdeltour left review comments

@josteinaj josteinaj josteinaj requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.13.0
Development

Successfully merging this pull request may close these issues.
3 participants
@bertfrees @josteinaj @rdeltour