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

Improve handling of URI fragment identifiers #5630

Merged
merged 2 commits into from Dec 4, 2018

Conversation

@AlbertHilb
Copy link
Member

@AlbertHilb AlbertHilb commented Nov 15, 2018

Fixes #5599, #4190.

Supersedes #5613.

This adds handleFragment method to IDocumentWidget and implements fragment handling for markdown and PDF documents, and notebooks.

Cheers

@jasongrout jasongrout added this to the 1.0 milestone Nov 15, 2018
@ian-r-rose ian-r-rose self-assigned this Nov 19, 2018
Copy link
Member

@ian-r-rose ian-r-rose left a comment

Thanks @AlbertHilb. I have a few questions/comments, but I like your overall strategy here. Did you find that this worked well for PDF documents as well? Did you notice any compatibility issues between browsers there?

/**
* The `id` of the element should be scrolled into view.
*/
fragment: string | null;
Copy link
Member

@ian-r-rose ian-r-rose Nov 19, 2018

Choose a reason for hiding this comment

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

Can we make this an optional parameter instead of string | null?

Copy link
Member

@ian-r-rose ian-r-rose Nov 19, 2018

Choose a reason for hiding this comment

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

I think the fragment parameter can be moved into the renderedHTML options, since they are both dealing with rendered DOM content. Then we can use fragments for both html content and markdown content.

Copy link
Member Author

@AlbertHilb AlbertHilb Nov 20, 2018

Choose a reason for hiding this comment

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

Can we make this an optional parameter instead of string | null?

Yes. I made it string | null for coherence with the other optional interface members.

I think the fragment parameter can be moved into the renderedHTML options

It would be an excellent idea if renderMarkdown used renderHTML to put the HTML code into the host element. But, if I'm right, it does not at the moment.

@@ -398,6 +399,14 @@ export function renderMarkdown(
if (shouldTypeset && latexTypesetter) {
latexTypesetter.typeset(host);
}
})
.then(() => {
Copy link
Member

@ian-r-rose ian-r-rose Nov 19, 2018

Choose a reason for hiding this comment

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

I think this .then clause can be consolidated with the one above, since it resolves more or less immediately (the typesetting is asynchronous, but we don't wait on it).

Copy link
Member Author

@AlbertHilb AlbertHilb Nov 20, 2018

Choose a reason for hiding this comment

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

OK. If we don't wait for LaTeX typesetting, we can join the two clauses.

@@ -864,6 +864,7 @@ export class MarkdownCell extends Cell {
this._updateRenderedInput().then(() => {
this._ready.resolve(void 0);
});
this.renderInput(this._renderer);
Copy link
Member

@ian-r-rose ian-r-rose Nov 19, 2018

Choose a reason for hiding this comment

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

What does this change accomplish?

Copy link
Member Author

@AlbertHilb AlbertHilb Nov 20, 2018

Choose a reason for hiding this comment

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

In order to scroll into view the fragment we have to wait cell output is inserted into DOM.
However _updateRenderedInput() just generates the output without injecting it into DOM.
So, without that change, the ready promise resolves too early for our needs.

@@ -105,6 +113,7 @@ export class MimeContent extends Widget {
protected onUpdateRequest(msg: Message): void {
if (this._context.isReady) {
this._render();
this._fragment = null;
Copy link
Member

@ian-r-rose ian-r-rose Nov 19, 2018

Choose a reason for hiding this comment

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

Any particular reason to null this out (not that I see a problem with it)?

Copy link
Member Author

@AlbertHilb AlbertHilb Nov 20, 2018

Choose a reason for hiding this comment

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

_fragment value makes sense only until the document is rendered the first time after that value is set. Later the user can scroll the page, for example, and he doesn't want it jumps back to the old position on following rerenderings.

@AlbertHilb
Copy link
Member Author

@AlbertHilb AlbertHilb commented Nov 20, 2018

Hi @ian-r-rose. I tested this new functionality on Firefox and Chrome for Ubuntu. Everything seems to work fine, except for a (minor) issue with the PDF viewer integrated in Chrome: it recognizes fragment of type #page=x, but not of type search=x.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Nov 21, 2018

Caught by the linter:

ERROR: packages/docregistry/src/default.ts[469, 42]: block is empty.

You can put a /* no-op */ comment in there to avoid the lint error.

@AlbertHilb
Copy link
Member Author

@AlbertHilb AlbertHilb commented Nov 21, 2018

Comment added.

@saulshanabrook saulshanabrook changed the title Improve handling of URI fragment indetifiers. Improve handling of URI fragment identifiers Nov 29, 2018
Copy link
Member

@ian-r-rose ian-r-rose left a comment

Thanks @AlbertHilb, this is beautiful (and sorry for the slow turnaround). I have a couple of minor requests, but I think this is in great shape.

packages/rendermime/src/widgets.ts Outdated Show resolved Hide resolved
packages/rendermime/src/renderers.ts Outdated Show resolved Hide resolved
packages/notebook/src/widget.ts Outdated Show resolved Hide resolved
packages/docregistry/src/mimedocument.ts Show resolved Hide resolved
@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Dec 3, 2018

I especially love the handling of PDF fragments.

@AlbertHilb
Copy link
Member Author

@AlbertHilb AlbertHilb commented Dec 4, 2018

Hi @ian-r-rose, don't worry for the slowdown.

I rebased this PR to remove conflicts introduced by #5674.
Following your suggestions I added console warning for malformed URI fragments and some note about fragment attribute in IMimeModel metadata documentation.
Finally, I moved the markdown fragment handling logic from rendermime/renderers to rendermime/widgets and I reshaped it in order to share code among all the widgets based on RenderedHTMLCommon.

Copy link
Member

@ian-r-rose ian-r-rose left a comment

Thanks @AlbertHilb !

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Dec 4, 2018

I am seeing some problems with how Lab is handling fragment identifiers (especially with special characters ':' and additional HTML markup), but that is not your fault, so let's move forwards with this and I'll add a new issue for that.

@ian-r-rose ian-r-rose merged commit 6185bda into jupyterlab:master Dec 4, 2018
2 of 3 checks passed
@AlbertHilb AlbertHilb deleted the HandleFragment branch Dec 4, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants