Page MenuHomePhabricator

Remove the SubHeadingTransform (Don't apply in-block to headings)
Closed, ResolvedPublicBUG REPORT

Description

In T261805 it was determined that the in-block class is not needed.

Background

What happens?:
<h2>s (inside a table?) apparently get a new class in-block which sets display: table. Effect, the lack of full-width titles on the main page:

image.png (350×1 px, 118 KB)

Occurs only on mobile domain, all skins; all skins on desktop fine.

What should have happened instead?:
No in-block class should be added on any page.

TODO

QA

  • Verify that in-block class is not present on sub-headings (after purging the page)
  • Verify that nothing is broken as a result of that

Event Timeline

Izno updated the task description. (Show Details)

I have no idea why this is happening

It seems you made some changes to the Main page...? It looks like this introduced the issue:
https://en.m.wikipedia.org/w/index.php?title=Special:MobileDiff/1069328725&type=revision&diffmode=source

This has changed the structure of the page as the in-block class is added (and always has been added) to any subheadings. The introduction of an h1 has confused the mobile formatting code.

Out of curiosity why the change? There is already an h1 in the page hidden via CSS.. If you want to add an h1, you might want to consider setting https://en.wikipedia.org/wiki/MediaWiki:Mainpage-title to Welcome to Wikipedia and incorporating the header in the main page styling .mw-first-heading.

Yay, I knew there would be something wrong with that change.

There is already an h1 in the page hidden via CSS

I did this at the request of @stjn since a display: none heading is not read out for accessibility aids (etc. etc.) and not having an h1 is generally bad. This is in duo with the change made on WMF's side to add the message in question. (It was bad before WMF's change too.)

That said, I think MF is wrong here to be adjusting how headings work. What use case is MF trying to fix here?

Added for T106347: Safari regression: Edit button for H2 left aligned in mobile.init.styles.less. Blgh. Does Safari still need that hack?

Alternative other place to change would be SubHeadingTransform.php, which does already have a TODO (though it looks like a TODO of the 'change the name' sort?).

Yeah, if we want to say that <h1 id="firstHeading"> should be the only <h1> on the page, then it should not be hidden completely (but via visually hidden class). Also, since most wikis do not consider this at all, someone would need to get the list of main pages where <h1> is already present. All in all, it seems that easier solutions are either to see that the hack is relevant, as Izno said, or maybe at least remove it where it is irrelevant (such as here, actually: why add in-block class into headings without edit section links?).

Jdlrobson renamed this task from Don't apply in-block to headings to Remove the SubHeadingTransform (Don't apply in-block to headings).Apr 5 2022, 3:42 PM
Jdlrobson triaged this task as Medium priority.
Jdlrobson updated the task description. (Show Details)

It looks like we accidentally fixed the original issue by way of T305971: Change styling for Minerva/MobileFrontend section headings to use flexbox instead of table-cells, so this task may no longer be necessary.

It looks like we accidentally fixed the original issue by way of T305971: Change styling for Minerva/MobileFrontend section headings to use flexbox instead of table-cells, so this task may no longer be necessary.

I don't think it is necessary, but let me test how the h2s on the main page react with the flex styling. So yes, the original issue is resolved. :) in-block doesn't appear anywhere else in either Minerva or MF besides the PHP class the current task is scoped as.

The task is still valid IMO. We don't use these transform for anything so we should remove the code (more info @ T261805).

Shresta05 subscribed.

Removing the in-block for Sub headings

Change 851166 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/extensions/MobileFrontend@master] Remove SubHeadingTransform and the 'in-block' CSS class

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

I'd like to do this ahead of T13555, so that there is one less thing to update.

Change 851166 merged by jenkins-bot:

[mediawiki/extensions/MobileFrontend@master] Remove SubHeadingTransform and the 'in-block' CSS class

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

Nothing seems broken to me but I still see the in-block class

Screenshot 2022-11-06 at 01.09.44.png (2×3 px, 1 MB)

However, there's no styling applied as shown below:

Screenshot 2022-11-06 at 01.11.48.png (1×3 px, 486 KB)

@matmarex , any thoughts on this? Looks good to me but I could be missing something.

Nothing seems broken to me but I still see the in-block class

This task is on next week's train.

Nothing seems broken to me but I still see the in-block class

This task is on next week's train.

Makes sense. Thanks for clarifying that.

It works fine on the Beta Cluster. I'll go ahead to close it.