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

theorem's target anchor is sometimes placed in the heading of the previous page if heading contains a \vbox #332

Open
boritomi opened this issue Mar 15, 2024 · 6 comments

Comments

@boritomi
Copy link

boritomi commented Mar 15, 2024

Test case (pdflatex):

\documentclass{article}
\usepackage{fancyhdr}
\pagestyle{fancy}
\usepackage{hyperref}
\newtheorem{thm}{Theorem}
\textheight60pt
\begin{document}
1\\2\\3\\4\\5\\6

\begin{thm}\label{th}
Theorem text.
\end{thm}
Ref to Theorem \ref{th}.
\end{document}

Checked with recent versions, each fails since 7.00w, 7.00v (and previous versions) are OK. (Perhaps since Feb 13, 2023 [new thm implementation]?)

@u-fischer
Copy link
Member

I hate theorem anchors ;-). Every time you fix one instance another one breaks.

I don't think that one can really resolve all the problems through patches in hyperref. I will try to trigger a change in the LaTeX commands instead, so that the anchor is directly where it belongs and hyperref can stop to mess around.

\documentclass{article}
\makeatletter\let\@kernel@refstepcounter\refstepcounter\makeatother
\usepackage{fancyhdr}
\pagestyle{fancy}
\usepackage{hyperref}
\fancyhead{}
\makeatletter
\def\@thm#1#2{%
  \@kernel@refstepcounter{#1}%
  \@ifnextchar[{\@ythm{#1}{#2}}{\@xthm{#1}{#2}}}
\def\@begintheorem#1#2{\trivlist
   \item[\MakeLinkTarget{\@currentcounter}\hskip \labelsep{\bfseries #1\ #2}]\itshape}
\def\@opargbegintheorem#1#2#3{\trivlist
   \item[\MakeLinkTarget{\@currentcounter}\hskip \labelsep{\bfseries #1\ #2\ (#3)}]\itshape} 
          
\newtheorem{thm}{Theorem}
\textheight60pt
\begin{document}
1\\2\\3\\4\\5\\6

\begin{thm}\label{th}
Theorem text.
\end{thm}
Ref to Theorem \ref{th}.

\end{document}

@boritomi
Copy link
Author

I hate theorem anchors ;-). Every time you fix one instance another one breaks.

I can deeply understand that!

I think that in this case there is an uglier, but in most times working workaround for this, which also deals with cases when amsthm is loaded (as was in our case, I just inserted the fancyhdr and tested it with and without amsthm loaded – in both case hyperref fails – to simplify the MWE), i.e., inserting \phantomsection automatically for the theorems:

\usepackage{fancyhdr}
\pagestyle{fancy}
%\usepackage{amsthm}
\usepackage{hyperref}
\makeatletter
\@ifpackageloaded{amsthm}{\def\@begintheorem#1#2[#3]{%
  \deferred@thm@head{\the\thm@headfont \thm@indent
    \@ifempty{#1}{\let\thmname\@gobble}{\let\thmname\@iden}%
    \@ifempty{#2}{\let\thmnumber\@gobble}{\let\thmnumber\@iden}%
    \@ifempty{#3}{\let\thmnote\@gobble}{\let\thmnote\@iden}%
    \thm@swap\swappedhead\thmhead{#1\phantomsection}{#2}{#3}%
    \the\thm@headpunct
    \thmheadnl % possibly a newline.
    \hskip\thm@headsep
  }%
  \ignorespaces}}{\def\@begintheorem#1#2{\trivlist
   \item[\hskip\labelsep{\bfseries #1\ #2}\phantomsection]\itshape}
\def\@opargbegintheorem#1#2#3{\trivlist
   \item[\hskip\labelsep{\bfseries #1\ #2\phantomsection\ (#3)}]\itshape}}
\makeatother
\newtheorem{thm}{Theorem}
\textheight60pt
\begin{document}
1\\2\\3\\4\\5\\6

\begin{thm}\label{th}
Theorem text.
\end{thm}
Ref to Theorem \ref{th}.
\end{document}

But it is just a temporary workaround. In the long run, that would be very nice if hyperref could deal with theorems' target anchors better...

@u-fischer
Copy link
Member

inserting \phantomsection automatically for the theorems:

That is basically what my code above is doing: \item[\MakeLinkTarget{\@currentcounter}\hskip \labelsep{\bfseries #1\ #2\ (#3)}]\itshape} (and imho the target should be at the begin of the label and not at the end.)

But \MakeLinkTarget is better as it works also without hyperref.

In the long run, that would be very nice if hyperref could deal with theorems' target anchors better...

No in the long run it would be better if hyperref wouldn't have to deal with that at all. It is simply a pain to have to patch these commands from the outside. It would be much better if amsthm etc would insert the targets directly.

I have now opened a pull request to change at least the kernel definitions. latex3/latex2e#1301.

@boritomi
Copy link
Author

No in the long run it would be better if hyperref wouldn't have to deal with that at all. It is simply a pain to have to patch these commands from the outside. It would be much better if amsthm etc would insert the targets directly.

I have now opened a pull request to change at least the kernel definitions. latex3/latex2e#1301.

I have to agree, however, it seems that your patch will break amsthm and maybe other libraries redefining \@begintheorem (or maybe also \@thm, \@opargbegintheorem, etc.) (try your code with \usepackage{amsthm} in the preamble).

@u-fischer
Copy link
Member

I have to agree, however, it seems that your patch will break amsthm

No, the kernel change should not affect them as they overwrite the definitions later (I checked that ...). So they only loose the target again.

@u-fischer
Copy link
Member

with a current latex-dev (released a few day ago) this now works correctly.

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

No branches or pull requests

2 participants