Page MenuHomePhabricator

[BUG] Labels for image keep jumping outside the image frame
Closed, ResolvedPublic5 Story PointsBug

Description

Testing with File:Culex_pipiens_diagram_en.svg.

Current behavior:

Image initially loads correctly but a second later, some labels (Head, Thorax, Abdomen) jump to the right - outside the frame.

Correct imageWhat gets displayed

Here's a video showing the animation happening on page refresh -

Expected behavior:

No weird label jumping happens. The image renders exactly as it is on Commons.

Other existing bugs with image rendering:

Troubleshooting approach:

First, determine if the SVG is valid. If invalid, move on with your life in full knowledge that SVG is finicky.
Second, whittle down the SVG to a minimal breaking piece of content.
Third, fix the bug with beautiful codes.

Event Timeline

Niharika triaged this task as Normal priority.Feb 8 2019, 12:55 AM
Niharika created this task.
Restricted Application added a project: Community-Tech. · View Herald TranscriptFeb 8 2019, 12:55 AM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Niharika updated the task description. (Show Details)Feb 8 2019, 12:58 AM
Niharika moved this task from Untriaged to To be estimated/discussed on the Community-Tech board.
Niharika set the point value for this task to 5.Feb 13 2019, 12:57 AM
aezell updated the task description. (Show Details)Feb 13 2019, 12:57 AM
aezell removed the point value for this task.
Niharika set the point value for this task to 5.Feb 13 2019, 12:58 AM
MaxSem added a subscriber: MaxSem.Feb 13 2019, 1:52 AM

The difference is that <text> in the processed image is missing text-anchor="end.

...Which raises a question: what shall we do with attributes? Currently, we only preserve id and style when making the DOM translatable.

Can we keep them all? Or are there other problems that arise if we do that?

...Which raises a question: what shall we do with attributes? Currently, we only preserve id and style when making the DOM translatable.

Why do we only keep those two and not all?

That's what Jarry's code did.

aezell added a subscriber: aezell.Feb 13 2019, 9:07 PM

It's maybe worth pointing out in reference to Max's comment that we took the existing code and Max just adapted it to our purposes.

Any assumptions that code made or bugs it may have had are also present in our work. But, we don't have the benefit of the context that the original author had.

So, the answer to a lot of these questions about SvgFile.php are going to be, "I don't know why it does that. What should it do instead?"

Let's try keeping all attributes and see if that resolves some of the issues we are seeing.

Glrx added a subscriber: Glrx.Feb 14 2019, 6:52 PM
Samwilson moved this task from Ready to In Development on the Community-Tech-Sprint board.

A simplistic fix for this particular bug is https://github.com/wikimedia/svgtranslate/pull/89

As far as allowing all attributes goes, I'm not across the whole code enough to know what that will mean — a bit of work has gone into not doing that, so I'd want to understand more about why before doing it. If someone else wants to take this up, go for it.

I think fixing just this image is not enough as we already have more rendering bugs that may be caused by removal of other properties. We might want to just boil the oceans, allow all attributes and see how it looks in QA.

Good point. I've updated PR89 to remove all the whitelisting of attributes. Seems to work with the images I've tested it on.

MBinder_WMF changed the subtype of this task from "Task" to "Bug Report".Mar 28 2019, 11:12 PM

This is merged and now on the staging site.

However, the bug seems to have gone away on the prod site already!

I've just released 0.8.0, containing code relating to this task.

dom_walden moved this task from QA to Product sign-off on the Community-Tech-Sprint board.EditedApr 16 2019, 3:59 PM
dom_walden added a subscriber: dom_walden.

On staging, I did not notice any dramatic differences in rendering of text between svgtranslate and Commons, or anything that looked obviously wrong (like the image in the description).

However, I was limited in the amount of testing I could do on staging due to the problems we are having at the moment. So, I did the rest of my testing on production, where this code should have been pushed.

I took a two pronged approach:

  1. A visual comparison of .png previews from svgtranslate and Commons
  2. An XML comparison of the SVG files from svgtranslate and Commons

Visual Comparison

For a number of different SVG images with text I downloaded the fallback.png image from svgtranslate and the .png of the same size from Commons. I used compare to produce a visual comparison of both .png files.

There are differences in the way svgtranslate and Commons render svgs, in particular text, but these appear to be due to svgtranslate using rsvg (for instance, they render the same for me using programs like GIMP). For example, in this svg, "АТЛАНТСКИ" is slightly cut off in the preview, which is not the case on Commons.

However, none of the differences I saw would appear to prevent someone from doing a translation of the file (with the exception of bugs already raised in T220560 and T213139).

XML Comparison

svgtranslate adds certain elements to the SVGs for ease of translation (e.g. <switch> and extra <tspans>), so it is hard to compare. So, for the SVGs already mentioned from Commons I used a Python script to check that the <tspan>'s in the original still exist in the SVG downloaded from svgtranslate and that their attributes are preserved.

It removes any empty <tspan>'s. I have not seen any bad side-effects of this, but I don't know enough about SVG to know what to look for. (EDIT: Example here)

svgtranslate adds id attributes to all tspans (id="trsvgNNN"), which I assume is fine.

Niharika closed this task as Resolved.Apr 18 2019, 8:39 PM
Niharika moved this task from Product sign-off to Done on the Community-Tech-Sprint board.

Thanks for being so thorough, @dom_walden! I think that we are good to close this ticket. We can't hope to fix every small rendering issue but it should be fine as long as there are no major rendering issues.

Niharika moved this task from In progress to Done on the SVG Translate Tool board.Apr 30 2019, 6:47 PM