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

Lifecycle integration #130

Merged
merged 13 commits into from
Feb 8, 2018
Merged

Lifecycle integration #130

merged 13 commits into from
Feb 8, 2018

Conversation

HadrienGardeur
Copy link

@HadrienGardeur HadrienGardeur commented Feb 5, 2018

This pull request covers all the lifecycle changes that were previously drafted in a separate Markdown document:

  • WebIDL definitions in an appendix
  • the lifecycle itself (obtaining a manifest, processing a manifest and processing the default reading order)
  • additions and changes to affordances (previously called reading enhancements)

Preview | Diff

Hadrien Gardeur added 8 commits January 29, 2018 15:31
@HadrienGardeur
Copy link
Author

I just linked my W3C account to my GitHub account to solve the IPR check.

@llemeurfr
Copy link
Contributor

It appears that adapting the precise processing model of WAM / "Obtaining the manifest" to the Web Publication context is doable in practice, thanks to Hadrien. I feel some wordings obscure in the WAM spec (esp. 6.1 item 11), but this is another matter.

Some remarks:
a. Some HTML notions are linked to their definition in the WAM spec, but the equivalent link is missing in this draft (opaque origine, parsing, request ...). If we adopt this spec, these will have to be completed.
b. 6.2 item 5. seems using manifest["reading_order"] as an input AND an output. Isn't it "Set default reading order ..." or "Let default reading order be ..."?
c. 6.2 item 2 / 3: what if navigation document link is null, what if its href member is empty?
d. 7.2, list of publications. Could we be more illustrative and speak about "publication-shelf" or equivalent?
e. 7.3.2: it should be clear that standard user setting persist from one book to another, i.e. are defined at the level of the user agent, not internal to the publication (even if some publication authors want to embed advanced user settings in the publication itself, I'm thinking about some ebooks targeting dyslexic people).
f. 7.3.4, a typo was introduced -> oneshould
g. 7.4.2: I wonder if many reading apps asks for the user if he want to come back to the first page ...

@HadrienGardeur
Copy link
Author

Some HTML notions are linked to their definition in the WAM spec, but the equivalent link is missing in this draft (opaque origine, parsing, request ...). If we adopt this spec, these will have to be completed.

Well, it's actually the opposite. If we adopt the WAM, we can drop this section since it'll be redundant with it.

b. 6.2 item 5. seems using manifest["reading_order"] as an input AND an output. Isn't it "Set default reading order ..." or "Let default reading order be ..."?

This looks OK to me and consistent with other examples in WAM where you need the current value to set the final one.

c. 6.2 item 2 / 3: what if navigation document link is null, what if its href member is empty?

Do you mean 6.3?
If they're respectively null and empty, then we attempt to look for a nav element on the document where we obtained a manifest.

d. 7.2, list of publications. Could we be more illustrative and speak about "publication-shelf" or equivalent?

A shelf is IMO very book centric, I'd rather use the term "list".

e. 7.3.2: it should be clear that standard user setting persist from one book to another, i.e. are defined at the level of the user agent, not internal to the publication (even if some publication authors want to embed advanced user settings in the publication itself, I'm thinking about some ebooks targeting dyslexic people).

I disagree, not all reading systems do that and it should entirely up to the user agent to decide that.

f. 7.3.4, a typo was introduced -> oneshould

Should be fixed in the latest commit.

g. 7.4.2: I wonder if many reading apps asks for the user if he want to come back to the first page ...

In an EPUB reading app, you can't discover a publication through its fifth chapter, so you always start from the beginning.

A WP is different, if I discover a publication though its fifth chapter, it makes sense to let the user decide between reading from the beginning, or continuing from the current position.

Copy link
Member

@iherman iherman left a comment

Choose a reason for hiding this comment

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

  • In 6.1, it is not clear what "Document" refers to. I presume this is, actually, the HTML content referred to in section 5.1; it is worth making a link there.

  • The Wiki page explored the fact that there may be a difference between a browser and a dedicated app. I believe that is something that should be brought into the document. It greatly affects sections 7.1 and 7.2. Indeed:

    • I am not sure whether a publication mode is appropriate when it is in a browser. It may, of course (just like the reader mode) but it is not. Ie, SHOULD is a bit too strong for my taste.
    • The same reservations for 7.2. For a purely browser view, when I look at a set of scholarly papers in a journal, there is no reason why I would get a list of publications. Again, SHOULD is way too strong

    That being said, 7.1 and 7.2 are perfectly fine for separate apps.

  • There are some features that are, at this point, without prior definitions (like "publication context"). It is fine to have them there, but an editorial comment pointing out that this is still a new concept should be added.

  • (minor thing) in A.3. I believe the default for "lang" is "und". If we give the default value for 'dir', that should also appear.

@HadrienGardeur
Copy link
Author

@iherman the publication mode is meant to implement affordances listed in presentation and navigation. If you don't think that browsers SHOULD implement those, what are your expectations for a browser?

@iherman
Copy link
Member

iherman commented Feb 5, 2018

@HadrienGardeur

the publication mode is meant to implement affordances listed in presentation and navigation.

I am fine with that, but the document should make this connection explicit. At this moment, it is implicit.

If you don't think that browsers SHOULD implement those, what are your expectations for a browser?

The point is: it is very much dependent on the type of publication. To use the same example as before: if I am a reader of a scientific journal article in a browser, the most important thing I expect is that I can continue reading the article, including all the figures, tables, videos, what not, when I am offline, because I am supposed to, say, review the paper while I am on a plane. I may not interested in the list of publications or, in a browser, using an existing bookmarking is all what I need.

I am fine with a MAY for all those. SHOULD means something like "you are expected to implement this unless you hit some major problems when doing so". For me, this is too strong.

@HadrienGardeur
Copy link
Author

@iherman

I am fine with that, but the document should make this connection explicit. At this moment, it is implicit.

I'll quote the updated draft:

Publication mode is a display mode implemented by the User Agent that follows the conventions listed in presentation and navigation.

Do you want me to update that statement?

The point is: it is very much dependent on the type of publication. To use the same example as before: if I am a reader of a scientific journal article in a browser, the most important thing I expect is that I can continue reading the article, including all the figures, tables, videos, what not, when I am offline, because I am supposed to, say, review the paper while I am on a plane. I may not interested in the list of publications or, in a browser, using an existing bookmarking is all what I need.

But I don't think that we can trigger specific affordances based on the nature of the publication. Which means that this sections should list all affordances and give them a different level of importance using MUST/SHOULD/MAY.

Right now:

  • publication mode is a SHOULD
  • list of publications is now a MAY

Publication mode contains the following requirements:

  • affordance to go backward/forward in the default reading order is a MUST
  • user settings are a SHOULD
  • progression related items are a SHOULD
  • TOC is a SHOULD

Offline access and search are still empty.

@iherman
Copy link
Member

iherman commented Feb 5, 2018

@HadrienGardeur,

I'll quote the updated draft:

Publication mode is a display mode implemented by the User Agent that follows the conventions listed in presentation and navigation.

Do you want me to update that statement?

That is fine. My problem was that there is no relationship between this and the rest of the document. When does a browser "enter" such a publication mode? Is it the same as the "publication context" in 6.1, for example?

(Note that I do not expect you to provide a full blown answer. Just flagging in the document that this is a point that has to be specified would do it for now...)

@iherman
Copy link
Member

iherman commented Feb 5, 2018

Oops. I realize this sentence:

When a User Agent obtains a manifest it SHOULD display an affordance for switching the display to publication mode.

Which says what I was looking for. Careless of me:-)

@iherman
Copy link
Member

iherman commented Feb 5, 2018

@HadrienGardeur I agree with you that it should be in terms of SHOULD/MAY.

I propose to leave it as it is now (thanks for changing the list of publication to a MAY) and let it decide in a general discussion in the WG

@rdeltour
Copy link
Member

rdeltour commented Feb 6, 2018

I don’t believe we should include sections "Obtaining a manifest" and "Processing a manifest" in our draft. It looks like we’re monkey patching the Web App Manifest spec, when we’ve not yet decided whether and how we can extend it.

I appreciate that the value of defining/adapting these algorithms in the context of Web Publications (great and helpful work from Hadrien 👍), but I believe this should stay in the wiki; adding that to a draft is not the right approach IMHO.

@HadrienGardeur
Copy link
Author

@rdeltour our infoset currently relies on a different rel value. Which means that right now we need to have a specific section for obtaining a manifest, what's in the WAM won't work with rel=publication.

Processing a manifest would also be required, even if we go down the road of adopting the WAM since we need to process our extensions to it. It would probably read a little differently, but we still need to convert JSON into the WebIDL dictionary and process the default reading order.

I think that removing those two sections would be a mistake, but we can add editorial notes about them.

@iherman
Copy link
Member

iherman commented Feb 6, 2018

@HadrienGardeur @rdeltour : I think adding an editorial note is a good idea. Let us make it clear that, in case we go with the WAM, these sections would disappear.

@rdeltour
Copy link
Member

rdeltour commented Feb 6, 2018

@rdeltour our infoset currently relies on a different rel value. Which means that right now we need to have a specific section for obtaining a manifest, what's in the WAM won't work with rel=publication.

That’s right, but it would be more obvious to the reader if it was explained like that in a note, rather than implied in the monkey-patched algorithm :-)

Processing a manifest would also be required, even if we go down the road of adopting the WAM since we need to process our extensions to it. It would probably read a little differently, but we still need to convert JSON into the WebIDL dictionary and process the default reading order.

Right as well. 6.3 would likely stay (+ processing sections for any other extensions).

I think that removing those two sections would be a mistake, but we can add editorial notes about them.

My comment was really only about 6.1 and 6.2, which would mostly disappear (or would look 99% different) if we end up using WAM.

But I will happily settle for an editorial note for now. 😄

(and just to clarify once more: my comment is absolutely not about dismissing the value of writing these; I was just concerned about sending the wrong message about us intending to monkey patch other specs).

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.

Just a few minor comments that came up when I read the "processing the reading order" section.
Except the typo, feel free to ignore at this early stage.

index.html Outdated
Let <var>document</var> be the result of UTF-8 decoding <var>response</var>'s <a data-cite="!FETCH#concept-request-body">body</a>.
</li>
<li>
If <var>document</var> is not a valid [[!HTML]] document, terminate this algorithm.
Copy link
Member

Choose a reason for hiding this comment

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

remove "valid"?

Copy link
Author

Choose a reason for hiding this comment

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

Fixed in latest commit.

index.html Outdated
If <var>document</var> is not a valid [[!HTML]] document, terminate this algorithm.
</li>
<li>
Let <var>default reading order</var> be the result of processing <var>text</var>:
Copy link
Member

Choose a reason for hiding this comment

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

typo: text should be document

Copy link
Author

Choose a reason for hiding this comment

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

Fixed in latest commit.

<li>
If <var>document</var> contains a <code>nav</code> element:
<ol>
<li>Extract a list of resource paths referenced from the <code>href</code> attribute of all <code>a</code> elements.</li>
Copy link
Member

Choose a reason for hiding this comment

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

agree with the logic, but at some point we’ll need to define it a bit more accurately (e.g. "resource paths" sounds ambiguous wrt to the URL standard, probably add "in document order", etc).

Copy link
Author

Choose a reason for hiding this comment

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

For now, I've mostly re-used what we had in the infoset for the default reading order.

I really dislike our current status quo, and would rather drop the ability of defining the default reading order in an HTML document than continue to make this algorithm even more complex than it is.

<li>Extract a list of resource paths referenced from the <code>href</code> attribute of all <code>a</code> elements.</li>
<li>Strip any fragment identifiers from the references.</li>
<li>Resolve all relative paths to full URLs.</li>
<li>Remove all consecutive references to the same resource, leaving only the first.</li>
Copy link
Member

Choose a reason for hiding this comment

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

consecutive meaning that the same resource can appear twice if references to it are not consecutive?

Copy link
Author

Choose a reason for hiding this comment

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

Same comment as above.

@HadrienGardeur
Copy link
Author

Quick note for the editors (@iherman and @mattgarrish): until Monday, the IDL part of this pull request looked fine, but now it seems that the IDL definitions are not correctly formatted.

Nothing changed in the document itself, so I'm not entirely sure what triggered this issue with ReSpec.

@iherman
Copy link
Member

iherman commented Feb 6, 2018

@HadrienGardeur it may have been a temporary glitch in respec. If I look a the preview it looks o.k. now...

@HadrienGardeur
Copy link
Author

Any update on this?

If it's good enough as-is, it would be nice to have it merged before the next meeting (Monday) to discuss it further.

@mattgarrish
Copy link
Member

One problem I noticed is that there are changes in the case of reference identifiers. [[HTML]] is a reference to the WHATWG spec, which is why we've been using lowercase for these. I can fix this back up after merging, though.

@HadrienGardeur
Copy link
Author

@mattgarrish done

@mattgarrish mattgarrish merged commit f01812c into master Feb 8, 2018
@mattgarrish mattgarrish deleted the lifecycle branch February 8, 2018 15:02
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.

5 participants