Skip to content

Add zoomable images feature#3127

Open
GuillaumeGomez wants to merge 7 commits into
rust-lang:masterfrom
GuillaumeGomez:zoom-in
Open

Add zoomable images feature#3127
GuillaumeGomez wants to merge 7 commits into
rust-lang:masterfrom
GuillaumeGomez:zoom-in

Conversation

@GuillaumeGomez

@GuillaumeGomez GuillaumeGomez commented May 26, 2026

Copy link
Copy Markdown
Member

Fixes #2075.

The "zoomed-in" image looks like this:

image

The JS is only used to "zoom out" images with the "escape key", otherwise the feature works entirely without JS.

It works as follows: there is a (hidden) checkbox right before the image. The image itself is inside the label associated to the checkbox, meaning that when you click on the image, you switch the value of the checkbox. And thanks to :checked CSS selector, it changes the display of the zoomed out image.

To make it obvious that you can "zoom in"/"zoom out" on an image, I changed the cursor to look like a loop using the CSS cursor: zoom-in|zoom-out property.

I also realized that images had the alt attribute, but not the title one, so I added it while I was at it.

@GuillaumeGomez GuillaumeGomez requested a review from ehuss May 26, 2026 16:14
@GuillaumeGomez GuillaumeGomez force-pushed the zoom-in branch 2 times, most recently from e172cff to 4d67425 Compare May 26, 2026 16:19
@rustbot rustbot added the S-waiting-on-review Status: waiting on a review label May 26, 2026
@GuillaumeGomez

Copy link
Copy Markdown
Member Author

Updated failing tests as well, ready for review. :)

@GuillaumeGomez

Copy link
Copy Markdown
Member Author

ping @ehuss

@ehuss

ehuss commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Won't this break images that are links?

@GuillaumeGomez

Copy link
Copy Markdown
Member Author

I think it would yes. Didn't think about that. If I only allow to zoom on non-link images, does it sound good to you?

@shenef

shenef commented Jun 8, 2026

Copy link
Copy Markdown

The description mentions the escape key for zooming back out, how would that work on a mobile device? Or does tapping it again toggle off the invisible checkbox?
I have a case where i currently use an image with a link to open a separate full-size version of the image. (740x638 -> 3712x3200)
So with this feature there would be no more need to do that.

@GuillaumeGomez

Copy link
Copy Markdown
Member Author

The description mentions the escape key for zooming back out, how would that work on a mobile device? Or does tapping it again toggle off the invisible checkbox?

Tapping again will zoom-out.

@rustbot

rustbot commented Jun 8, 2026

Copy link
Copy Markdown
Collaborator

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@GuillaumeGomez

Copy link
Copy Markdown
Member Author

@ehuss Disabled the feature on images inside links and extended test to check it as well.

@ehuss ehuss left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks!

I'm not going to block on it, but this doesn't seem to support the ability to trigger with the keyboard. Generally we try to ensure to follow accessibility guidelines as much as we can.

Just curious, was it intentional that you can still scroll when the image is zoomed in? (I'm not sure if I have a preference, just checking.)

I'm slightly uneasy not having a way to turn this off. I'm fine with starting without it, but if people have issues, we may need to add something. In particular, small images like icons probably don't need or want to be zoomed in this way.

View changes since this review

Comment thread crates/mdbook-html/src/html/tree.rs Outdated

let mut label = Element::new("label");
label.insert_attr("class", "checkbox-label".to_tendril());
label.insert_attr("for", img_input_id.to_tendril());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The IDs end up getting broken on the print page because it rebuilds the IDs to be unique. I think the "for" here ends up being the wrong value in that case.

I'm not sure how to fix that, other than the print page also handling this attribute.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It's the print page, so I don't think it matters in this case, no?

Comment on lines +578 to +579
} else {
img.insert_attr("title", alt.into());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why was this fallback to add the alt as the title added?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I mentioned it in my first comment:

I also realized that images had the alt attribute, but not the title one, so I added it while I was at it.

It's better for accessibility, and for other users, to have text when they hover an image without moving for a while.

label.insert_attr("for", img_input_id.to_tendril());
self.push(Node::Element(label));

self.append(Node::Element(img.clone()));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm a little concerned about this approach of using a duplicated <img> tag. Screen readers may have difficulty handling this. I think at a minimum it should have aria-hidden="true". I'm a bit uncomfortable with this approach in general, though I'm willing to give it a try.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think screen readers all handle display: none, so I think we're fine (although if we could test it, would be even better). However, won't aria-hidden=true be an issue when the image is zoomed-in?

Comment thread crates/mdbook-html/src/html/tree.rs Outdated

self.append(Node::Element(img));

// We exit the `label` and the `div`.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This comment seems off. There is no div AFAIK.

Perhaps it means:

Suggested change
// We exit the `label` and the `div`.
// We exit the `label` and the `span` (which used push_no_stack).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed.

margin-right: 8px;
}

.content .checkbox-img, .checkbox-label > .img-wrapper {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you add comments explaining what these rules are for and what they do? It can be just a short comment on each selector to explain what it's for (image zoom) and what that selector is doing.


(function chapterNavigation() {
function zoomOutImages() {
for (const elem of Array.from(document.querySelectorAll('input.checkbox-img'))) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I believe this can be simplified:

Suggested change
for (const elem of Array.from(document.querySelectorAll('input.checkbox-img'))) {
for (const elem of document.querySelectorAll('input.checkbox-img')) {

@GuillaumeGomez GuillaumeGomez Jun 22, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It can, but since we make modifications to the DOM, there is a non-zero chance that the selector list will recompute itself between each items (that was a huge issue in older rustdoc when we used it to generate [-] buttons).

Comment thread tests/gui/image-zoom.goml
call-function: ("check-image-zoomed-in", {})

// We click on the wrapper to "zoom out".
click: ".checkbox-label > img"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is it intentional that this clicks on the image underneath the wrapper? That is, should this be something like ".image-wrapper > img" or ".image-wrapper" to simulate what the user would actually click on?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Well, they click on the image, but the image being part of the label, since there is no preventDefault() on the image, the event is propagated to the parent until the parent is the label which then handles the click event and stops its propagation. So it is how users will interact with it.

Comment thread crates/mdbook-html/src/html/tree.rs Outdated
}

/// Returns the current `img_label_id` and increase its value.
fn get_current_img_label_id(&mut self) -> usize {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just a very minor nit, but this method name seemed a little confusing to me since it has a side effect. Perhaps something like get_next_img_label_id would be a little clearer?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

👍

@ehuss

ehuss commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

Also, it's probably a good idea to include at least some note in the documentation that this exists. Perhaps on the Images section?

@GuillaumeGomez

Copy link
Copy Markdown
Member Author

I'm not going to block on it, but this doesn't seem to support the ability to trigger with the keyboard. Generally we try to ensure to follow accessibility guidelines as much as we can.

I was sure I tested that, I'll double-check and add a regression test to ensure it works.

Just curious, was it intentional that you can still scroll when the image is zoomed in? (I'm not sure if I have a preference, just checking.)

I didn't consider it an issue but I can do it if you want as even with JS (it's required for that as I need to change a parent's style, so cannot be done with only CSS).

I'm slightly uneasy not having a way to turn this off. I'm fine with starting without it, but if people have issues, we may need to add something. In particular, small images like icons probably don't need or want to be zoomed in this way.

I can add a CLI option for that if you want? As for small images, if I want to know their size, I would need another dependency for that. Not sure it's worth it but again, your call.

@GuillaumeGomez

Copy link
Copy Markdown
Member Author

I simplified the DOM generation: no need for an ID anymore. We can now make the zoom in feature work with the keyboard (still no JS, works with spacebar). Added a GUI test for it as well. And mentioned it in the mdbook guide.

I didn't add a new CLI option to disable it yet. Waiting for you to confirm if you want it or not.

@GuillaumeGomez

Copy link
Copy Markdown
Member Author

Updated tests with HTML comparisons. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review Status: waiting on a review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Zoomable Images

4 participants