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

Whitespaces are correctly handled in bookmark labels #607

Closed
gsnedders opened this issue Mar 29, 2018 · 4 comments
Closed

Whitespaces are correctly handled in bookmark labels #607

gsnedders opened this issue Mar 29, 2018 · 4 comments
Labels
bug Existing features not working as expected
Milestone

Comments

@gsnedders
Copy link

Something like:

        <h1>
            <span>Foo</span>,
            <span>
                Bar
            </span>
        </h1>

Seems to get turned into a bookmark with a label of:

            Foo,
            
                Bar

This is… unexpected, as it doesn't match the rendering at all. (i.e., I'd expect Foo, Bar with no more whitespace.)

I guess technically we probably want to take the white-space property into account here?

@liZe liZe added the bug Existing features not working as expected label Apr 3, 2018
@Tontyna
Copy link
Contributor

Tontyna commented Jul 2, 2018

@gsnedders -- where does whitespace in bookmark labels distract you? My PDF reader's bookmark-tree is normalized by the readers, never ever realized that there was superfluous whitespace at all.
And why/where would the white-space property be of use?

Could create a PR which removes the whitespace, like that:

bookmark_label = ' '.join(bookmark_label.split())

@gsnedders
Copy link
Author

gsnedders commented Jul 2, 2018

@Tontyna In Okular they aren't normalised.

As for white-space, I was thinking of something like:

<h1><pre>foo
bar</pre></h1>

where it would seem semantically wrong to collapse the whitespace (and, of course, pre gets that behaviour purely based on the white-space property).

That said, that's mostly a hypothetical case; collapsing whitespace always would seem better than current given having a descendant of a heading with white-space set to something that isn't normal is decidedly unusual.

@Tontyna
Copy link
Contributor

Tontyna commented Jul 3, 2018

Not shure how to deal with this. The current implementation with all the space from the source included, conforms (almost) to the definition of textContent. Normalizing the whitespace and taking white-space into account would be innerText aka the element's text content "as rendered".

But I think none of the above is correct, the bookmark-label should contain the content(text) of the element, specified as

The string value of the element, determined as if white-space: normal had been set. This is the default value.

But. WeasyPrint's implementation of content(text) doesnt normalize whitespace. I'd say: Thats why @liZe added the bug-label. Am I right?

@liZe liZe changed the title Whitespace should be collapsed in bookmark labels Whitespaces are correctly handled in bookmark labels Nov 15, 2018
@liZe
Copy link
Member

liZe commented Oct 12, 2021

collapsing whitespace always would seem better than current given having a descendant of a heading with white-space set to something that isn't normal is decidedly unusual.

The spec actually says that white spaces have to be handled by CSS properties in <content-list>. A bit strange, but this case won’t happen in real cases.

The solution is simply to normalize white spaces before calculating bookmark labels (and named strings).

@liZe liZe closed this as completed in e5beff6 Oct 13, 2021
@grewn0uille grewn0uille added this to the 54.0 milestone Oct 13, 2021
liZe added a commit that referenced this issue Oct 13, 2021
This "hack" was introduced to ignore the text-transform property for bookmarks.
Now that the whitespace management logic is done after the box creation (just
before building the bookmark label), we can do the same for the text
transformation (just after building the bookmark label).

Related to #607 and #137.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Existing features not working as expected
Projects
None yet
Development

No branches or pull requests

4 participants