In the media dialog, gallery dialog, and possibly more, some fieldset layouts contain widgets, with no field layout in between.
The documentation says this shouldn't be done, and should even throw an error - but clearly no error is thrown.
Should we (i) make sure the visual editor components all follow this rule, and (ii) enforce it in the code?
Should we (i) make sure the visual editor components all follow this rule (…)?
I agree that ve.ui.MWMediaDialog should use FieldLayouts to wrap and labels its widgets, rather than FieldsetLayouts. I never understood that design decision.
The documentation says this shouldn't be done, and should even throw an error - but clearly no error is thrown.
Should we (…) (ii) enforce it in the code?
I think that would have to be treated as a breaking change in OOUI… probably not worth it. I corrected the documentation instead: https://www.mediawiki.org/w/index.php?title=OOUI/Layouts/Fields_and_Fieldsets&diff=2896269&oldid=2689171&diffmode=source
Even if we did enforce it, that wouldn't affect the VE code – we could only enforce it for the addItems() methods and items config option, but ve.ui.MWMediaDialog doesn't use them, it just appends stuff to the .$element and we can't prevent that.
I agree that ve.ui.MWMediaDialog should use FieldLayouts to wrap and labels its widgets, rather than FieldsetLayouts.
Ok, I can go through and correct those. For now I could put field layouts between the fieldset layouts and the widgets, so it's at least correct (I already had to do this in the media size widget to correct the layout), then in the future the design could be reviewed properly, to decide whether we should even have the fieldset layouts.
Even if we did enforce it, that wouldn't affect the VE code – we could only enforce it for the addItems() methods and items config option, but ve.ui.MWMediaDialog doesn't use them, it just appends stuff to the .$element and we can't prevent that.
While I'm doing this, do you think we should be using addItems() and the config option instead?
No need, I think we can basically replace the FieldsetLayouts with FieldLayouts? I'll just make the patch, easier than explaining.
Change 463304 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/VisualEditor@master] [WIP] ve.ui.MWMediaDialog: Replace FieldsetLayout with FieldLayout (make labels smaller)
Thanks! I've commented on the patch - I think the fieldsets work semantically (unless I've misunderstood something) - particularly on the advanced tab.
Change 463304 abandoned by Bartosz Dziewoński:
ve.ui.MWMediaDialog: Replace FieldsetLayout with FieldLayout (make labels smaller)
Reason:
OK, I don't feel strongly about this either, it was just a demo of an idea.
Yeah, I think we could do this. I don't think the current situation with widgets being nested directly inside fieldset layouts is problematic, but it would probably be neater to have field layouts there.
Change 464421 had a related patch set uploaded (by Tchanders; owner: Tchanders):
[mediawiki/extensions/VisualEditor@master] Use correct OOUI hierarchy in media dialog
Change 464433 had a related patch set uploaded (by Tchanders; owner: Tchanders):
[mediawiki/extensions/VisualEditor@master] Use correct OOUI hierarchy in gallery dialog
While looking into this, I noticed the spacing between elements is affected by whether we have fieldset->widget or fieldset->field->widget. It's also affected by whether we use addItems() or append one element to the other. We should use these components properly to get a predictable appearance, and if there's a need to deviate, we should comment why.
Change 464421 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] Use correct OOUI hierarchy in media dialog
Change 464433 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] Use correct OOUI hierarchy in gallery dialog
@Ryasmeen We've only wanted you to check that we didn't break anything in the dialog ;), it should look and behave just like before (other than some margins being changed by a few pixels).