1287075 - Graphics artifacts in Australis panels, when I open sidebars

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[arni2033]

Attachment: screencast 1 - Graphics artifacts in Australis panels.webm

>>>   My Info:   Win7_64, Nightly 50, 32bit, ID 20160714030208 (2016-07-14)
STR:
1. Click Australis Menu button in toolbar
2. Click on (?) button in Australis Menu
3. Wait 5 seconds w/o moving mouse

AR:  Graphic artifacts
ER:  No graphic artifacts

This is regression from bug 1273250. Regression range:
> https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=abfe6f9b1e082aff84afcf83337747386a2031ef&tochange=eeee65feb9ade388336f2a67786bd444ad58fd64

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

[Tracking Requested - why for this release]: This is a very visible regression in the Firefox menu when opening any of the subviews such as History, Developer, Synced Tabs, Bookmarks, Forget, and others and we shouldn't ship Firefox with it.

Botond, can you take this?

Comment 2

[Botond Ballo [:botond]]

Thanks, I'll investigate.

Comment 3

[Botond Ballo [:botond]]

Interestingly, I only see the particular glitch depicted in the video on Windows. On Linux, I see a much milder and harder-to-see glitch. Based on the timing I assume it's the same underlying problem.

Comment 4

[Botond Ballo [:botond]]

I've traced the problem to the Part 4 patch of bug 1273250.

That patch took code in FrameLayerBuilder that looked like this:

  bool clipMovesWithLayer = (animatedGeometryRoot == animatedGeometryRootForClip);

  bool shouldFixToViewport = !clipMovesWithLayer &&
      !(*animatedGeometryRoot)->GetParent() &&
      item->ShouldFixToViewport(mBuilder);

and simplified shouldFixToViewport to be just:

  bool shouldFixToViewport = !clipMovesWithLayer;

(and then inlined this definition into use sites).

At the time we believed this to be just a refactoring, but the fact that this caused a regression suggests that the second two conditions are important.

In the affected scenario where those extra conditions would have made shouldFixToViewport false, 'item' has type nsDisplayTransform. I note that nsDisplayTransform was changed to override AnimatedGeometryRootForScrollMetadata() (thus making animatedGeometryRoot != animatedGeometryRootForClip) in bug 1240073.

Comment 5

[Botond Ballo [:botond]]

Markus, as a simple, low-risk (as in, suitable to uplift to 49) fix, could we simply back out this Part 4 patch?

Comment 6

[Markus Stange [:mstange]]

Good idea, yes, let's do that.

Comment 7

[Botond Ballo [:botond]]

Attachment: Bug 1287075 - Back out bug 1273250, part 4 for regressing the rendering of the Australis submenus.

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

Comment 8

[Botond Ballo [:botond]]

^ This is a backout of the Part 4 patch, rebased to apply to current trunk.

Comment 9

[Markus Stange [:mstange]]

Comment on attachment 8777510 [details]
Bug 1287075 - Back out bug 1273250, part 4 for regressing the rendering of the Australis submenus.

https://reviewboard.mozilla.org/r/69048/#review66132

Comment 10

[Botond Ballo [:botond]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef43c0468372

Comment 11

[Botond Ballo [:botond]]

Confirmed that the backout fixes the Windows glitch as well in a local build.

Comment 12

[Botond Ballo [:botond]]

> as a simple, low-risk (as in, suitable to uplift to 49) fix, could
> we simply back out this Part 4 patch?

(Markus and I discussed the possibility of implementing a more proper fix afterwards, and decided that it's not worth doing, because Markus' work in bug 1214146 will rewrite the affected code anyways.)

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/354e5ccbd5b9
Back out bug 1273250, part 4 for regressing the rendering of the Australis submenus. r=mstange

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Thanks for the quick fix!

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/354e5ccbd5b9

Comment 16

[Botond Ballo [:botond]]

The fix should now be in the latest nightly. Arni, could you verify that you no longer see the glitch in the latest nightly? Thanks!

Comment 17

[arni2033]

Fixed on:   Win7_64, Nightly 51, 32bit, ID 20160805030444 (2016-08-05)

Comment 18

[Botond Ballo [:botond]]

Great, thanks for testing! Let's get the fix uplifted then.

Comment 19

[Botond Ballo [:botond]]

Comment on attachment 8777510 [details]
Bug 1287075 - Back out bug 1273250, part 4 for regressing the rendering of the Australis submenus.

Approval Request Comment
[Feature/regressing bug #]:
  Bug 1273250 

[User impact if declined]:
  When a submenu of the hamburger ("Australis") menu (such as
  History, Developer, or Help) is opened, a graphical glitch
  (a few extra columns of pixels being rendered on the left-
  hand side of the window) can be seen briefly. The glitch goes
  away after a second or so, but it's still unsightly.

[Describe test coverage new/current, TreeHerder]:
  The fix was verified by me locally, and by the reporter.

[Risks and why]: 
  Low. The fix reverts one of the patches of the regressing bug,
  thus restoring the affected part of the code to a previous 
  state that was known to work.

[String/UUID change made/needed]:
  None.

Comment 20

[Gerry Chang [:gchang]]

Comment on attachment 8777510 [details]
Bug 1287075 - Back out bug 1273250, part 4 for regressing the rendering of the Australis submenus.

This patch fixes a regression of Australis submenus. Take it in 49 beta and aurora.

Comment 21

[Gerry Chang [:gchang]]

Track 49+ as this is a visible regression for Australis submenu.

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d7f9d73f11d8

Comment 23

[Carsten Book [:Tomcat]]

has problems applying to beta

warning: conflicts while merging layout/base/FrameLayerBuilder.cpp! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')

Comment 24

[Botond Ballo [:botond]]

Rebased onto beta and landed a=gchang:

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

Comment 25

[David Bolter [:davidb] (NeedInfo me for attention)]

Thanks!