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

[fix #759] fixes nav doc validation to be more faithful to the spec #763

Merged
merged 1 commit into from
Jun 6, 2017
Merged

[fix #759] fixes nav doc validation to be more faithful to the spec #763

merged 1 commit into from
Jun 6, 2017

Conversation

mattgarrish
Copy link
Member

  • only restrict a nav element's content model when it carries an epub:type attribute
  • require a first-child heading for all nav elements not defined in the spec (including without an epub:type)
  • require at least one li child for ol (also fixes existing errors in test docs)
  • add warning that nav elements should have an epub:type attribute

- only restrict a nav element's content model when it carries an epub:type attribute
- require a first-child heading for all nav elements not defined in the spec (including without an epub:type)
- require at least one li child for ol (also fixes existing errors in test docs)
- add warning that nav elements should have an epub:type attribute
@mattgarrish mattgarrish requested review from tofi86 and rdeltour June 5, 2017 20:17
@mattgarrish
Copy link
Member Author

For reference, this PR addresses issue #759

@tofi86 tofi86 changed the title fixes nav doc validation to be more faithful to the spec: [fix #759] fixes nav doc validation to be more faithful to the spec Jun 5, 2017
@tofi86 tofi86 added the bugfix label Jun 5, 2017
@tofi86 tofi86 added this to the 4.1.0 milestone Jun 5, 2017
Copy link
Collaborator

@tofi86 tofi86 left a comment

Choose a reason for hiding this comment

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

From a technical point, the changes LGTM, though I didn't review the specs which lead to the changes in detail and therefore trust your expertise on this topic, Matt!

However, I'm not sure about the new warning for nav elements without epub:type attribute. Should this really issue a warning?

@mattgarrish
Copy link
Member Author

Technically it has to be a warning as the other nav elements section says:

EPUB Navigation Documents optionally MAY include one or more nav elements in addition to the toc, page-list and landmarks nav elements defined above. Such additional nav elements SHOULD have an epub:type attribute to provide a machine-readable semantic, and MUST have a human-readable heading as their first child.

It's trying to encourage people to use semantics in the document because the nav elements are supposed to be machine-processable to some degree.

I don't really like it, either, though, as I think it breaks the case that made me start all this, and sort of forces you to the restricted content model. I think we'd have to open a spec issue to drop it to "may have".

@mattgarrish
Copy link
Member Author

Actually, the more I look at the specification, the more flaws I see in the navigation document design. It says that reading systems must present the links, but that's not why the landmarks are there for example. They facilitate behaviours, if such behaviours are supported. I don't know if any reading system gives blanket access to all the nav elements except when the document is in the spine, either.

It also says that a reading system must relocate the reading position, but we clarified in 3.1 that the navigation document can have lists of links to external resources.

Not that the above are directly relevant to interpreting what 3.0.1 says, but the design is showing some cracks.

I was also assuming that a nav without an epub:type was completely detached from processing before digging in, as the nav document is also supposed to be a fully-functional content document allowing any kind of content.

Some more thought as to the purpose of epub:type in the nav document, when it is required and how nav elements are used might be needed, so I'm okay with taking the epub:type test out.

@tofi86
Copy link
Collaborator

tofi86 commented Jun 6, 2017

It's trying to encourage people to use semantics in the document because the nav elements are supposed to be machine-processable to some degree.

Okay, got it, thanks!

Some more thought as to the purpose of epub:type in the nav document, when it is required and how nav elements are used might be needed, so I'm okay with taking the epub:type test out.

But I think your proposed changes now reflect the current spec much better than before, so I'd vote for leaving the epub:type tests in!

@mattgarrish
Copy link
Member Author

I opened three issues last night in the epub revision repository around this (w3c/epub-specs#975, w3c/epub-specs#976, w3c/epub-specs#977), so maybe in the future the requirements will change, but there's no real arguing that it's not what the spec says right now.

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.

LGTM.
I agree that the spec could use some improvements or clarifications, but I'll comment on the dedicated issues.

@rdeltour rdeltour merged commit d32de85 into w3c:master Jun 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants