Page MenuHomePhabricator

Dialog open on mount does not prevent background from scrolling
Closed, ResolvedPublic1 Estimated Story PointsBUG REPORT

Description

Steps to replicate the issue (include links if applicable):

  • create a App that starts out with an open Dialog
  • mount that App

What happens?:
The background still scrolls.

What should have happened instead?:
The background doesn't scroll.

Other information (browser name/version, screenshots, etc.):

I've create a minimal failing example(MFE): https://test.wikipedia.org/wiki/User:Zvpunry/codexDialogProblem.js
You can try it out by adding mw.loader.load( 'https://test.wikipedia.org/w/index.php?title=User:Zvpunry/codexDialogProblem.js&action=raw&ctype=text/javascript' ); to your common.js

Use case:
I'm writing a Statement editing interface for mobile Wikidata, which means adding edit-buttons in a lot of places that open the same dialog with different data. It doesn't seem like a great idea to already mount the whole app in a lot of different places just to have the button there that might or might not be clicked.

Current workaround:

const open = ref(false);
window.setTimeout(() => {
	open.value = true;
}, 0);

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript
CCiufo-WMF set the point value for this task to 3.Aug 21 2023, 5:08 PM

This may be addressed in T343476. We'll come back to test once that task is resolved.

Looking at this more closely, I think it's likely that T343476 will not address this bug, because this bug is specific to the dialog being initially open. The code to prevent the background from scrolling only runs when the dialog's open state changes from closed to open, but it doesn't run if the dialog starts out as being open.

We could fix this by setting { immediate: true } on that watcher. However, I don't think that initially-open dialogs are something we should encourage or support.

I'm writing a Statement editing interface for mobile Wikidata, which means adding edit-buttons in a lot of places that open the same dialog with different data. It doesn't seem like a great idea to already mount the whole app in a lot of different places just to have the button there that might or might not be clicked.

Is your concern that creating lots of copies of this dialog in the DOM would be wasteful if the dialog is never opened? If that's the main issue, would it be helpful if we changed the Dialog component so that it didn't render itself unless it was open (i.e. if it applied v-if="open" to its contents)? Alternatively, would there be a way that all these buttons could all share one dialog instance?

Current workaround:

const open = ref(false);
window.setTimeout(() => {
	open.value = true;
}, 0);

If you do need to do something like this in a Vue app, I suggest using Vue.nextTick() instead. In this case, the onMounted lifecycle hook might also work.

[...]

I'm writing a Statement editing interface for mobile Wikidata, which means adding edit-buttons in a lot of places that open the same dialog with different data. It doesn't seem like a great idea to already mount the whole app in a lot of different places just to have the button there that might or might not be clicked.

Is your concern that creating lots of copies of this dialog in the DOM would be wasteful if the dialog is never opened? If that's the main issue, would it be helpful if we changed the Dialog component so that it didn't render itself unless it was open (i.e. if it applied v-if="open" to its contents)? Alternatively, would there be a way that all these buttons could all share one dialog instance?

Mh, some large Items can have a very large amount of statements, for example France (Q142). Also, code like Vue.createMwApp( Main ).use(createPinia()).provide(...).mount( '#mountPoint' ); would run for every statement on every page view, even when nothing is edited. That doesn't seem like a good idea performance wise.

The current approach is to add the button to every statement as Codex CSS-only button, and then to only mount the Vue-app when the button is actually clicked.

I guess an alternative would be to render one app right away that then to teleports buttons to every Statement? But I'm not sure how much would be gained by that.

However, I don't think that initially-open dialogs are something we should encourage or support.

Could you elaborate on your thinking in this regard?

Mh, some large Items can have a very large amount of statements, for example France (Q142). Also, code like Vue.createMwApp( Main ).use(createPinia()).provide(...).mount( '#mountPoint' ); would run for every statement on every page view, even when nothing is edited. That doesn't seem like a good idea performance wise.

The current approach is to add the button to every statement as Codex CSS-only button, and then to only mount the Vue-app when the button is actually clicked.

I guess an alternative would be to render one app right away that then to teleports buttons to every Statement? But I'm not sure how much would be gained by that.

Ah, I see, you're enhancing something that already exists, so you can't easily put everything in one big app. Yes, I mean that you could either have one app that creates all the statement buttons (but they'd have to be teleported all over the place as you point out), or one app that just houses a single dialog that can be opened with different data, but then you'd have to communicate between these apps somehow, which Vue doesn't make easy to do.

However, I don't think that initially-open dialogs are something we should encourage or support.

Could you elaborate on your thinking in this regard?

I meant that Dialogs that are open on mount would be rare and would only happen if you're doing this kind of weird trick. It also opens the door to multiple dialogs being open at the same time, which is messy. We may try to fix this bug in Codex, but it's not a high priority, since it's a niche use case and there is a relatively simple workaround ( onMounted( () => open.value = true; );).

Is your concern that creating lots of copies of this dialog in the DOM would be wasteful if the dialog is never opened? If that's the main issue, would it be helpful if we changed the Dialog component so that it didn't render itself unless it was open (i.e. if it applied v-if="open" to its contents)? Alternatively, would there be a way that all these buttons could all share one dialog instance?

Correction: we already do this, closed dialogs are not rendered in the DOM (no longer relevant given your latest comment, but I wanted to make sure this was clear.)

AnneT lowered the priority of this task from High to Medium.Aug 28 2023, 5:08 PM

Change 955042 had a related patch set uploaded (by Catrope; author: Catrope):

[design/codex@main] Dialog: Put all on-open code together, and run it on mount if needed

https://gerrit.wikimedia.org/r/955042

AnneT subscribed.

We don't have any demos of dialogs open on mount, so I'm skipping design review and moving straight to pending release. This fix will go out with the Codex release next week @Celenduin!

We don't have any demos of dialogs open on mount, so I'm skipping design review and moving straight to pending release. This fix will go out with the Codex release next week @Celenduin!

Thank you all ❤️

Change 955042 merged by jenkins-bot:

[design/codex@main] Dialog: Put all on-open code together, and run it on mount if needed

https://gerrit.wikimedia.org/r/955042

Change 956973 had a related patch set uploaded (by Eric Gardner; author: Eric Gardner):

[mediawiki/core@master] Update Codex from v0.18.0 to v0.19.0

https://gerrit.wikimedia.org/r/956973

Change 956973 merged by jenkins-bot:

[mediawiki/core@master] Update Codex from v0.18.0 to v0.19.0

https://gerrit.wikimedia.org/r/956973