Page MenuHomePhabricator

[modern Vector only] Move indicators underneath firstHeading
Open, MediumPublicSpike

Description

In preparation fro moving the languages, the first heading should come as far up as possible in DOM on new Vector. For now, Old Vector will remain the same and this will be the subject of a new task.

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.

Also consider subtask T254666: Indicators are unclickable on huwiki

Prototypes

DEMO 2 (flex layout)
DEMO 3 (float layout)
Q: Is there any benefit to using float instead of flex or the opposite?

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
Test matrix

Variations:

  1. Skin version: legacy / modern
  2. Media: screen (code) / print (code)
  3. Window width: narrow (?px) / medium (?px) / wide (?px) -- heading wrapping into 2 or more lines
  4. Uncommon cases: works well with flaggedrevs? (T254666, example page | with site-local repositioning: Vector.js)
  5. Caching: new HTML with old CSS (for 30 min?), old HTML with new CSS (for 4-7-14 days)
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 or https://de.wikipedia.org/wiki/Rindfleischetikettierungs%C3%BCberwachungsaufgaben%C3%BCbertragungsgesetz (for overlong title)

  • 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.

Sign off steps

  • Discuss whether we want to do this for old Vector (and why) and create a new task if necessary

Related Objects

StatusSubtypeAssignedTask
ResolvedGoalovasileva
OpenNone
Resolvedovasileva
ResolvedSpikeovasileva
ResolvedSpikephuedx
Resolvedovasileva
OpenSpikeNone
ResolvedSpikeovasileva
Resolvedovasileva
ResolvedBUG REPORTmatmarex
Resolvedovasileva
ResolvedJdlrobson
Resolvedphuedx
Resolvednray
ResolvedMayakp.wiki
ResolvedMayakp.wiki
Stalledovasileva
OpenNone
ResolvedEdtadros
OpenNone
OpenNone
OpenNone
OpenNone
OpenSpikeNone
OpenBUG REPORTNone

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

@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…

@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.

  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.May 15 2020, 4:58 PM

A solution along these lines works. Not tested extensively.

<div class="mw-first-heading">
  <h1 class="firstHeading">
  {{{html-indicators}}}
</div>
.mw-body .mw-first-heading { display: flex; margin, border rules from .mw-body h1.firstHeading }
.mw-body h1.firstHeading { margin, border: 0; }
.mw-body .mw-indicators { margin-left: auto; }

If we use flex, we should use only flex:

.mw-body h1.firstHeading {
	margin: 0;
	border: none;
	flex-grow: 1;
}
.mw-body .mw-indicators {
}

(.mw-body .mw-first-heading not changed from your proposal). Also, this can cause indicators break but horizontal space being used in all title lines, as opposed to the current situation:

CurrentProposed
Demian updated the task description. (Show Details)Jun 7 2020, 11:24 AM
Demian updated the task description. (Show Details)

margin-left on indicators does not work well with flaggedrevs...
Thanks, @Tacsipacsi!

Demian updated the task description. (Show Details)Jun 7 2020, 11:36 AM
Demian updated the task description. (Show Details)Jun 7 2020, 11:40 AM

Change 605300 had a related patch set uploaded (by Aron Manning; owner: Aron Manning):
[mediawiki/skins/Vector@master] [WIP] Move indicators underneath firstHeading

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

Demian updated the task description. (Show Details)
Demian updated the task description. (Show Details)Jun 12 2020, 11:43 PM
Demian updated the task description. (Show Details)Jun 13 2020, 12:53 AM

Change 607412 had a related patch set uploaded (by Aron Manning; owner: Aron Manning):
[mediawiki/skins/Vector@master] Move indicators underneath firstHeading (float layout)

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

DEMO 2 (flex layout)
DEMO 3 (float layout)

Demian added a comment.EditedJun 24 2020, 5:33 PM

Is there any benefit to using float instead of flex or the opposite?

Demian updated the task description. (Show Details)Jun 25 2020, 6:42 AM
Demian updated the task description. (Show Details)
Volker_E updated the task description. (Show Details)Jun 26 2020, 6:24 PM

Flexbox doesn't seem acceptable to me from current browser support matrix.
Even in IE 11 the support is more than wonky. IE 9 & 10 would be put into graceful degradation position instead of solid support or progressive enhancement, which is our preferred approach with CSS.

@Demian In both of your demos, float and flexbox the indicator shrink with overlong titles.

@Demian In both of your demos, float and flexbox the indicator shrink with overlong titles.

Please open DevTools and observe the intentionally added <span style="font-size:.5em">🌟</span>.
That was done as part of a test for Tacsipacsi. It's deliberately tiny, not shrinking. For convenience now I've returned it to the same size as on other pages.

Dropping this back to the backlog for now as it's not a priority right now (no updates in over a month).

@Jdlrobson I think this task is about DOM order rather than visual presentation, but just wanted to note that as part of the language switcher work we will need to shift the indicators below the article title line to make room for the language switcher:

currently
updated

So if this issue doesn’t exist in new Vector, probably it should be left untouched in legacy Vector not only visually, but also at DOM level—search engine optimization doesn’t apply in a soon-to-be-retired skin, and it turned out that it’s impossible to change the DOM with zero visual difference (which is the promise of legacy Vector).

ovasileva removed Demian as the assignee of this task.Aug 26 2020, 6:04 PM
ovasileva moved this task from Incoming to Triaged but Future on the Readers-Web-Backlog board.
Jdlrobson renamed this task from Move indicators underneath firstHeading to [modern Vector only] Move indicators underneath firstHeading.Sep 1 2020, 10:11 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson removed a project: Patch-For-Review.

@Tacsipacsi I think for now we can scope this to the new version of Vector to manage the risk.

Team: this is back on the agenda now and a necessary precursor for the next phases of the design where we'll be moving the languages.