Break up TextInputWidget
Closed, ResolvedPublic8 Estimated Story Points
Actions

Assigned To

Authored By

	matmarex
	Mar 19 2016, 12:21 AM

Description

TextInputWidget is massive. By line count, it's the second largest widget we have (behind SelectWidget), and it's supposed to be just a text field. This is ridiculous.

Split proposal:

TextInputWidget
- MultilineTextInputWidget (guess what this is)
- AnnotatedTextInputWidget (here we split off everything that has to do with the 'label' config option) – we could also move this out of the 'core' module, reducing its size, since it's not supported in PHP
- SearchInputWidget (here we split off everything that has to do with type: 'search' config option)

I think the three features listed above are already (mostly) mutually exclusive.

We could do this without breaking backwards compatibility by still checking for the removed config options in TextInputWidget constructor, and returning the right subclass (yes, you can return stuff from object constructors in JS, I agree this is weird, but you can).

Details

Subject	Repo	Branch	Lines +/-
Update OOUI to v0.46.0	mediawiki/core	master	+139 -85
Remove unused "multiline" configuration	oojs/ui	master	+1 -8
Update OOUI to v0.29.1	mediawiki/core	master	+484 -1 K
Update OOUI to v0.29.0	mediawiki/core	master	+451 -1 K
[BREAKING CHANGE] TextInputWidget: Drop support for multiline: true	oojs/ui	master	+17 -64
[DEPRECATING CHANGE] TextInputWidget: Move multi-line support out	oojs/ui	master	+217 -178

Customize query in gerrit

Related Objects
Search...

Status	Subtype	Assigned	Task
Resolved		• Prtksxna	T130434 Break up TextInputWidget
Resolved		Mooeypoo	T124856 NumberInputWidget is not actually an InputWidget
Resolved		• Prtksxna	T132488 Disabled NumberInputWidget takes scrolling (touchpad, scrollwheel) into account when it shouldn't
Resolved		• Prtksxna	T132489 NumberInputWidget: Triage changing `type=text` to `type=number`
Resolved		• Prtksxna	T148471 Break out search stuff in TextInputWidget into SearchInputWidget
Resolved		matmarex	T166685 Implement SearchInputWidget in PHP
Resolved		• Prtksxna	T166686 Implement MultilineTextInputWidget in PHP
Resolved		• Prtksxna	T171835 Migrate Index: pages editing interface to OO.ui.MultilineTextInputWidget
Resolved		matmarex	T169194 Work out how to fix mw.widgets.SearchInputWidget's inheritance
Resolved		• Prtksxna	T170275 Replace deprecated OO.ui.TextInputWidget({multiline: true}) with OO.ui.MultilineTextInputWidget
Resolved	PRODUCTION ERROR	• Prtksxna	T177529 Replace deprecated PHP `multiline` option by MultilineTextInputWidget and remove from codebase in JS & PHP

Event Timeline

matmarex created this task.Mar 19 2016, 12:21 AM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 19 2016, 12:21 AM

matmarex added a project: OOUI.Mar 19 2016, 12:22 AM

Jdforrester-WMF added a project: Technical-Debt.Mar 19 2016, 12:22 AM

I don't think they are necessarily mutually exclusive. The text input label can be used for validation, and so would be reasonable to add to a search input widget. Similarly with a multiline input.

you may even conceivably want a multiline search input widget, e.g. to provide autocomplete for commit messages in VE

Okay, but something being "conceivable" is a very low barrier. I can conceive of many things you'd never want to implement. I think we should shoot for "reasonable" as a minimal requirement for including something in the library ;), and frankly I'd prefer if we required it to be "common".

A TextInputWidget label, as currently implemented, must not be used for validation (as in, displaying error messages). The width of the label is subtracted from the width of the text field, and any error message could probably easily exceed it. This would be even worse for translations to other languages (English is very terse) or multiline text fields (where it still is subtracted from the width, that is of every line of text). If you need to display error messages (and not just valid/invalid state), you should wrap your TextInputWidget in a FieldLayout, then call setErrors(...) on it as needed.

Jdforrester-WMF triaged this task as Medium priority.Mar 22 2016, 6:17 PM

Jdforrester-WMF moved this task from Backlog to Reviewing on the OOUI board.

Jdforrester-WMF set the point value for this task to 8.

Single line text inputs scroll horizontally so overlap isn't necessarily an issue, and it could be fixed for multiline.

We may also use it to display other state information other than validation (e.g. allowed characters remaining)

Calling the behavior that <input type=text> has when the text is longer than available width "scrolling horizontally" is an insult to horizontal scrolling. Any case when a single-line input does this should be considered a bug.

I don't see how you'd fix it for multiline inputs without turning them into contenteditables or allowing label to overlap text. You can't have floats inside <textarea>.

We don't actually use them to display allowed characters remaining anywhere. The one place where we could (VisualEditor's save dialog) uses a different and much better design.

It sounds like you are advocating that there are no correct uses of text input inner labels? That's one way of saying it is mutually exclusive.

In T130434#2143892, @Esanders wrote:

It sounds like you are advocating that there are no correct uses of text input inner labels?

I wouldn't quite say that, although I do think that every interface using them I've seen so far would be better if it used something else; this is just a personal opinion.

Volker_E subscribed.Apr 11 2016, 7:33 PM

Change 283652 had a related patch set uploaded (by Prtksxna):
Break out parts of TextInputWidget into a new SearchInputWidget

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

gerritbot added a project: Patch-For-Review.Apr 15 2016, 1:03 PM

Change 284168 had a related patch set uploaded (by Prtksxna):
Break out parts of TextInputWidget into a new MultilineInputWidget

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

• Prtksxna added a subtask: T124856: NumberInputWidget is not actually an InputWidget.Jul 20 2016, 7:09 AM

I would really really really like to get the first part of this (new set of widgets with back-compat. code and deprecation of the old way of doing things) in 0.18.0…

I am sorry @Jdforrester-WMF, its been on my list. I'll get it done this week.

• Prtksxna created subtask T148471: Break out search stuff in TextInputWidget into SearchInputWidget.Oct 17 2016, 10:24 PM

Jdforrester-WMF moved this task from Reviewing to Next-up on the OOUI board.Nov 29 2016, 11:15 PM

On https://gerrit.wikimedia.org/r/#/c/284168/ @Esanders said

Per my comments on the bug I think there is a case for having multiline as a flag so it can be used with other sub-types.

Have we started using multiline text inputs anywhere in the last 6 months? Do you think we still need them? If yes, lets abandon that patch and revise this task.

Ping @Esanders – your feedback is needed here, it's blocking T124856 and T149644

Volker_E closed subtask T124856: NumberInputWidget is not actually an InputWidget as Resolved.Apr 24 2017, 9:34 PM

In T130434#2971529, @Prtksxna wrote:

On https://gerrit.wikimedia.org/r/#/c/284168/ @Esanders said

Per my comments on the bug I think there is a case for having multiline as a flag so it can be used with other sub-types.

Have we started using multiline text inputs anywhere in the last 6 months? Do you think we still need them? If yes, lets abandon that patch and revise this task.

Thanks for the links @Jdforrester-WMF! I think I oversimplified the question though.

In https://gerrit.wikimedia.org/r/#/c/284168/ , @Esanders wrote:

Per my comments on the bug I think there is a case for having multiline as a flag so it can be used with other sub-types.

So the question should've been if there are any widgets that are multiline text inputs and are using a sub-type other than text.
The usage in the links doesn't use any other sub-type, so I guess we're good to move forward?

I tried rebasing the patch and its taking way too long. I might submit a new one instead.

There are three combinations in question:

Multiline & Search - I think these are the least likely to ever be used together
Multiline & Annotation - The main argument here is technical (can't the text to wrap around the label) - conceptually I don't have a problem with using them together, e.g. to show a character limit, but if the width issue is seen as critical it would understandable to drop support
Search & Annotation - Excluding arguments against Annotation in general, I see no reason not to support this.

Late to the party, but I'm wondering if we do ourselves and contributors a favor by calling it “MultilineInputWidget” instead of something more common like “TextareaInputWidget“…?

Maybe "WordWrappedTextInputWidget" to be more about the "what" and less about the "how"?

Jdforrester-WMF closed subtask T148471: Break out search stuff in TextInputWidget into SearchInputWidget as Resolved.May 25 2017, 2:24 PM

In T130434#3291884, @Volker_E wrote:

Late to the party, but I'm wondering if we do ourselves and contributors a favor by calling it “MultilineInputWidget” instead of something more common like “TextareaInputWidget“…?

Makes sense to me.

I disagree, we haven't named widgets after their poorly named HTML counterparts in the past ("dropdown" not "select") so why start now?

• Prtksxna created subtask T166685: Implement SearchInputWidget in PHP.May 31 2017, 2:53 PM

• Prtksxna created subtask T166686: Implement MultilineTextInputWidget in PHP.

Change 284168 merged by jenkins-bot:
[oojs/ui@master] [DEPRECATING CHANGE] TextInputWidget: Move multi-line support out

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

Volker_E moved this task from Next-up to Reviewing on the OOUI board.Jun 10 2017, 1:41 PM

Jdforrester-WMF mentioned this in T169194: Work out how to fix mw.widgets.SearchInputWidget's inheritance.Jun 29 2017, 10:26 AM

Jdforrester-WMF created subtask T169194: Work out how to fix mw.widgets.SearchInputWidget's inheritance.

Jdforrester-WMF closed subtask T169194: Work out how to fix mw.widgets.SearchInputWidget's inheritance as Resolved.Jun 30 2017, 11:15 AM

Opening a Gallery dialog in VisualEditor also triggers this one:

TextInputWidget: config.multiline is deprecated. Use the MultilineTextInputWidget instead. See T130434 for details.

OoUiTextInputWidget

VeUiWhitespacePreservingTextInputWidget

ve.ui.MWExtensionWindow.initialize

ve.ui.MWGalleryDialog.initialize

So, important note about returning things from an object constructor: it breaks OOjs subclassing unless the subclasses are written to expect it. (And nobody does that.) This caused T169272, which was the issue Krinkle mentioned in the previous comment.

Hmm, I suppose we never tested this magical deprecation with custom subclasses.

I think by now this is spilled milk. But we should keep this in mind for the future and do any other deprecations the boring way (by just keeping all the old code in place, unchanged except for a deprecation warning).

Volker_E created subtask T170275: Replace deprecated OO.ui.TextInputWidget({multiline: true}) with OO.ui.MultilineTextInputWidget.Jul 11 2017, 2:15 PM

Volker_E closed subtask T170275: Replace deprecated OO.ui.TextInputWidget({multiline: true}) with OO.ui.MultilineTextInputWidget as Resolved.Jul 17 2017, 5:40 PM

Krinkle removed a project: Patch-For-Review.Jul 18 2017, 12:27 AM

Tpt added a subtask: T171835: Migrate Index: pages editing interface to OO.ui.MultilineTextInputWidget.Jul 27 2017, 10:58 AM

rugk mentioned this in T176066: JavaScript freeze/loop when editing or adding references.Sep 16 2017, 9:57 PM

Volker_E created subtask T177529: Replace deprecated PHP `multiline` option by MultilineTextInputWidget and remove from codebase in JS & PHP.Oct 5 2017, 8:00 PM

Krinkle unsubscribed.Oct 5 2017, 8:39 PM

Volker_E closed subtask T166686: Implement MultilineTextInputWidget in PHP as Resolved.Oct 6 2017, 8:26 PM

Volker_E closed subtask T166685: Implement SearchInputWidget in PHP as Resolved.Oct 10 2017, 10:38 PM

matmarex closed subtask T177529: Replace deprecated PHP `multiline` option by MultilineTextInputWidget and remove from codebase in JS & PHP as Resolved.Oct 19 2017, 6:34 PM

Jdforrester-WMF moved this task from Reviewing to Waiting for breaking release on the OOUI board.Jan 8 2018, 7:42 PM

Change 463122 had a related patch set uploaded (by Jforrester; owner: Jforrester):
[oojs/ui@master] [BREAKING CHANGE] TextInputWidget: Drop support for multiline: true

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

gerritbot added a project: Patch-For-Review.Sep 26 2018, 6:14 PM

Change 463122 merged by jenkins-bot:
[oojs/ui@master] [BREAKING CHANGE] TextInputWidget: Drop support for multiline: true

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

Jdforrester-WMF closed subtask T171835: Migrate Index: pages editing interface to OO.ui.MultilineTextInputWidget as Resolved.Oct 2 2018, 12:05 AM

Jdforrester-WMF moved this task from Waiting for breaking release to OOUI-0.29.0 on the OOUI board.

Jdforrester-WMF edited projects, added OOUI (OOUI-0.29.0); removed OOUI.

Jdforrester-WMF closed this task as Resolved.Oct 2 2018, 12:16 AM

Jdforrester-WMF assigned this task to • Prtksxna.

Jdforrester-WMF removed a project: Patch-For-Review.

Change 464370 had a related patch set uploaded (by VolkerE; owner: VolkerE):
[mediawiki/core@master] Update OOUI to v0.29.0

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

gerritbot added a project: Patch-For-Review.Oct 3 2018, 5:31 PM

Volker_E removed a project: Patch-For-Review.Oct 3 2018, 5:39 PM

Change 464607 had a related patch set uploaded (by Jforrester; owner: Jforrester):
[mediawiki/core@master] Update OOUI to v0.29.1

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

Change 464370 abandoned by Jforrester:
Update OOUI to v0.29.0

Reason:
Replaced by Ie847465af6762.

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

Volker_E removed a project: Patch-For-Review.Oct 4 2018, 6:18 PM

Change 464607 merged by jenkins-bot:
[mediawiki/core@master] Update OOUI to v0.29.1

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

Jdforrester-WMF added a project: MW-1.32-notes (WMF-deploy-2018-10-16 (1.32.0-wmf.26)).Oct 4 2018, 7:26 PM

Change 838250 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[oojs/ui@master] Remove unused "multiline" configuration

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

gerritbot added a project: Patch-For-Review.Oct 4 2022, 7:20 PM

Change 838250 merged by jenkins-bot:

[oojs/ui@master] Remove unused "multiline" configuration

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

Change 865677 had a related patch set uploaded (by Jforrester; author: Jforrester):

[mediawiki/core@master] Update OOUI to v0.46.0

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

Change 865677 merged by jenkins-bot: