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

FXL Properties - Orientation Tests #91

Merged
merged 11 commits into from
Nov 23, 2021
Merged

FXL Properties - Orientation Tests #91

merged 11 commits into from
Nov 23, 2021

Conversation

wareid
Copy link
Contributor

@wareid wareid commented Nov 19, 2021

Testing the Orientation properties. These tests cover the default statement, the SHOULD in RS, and the MUST NOT in the core spec (will add appropriate test hooks).

Copy link
Collaborator

@dlazin dlazin left a comment

Choose a reason for hiding this comment

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

Can you rename these, please? They should be, for example, fxl-orientation_default, fxl-orientation_auto, fxl-orientation_duplicate, etc.

I will review more thoroughly post-rename.

Copy link
Collaborator

@dlazin dlazin left a comment

Choose a reason for hiding this comment

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

Initial comments — please fix throughout, more later.

tests/fxl-orientation_default/OPS/nav.xhtml Outdated Show resolved Hide resolved
tests/fxl-orientation_default/OPS/package.opf Outdated Show resolved Hide resolved
tests/fxl-orientation_default/OPS/package.opf Outdated Show resolved Hide resolved
tests/fxl-orientation_default/OPS/page_001.xhtml Outdated Show resolved Hide resolved
tests/fxl-orientation_default/META-INF/container.xml Outdated Show resolved Hide resolved
tests/fxl-orientation_default/OPS/fixed.css Show resolved Hide resolved
tests/fxl-orientation_default/OPS/nav.xhtml Show resolved Hide resolved
Copy link
Collaborator

@dlazin dlazin left a comment

Choose a reason for hiding this comment

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

Great tests, thanks! Should there be a separate test for explicit "portrait"? I can see a RS supporting portrait but not landscape, or in theory vice versa.

<metadata xmlns:dc="http://purl.org/dc/elements/1.1/">
<dc:coverage>Fixed Layout</dc:coverage>
<dc:creator>Wendy Reid</dc:creator>
<dc:date>2021-09-28</dc:date>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider updating this date to match the _default test.

<link rel="dcterms:rights" href="https://www.w3.org/Consortium/Legal/2015/copyright-software-and-document"/>
<link rel="dcterms:rightsHolder" href="https://www.w3.org"/>
<meta property="dcterms:isReferencedBy">https://www.w3.org/TR/epub-33/#orientation</meta>
<meta property="dcterms:modified">2021-01-01T00:00:00Z</meta>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Update this to match the above, please.

<dc:coverage>Fixed Layout</dc:coverage>
<dc:creator>Wendy Reid</dc:creator>
<dc:date>2021-09-28</dc:date>
<dc:description>EPUB creators must not declare the orienation property more than once.</dc:description>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess this is our first intentional core-spec test. Should this say something like "EPUBCheck should produce an error."?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also I think the description should be in this format: "rendition:orientation is specified twice. EPUBCheck should produce an error."

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, note typo: "orienation"

<dc:description>EPUB creators must not declare the orienation property more than once.</dc:description>
<dc:identifier id="pub-id">fxl-orientation_duplicate</dc:identifier>
<dc:language>en</dc:language>
<dc:title>rendition:orientation duplicate</dc:title>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: "duplication", to match your other <title> elements. Noun is better in a title anyway.

<dc:coverage>Fixed Layout</dc:coverage>
<dc:creator>Wendy Reid</dc:creator>
<dc:date>2021-09-28</dc:date>
<dc:description>Reading systems should display or inform the user of the intended orientation of the EPUB.</dc:description>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please rewrite to be in the same format as other descriptions: "rendition:orientation is X, so Y".

<dc:description>Reading systems should display or inform the user of the intended orientation of the EPUB.</dc:description>
<dc:identifier id="pub-id">fxl-orientation_landscape</dc:identifier>
<dc:language>en</dc:language>
<dc:title>rendition:orientation portrait</dc:title>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This says portrait, but it's landscape on line 16.

@wareid
Copy link
Contributor Author

wareid commented Nov 22, 2021

Great tests, thanks! Should there be a separate test for explicit "portrait"? I can see a RS supporting portrait but not landscape, or in theory vice versa.Great tests, thanks! Should there be a separate test for explicit "portrait"? I can see a RS supporting portrait but not landscape, or in theory vice versa.

I did consider this, but I think what I want to focus on is the RS behaviour as described in the spec, and portrait or landscape, the SHOULD focuses on the RS either displaying in that orientation or informing the user that the content author would like that orientation shown. I actually am really curious about this because it might actually be under-supported.

It's easy enough to add a portrait one if you feel I should go that route though.

@dlazin
Copy link
Collaborator

dlazin commented Nov 22, 2021

Great tests, thanks! Should there be a separate test for explicit "portrait"? I can see a RS supporting portrait but not landscape, or in theory vice versa.Great tests, thanks! Should there be a separate test for explicit "portrait"? I can see a RS supporting portrait but not landscape, or in theory vice versa.

I did consider this, but I think what I want to focus on is the RS behaviour as described in the spec, and portrait or landscape, the SHOULD focuses on the RS either displaying in that orientation or informing the user that the content author would like that orientation shown. I actually am really curious about this because it might actually be under-supported.

It's easy enough to add a portrait one if you feel I should go that route though.

OK, up to you!

Copy link
Collaborator

@dlazin dlazin left a comment

Choose a reason for hiding this comment

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

Note that you have a merge conflict on .DS_Store :(

<dc:coverage>Fixed Layout</dc:coverage>
<dc:creator>Wendy Reid</dc:creator>
<dc:date>2021-11-18</dc:date>
<dc:description>rendition:orientation is landscape, Reading Systems should either display the content in landscape or inform the user it should be.</dc:description>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: comma splice! Either use a period, colon, or semi-colon instead of the comma, or add a word like "so".

Copy link
Collaborator

Choose a reason for hiding this comment

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

And while we're at it, I prefer your wording on the pages themselves: "inform the user it should be viewed in landscape."

@wareid wareid merged commit a4ef576 into main Nov 23, 2021
@wareid wareid deleted the fxl-properties3 branch November 23, 2021 19:14
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.

None yet

2 participants