The action should instead be enabled/disabled according to whether any changes have been made.
Description
Status | Subtype | Assigned | Task | ||
---|---|---|---|---|---|
Resolved | matmarex | T206000 Apply changes action is enabled on several dialogs before any changes are made | |||
Resolved | matmarex | T206534 The gallery dialog's Apply changes action is always enabled if there are images in the gallery |
Event Timeline
Change 471272 had a related patch set uploaded (by Tchanders; owner: Tchanders):
[mediawiki/extensions/VisualEditor@master] ve.ui.MWGalleryDialog: Improve how dialog actions are enabled
Change 471272 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] ve.ui.MWGalleryDialog: Improve how dialog actions are enabled
@matmarex: Assuming this is not on Beta cluster yet? Since, I see the "Apply changes" button being enabled, if I just open the dialog for an existing gallery.
It should be, but maybe it wasn't yet when you tested. Or maybe you just found some case where it's broken. It works as expected for me, at a glance (testing on https://en.wikipedia.beta.wmflabs.org/wiki/Canis).
Ok, there are at-least two workflows that are broken now :(
- Adding a new image to an existing gallery is not enabling the "Apply changes" button, reported: T209451
- Sometimes I am seeing the "Apply changes" button staying enabled even if there hasn't been any changes (Haven't found the exact steps yet for this, trying to).
Change 473600 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/VisualEditor@master] ve.ui.MWGalleryDialog: Update "Apply" button when image added
Changing the test priority for this one, a change that has already caused a regression needs thorough testing imho.
@Ryasmeen I can help you here! I think you should find that everything in the Options tab[1] behaves as you would expect, whereas in the Images tab, once a change has made the action stays enabled, even if you undo. This is essentially because the Options tab[1] relates to attributes on the gallery node itself, whereas the Images tab relates to descendant nodes (the gallery caption is also a descendant of the gallery).
It's not ideal, but I think it's better than what went before... I guess the task should arguably remain open until fixed properly, but having to check changes to the gallery's descendant nodes is a bit more complicated.
[1] except for the gallery caption
I assumed that disabling the "Apply" button again after changes are undone was not a requirement here, so I did not try to get it working. @Tchanders is correct that it is a bit more difficult for this dialog than for others. But I looked into it now and it actually seems feasible, so let's do it as well. I'll submit a separate patch for it (we should get the existing one merged quickly to fix the regression).
Change 473840 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/VisualEditor@master] ve.ui.MWGalleryDialog: Improve modification checks
After this new patch is merged, undoing the changes to an image caption or alt text, or to the gallery caption, or to the order of images, or removing a previously added image, will now disable the "Apply" button again.
The following cases will *not* disable the button again, and it is not feasible to implement them:
- Re-adding a previously removed image with identical options
- Changing any caption to the old value by other means than "Undo"
- Changing image caption or alt text to the old value after switching to a different image and then back
Change 473600 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] ve.ui.MWGalleryDialog: Update "Apply" button when image added
@Tchanders: Yeah, I understand your point :) My comment was however about a straightforward scenario where I just re-opened the gallery dialog and saw the "Apply Changes" button being enabled, didn't do any "undo" action before that step. It's probably a very edge case therefore finding it difficult to replicate.
@matmarex: So if I understand correctly the patch that was supposed to do all these has been reverted because it breaks other dialogs? Is it still being worked on? If so, should we keep this task open until these scenarios are all passed?
@Ryasmeen No, nothing relevant to this task was reverted, AFAIK. These cases are unfeasible to fix due to how the dialog stores its state internally (either we don't have the "old" state to compare to, or we don't have a way of comparing that ignores internal changes that don't matter), and would require more work to resolve than I'm comfortable doing on a random task not really related to what we're working on as a team.
(I would prefer filing a separate task if we want to track that work.)
All other cases will be fixed by the latest patch I submitted (https://gerrit.wikimedia.org/r/c/mediawiki/extensions/VisualEditor/+/473840). Which, by the way, is stalled: @Esanders left a -1 review that I disagreed with and I'm waiting for a reply.
Change 473840 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] ve.ui.MWGalleryDialog: Improve modification checks