Page MenuHomePhabricator

Regression: A tab does not jump from edit text box to summary anymore
Closed, ResolvedPublic

Description

I bisect this to 5e4b02c. Since 5e4b02c a tab does not jump from edit text box to summary anymore.

Event Timeline

Fomafix created this task.Jun 5 2017, 9:40 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 5 2017, 9:40 PM
matmarex claimed this task.Jun 5 2017, 10:17 PM
matmarex added a subscriber: matmarex.

There is no 'tabindex' on #wpSummary, so its gets put at the end of the tab sequence. Looks like it disappears after the widget (#wpSummaryWidget) is infused.

Apparently, in PHP, the tabindex is given as a string '1':

	private function getSummaryInputAttributes( array $inputAttrs = null ) {
		// Note: the maxlength is overridden in JS to 255 and to make it use UTF-8 bytes, not characters.
		return ( is_array( $inputAttrs ) ? $inputAttrs : [] ) + [
			'id' => 'wpSummary',
			'name' => 'wpSummary',
			'maxlength' => '200',
			'tabindex' => '1',
			'size' => 60,
			'spellcheck' => 'true',
		] + Linker::tooltipAndAccesskeyAttribs( 'summary' );
	}

This works fine for the PHP TextInputWidget.

The value is passed on all the way to the infusion data in page source:

<div id='wpSummaryWidget' ... data-ooui='{"_":"OO.ui.TextInputWidget","maxLength":"200","name":"wpSummary","inputId":"wpSummary","tabIndex":"1","title":"Enter a short summary [b]","accessKey":"b"}'>

And apparently, the JS TextInputWidget doesn't like it and discards it.


There's a tiny implementation difference between the PHP and the JS version:

trait TabIndexedElement {
	...
	public function setTabIndex( $tabIndex ) {
		$tabIndex = is_numeric( $tabIndex ) ? $tabIndex : null;

		if ( $this->tabIndex !== $tabIndex ) {
			$this->tabIndex = $tabIndex;
			$this->updateTabIndex();
		}

		return $this;
	}
	...
}
OO.ui.mixin.TabIndexedElement.prototype.setTabIndex = function ( tabIndex ) {
	tabIndex = typeof tabIndex === 'number' ? tabIndex : null;

	if ( this.tabIndex !== tabIndex ) {
		this.tabIndex = tabIndex;
		this.updateTabIndex();
	}

	return this;
};

PHP's is_numeric checks for "a number or a numeric string". JS's typeof tabIndex === 'number' only accepts actual numbers.

Change 357323 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/core@master] EditPage: Give edit summary field's 'tabindex' as a number

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

matmarex triaged this task as Unbreak Now! priority.Jun 5 2017, 10:33 PM

This is potentially very annoying.

Restricted Application added subscribers: Jay8g, TerraCodes. · View Herald TranscriptJun 5 2017, 10:33 PM
Krinkle added a subscriber: Krinkle.EditedJun 5 2017, 10:39 PM

For attributes, strings are appropiate. The key there is lowercase tabIndex (as it should be for attributes). For constructor options and ooui-data, rich formats are supported, and in the case of tabIndex (camelcase) a number is expected.

Given that the PHP output <div id='wpSummaryWidget' ... data-ooui='{"_":"OO.ui.TextInputWidget","maxLength":"200","tabIndex":"1" contains tabIndex and not tabindex, I imagine there is already a mapping of this somewhere. This is not just a blind hash of attribute key/values. As such, presumably we can update that mapping to also convert this to a number accordingly.

(Same for maxLength by the way.)

I'm just going to make number-as-string for tabIndex accepted. We want PHP and JS to be consistent, and because it's currently allowed in PHP (although probably unintentionally) and removing that could be a breaking change, then let's allow it in JS too.

Regardless, we should merge the code change first, since a) it fixes the issue immediately, rather than whenever we upgrade OOjs UI again b) the documentation doesn't say that number-as-string is valid.

(And using a number is not wrong for an attribute in Html::element() parameters, as evidenced by the naked number in 'size' => 60 below.)

I'm just going to make number-as-string for tabIndex accepted. We want PHP and JS to be consistent, [..]

Does the PHP constructor options actually support it though? It seems like we're returning it from an attributes array (getSummaryInputAttributes) not explicit use of the options. I'd rather not have the JS support this, but no strong objection.

I'm not sure what you mean. It's not documented as supported, but it clearly works. We also don't check for numeric-ness at all in other cases, e.g. the 'maxLength' you mentioned. The attributes returned by that function are later converted to OOUI config options with OOUI\Element::configFromHtmlAttributes(), but that doesn't currently turn numbers-as-strings into actual numbers.

Change 357323 merged by jenkins-bot:
[mediawiki/core@master] EditPage: Give edit summary field's 'tabindex' as a number

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

Change 357325 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[oojs/ui@master] TabIndexedElement: Accept number-as-string in JS too

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

matmarex closed this task as Resolved.Jun 5 2017, 11:21 PM
matmarex removed a project: Patch-For-Review.

(Fixed from the MediaWiki perspective. If we need to discuss what to do about the inconsistency between JS and PHP TabIndexedElement, let's file another task.)

Change 357325 merged by jenkins-bot:
[oojs/ui@master] TabIndexedElement: Fix validation and make consistent in PHP and JS

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