latex3 / hyperref

Hypertext support for LaTeX
169 stars 36 forks source link

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

Open boritomi opened 8 months ago

boritomi commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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. https://github.com/latex3/latex2e/pull/1301.

boritomi commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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