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.

Sign up for GitHub

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

fix: reset title element to previous value on removal #14116

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

fix: reset title element to previous value on removal #14116

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c9f5147
fix: reset title element to previous value on removal
dummdidumm Nov 2, 2024
3265021
export $.title
Rich-Harris Nov 4, 2024
963d393
Merge branch 'main' into title-removal-fix
Rich-Harris Dec 4, 2024
2072638
fixes
Rich-Harris Dec 4, 2024
3904b8f
belt and braces
Rich-Harris Dec 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions packages/svelte/src/compiler/phases/3-transform/client/visitors/TitleElement.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,10 @@ export function TitleElement(node, context) {
context.state
);

const statement = b.stmt(b.assignment('=', b.id('$.document.title'), value));
context.state.init.push(b.stmt(b.call('$.title', value)));

if (has_state) {
const statement = b.stmt(b.assignment('=', b.id('$.document.title'), value));
context.state.update.push(statement);
} else {
context.state.init.push(statement);
}
}
12 changes: 12 additions & 0 deletions packages/svelte/src/internal/client/dom/elements/misc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { teardown } from '../../reactivity/effects.js';
import { hydrating } from '../hydration.js';
import { clear_text_content, get_first_child } from '../operations.js';
import { queue_micro_task } from '../task.js';
Expand Down Expand Up @@ -56,3 +57,14 @@ export function add_form_reset_listener() {
);
}
}

/**
* @param {string} text
*/
export function title(text) {
const previous = document.title;
document.title = text;
teardown(() => {
document.title = previous;
});
Copy link
Member

Choose a reason for hiding this comment

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

I haven't been able to break this, but it feels like we should put the setup in an effect so that there's no possibility of another component setting the title to a new value immediately before the teardown nukes it. might just be superstition

Suggested change
const previous = document.title;
document.title = text;
teardown(() => {
document.title = previous;
});
const previous = document.title;
effect(() => {
document.title = text;
return () => {
document.title = previous;
};
});

Copy link
Member Author

Choose a reason for hiding this comment

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

The effect does nothing, since text is a static value; I kept the "add document.title = .. to the shared effect" part within the compiler which is why it's updating still. The assumption is that it's rare that people have a reactive title. But we can move it into here

Copy link
Member

Choose a reason for hiding this comment

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

It's not about reactivity, it's about preventing this sequence of events from being a possibility:

  1. component A is removed and component B is created at the same moment
  2. component B sets document.title
  3. component A is removed and reverts document.title to whatever it was before A was created

That may already be impossible, hence 'superstition'

Copy link
Member Author

Choose a reason for hiding this comment

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

In which way does adding a render_effect help with this? The render effect is called synchronously, and teardown is a render effect under the hood.

Copy link
Member

Choose a reason for hiding this comment

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

was more of a belt-and-braces thing — see #14116 (comment). but it also means that if/when we introduce forking, we won't set the title until the fork is committed

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I was going on about the above suggestion. If it's a render_effect, then that's not right.

Copy link
Member

Choose a reason for hiding this comment

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

Surely render effects other than blocks wouldn't run until forks are committed? The whole point is to not update the DOM until then

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe. Too early to say for sure how that will all end up. Let's just keep it as is then.

Copy link
Member Author

Choose a reason for hiding this comment

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

But keeping it as is currently, i.e. using a render_effect for the thing instead of just a teardown does nothing, as I've just explained.

Not gonna die on this hill, maybe I don't understand what this proofs against exactly, and using teardown is a microoptimization regardless.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, no-one is updating the title at 60fps. given that there's potential upside and no real downside i reckon we can just keep things as they are here — this is ready to merge from my perspective

}
2 changes: 1 addition & 1 deletion packages/svelte/src/internal/client/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ export {
} from './dom/elements/attributes.js';
export { set_class, set_svg_class, set_mathml_class, toggle_class } from './dom/elements/class.js';
export { apply, event, delegate, replay_events } from './dom/elements/events.js';
export { autofocus, remove_textarea_child } from './dom/elements/misc.js';
export { autofocus, remove_textarea_child, title } from './dom/elements/misc.js';
export { set_style } from './dom/elements/style.js';
export { animation, transition } from './dom/elements/transitions.js';
export { bind_active_element } from './dom/elements/bindings/document.js';
Expand Down
Loading