Closed Bug 1415214 Opened 7 years ago Closed 7 years ago

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

Categories

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

Product:
Core
Component:
Layout: Text and Fonts
Version:
58 Branch
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- unaffected
firefox57 --- unaffected
firefox58 + verified
firefox59 --- verified

People

(Reporter: RyanVM, Assigned: kuoe0.tw)

References

Details

(Keywords: regression)

Attachments

(3 files)

text-decoration.html
2.02 KB, text/html
Details
Bug 1415214 - (Part 1) Fix the bound of the clip rect for drawing decoration lines
59 bytes, text/x-review-board-request
jfkthame
: review+
Details
Bug 1415214 - (Part 2) Add test case for the regression
59 bytes, text/x-review-board-request
jfkthame
: review+
Details 
[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.
Flags: needinfo?(kuoe0)
Tracking 58+ for this regression.
tracking-firefox58: ? → +
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.
Flags: needinfo?(kuoe0)
Attached file text-decoration.html — 
The decoration lines seem to disappear under a specific position.
I think sometimes the clip rect would be wrong.
Assignee: nobody → kuoe0
Attachment #8926289 - Flags: review?(jfkthame)
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!
Attachment #8926289 - Flags: review?(jfkthame) → review+
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
Flags: needinfo?(jfkthame)
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
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?
Flags: needinfo?(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.
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.
Priority: -- → P2
Attachment #8926759 - Flags: review?(jfkthame)
Attachment #8926289 - 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.
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!
Attachment #8926289 - Flags: review?(jfkthame) → review+
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+
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 :(
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).
Keywords: checkin-needed
Pushed by mtseng@mozilla.com:
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
Keywords: checkin-needed
The renamed file is not checked-in. Fixed now.
Flags: needinfo?(kuoe0)
Keywords: checkin-needed
Keywords: checkin-needed
Keywords: checkin-needed
Pushed by mtseng@mozilla.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
Keywords: checkin-needed
Pushed by tokuo@mozilla.com:
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?
Flags: needinfo?(philringnalda)
And the correct patches was already landed in comment 39.
Backout by nerli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/476c4d0aadcc
Backed out 2 changesets for one request from developer r=backout on a CLOSED TREE
Flags: needinfo?(philringnalda)
(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.
https://hg.mozilla.org/mozilla-central/rev/647c6595838e
https://hg.mozilla.org/mozilla-central/rev/cd5fb1626441
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Verified fixed in 58 and 59.
Status: RESOLVED → VERIFIED
status-firefox58: fixed → verified
status-firefox59: --- → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: