Skip to content

WIP: Resolve "Picture and text misaligned in namespace selection buttons"

What does this MR do?

This changeset adds an override to some breaking CSS that causes .avatar-containers to not be centered when presented on the fork namespace selection screen.

I'm not sure exactly when this was broken - if it was ever right - but here's the traceback of the intent:

  1. The .fork-thumbnail .avatar-container is supposed to be centered
  2. .avatar-container extends .avatar-circle
  3. All .avatar-circle.s*s have an explicit pixel value right margin (provided by the avatar-size mixin)

Notably, this change is "rolling back" some CSS applied in that above chain by overriding it with conflicting properties in a rule of slightly higher specificity. Note that .mx-auto is applied to the .avatar-container - presumably in an attempt to maintain the centering - but because its specificity is so low (010) the offending rule is still overriding it. The rule that applies the right margin is .s*.avatar-container (020) and the newly added rule that sets it back to the intended auto value is .fork-thumbnail .avatar-container.s* (030).

As an aside: I am fairly strongly of the opinion that avatar-size should not be applying a margin. The principle of least astonishment dictates that if something is named size it shouldn't also affect the margin.

An aside to the aside: The correct fix here is to componentize the avatar so that the styles cannot cross the component boundary (Shadow DOM!?!) and then provide a way to opt in/out of the margin at the component level. That fix, however, would be a very significant overhaul of every place that uses the avatar, and would be very, very far outside the scope of this issue.

Does this MR meet the acceptance criteria?

Conformity

Performance and Testing

I chose to override the offending rule with a rule of higher specificity because there are dozens of places that have the .avatar-container.s* class, while there is only one place where .fork-thumbnail .avatar-container.s* applies. By making only this single change, I drastically reduce the risk of unintended side effects elsewhere. However, this is slightly less efficient, since the browser will apply the previous style and then re-apply the fix. If a user has many thousands of potential namespaces (is this possible??) it may have a barely noticeable impact.

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • ~~Label as security and @ mention @gitlab-com/gl-security/appsec~~
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team

Closes #64068 (closed)

Edited by Thomas Randolph

Merge request reports