1837424 - Use the computed style to determine inline vs block elements in the TranslationsDocument

Status: RESOLVED FIXED

(Firefox :: Translations, enhancement, P3)

Description

[Greg Tatum [:gregtatum]]

Right now there is a bunch of complicated heuristic-based code to determine if elements are block elements, or inline elements. This is something the browser engine can provide with the computed style. This would mean that we could simplify the code, and provide higher quality translations.

These heuristics are easy to get wrong, since it's ultimately the style, not the tag type, that determines if an element is block or inline. I'm not sure the overall cost for computing the style for each element we walk, but it's something that is worth experimenting with and measuring to see if it's feasible to do.

Comment 1

[Greg Tatum [:gregtatum]]

I just wrote a really bad version of this to measure the performance, and on elpais.com getComputedStyle was called 551 times, and it didn't show up in the profile, so it should be fast.

Comment 2

[Marco Castelluccio [:marco]]

(In reply to Greg Tatum [:gregtatum] from comment #1)

I just wrote a really bad version of this to measure the performance, and on elpais.com getComputedStyle was called 551 times, and it didn't show up in the profile, so it should be fast.

Could you attach the patch here if it is not too dirty so somebody else could finish it up?

Comment 3

[Marco Castelluccio [:marco]]

Removing from blocking bug 1838721, since it is already blocking bug 1837421, which in turn blocks bug 1838721.

Comment 4

[Greg Tatum [:gregtatum]]

Attachment: example.patch

> Could you attach the patch here if it is not too dirty so somebody else could finish it up?

I can't seem to find it locally. I must have deleted it since it was a fairly trivial change:

```

Comment 5

[Greg Tatum [:gregtatum]]

Attachment: patch

I can't find the patch I wrote, I must have deleted it. It was a pretty small one, something like this.

Comment 6

[Greg Tatum [:gregtatum]]

Attachment: Bug 1837424 - Change the TranslationsDocument to use computed inline values

Comment 7

[Greg Tatum [:gregtatum]]

getIsInline took 15ms and nodeNeedsSubdividing took 20ms on a very heavy page with lots of nodes: https://share.firefox.dev/3RDIClz

On the main branch nodeNeedsSubdividing took 13ms took https://share.firefox.dev/48c8sTb

It's a very minimal impact for the benefit it brings.

Comment 8

[Greg Tatum [:gregtatum]]

It had no real impact on my benchmark: https://gregtatum.github.io/taskcluster-tools/benchmark.html

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/be323c04c7dd
Change the TranslationsDocument to use computed inline values r=translations-reviewers,nordzilla

Comment 10

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/be323c04c7dd

Comment 11

[Greg Tatum [:gregtatum]]

Release Note Request
[Why is this notable]:

This improved the translations algorithm in a way that closed 12 bugs about broken translations. The results were really noticeably better.

[Suggested wording]:

The translations feature received an improvement to the quality of translated webpages. The results should be much more stable. This fixes issues where the content of a page could disappear when translated, or interactive widgets could break.

[Links (documentation, blog post, etc)]:

The translation feature is documented here: https://support.mozilla.org/en-US/kb/website-translation

Comment 12

[Donal Meehan [:dmeehan]]

Thanks, added to the Fx122 nightly release notes, please allow 30 minutes for the site to update.
Keeping the relnote-firefox flag as ? to keep it on the radar for inclusion in the final Fx122 release notes

Comment 14

[Marco Castelluccio [:marco]]

(In reply to Greg Tatum [:gregtatum] from comment #11)

This improved the translations algorithm in a way that closed 12 bugs about broken translations. The results were really noticeably better.

13 ;)