Closed Bug 1415214 Opened 2 years ago Closed 2 years ago
Text decorations (underline, strikethrough, etc) are intermittently missing on recent Nightlies
2.02 KB, text/html
59 bytes, text/x-review-board-request
59 bytes, text/x-review-board-request
[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.
Also seen on Linux, according to jcristau.
Also seen on macOS, now that I updated the Nightly I was using to try it.
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.
(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.
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
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
I'll take a look at it today.
The decoration lines seem to disappear under a specific position.
I think sometimes the clip rect would be wrong.
Assignee: nobody → kuoe0
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 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 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!
After ran a try, 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. : https://treeherder.mozilla.org/#/jobs?repo=try&revision=e97d1c0e527b466532eff9ce44d769aa33a13c62&selectedJob=142979152
Is there any way to get the full height/width of the current page? I tried to use  and got -1, but it seems to not be set when we called it. : https://searchfox.org/mozilla-central/rev/ed212c79cfe86357e9a5740082b9364e7f6e526f/layout/base/nsPresContext.h#488
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(); ```
(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 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.
Attachment #8926289 - Flags: review+
(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.
Attachment #8926759 - Flags: review?(jfkthame)
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.
The try result looks great. : https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d97814ebb14d621f042c894ee0bc5e8180b29a1
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 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).
Attachment #8926759 - Flags: review?(jfkthame) → review+
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
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 :(
Duplicate of this bug: 1415998
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).
Pushed by firstname.lastname@example.org: 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
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
The renamed file is not checked-in. Fixed now.
Pushed by email@example.com: 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
Pushed by firstname.lastname@example.org: 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
Hi :philor, I found autoland land the wrong patches in comment 40. Could you back it out?
And the correct patches was already landed in comment 39.
Backout by email@example.com: https://hg.mozilla.org/integration/autoland/rev/476c4d0aadcc Backed out 2 changesets for one request from developer r=backout on a CLOSED TREE
(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.
Verified fixed in 58 and 59.
You need to log in before you can comment on or make changes to this bug.