Page MenuHomePhabricator

Break up TextInputWidget
Closed, ResolvedPublic8 Story Points

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).

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 19 2016, 12:21 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.

Esanders renamed this task from TextInputWidget is a cow to Break up TextInputWidget.Mar 20 2016, 10:36 AM

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 Normal 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.

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.

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

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

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.

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

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

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

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"?

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?

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
Krinkle added a subscriber: Krinkle.EditedJun 30 2017, 9:15 PM

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
DLynch added a subscriber: DLynch.Jul 3 2017, 8:58 PM

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.

matmarex added a comment.EditedJul 3 2017, 8:59 PM

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).

Krinkle removed a subscriber: Krinkle.Oct 5 2017, 8:39 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

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 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

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

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

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