Page MenuHomePhabricator

Move indicators underneath firstHeading
Open, MediumPublicSpike

Description

The first heading should come as far up as possible in DOM on both new and old Vector.

Indicators as shown here (barnstar), here (combination of badges on Commons) or here (helplink) should come after firstHeading as it's more descriptive and clearer to, users of screenreader software.
Imagine hearing the article title and then the information that it's a featured article.

Proposal

Move indicators directly under #firstHeading to

  • improve screenreader experience
  • also let indicators stay on same place close to heading underline, wiith multiline headings
Note

Currently, the right floated indicators in variable width are letting headings break before its div.
We need to apply a layout flexible solution, that takes the variable width of both elements into account.

One minor possible side-effect could be that excerpts of Wikipedia articles in search engine results would include the indicators in the excerpt in the sequence:

  1. first heading
  2. indicators “This is a featured article”
  3. prebodyhtml
  4. siteSub

instead of

  1. first heading
  2. prebodyhtml
  3. siteSub
Local Testing
Test IE 8 with changed DOM
Applied, manual testing
jawikihewikicommons (en)
(with local padding rule in place)
(w removed local override)

QA

Test page: https://en.wikipedia.beta.wmflabs.org/wiki/Albert_Einstein

  • The indicators appear on the same position on legacy Vector
  • The indicators appear on the same position on modern Vector

Note,

  • it's acceptable if there's a 1px difference
  • there's a local CSS override on enwiki adding a padding-top to position indicators, that's good to be removed with this change. Request captured on Vector.css talk page.

Related Objects

StatusSubtypeAssignedTask
ResolvedGoalovasileva
OpenNone
Resolvedovasileva
ResolvedSpikeovasileva
ResolvedSpikephuedx
Resolvedovasileva
OpenSpikeVolker_E
OpenSpikeVolker_E
ResolvedSpikeovasileva
Resolvedovasileva
ResolvedBUG REPORTmatmarex
Resolvedovasileva
ResolvedJdlrobson
Resolvedphuedx
Resolvednray
ResolvedMayakp.wiki
ResolvedMayakp.wiki
Openovasileva
OpenNone
ResolvedEdtadros
OpenNone

Event Timeline

Volker_E triaged this task as High priority.Mar 28 2020, 10:13 PM
Volker_E created this task.
ovasileva moved this task from Q3 2019-2020 to Q4 2020 on the Desktop Improvements board.

Would this apply to new Vector or old? Or both?

Volker_E added a comment.EditedMar 31 2020, 1:06 AM

It's as risky for both, as we will probably have to rely on negative margin-top. I'd pledge for doing it for both. It's contained, so we could easily roll it back if there are unforeseeable layout issues.

TheDJ added a subscriber: matmarex.Mar 31 2020, 8:05 AM

Note that sitesub is problematic because it is not always present. Interference of sitesub (and the many css hacks needed to support it) were actually one of the reasons that the indicators were centralized. @matmarex should know a lot about this as well, as he implemented Indicators if i'm not mistaken.

@Volker_E to clarify, are we talking about all elements with the mw-indicators class? Or is there some other way we're defining "indicators"?

Also noting that as part of Desktop Improvements we've identified that in order to put the language switcher in the desired location we'd have to move these article indicators below the border:

Note that sitesub is problematic because it is not always present. Interference of sitesub (and the many css hacks needed to support it) were actually one of the reasons that the indicators were centralized. @matmarex should know a lot about this as well, as he implemented Indicators if i'm not mistaken.

Yes, I've tried to capture this with the two links provided. Happily we can address these variants with CSS selectors.

Change 585365 had a related patch set uploaded (by VolkerE; owner: VolkerE):
[mediawiki/skins/Vector@master] [WIP] Move indicators underneath firstHeading in DOM

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

Demian added a comment.Apr 2 2020, 8:27 PM

Seeing that the patch moves html-indicators into #bodyContent, I've been wondering if there is a reason to keep the remaining 3 elements html-sitenotice, #firstHeading, html-prebodyhtml outside of #bodyContent. I've tested moving #firstHeading into #bodyContent, that worked well: POC demo.

matmarex added a comment.EditedApr 2 2020, 9:24 PM

@Volker_E This seems fine by me, as long as their visual position stays the same.

@Demian There might be possible breakages in existing wikis or gadgets related to such change. That's why this is scoped to only touch the one that is actually a user content presentation issue, while the others aren't. We try to scope carefully with our limited resources.

Demian added a comment.Apr 3 2020, 1:45 AM

@Demian There might be possible breakages in existing wikis or gadgets related to such change.

That's obvious. I wonder if somebody knows about actual things that depend on this DOM topology.

Wait, this task is actually about moving the indicator area visually too? I'm not sure if that's a good idea. The placement proposed in https://gerrit.wikimedia.org/r/c/mediawiki/skins/Vector/+/585365 conflicts with infoboxes, floating images, etc.

@matmarex No worries, not visually for the scope of this patch. The patch is labelled work in progress for a reason. Had to figure out a caching/code issue with the new template and the old template in use.

Volker_E added a comment.EditedApr 3 2020, 8:17 PM

While working on this, it seems less risky layout-wise and at the same time similarly appropriate to put indicators directly underneath #firstHeading and then have #siteSub following.
For a featured article the readout would be:

  1. Article title
  2. this is a featured article
  3. siteSub “From Wikipedia, the free encyclopedia”

For something like Revision history special page, #siteSub isn't rendered, so

  1. Page title
  2. “Help”

would be the order.

The combination of siteSub being sometimes rendered, but empty, or depending on your viewport size might feature wrapped content, together with possible central notice or site notice outputs, that prevent absolute positioning from happening leaves the reasonable layout solution to use negative top margin and this works only consistently when the indicators are the direct sibling of #firstHeading.

Volker_E updated the task description. (Show Details)Apr 3 2020, 8:20 PM
Volker_E updated the task description. (Show Details)Apr 3 2020, 8:27 PM
Volker_E updated the task description. (Show Details)Apr 3 2020, 8:46 PM
Volker_E updated the task description. (Show Details)Apr 3 2020, 8:58 PM

This impacts cached HTML

@Volker_E feel free to add QA steps for beta cluster and move to QA if necessary.

Change 585365 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Move indicators underneath #firstHeading in DOM

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

Change 587357 had a related patch set uploaded (by VolkerE; owner: VolkerE):
[mediawiki/skins/Vector@master] [dev][Less] Remove caching workaround

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

Volker_E updated the task description. (Show Details)Apr 8 2020, 1:08 AM
Volker_E updated the task description. (Show Details)Apr 8 2020, 1:10 AM
Volker_E updated the task description. (Show Details)Apr 8 2020, 1:36 AM
Krinkle added a comment.EditedApr 8 2020, 1:45 AM

Change 585365 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Move indicators underneath #firstHeading in DOM

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

We've generally made less than two cache-breaking changes to the HTML output a year. Merging such change on a Tuesday before the branch cut with effectively no exposure to beta, other team's QA, or local development testing etc is imho quite a high risk. Normally, but especially now.

Shall we back this out at least from wmf.27, and let it roll out from master next week?

From Gerrit:
#firstHeading + .mw-indicators {
  margin-top: @margin-top-indicators;
}

I don't think this can work. Using negative margins disables the text and layout flow control of the browser.
This causes overlap and unreadable text quite easily.
Example:
https://en.wikipedia.beta.wmflabs.org/wiki/The_Parsonage_Garden_at_Nuenen

Compared to current prod where it works fine:

There's may be other problems as well. For example, with this patch there is no longer a strategy for line wrapping. Because negative margin takes it out of the layout flow, it can no longer interact with the header text. Yet you're still trying to render it as if it was there, that can't go well.

For example, if there are several wide indicators (which is common on-wiki), these would previously wrap within the header, above the first heading's bottom line. Instead with this patch, they are going to overlap the header title and maybe eventually drop partly below and partly above.

Consider things like coordinates and "Spoken Wikipedia" (nlwiki) which are quite large:

I don't see how that's gonna work with average length article titles and average width monitors/screens.

Have we experimented with this beyond enwiki and the few popular articles and templates we know there?

Consider notifying Tech-News to crowdsource this and enable community experts to try this out and preare their local templates if needed after trying on Beta Cluster for a few days or testwiki, or a gadget.

Or, we could do that reseach for them as well. For example, the parsing team have a lot of experience with running render diffs on a large scale scale, we could use that to easily find out any rendering diferences beyond 1px.

In any event, I don't want to spend anymore time on this in the critical path. Let's back out of wmf.27 at the very least now that we still have the option not to corrupt the CDN cache in a way we can't easily fix?

Volker_E updated the task description. (Show Details)Apr 8 2020, 2:08 AM

@Krinkle The local override is less of a concern, but the line-wrapping fails with the first approach (have had a more limiting solution in mind, but that's not viable with the variance and length in indicators).
Created a revert of the first attempt. Let's move slightly backward on this and try again…

Demian added a comment.Apr 8 2020, 2:14 AM

@Krinkle good points! This seems to require a different approach.

Were these considered?

  1. Wrapping both h1 and {{{html-indicators}}} into a div, float-ed or with flex. The border and margin CSS moves up to the div. This works well.
  2. Duplicate {{{html-indicators}}}: 1) visible above h1, but aria-hidden and 2) audible after h1, but off-screen.
jeena added a subscriber: jeena.Apr 8 2020, 7:41 AM

Can someone confirm whether this is a train blocker? Thanks :)

zeljkofilipin raised the priority of this task from High to Unbreak Now!.Apr 8 2020, 12:42 PM
zeljkofilipin added a subscriber: zeljkofilipin.

Train blockers are UBN. Please lower the priority if/when it is no longer a blocker.

Restricted Application added a subscriber: Liuxinyu970226. · View Herald TranscriptApr 8 2020, 12:42 PM

Change 587390 had a related patch set uploaded (by Krinkle; owner: VolkerE):
[mediawiki/skins/Vector@master] Revert "Move indicators underneath #firstHeading in DOM"

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

Change 587520 had a related patch set uploaded (by Krinkle; owner: VolkerE):
[mediawiki/skins/Vector@wmf/1.35.0-wmf.27] Revert "Move indicators underneath #firstHeading in DOM"

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

Change 587390 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Revert "Move indicators underneath #firstHeading in DOM"

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

Volker maybe we could reconsider applying this to the old Vector. In the long term use of that skin will be limited to small audience of editors and doing this first in new Vector would allow editors time to fix.

Jdlrobson lowered the priority of this task from Unbreak Now! to Medium.Apr 8 2020, 5:11 PM

Train should be unblocked given the revert is merged.

Demian added a comment.Apr 8 2020, 6:12 PM
  1. Wrapping both h1 and {{{html-indicators}}} into a div, float-ed or with flex. The border and margin CSS moves up to the div. This works well.
  2. Duplicate {{{html-indicators}}}: 1) visible above h1, but aria-hidden and 2) audible after h1, but off-screen.

@Volker_E

  1. Were these options considered?
  2. Should I make a patch with solution 1 - verified, working, detailed in the gerrit comment?

Change 587520 merged by jenkins-bot:
[mediawiki/skins/Vector@wmf/1.35.0-wmf.27] Revert "Move indicators underneath #firstHeading in DOM"

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

Mentioned in SAL (#wikimedia-operations) [2020-04-08T18:37:41Z] <jforrester@deploy1001> Synchronized php-1.35.0-wmf.27/skins/Vector: T248761: Revert moving indicators in DOM (duration: 01m 07s)

Change 587357 abandoned by Jdlrobson:
[dev][Less] Remove caching workaround

Reason:
No longer needed now we reverted.

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

Moving back to needs analysis to respect the bandwidth of the team.

Jdlrobson moved this task from Needs triage to Technical on the Vector board.Fri, May 15, 4:58 PM