Page MenuHomePhabricator

HTML previews displaying changes made by gadgets
Closed, ResolvedPublic

Description

Background

After turning on html previews on cswiki, we're seeing certain gadgets affect the previews, namely Zvýrazňovat přesměrování but potentially others. Reported by @Vachovec1 in T182321#3992796

Steps to recreate

  1. As a logged in user, turn on the gadget "Zvýrazňovat přesměrování", which highlights redirects
  2. Go to https://cs.wikipedia.org/wiki/Kategorie:%C5%BDensk%C3%A1_jm%C3%A9na
  3. Hover over Anna

Observed:

Question: if the gadget is enabled and redirects are highlighted throughout the page, do we want to include them within previews?

Details

Related Gerrit Patches:
mediawiki/services/mobileapps : masterSummary: flatten new and redirect links

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 22 2018, 3:45 PM
ovasileva triaged this task as Medium priority.Feb 22 2018, 3:46 PM
ovasileva updated the task description. (Show Details)Feb 22 2018, 3:49 PM
ovasileva added subscribers: Vachovec1, Nirzar.
Jdlrobson added a subscriber: Jdlrobson.

This should be fixed on wiki. We can't possibly guarantee compatibility with all gadgets. The styles shipped by the gadget should be modified to only apply within the article not the page preview.

bearND added a subscriber: bearND.EditedFeb 22 2018, 4:46 PM

Just explaining what's happening here technically:
Links with redirects get a special class in Parsoid: <a class="mw-redirect">.
When MCS creates the summary it turns the <a> tags into <span> tags,
so the result is: <span class="mw-redirect">. MCS removes the id attributes but class and style attributes would be preserved.

I was wondering whether we should strip these mw-redirect attributes, which would most likely result into completely flattening the span element into just a text node.

During development I was pushing for removing style attributes. I think classes provide semantic value so are worth keeping but this kind of thing is always going to be a problem.

This is not a decision for WMF to take. The wiki/gadget developer needs to decide whether this is correct behaviour (they might see this as a feature) or whether to adjust their gadget. This is out of scope for us to fix.

Fix is simple:
change rule

.mw-redirect

to

#bodyContent .mw-redirect

@ovasileva - T182321#3992796 is an example of an edge case or a bug? The preview should probably not accept css definitions designed for the normal article text. The above mentioned gadget may not be the only one that will influence the preview.

that should be fixed in the gadget. The styling introduced by said gadget is very generic. Vachovec1 would you be so kind to flag that to the gadget author(s)?

Well, the gadget definition is here:
MediaWiki:Gadget-HighlightRedirects.css
It's actualy very simple/basic code. So what would you propose?
The en-wiki has something very similar:
MediaWiki:Gadget-DisambiguationLinks.css (author: @kaldari)
I presume that the behaviour would be the same.

Let's continue conversation here - https://phabricator.wikimedia.org/T188005#3993640 suggests a way this could be fixed in the gadget.

Dvorapa added a comment.EditedFeb 22 2018, 9:09 PM

Well, the gadget definition is here:
MediaWiki:Gadget-HighlightRedirects.css
It's actualy very simple/basic code. So what would you propose?
The en-wiki has something very similar:
MediaWiki:Gadget-DisambiguationLinks.css (author: @kaldari)
I presume that the behaviour would be the same.

I can see a difference between cswiki and enwiki gadget: .mw- (cs), a.mw- (en). Probably the easiest fix to make both disambiguation and redirect gadgets work only on links in articles, not on spans (as @bearND pointed) in previews.

This can be fixed easily and very quickly on cswiki's side, it is up to developers whether to fix this in previews as well.

Well, the gadget definition is here:
MediaWiki:Gadget-HighlightRedirects.css
It's actualy very simple/basic code. So what would you propose?
The en-wiki has something very similar:
MediaWiki:Gadget-DisambiguationLinks.css (author: @kaldari)
I presume that the behaviour would be the same.

I can see a difference between cswiki and enwiki gadget: .mw- (cs), a.mw- (en). Probably the easiest fix to make both disambiguation and redirect gadgets work only on links in articles, not on spans (as @bearND pointed) in previews.
This can be fixed easily and very quickly on cswiki's side, it is up to developers, if they will fix this in previews as well.

@Dvorapa, @bearND, @Jdlrobson: I am not sure if the solution is so simple. Will the gadget work on special pages like Watchlist or Recent changes? It's absolutely necessary for it to work there.

@Jdlrobson I think MCS could still chose to drop any span.mw-redirects from summary output, knowing that these came from a.mw-redirects. Since the summary changes the links to spans.

Dvorapa added a comment.EditedFeb 22 2018, 9:30 PM

@Dvorapa, @bearND, @Jdlrobson: I am not sure if the solution is so simple. Will the gadget work on special pages like Watchlist or Recent changes? It's absolutely necessary for it to work there.

Yes, the solution a.mw- would work everywhere, where the link to the redirect/disambiguation pages is clickable, also on special pages. The solution #bodyContent .mw-redirect suggested by @Jdlrobson would work everywhere too, but only in page content (e.g. on link to talk page would not work anymore)

#content or .mw-body is available on special pages. .mw-body .mw-redirect can be used instead.

(@Jdlrobson I think I saw #bodyContent on special pages too)

This was fixed on wiki now

I changed the gadget definition from .mw-redirect to a.mw-redirect and it works! The gadget now works in articles and special pages, but not in the previews..

Jdlrobson closed this task as Resolved.Feb 22 2018, 9:54 PM
Jdlrobson claimed this task.

Great! Sounds like this can be resolved?

@Jdlrobson If you do not plan to fix this also in page previews

phuedx added a subscriber: phuedx.Feb 23 2018, 10:26 AM

Excellent discussion and work all!

bearND reopened this task as Open.Feb 26 2018, 8:18 AM

I still think MCS should drop these classes (mw-redirect and new). They have meaning for <a> elements but are useless for <span> elements in summaries. Removing these often results in being able to turn those into text nodes, which also improves compression by making the html extract more similar to the plain text extract. It's not that difficult to do, see my patch.

Change 414625 had a related patch set uploaded (by BearND; owner: BearND):
[mediawiki/services/mobileapps@master] Summary: flatten new and redirect links

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

ovasileva moved this task from Backlog to For Review on the Page-Previews board.Feb 26 2018, 4:05 PM

Change 414625 merged by Mholloway:
[mediawiki/services/mobileapps@master] Summary: flatten new and redirect links

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

bearND closed this task as Resolved.Feb 26 2018, 10:34 PM
bearND moved this task from Needs triage to Kanban on the Product-Infrastructure-Team-Backlog board.

Deployed today (tag deploy/2018-02-26/9970f977).

Dvorapa moved this task from For Review to Done on the Page-Previews board.Mar 2 2018, 3:46 PM