Page MenuHomePhabricator

Make icon/indicator visibility handling several widget nesting levels proof
Closed, ResolvedPublic

Description

Starting with T160593 and after fighting with always different inner DOM structure in T178251 & T178336 we're now facing a limitation of widgets' icon/indicator visibility handling that might get mixed up with widgets being nested more than two levels deep.
A collection of patch/IRC comments:

Bartosz: you currently must not have Widgets that mix in IconElement or IndicatorElement nested in the DOM more than two levels deep, otherwise the most nested widgets' icons/indicators display incorrectly). This patch does not make this any more broken, it is a minor improvement (it is now at least consistently broken everywhere). But I feel we will need to do a lot of reverting soon.

Another approach of refined generalization that seemed promising first (for use with just IE > 9 browsers), but doesn't work practically:

.oo-ui-widget.oo-ui-iconElement > .oo-ui-iconElement-icon, 
.oo-ui-widget.oo-ui-iconElement *:not( .oo-ui-widget ) .oo-ui-iconElement-icon, 
.oo-ui-widget.oo-ui-indicatorElement > .oo-ui-indicatorElement-indicator, 
.oo-ui-widget.oo-ui-indicatorElement *:not( .oo-ui-widget ) .oo-ui-indicatorElement-indicator {
    display: block;
}

And a testcase for the demo:

new OO.ui.FieldLayout(
	new OO.ui.PopupButtonWidget( {
		icon: 'menu',
		label: 'Options',
		popup: {
			$content: new OO.ui.TagMultiselectWidget( {
				icon: 'feedback',
				placeholder: 'Add tags',
				allowArbitrary: true,
				inputWidget: new OO.ui.NumberInputWidget(),
				inputPosition: 'outline'
			} ).$element,
			padded: true,
			align: 'forwards'
		}
	} ),
)

Related Objects

Event Timeline

Volker_E created this task.Oct 17 2017, 9:10 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 17 2017, 9:10 PM

Change 384887 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[oojs/ui@master] [DO NOT MERGE] Demonstration of incorrect rendering of icons with IconElements nested three deep

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

Change 384887 abandoned by Bartosz Dziewoński:
[DO NOT MERGE] Demonstration of incorrect rendering of icons with IconElements nested three deep

Reason:
Just a demo. See the task for more details.

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

After the rewrite of how icons and indicators are styled (between v0.23.3 and v0.24, changes 8e31b2f2, 6604d0f2, e92c5f0f) widgets that mix in IconElement or IndicatorElement render incorrectly when nested (in the resulting DOM) three or more levels deep.

This is most likely to be a problem when using custom widgets, but it's fairly easy to run into even with only the standard ones, because many widgets use other widgets inside them automatically.

The simplest case to reproduce is when an iconless IconElement is nested inside an iconed IconElement which is itself nested inside an iconed IconElement. This occurs with (for example) DropdownWidget, TagMultiselectWidget or SelectFileWidget when placed inside the popup of PopupButtonWidget. See https://gerrit.wikimedia.org/r/384887 for example code. Screenshots:

Broken when nestedOkay when not nested
Apex theme
WikimediaUI theme

For some reason the Apex theme brokenness is much more visible, but "phantom" icons appear in the WikimediaUI theme as well (they just don't break the entire layout).

@matmarex The reason for the difference in Apex is, that its icons/indicators don't have position: absolute applied everywhere, which results in taking them out of document flow.

I've been thinking really hard about this and I'm convinced that the easiest course of action is to revert .oo-ui-iconElement/.oo-ui-indicatorElement styling to how it was in OOjs UI v0.23.3, that is, to revert commits 8e31b2f2, 6604d0f2 and e92c5f0f. This will fix this issue and return us to a known good (if imperfect) state.

These three commits were aiming to resolve these two tasks, which will have to be reopened:

For T160593, I've been thinking about and trying out some solutions, I should be able to take over that one. I've got a pretty good idea how to do this without breaking nested widgets and without much disruption.

For T161177, there have been many patches there before the problematic ones, so I suspect we will only need to redo a small part of the work? I don't feel competent to take over that though.

@matmarex What are the exact problems with 8e31b2f2?
The display handling is currently identified as the main concern from my understanding. There's only one request on handling text centered buttons with icons, where the positioning would be a specific issue to overcome.

The display handling is the only concern, and that's what's causing this issue. But I don't suppose we can just delete that block and be done with it? The other styles probably already assume that .oo-ui-iconElement-icon will be hidden if there is no icon.

(One other thing I don't like about it is that a lot of the new selectors have high specificity, which requires overrides to be similarly specific, but that's only inconvenient and easy to work around.)

The other request you mention is T178424: Absolute positioned icons makes wide+centred buttons hard to make, but the absolute positioning was added six months ago and is not relevant here.

I tried doing the revert today and it seems that T161177 is actually still fixed if we revert those.

The widgets demo looks pixel-perfect identical after the revert and before it, in both WikimediaUI and Apex (I took screenshots and subtracted them in GIMP). So apparently the reverted changes were not making any visual tweaks, despite referencing T161177, they were just fixing T160593 and doing some cleanup. I reviewed the diff too (I did not have a chance to do that before) and no numbers are changed at all.

@Volker_E We can safely revert that without undoing any visual tweaks you've recently made.

Change 385236 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[oojs/ui@master] Revert icon/indicator positioning to OOjs UI v0.23.3

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

Volker_E moved this task from Backlog to Next-up on the OOUI board.Oct 28 2017, 3:01 AM

Change 385236 abandoned by Bartosz Dziewoński:
Revert icon/indicator positioning to OOjs UI v0.23.3

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

Apparently not quite the same, but similar, just broke icons in buttons in Special:Notifications T184023: [wmf.12-minor] monobook Special:Notification - drop-down options for the cog icon should be aligned with the icons

We had to override with display: inline-block !important; but I am now going over all iconed buttons in Apex in Special:Notifications to see if anything else is broken.

Change 385236 restored by Bartosz Dziewoński:
Revert icon/indicator positioning to OOjs UI v0.23.3

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

This continues causing issues. Until another solution is proposed, we should bite the bullet and restore the working code. I'm upset that we allowed this situation to go on for nearly a year.

What are the new issues?

It's still the same issue, not any new ones: You can't reliably next more than two OOUI widgets inside each other. Depending on which of the widgets you used have icons, it might work fine, it might cause your icons to disappear, or it might cause icons to appear where there should be none. I think this is ridiculous for a UI library.

There is also a secondary issue in that the broken rules causing the first issue have ridiculously high specificity and thus are difficult to override.

I can't keep track of all the situations where this forced other developers to add workarounds to their code, but here are a few: T194085 T178933 T184023. There are also several in our own code in OOUI, which you can find by searching the code for "Stupid specific rules".

We definitely need to avoid descendent selectors in favour of explicit child selectors, especially if that selector applies "display:none".

I looked into this some more and it seems to me that it is no longer viable to just revert to the old approach (no 'display: none', each widget styles .oo-ui-iconElement … .oo-ui-iconElement-icon instead of just .oo-ui-iconElement-icon). The revert patch has like 20 different merge conflicts, and there are probably many more "hidden" ones where we simplified other code in a way that relies on the 'display: none' rules.

So, I would like to propose a different solution: in JS/PHP code, add another class to the <span class="oo-ui-iconElement-icon"> element when no icon is set (and similarly for indicators). Then just use that class in CSS to set 'display: none', instead of the current insane selector. That resolves both the correctness issue and the high specificity issue.

It irks me a bit to do it though, since there is probably some tiny performance cost to adding more stuff to the DOM, and it will make the library a tiny bit more difficult to understand for users/contributors. But it is much more practical to implement right now (probably a few hours of work, as opposed to a few days to do the revert), it's less likely to break things for users of the library all over again, and I will admit that it is convenient to hide the unused icon/indicator with 'display: none', it makes writing other CSS easier (and makes the required selectors less specific).

I've also considered changing the current insane selector to more precise selectors for each widget. That would solve the correctness issue, but not the high specificity issue, and it would have to be implemented by library users for their custom widgets, while the proposed approach works out of the box for them. (And it would be a lot more work to do as well.)

Change 385236 abandoned by Bartosz Dziewoński:
Revert icon/indicator positioning to OOjs UI v0.23.3

Reason:
Taking a different approach, see T178437#4277330.

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

Change 440043 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[oojs/ui@master] Refactor how we apply 'display: none' to unused icons and indicators

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

Change 440044 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[oojs/ui@master] IconElement/IndicatorElement: Reduce specificity of basic styles

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

matmarex triaged this task as High priority.Jun 15 2018, 9:28 PM

Change 440043 merged by jenkins-bot:
[oojs/ui@master] Refactor how we apply 'display: none' to unused icons and indicators

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

Change 440044 merged by jenkins-bot:
[oojs/ui@master] IconElement/IndicatorElement: Reduce specificity of basic styles

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

matmarex closed this task as Resolved.Jun 28 2018, 1:32 PM
matmarex removed a project: Patch-For-Review.
Volker_E moved this task from Next-up to OOUI-0.27.4 on the OOUI board.Jun 28 2018, 1:36 PM
Volker_E edited projects, added OOUI (OOUI-0.27.4); removed OOUI.