1612610 - Titles with emojies wierd behavior with fullscreen

Status: VERIFIED FIXED

(Core :: Graphics: Text, defect, P3)

Description

[Nikolas Spiridakis]

Attachment: Screen record. Demonstation of the bug

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:72.0) Gecko/20100101 Firefox/72.0

Steps to reproduce:

1. Open a new tab
2. Enter a url of a site which has emojis on the title BUT it should have emojies on the part of the title which is not shown (lets say emojipedia.org)
3. Make the tab last in order (to the left of the plus button)
4. In another tab (shouldnt be last in order) open a site with fullsceen support (lets say a youtube video) and then enter fullscreen and exit it

Actual results:

All the emojies of the part of the title which is not shown, show up to the right of the plus button
A video is attached for reference

Expected results:

Not show these emojies

Comment 1

[Peter Magyari (Desktop QA)]

Hi Nikolas,

I was able to reproduce this issue using Firefox 72.0.2, Firefox Beta 73 and Firefox Nightly 74.0a1 on MacOS 10.14 and MacOS 10.15.

I'm setting a component in order to involve the development team in reviewing this issue.

Thank you for reporting!

Comment 2

[Dão Gottwald [:dao]]

Peter, could you help us find a regression range here?

Comment 3

[Oana Botisan, Desktop QA]

I managed to reproduce the issue on macOS 10.15 on Latest Nightly 74.0a1.
I ran mozregression to find the pushlog. These are the results:

• Last good revision: caf55914ccddba34d462a1206530d7868b6c4992
• First bad revision: caf55914ccddba34d462a1206530d7868b6c4992
• Pushlog:
  https://hg.mozilla.org/mozilla-central/pushloghtml?changeset=caf55914ccddba34d462a1206530d7868b6c4992

Comment 4

[Julien Cristau [:jcristau]]

Last good and first bad being the same doesn't sound right, caf5591 can't be good and bad at the same time?

Comment 5

[Oana Botisan, Desktop QA]

I'm sorry, I haven't noticed that.
I redid the regression, but mozregression gave me the same results, but the program did skipped a lot of builds. Maybe something doesn't work right with those.
Maybe the dates will help more:

• Last good: 2019-11-13
• First bad: 2019-11-14

Comment 6

[Dão Gottwald [:dao]]

Can we please get a pushlog link in the following format as provided by mozregression? https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=______&tochange=______

Comment 7

[Oana Botisan, Desktop QA]

I hope this pushlog helps. I had to create it manually, because mozregression kept giving me the regression from comment 3.
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=2f19e7b646e0a52fa855b75c868f0a3f3a990ad3&tochange=a4ef4d6cdff03e76ed4471faea08e1071b1bf05c

Comment 8

[Nikolas Spiridakis]

Let me add something to the report. This also happens if the tab is not last in order but the title has to be long enough to go through the other tabs (and emojies have to be on the end of the tile) in order this to happen.
Imagine the title is being continued throughout the other tabs but only emojies show up between the + button and the right end of the tab bar. Hope this helps

Comment 9

[Dão Gottwald [:dao]]

(In reply to Oana Botisan, Desktop Release QA from comment #7)

I hope this pushlog helps. I had to create it manually, because mozregression kept giving me the regression from comment 3.
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=2f19e7b646e0a52fa855b75c868f0a3f3a990ad3&tochange=a4ef4d6cdff03e76ed4471faea08e1071b1bf05c

I don't see anything suspicious there on first glance.

Anyway, this looks like a layout or graphics issue to me. I'm moving this bug to Layout for now.

FYI, tab label overflow is implemented here: https://searchfox.org/mozilla-central/rev/a1592902acabf9bded973067133baaac1457f3d3/browser/base/content/tabbrowser.css#41-45

Comment 10

[Erik Nordin [:nordzilla]]

Hi. Unfortunately I am unable to reproduce this locally using the same method in your video.

For anyone who is able to reproduce, can you check if this preference in about:config makes any difference?
gfx.webrender.force-disabled

Comment 11

[Jonathan Kew [:jfkthame]]

I can reproduce this (on macOS 10.15.2). My system is not using WebRender by default. If I force-enable WR by setting the gfx.webrender.all pref, then the issue does not reproduce any longer.

Smells more like Graphics than Layout to me (or maybe something nearer the front-end).

Comment 12

[Jonathan Kew [:jfkthame]]

mozregression tells me this regressed in https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=16bc115ecd46ed37ff921ed4273532c16d3b0205&tochange=7071b1bdd74110deca5dea19250a2d571223d698 which means this is a regression from my patch in bug 1589888.

Comment 13

[Jonathan Kew [:jfkthame]]

The bug here occurs only if there is a run of color glyphs that begins beyond the place where the tab title text is (supposed to be) clipped:

If I have a tab title that is a long string of only emojis, so that the entire (overflowing) title is a single emoji run, the problem doesn't occur.

If I break up the emoji run by inserting a regular ASCII letter, but near the beginning (such that the overflowing emoji run still begins within the width of the tab), the problem still doesn't occur.

But once the ASCII letter is moved later in the title, so that the trailing emoji run doesn't begin until beyond the edge of the tab, then the bug happens.

So the issue seems to be related to the bounding box passed to PushSolidColor here:
https://searchfox.org/mozilla-central/rev/7e92a667e3829831c31e8d46aefe7ef67ad5be1c/gfx/thebes/gfxTextRun.cpp#625-626
The buffer is then composited here:
https://searchfox.org/mozilla-central/rev/7e92a667e3829831c31e8d46aefe7ef67ad5be1c/gfx/thebes/gfxTextRun.cpp#659-661
and what seems to be happening is that if the X-coordinate of the bounding box was beyond the width of the tab, such that we'd expect it to be entirely clipped away, it actually fails to clip at all and the spurious glyphs appear.

If I hack the metrics.mBoundingBox rect here such that we keep its X-coordinate pinned to the original left edge of the entire text, the issue no longer happens. But obviously that's not a desirable fix, as it means we'd be buffering and compositing a bunch of extra empty space to the left of the actual run of interest.

It seems to me the problem here is somewhere below the actual text-painting code, in the layer buffering, compositing, etc... Lee, any ideas why we'd fail to clip the layer that's being used to double-buffer the glyph painting here, only when it does not intersect the actual tab title rect (and only when transitioning back from full-screen mode)?

Note that the bug is not triggered by transitioning to and from the Firefox "full-screen" display (View / Enter Full Screen); it only happens when transitioning back from full-screening an element like a YouTube video, where the browser chrome (including tab titles) disappears entirely.

Comment 14

[Lee Salzman [:lsalzman]]

If I'm understanding what you're saying, the clip bounds and the run bounds aren't intersecting, so that when Skia intersects them, maybe it is producing an empty bounds. And in gfxContext, when we go to PushGroupForBlendBack, I think Skia is messing up here...

I have two theories, though the first one is unlikely:

1. Maybe Skia's clipping code is misinterpreting an empty clip? That doesn't look like it's the case through a scan of Skia's clip code, though...
   or the more likely:
2. It looks like in SkCanvas::internalSaveLayer, if it detects the bounds of the new layer to be pushed are empty, it never creates a new surface/device to back the layer (on purpose), where somehow any subsequent drawing just lands into the layer sitting below it... Though theoretically the clip stack should still be preventing that, unless it is a mixture of scenario 1 and 2 somehow? :/

Comment 15

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

The component has been changed since the backlog priority was decided, so we're resetting it.
For more information, please visit auto_nag documentation.

Comment 16

[Jonathan Kew [:jfkthame]]

(In reply to Lee Salzman [:lsalzman] from comment #14)

Yes, it looks to me like something on those lines is happening.

From a little poking around, I'm suspicious that the mContext->SnappedClip call at https://searchfox.org/mozilla-central/rev/a4be2fbe9bd4f405c91cc16e4e3a80400f5a9301/gfx/thebes/gfxTextRun.cpp#525 doesn't work as expected when the new clip being applied does not intersect with the existing clip, so the result should be an empty clip.

In particular, if I comment out that SnappedClip call in PushSolidColor, the bug here no longer occurs; or if I hack the parameters to SnappedClip so that I use 0 instead of aBounds.X(), and replace aBounds.Width() with aBounds.XMost() (so in other words, always "extend" the given bounds leftwards to the origin), the bug no longer occurs.

So it seems like gfxContext::SnappedClip (or just Clip, the snapping isn't important) isn't working correctly if the X-coordinate of the new clip is greater than the XMost of the existing clip extents; we end up failing to clip the subsequent drawing at all. But I'm not sure where the brokenness actually originates.

This only seems to happen on macOS using the Skia drawtarget; I haven't been able to reproduce on other platforms.

Comment 17

[Jonathan Kew [:jfkthame]]

I don't actually understand what's going wrong here - it seems to be related to skia and clipping, as described above, but I haven't pinned down exactly why it fails. Maybe I just don't understand how the boundingBox being passed to PushSolidColor() is supposed to behave.

I do have a workaround that fixes the bad display here, by partially reverting to how things worked before bug 1589888. Specifically, instead of computing a separate bounding box for each individual run of coloured or synthetic-bold text, if we compute the bounds of the entire range that gfxTextRun::Draw is processing, and pass this to PushSolidColor() each time, the problem no longer occurs.

While I still don't properly understand the bug here, the workaround seems safe enough that perhaps we should take it to quickly resolve this visual glitch - unless you can figure out what's really going on inside Skia here, Lee?

Comment 18

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1612610 - Use the bounding box of the full textrun range, not just an individual glyph run, when double-buffering for color/bold drawing. r=lsalzman

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/76dfb8c9daf7
Use the bounding box of the full textrun range, not just an individual glyph run, when double-buffering for color/bold drawing. r=lsalzman

Comment 20

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/76dfb8c9daf7

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider taking on Beta?

Comment 22

[Jonathan Kew [:jfkthame]]

It's a fairly obscure edge-case issue, IMO, but on the other hand it's visually pretty bad when it happens, and the patch is straightforward... so I'd be inclined to say yes, I think.

Comment 23

[Jonathan Kew [:jfkthame]]

Comment on attachment 9126708 [details]
Bug 1612610 - Use the bounding box of the full textrun range, not just an individual glyph run, when double-buffering for color/bold drawing. r=lsalzman

Beta/Release Uplift Approval Request

• User impact if declined: Possible visual glitch when restoring window from full-screen and tab title has emoji
• Is this code covered by automated tests?: Unknown
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: As described in comment 0.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Straightforward patch that restores previous behavior of using the whole-text bounding box for compositing.
• String changes made/needed:

Comment 24

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9126708 [details]
Bug 1612610 - Use the bounding box of the full textrun range, not just an individual glyph run, when double-buffering for color/bold drawing. r=lsalzman

Edge case regression fix but the patch has tests and we are reverting to a previous correct logic, uplift approved for 74.0b6, thanks.

Comment 25

[Peter Magyari (Desktop QA)]

Verified fixed on Firefox Nightly 75.0a1 (20200220093508)
I'll be back to check on Beta once it gets uplifted.

Comment 26

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f2ee6d14680e

Comment 27

[Peter Magyari (Desktop QA)]

Verified fixed on Firefox Beta 74.0b7 (20200221211950)