1415214 - Text decorations (underline, strikethrough, etc) are intermittently missing on recent Nightlies

Status: VERIFIED FIXED

(Core :: Layout: Text and Fonts, defect, P2)

Description

[Ryan VanderMeulen [:RyanVM]]

[Tracking Requested - why for this release]: Text rendering regression

I don't have a reliable testcase for this yet, but I've seen this issue on Bugzilla, GMail, and gdocs at least. Doing some IRC debugging with Jonathan, Stylo on/off doesn't affect this. Zooming *out* makes the decorations appear, but zooming *in* doesn't.

I'm on Win10. If/when I can find a public site that reproduces, I'll update the bug.

Comment 1

[Jonathan Kew [:jfkthame]]

Also seen on Linux, according to jcristau.

Comment 2

[Jonathan Kew [:jfkthame]]

Also seen on macOS, now that I updated the Nightly I was using to try it.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

On Win10, the testcase below reliably reproduces for me (though it apparently didn't for jcristau on Linux).
https://docs.google.com/document/d/191lO7n0jOpchFYMKOG7ZtYALKveP1YZxbQLKT52iY5M/edit#heading=h.wo5c95ft1jdl

Using mozregression, I was able to confirm bug 1399310 as the culprit.

Comment 4

[Jonathan Kew [:jfkthame]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #3)
> On Win10, the testcase below reliably reproduces for me (though it
> apparently didn't for jcristau on Linux).
> https://docs.google.com/document/d/
> 191lO7n0jOpchFYMKOG7ZtYALKveP1YZxbQLKT52iY5M/edit#heading=h.wo5c95ft1jdl

This also reproduces for me on macOS, though the exact failure depends on zoom level (meaning browser zoom, not google-docs document zoom): if I zoom in to 110% or larger, an additional link fails.

Behavior is also dependent on the scroll position of the document. For more fun, enable webrender: then you can watch the underlines appear/disappear as the document is scrolled up and down, as the failure apparently kicks in at a certain place in the window. I'm guessing there's a connection to tiled painting, or something like that, whereby the clip ends up wrong in some tiles and right in others.

Comment 6

[Marcia Knous [:marcia]]

Tracking 58+ for this regression.

Comment 7

[Chris Peterson [:cpeterson]]

I see this problem on macOS without zooming. On Hacker News, the first couple links on a page are properly underlined, but subsequent links are not underlined. I bisected this regression to bug 1399310.

For example, on the following page, the link "http://2ality.com/2017/02/ecmascript-2018.html" is underlined, but the second link "http://www.ecma-international.org/publications/files/ECMA-ST..." is not underlined.

https://news.ycombinator.com/item?id=15644798

Comment 8

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

STR:
1. Open this URL: https://bugzilla.mozilla.org/show_bug.cgi?id=1400259
2. Scroll down to about 1/4 of website page by mouse scroll
3. Scroll up to top of website page by mouse scroll
and see that RESOLVED and VERIFIED bugs aren't crossed out anymore

Workaround:
Moving mouse pointer/cursor on these links restore crossing out

Comment 9

[Chih-Hsuan Kuo [:tommykuo]]

I'll take a look at it today.

Comment 10

[Chih-Hsuan Kuo [:tommykuo]]

Attachment: text-decoration.html

The decoration lines seem to disappear under a specific position.

Comment 11

[Chih-Hsuan Kuo [:tommykuo]]

I think sometimes the clip rect would be wrong.

Comment 12

[Chih-Hsuan Kuo [:tommykuo]]

Attachment: Bug 1415214 - (Part 1) Fix the bound of the clip rect for drawing decoration lines

Review commit: https://reviewboard.mozilla.org/r/197566/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/197566/

Comment 13

[Chih-Hsuan Kuo [:tommykuo]]

I found the position of the rect is wrong, and we should use the framePt as the position to make the clipRect be put at the correct position.

Comment 14

[Code Review Bot [:reviewbot]]

Comment on attachment 8926289 [details]
Bug 1415214 - (Part 1) Fix the bound of the clip rect for drawing decoration lines

https://reviewboard.mozilla.org/r/197566/#review202820


C/C++ static analysis found 0 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

Comment 15

[Jonathan Kew [:jfkthame]]

Comment on attachment 8926289 [details]
Bug 1415214 - (Part 1) Fix the bound of the clip rect for drawing decoration lines

https://reviewboard.mozilla.org/r/197566/#review202842

Yes, that makes sense - thanks for the quick fix!

Comment 16

[Chih-Hsuan Kuo [:tommykuo]]

After ran a try[1], I think this patch make the clip rect too small and cause lots of problems. I found the the wavy overline, the underline with vertical-align, etc... are clipped. I think we maybe we should enlarge the vertical size (in horizontol writing mode) to entire page to make it correct.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e97d1c0e527b466532eff9ce44d769aa33a13c62&selectedJob=142979152

Comment 17

[Chih-Hsuan Kuo [:tommykuo]]

Is there any way to get the full height/width of the current page? I tried to use [1] and got -1, but it seems to not be set when we called it.

[1]: https://searchfox.org/mozilla-central/rev/ed212c79cfe86357e9a5740082b9364e7f6e526f/layout/base/nsPresContext.h#488

Comment 18

[Chih-Hsuan Kuo [:tommykuo]]

I also tried to use the size of context, but the issue of decoration lines missing when scrolling still exist. Do you have any suggestion on this?

```
const gfx::IntSize contextSize = params.context->GetDrawTarget()->GetSize();
```

Comment 19

[Jonathan Kew [:jfkthame]]

(In reply to Tommy Kuo [:kuoe0] at UTC+8 from comment #16)
> I think we maybe we should enlarge the
> vertical size (in horizontol writing mode) to entire page to make it correct.

That sounds like it could work, but it seems a bit hacky; we should be able to have the proper rect for the decoration we're actually drawing. The baselineOffset of the decoration probably needs to be taken into account, and the total height of the decoration line(s).

The exact rect of the decoration should be available from nsCSSRendering::GetTextDecorationRect, actually; and then we'd just need to trim the inline start/end to do the required clipping. Maybe investigate that approach?

Comment 20

[Jonathan Kew [:jfkthame]]

Comment on attachment 8926289 [details]
Bug 1415214 - (Part 1) Fix the bound of the clip rect for drawing decoration lines

Cancelling review here, given that tryserver didn't entirely like it.

Comment 21

[Jonathan Kew [:jfkthame]]

(In reply to Jonathan Kew (:jfkthame) from comment #19)
> The exact rect of the decoration should be available from
> nsCSSRendering::GetTextDecorationRect, actually; and then we'd just need to
> trim the inline start/end to do the required clipping. Maybe investigate
> that approach?

On second thought, this probably isn't the best approach. GetTextDecorationRect would give us the rect for a single decoration line, but we may want the clip here to include both an overline and underline (for example); but it would be extra work if we had to call GetTextDecorationRect for all the decorations and union them, or reset the clip separately for each line.

A better option might be to start with GetVisualOverflowRect for the frame, because (if I remember correctly) the visual overflow area will have been enlarged (if necessary) to include the area needed to paint any decorations that are present.

Comment 22

[Chih-Hsuan Kuo [:tommykuo]]

Attachment: Bug 1415214 - (Part 2) Add test case for the regression

Review commit: https://reviewboard.mozilla.org/r/197998/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/197998/

Comment 23

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8926289 [details]
Bug 1415214 - (Part 1) Fix the bound of the clip rect for drawing decoration lines

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/197566/diff/1-2/

Comment 24

[Chih-Hsuan Kuo [:tommykuo]]

Hi Johnathan, please review the patches again! I use GetVisualOverflowRect() to get the correct rect size that we need and apply the framePt as the offset to make the clip rect be correct. And I also add a test case for this regression. I have tried it on try server and it would fail without the part 1 patch.

Comment 25

[Chih-Hsuan Kuo [:tommykuo]]

The try result[1] looks great.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d97814ebb14d621f042c894ee0bc5e8180b29a1

Comment 26

[Jonathan Kew [:jfkthame]]

Comment on attachment 8926289 [details]
Bug 1415214 - (Part 1) Fix the bound of the clip rect for drawing decoration lines

https://reviewboard.mozilla.org/r/197566/#review203258

Looks good!

Comment 27

[Jonathan Kew [:jfkthame]]

Comment on attachment 8926759 [details]
Bug 1415214 - (Part 2) Add test case for the regression

https://reviewboard.mozilla.org/r/197998/#review203264

Thanks for adding a test; just one detail I'd like adjusted, please:

::: layout/reftests/text-decoration/reftest.list:115
(Diff revision 1)
>  == 1133392.html 1133392-ref.html
>  != 1159729-offset-adjustment.html 1159729-offset-adjustment-notref.html
>  == emphasis-style-dynamic.html emphasis-style-dynamic-ref.html
>  == vertical-mode-decorations-1.html vertical-mode-decorations-1-ref.html
>  == vertical-mode-decorations-2.html vertical-mode-decorations-2-ref.html
> +!= 1415214.html 1415214-ref.html

For a != test, please name the reference file with the -notref suffix (instead of -ref). This helps to avoid confusing people who come back to the testcases later and automatically assume pairs of testcase and -ref files are expected to match (without necessarily checking the manifest).

Comment 28

[Johann Hofmann [:johannh]]

FWIW, this was also caught by mozscreenshots and seems to indeed affect Linux and MacOS.

https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=8b9167b8a937e09f6673d4d7d693c6ac1c25a3d7&newProject=mozilla-central&newRev=70f38f59f9fa45dda5d70d44881dc1d906de15f6

Comment 29

[Ryan VanderMeulen [:RyanVM]]

I was hoping to autoland this so we could get it into today's second nightlies still, but there's an unresolved issue in MozReview preventing it :(

Comment 31

[Daniel]

My issue #1416079 "text-decoration: underline; not working after using text-decoration-style: double; on alpha transparent background" may be a duplicate of this issue. There is a minimal test case attached to that issue for macOS and Windows (can’t reproduce it on Linux).

Comment 32

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8926759 [details]
Bug 1415214 - (Part 2) Add test case for the regression

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/197998/diff/1-2/

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbd2b14a7466
(Part 1) Fix the bound of the clip rect for drawing decoration lines. r=jfkthame
https://hg.mozilla.org/integration/mozilla-inbound/rev/54a656b4c587
(Part 2) Add test case for the regression. r=jfkthame

Comment 34

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/257e3b6e6cf8 for failures in the test, at least so far on Linux, like https://treeherder.mozilla.org/logviewer.html#?job_id=143577562&repo=mozilla-inbound

Comment 35

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8926759 [details]
Bug 1415214 - (Part 2) Add test case for the regression

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/197998/diff/2-3/

Comment 36

[Chih-Hsuan Kuo [:tommykuo]]

The renamed file is not checked-in. Fixed now.

Comment 37

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8926289 [details]
Bug 1415214 - (Part 1) Fix the bound of the clip rect for drawing decoration lines

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/197566/diff/2-3/

Comment 38

[Chih-Hsuan Kuo [:tommykuo]]

Comment on attachment 8926759 [details]
Bug 1415214 - (Part 2) Add test case for the regression

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/197998/diff/3-4/

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/647c6595838e
(Part 1) Fix the bound of the clip rect for drawing decoration lines. r=jfkthame
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd5fb1626441
(Part 2) Add test case for the regression. r=jfkthame

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0864306d64dc
(Part 1) Fix the bound of the clip rect for drawing decoration lines r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/5ba750f62b36
(Part 2) Add test case for the regression r=jfkthame

Comment 41

[Chih-Hsuan Kuo [:tommykuo]]

Hi :philor,

I found autoland land the wrong patches in comment 40. Could you back it out?

Comment 42

[Chih-Hsuan Kuo [:tommykuo]]

And the correct patches was already landed in comment 39.

Comment 43

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/476c4d0aadcc
Backed out 2 changesets for one request from developer r=backout on a CLOSED TREE

Comment 44

[Ryan VanderMeulen [:RyanVM]]

(In reply to Tommy Kuo [:kuoe0] at UTC+8 from comment #41)
> Hi :philor,
> 
> I found autoland land the wrong patches in comment 40. Could you back it out?

That's because it was already queued to push to autoland prior to the push in comment 33. In the future, please double-check MozReview before manually pushing to inbound as it'll tell you when that's the case.

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/647c6595838e
https://hg.mozilla.org/mozilla-central/rev/cd5fb1626441

Comment 46

[Chris Peterson [:cpeterson]]

Verified fixed in 58 and 59.