Page MenuHomePhabricator

Must `.indicatorElement-Indicator` be child of `.indicatorElement`?
Closed, DeclinedPublic

Description

Must .indicatorElement-Indicator be child of .indicatorElement?

From IndicatorElement.less:

.oo-ui-indicatorElement {
	.oo-ui-indicatorElement-indicator,
	&.oo-ui-indicatorElement-indicator {
		background-size: contain;
		background-position: center center;
		background-repeat: no-repeat;
		min-width: @size-indicator-min;
		width: @size-indicator;
		min-height: @size-indicator-min;
		height: @size-indicator;
	}

	.theme-oo-ui-indicatorElement();
}

It seems like those two selectors catch all instances of oo-ui-indicatorElement-indicator. Wait, no, because there's also:

<div class="oo-ui-capsuleMultiSelectWidget-handle">
  <span class="oo-ui-indicatorElement-indicator"></span>
  <span class="oo-ui-iconElement-icon"></span>
  <div class="oo-ui-capsuleMultiSelectWidget-content"></div>
</div>

where we partly repeat the oo-ui-indicatorElement-indicator CSS rules

Would it be possible (again from a pure architectural perspective) to go for something like that instead:

.oo-ui-indicatorElement {
	.theme-oo-ui-indicatorElement();
}
	.oo-ui-indicatorElement-indicator {
		background-size: contain;
		background-position: center center;
		background-repeat: no-repeat;
		min-width: @size-indicator-min;
		width: @size-indicator;
		min-height: @size-indicator-min;
		height: @size-indicator;
	}

Event Timeline

Must .indicatorElement-Indicator be child of .indicatorElement?

Technically, a descendant, not a direct child. And it must. :)

It seems like those two selectors catch all instances of oo-ui-indicatorElement-indicator. Wait, no, because there's also:

<div class="oo-ui-capsuleMultiSelectWidget-handle">
  <span class="oo-ui-indicatorElement-indicator"></span>
  <span class="oo-ui-iconElement-icon"></span>
  <div class="oo-ui-capsuleMultiSelectWidget-content"></div>
</div>

where we partly repeat the oo-ui-indicatorElement-indicator CSS rules

The HTML is actually:

<div class="oo-ui-widget oo-ui-widget-enabled oo-ui-indicatorElement oo-ui-iconElement oo-ui-capsuleMultiSelectWidget" aria-disabled="false">
  <div class="oo-ui-capsuleMultiSelectWidget-handle">
    <span class="oo-ui-indicatorElement-indicator"></span>
    <span class="oo-ui-iconElement-icon"></span>
    <div class="oo-ui-capsuleMultiSelectWidget-content"></div>
  </div>
  <div class="oo-ui-widget oo-ui-widget-enabled oo-ui-selectWidget …" ></div>
</div>

Note the additional wrapping div, which has the necessary oo-ui-indicatorElement class.

Some of the CSS is indeed unnecessarily repeated, fixing this in https://gerrit.wikimedia.org/r/#/c/283182/.

Would it be possible (again from a pure architectural perspective) to go for something like that instead:

.oo-ui-indicatorElement {
	.theme-oo-ui-indicatorElement();
}
	.oo-ui-indicatorElement-indicator {
		background-size: contain;
		background-position: center center;
		background-repeat: no-repeat;
		min-width: @size-indicator-min;
		width: @size-indicator;
		min-height: @size-indicator-min;
		height: @size-indicator;
	}

Nope, I'm pretty sure this wouldn't work. The oo-ui-indicatorElement class on the parent is only present when the widget actually has an indicator (and not if it just potentially can have one), and we rely on this to hide the <span class="oo-ui-iconElement-icon"></span> if there's no indicator to display.

@matmarex If I your understand last paragraph correctly, we're relying on an .oo-ui-indicatorElement available or not to hide. But hiding/showing is done via display property. So it's visibility is completely independent from the other properties above in a child selector implementation applied like in compiled

.oo-ui-indicatorElement .oo-ui-indicatorElement-indicator,
.oo-ui-indicatorElement.oo-ui-indicatorElement-indicator {}

rule.

Just .oo-ui-indicatorElement-indicator should work then with size and background properties alone.
I'm shooting for least specificity here ;)

@matmarex If I your understand last paragraph correctly, we're relying on an .oo-ui-indicatorElement available or not to hide. But hiding/showing is done via display property. So it's visibility is completely independent from the other properties

We don't, or at least, not consistently. It seems we do use display: none in ButtonWidget and TextInputWidget, but we don't in DropdownWidget or SelectFileWidget. Hmm.

Perhaps we could actually use display: none this way for all widgets, and write the main styles for just .oo-ui-indicatorElement-indicator… I don't see any reason why we couldn't (although I might be missing something, I don't really know why it was done this way so far). It would require some pretty boring work and testing to change this, but if you want to… ;)

@matmarex So this task has to be seen in relation to T150071 and T160593.
Another idea that came across my mind a short while ago, would be to get rid of the .oo-ui-indicatorElement-indicator DOM element and instead use :after (IE 8)/::after (everything else newer) pseudo elements and leave just .oo-ui-iconElement-icon as element and class="oo-ui-iconElement oo-ui-indicatorElement" combinations.

It would be a breaking change, but a handleable one. Currently code references:
Core:

  • /resources/src/mediawiki.special/mediawiki.special.apisandbox.css (1)
  • /resources/src/mediawiki.widgets/mw.widgets.DateInputWidget.less (3)
  • /resources/src/mediawiki.widgets/mw.widgets.StashedFileWidget.less (3)
  • /resources/src/mediawiki.widgets.datetime/DateTimeInputWidget.less (4)

ContentTranslation:

  • /modules/tools/styles/ext.cx.tools.template.editor.less (1)

VisualEditor

  • /modules/ve-mw/init/styles/ve.init.mw.MobileArticleTarget.css (1)