1644264 - `text-overflow: ellipsis` unexpectedly blinks in twitter.com

Status: VERIFIED FIXED

(Core :: Web Painting, defect, P2)

Description

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: ellipsis-twitter.webm

1. Access https://twitter.com/ActionMovieKid/status/1269353147010433024 and play the video
2. Move your mouse to hover over the video
3. See the ellipsis blinks

Comment 1

[Cameron McCormack (:heycam)]

Thanks, I can reproduce. Feels like it will be related to the -webkit-line-clamp work in bug 866102, so I should take a look at this.

Comment 2

[Cameron McCormack (:heycam)]

 5:58.74 INFO: No more integration revisions, bisection finished.
 5:58.74 INFO: Last good revision: 29241bff52099a13245515293d5e409a0a19d15b
 5:58.74 INFO: First bad revision: 3b85097d2d630ad354deed0925347cb2a5645d88
 5:58.74 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=29241bff52099a13245515293d5e409a0a19d15b&tochange=3b85097d2d630ad354deed0925347cb2a5645d88

This points to bug 1615811. I'll try to reduce the test case I have from my Twitter feed.

Comment 3

[Cameron McCormack (:heycam)]

Attachment: reduced test

When loading this page, the ellipsis will only briefly be shown before disappearing. After this, opening devtools and mousing around will cause it to be shown.

Comment 4

[Cameron McCormack (:heycam)]

Attachment: reduced test

Comment 5

[Cameron McCormack (:heycam)]

Emilio, do you have time to look at this?

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Sure, thanks for the reduced test-case (specially since I couldn't reproduce on Twitter to begin with).

Comment 7

[Kagami Rosylight [:saschanaz] (they/them)]

Is the test expected to overflow, because the text is shorter than 200px on my machine and thus does not? 👀

Interestingly I still can't repro with the testcase when I insert longer text.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

So I can't repro on a local build, which means this is going to be fun to debug... yay.

Matt, so in bug 1615811 we decided to call SchedulePaint(PAINT_DEFAULT, /* aFrameChanged = */ false), on the expectation that for cases like these we'd rebuild the display list for the text and figure out that we need / don't need a new ellipsis, see bug 1615811 comment 8.

However, in this case, the overflow changes because flex reflows the frame twice, without any content change, so I suspect for this case nothing guarantees that we go ahead and enter the block's display-list building code.

Do we have any facility to build the display list for a given frame without throwing away the display list for the whole subtree? Alternatively maybe I should be detecting whether this is an intermediate layout or something, I guess, but that's a bit hard as we don't keep around the state from before the intermediate layout.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1644264 - Mark frame with text-overflow ellipsis as modified when overflow changes. r=mattwoodrow

This prevents us not descending to rebuild the display list if the
overflow changes during an intermediate layout like a flex layout.

To capture Matrix discussion: This is a bit unfortunate, but the idea is
that text-overflow is usually pretty small (at most one line of content)
so we can probably live with this.

An alternative would be to do something like
nsDisplayListBuilder::MarkFrameForDisplayIfVisible, which could work as
well.

Note that this is a bit of a speculative fix because neither me or
Cameron have been able to reproduce the issue in local builds (I tried
all the combinations of gfx-things that I could think about). :-(

If this doesn't fix it on next nightly, we should back out and try to
repro some other way, I guess. But the hypothesis of why it happens
makes sense to me, and if it's correct it should fix the issue.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ba429413f7a6
Mark frame with text-overflow ellipsis as modified when overflow changes. r=mattwoodrow

Comment 11

[Bogdan Tara[:bogdan_tara | bogdant]]

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

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:emilio, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9155480 [details]
Bug 1644264 - Mark frame with text-overflow ellipsis as modified when overflow changes. r=mattwoodrow

Beta/Release Uplift Approval Request

• User impact if declined: Some ellipsis don't get properly updated.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: comment 0 or the attached test-case
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): one-liner that invalidates more aggressively.
• String changes made/needed: none

Comment 14

[Julien Cristau [:jcristau]]

Comment on attachment 9155480 [details]
Bug 1644264 - Mark frame with text-overflow ellipsis as modified when overflow changes. r=mattwoodrow

approved for 78.0b8

Comment 15

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/770b54605162

Comment 16

[Peter Magyari (Desktop QA)]

Hi,

Since I wasn't able to reproduce the issue either on Twitter or by using the reduced test-case I cannot verify the fix myself.
Kagami, would you mind verifying it on the latest Firefox Nightly?

Thanks!

Comment 17

[Kagami Rosylight [:saschanaz] (they/them)]

I haven't seen the issue on latest builds 👍

Comment 18

[Peter Magyari (Desktop QA)]

Cameron, would you mind verifying the fix on the latest Nightly and latest Beta with the test case from Comment 3 as well?
The fix should have also landed on Beta 78.0b8 today.

Thank you!

Comment 19

[Cameron McCormack (:heycam)]

Yes, this is fixed for me.

Comment 20

[Peter Magyari (Desktop QA)]

Thanks for verifying!