Repository navigation
fix(scss): code annotation line highlight overflow - #13429
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
This is a CSS change. CSS changes are scary, and we need stronger assurances of the before-after behavior; we don't have good regression protection here. Have you tested the following?
|
This comment has been minimized.
This comment has been minimized.
|
Thanks for the screenshots! They are very encouraging.
If that's the case, then we should be treating this as a regression, and hotfix 1.8. Do I understand this right? |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
So basically the change is removing these rules margin-left: -4em;
width: calc(100% + 4em);and using the same as the It looks scary to have this impact, and it raises a question for me about the underlying DOM change—as if Also I am seeing those rules are set in revealjs too... Is it impacted too? quarto-cli/src/resources/formats/revealjs/quarto.scss Lines 772 to 787 in 9272aaa is this the change with the scafold for code copy button that impact this? Sorry for all those question - this just warns in my head when I see CSS fix only - always wondered if we missed something in our after body processing that does post process in client or in our HTML post processor... hard to know... 🤔 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Let's do our best to be concrete here: "many things" and "not right" are not helpful terms to get us from here to there. |
This comment has been minimized.
This comment has been minimized.
f818af8 to
97d09d6
Compare
The band and the gutter band were placed from offsetTop, which is relative to the code element, but each band is positioned against another containing block. In dashboards the band sat above its lines, and in Reveal.js a negative margin pushed it out of the slide.
97d09d6 to
b72c021
Compare
cderv
left a comment
There was a problem hiding this comment.
Thanks for reworking this!
I'll push a small adjustment before merging, so no need to push on your side meanwhile.
…ghlight The band is placed from client rects relative to its offsetParent. When that offsetParent is a positioned element that scrolls, which happens with code-copy: false, the rect difference already includes the scroll, and setting it as an absolute offset applies it a second time, so the band sat above its lines by the scroll offset. Adding the container's scrollTop and scrollLeft places it back on the lines. A new Playwright case selects an annotation inside a scrolled positioned box. The light theme contrast test switches from test.fixme to test.fail so it keeps running and flags when the arrow theme contrast gap (quarto-dev#14959) is resolved, as the Playwright test conventions ask for known failures.
Reveal.js turns code-line-numbers on by default, so the slide meant to cover code without line numbers rendered numbered code too, and both slides exercised the same layout. Setting code-line-numbers="false" on that block restores the unnumbered case.
The Reveal.js slide meant for unnumbered code rendered numbered code, because Reveal.js turns code-line-numbers on by default, and the band geometry checks pass either way. Each case now checks whether its code block carries number-lines before measuring, so a fixture that drifts from its label fails instead of silently covering the wrong layout.
Description
Since v1.8.15, selecting a code annotation draws a highlight band that spills out to the left of the code block and sits above the gutter band.
The script placed the bands from
offsetTop, which is relative to the code element, but each band is positioned against a different containing block.In dashboards the band sat about 20 px too high.
In Reveal.js a negative margin pushed it off the slide.
Both bands are now placed from the line positions, converted to the CSS pixels of their own container, since Reveal.js scales slides.
The band spans the code block, line numbers included, even with
code-copy: false.htmldashboardrevealjsNew Playwright tests measure the bands against the code block and the annotated lines in
html,dashboardandrevealjs, with and without line numbers.They fail on
mainand pass in Chromium, Firefox and WebKit.One gap remains.
In the light
arrowtheme the band tint lowers the contrast of.imtokens to 4.24:1.This predates the change and is related to #14959, so the light-theme axe test is skipped with a note.
Closes #13448
Checklist
I have (if applicable):
AI-assisted PR
Note: autonomous AI agents submitting PRs without human oversight are not permitted — see the Code of Conduct.