1270023 - transform animation make numbers of ordered list disappear

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[fritzmg]

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
Build ID: 20160502172042

Steps to reproduce:

Create a key frame animation with transforms and apply it to an ordered list (<ol>).




Actual results:

The numbering of the ordered list disappears during the transform animation.

Examples:

https://jsfiddle.net/vn5c57gg/
https://jsfiddle.net/4cssxfqh/

Example showing that it only happens with transform keyframe animations:

https://jsfiddle.net/dyu3foso/


Expected results:

The numbering of the ordered list should be visible during the transform animation.

Comment 1

[GitHub Autolander Bot]

Attachment: [gaia] albertopq:1270022-history > mozilla-b2g:kanikani

Comment 2

[Loic]

Comment on attachment 8748522 [details] [review]
[gaia] albertopq:1270022-history > mozilla-b2g:kanikani

Unrelated bug report.

Comment 3

[Loic]

Attachment: 1270023.html

Comment 4

[Loic]

Regression range:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=14665b1de5ee&tochange=5d6ec4dddf14

Comment 5

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=165c3fd176ec&tochange=d954ed24e795

Via local build,
Last Good: 1dbdf3202f04
First Bad: d954ed24e795



Regression window w/ force use D2D1.1 :
  user_pref("gfx.canvas.azure.backends", "direct2d1.1,direct2d,skia,cairo");
  user_pref("gfx.content.azure.backends", "direct2d1.1,direct2d,cairo");
  user_pref("gfx.direct2d.use1_1", true);
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=2badc0f1f829&tochange=8e28464849fa

Via local build,
Last Good: 20e97c8496e4
First Bad: 7b9201731195

Regressed by: Bug 1046550 


FYI, It is depended on default font. 
The problem occurs with "Arial", but not with "MS Gothic".

Comment 6

[fritzmg]

My default font is 'Times News Roman' and yet the problem does still occur.

However, the problem does not occur in all cases on another machine of mine with Firefox 45.0.2 under Windows 7 x64, at least not for me (User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Firefox/45.0).

e.g. on this system, the problem does **not** occur with any of the jsfiddle links I provided. But it **does** occur in Loic's attachment ( https://bug1270023.bmoattachments.org/attachment.cgi?id=8748611 ).

Also I got various reports from other users saying that it does or does not occur on a variety of systems and Firefox versions. So far there is no clear pattern.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

[Tracking Requested - why for this release]: This is a content rendering regression

Comment 8

[Bas Schouten (:bas.schouten)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #7)
> [Tracking Requested - why for this release]: This is a content rendering
> regression

It should be noted this is an 'old' regression, it's been shipping for a while.

I can reproduce this, and can honestly say I have no idea what's going on, interestingly enough, this only seems to happen when using jsfiddle, but not when I create a standalone test case.. which is really annoying. If anyone is able to make a small html file that reproduces this it would be extremely helpful in debugging this.

Alice, have you ever seen this outside of jsfiddle?

Comment 9

[Bas Schouten (:bas.schouten)]

Hrm, okay, I can randomly sometimes repro this outside of jsfiddle as well, good.

Comment 10

[Alice0775 White]

(In reply to Bas Schouten (:bas.schouten) from comment #8)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #7)
> > [Tracking Requested - why for this release]: This is a content rendering
> > regression
> 
> It should be noted this is an 'old' regression, it's been shipping for a
> while.
> 
> I can reproduce this, and can honestly say I have no idea what's going on,
> interestingly enough, this only seems to happen when using jsfiddle, but not
> when I create a standalone test case.. which is really annoying. If anyone
> is able to make a small html file that reproduces this it would be extremely
> helpful in debugging this.
> 
> Alice, have you ever seen this outside of jsfiddle?

Yes I can reproduce with the standalone test case attachment 8748611 [details] .
But, it is necessary to use Arial font in my case (because, "MS Gothix" is default in Japanese locale).

And it might need to move your mouse in a circular motion on content area.

Comment 11

[Marcia Knous [:marcia]]

Tracking for 48 and 49.

Comment 12

[Marcia Knous [:marcia]]

Marking affected. Regression that would be nice to fix.

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Given that this is not a recent regression and there is no fix in progress, this is now too late to fix in Fx47. 

At this point, I would like to only accept uplifts for critical recent regressions, severe stability, security, perf, mlk issues only. This is to minimize code churn and to ensure we ship a high-quality Fx47 in 2 weeks.

Comment 14

[Bas Schouten (:bas.schouten)]

(In reply to Alice0775 White from comment #10)
> (In reply to Bas Schouten (:bas.schouten) from comment #8)
> > (In reply to Jeff Muizelaar [:jrmuizel] from comment #7)
> > > [Tracking Requested - why for this release]: This is a content rendering
> > > regression
> > 
> > It should be noted this is an 'old' regression, it's been shipping for a
> > while.
> > 
> > I can reproduce this, and can honestly say I have no idea what's going on,
> > interestingly enough, this only seems to happen when using jsfiddle, but not
> > when I create a standalone test case.. which is really annoying. If anyone
> > is able to make a small html file that reproduces this it would be extremely
> > helpful in debugging this.
> > 
> > Alice, have you ever seen this outside of jsfiddle?
> 
> Yes I can reproduce with the standalone test case attachment 8748611 [details]
> [details] .
> But, it is necessary to use Arial font in my case (because, "MS Gothix" is
> default in Japanese locale).
> 
> And it might need to move your mouse in a circular motion on content area.

Can you still reproduce this on nightly? For some reason this appears to have stopped happening for me.

Comment 15

[Bas Schouten (:bas.schouten)]

(In reply to Bas Schouten (:bas.schouten) from comment #14)
> (In reply to Alice0775 White from comment #10)
> > (In reply to Bas Schouten (:bas.schouten) from comment #8)
> > > (In reply to Jeff Muizelaar [:jrmuizel] from comment #7)
> > > > [Tracking Requested - why for this release]: This is a content rendering
> > > > regression
> > > 
> > > It should be noted this is an 'old' regression, it's been shipping for a
> > > while.
> > > 
> > > I can reproduce this, and can honestly say I have no idea what's going on,
> > > interestingly enough, this only seems to happen when using jsfiddle, but not
> > > when I create a standalone test case.. which is really annoying. If anyone
> > > is able to make a small html file that reproduces this it would be extremely
> > > helpful in debugging this.
> > > 
> > > Alice, have you ever seen this outside of jsfiddle?
> > 
> > Yes I can reproduce with the standalone test case attachment 8748611 [details]
> > [details] .
> > But, it is necessary to use Arial font in my case (because, "MS Gothix" is
> > default in Japanese locale).
> > 
> > And it might need to move your mouse in a circular motion on content area.
> 
> Can you still reproduce this on nightly? For some reason this appears to
> have stopped happening for me.

Oh wait no.. managed to get it back again.. the intermittentness of this problem is really weird.

Comment 16

[Bas Schouten (:bas.schouten)]

So this is interesting, it appears that in some cases we decide to layerize this as a BGRX layer and sometimes as a BGRA layer, when we make it a BGRA layer the numbers seem to disappear. This does indeed seem to be related to moving the mouse over the content area, as moving the mouse over the content area seems to cause a horizontal scrollbar to appear.. this is really weird.. I have no idea where that is coming from.

Comment 17

[Bas Schouten (:bas.schouten)]

So after some investigation, it looks like this is a bug with opaque rectangle tracking. The area behind the numbers is somehow presumed to be opaque, however that area is never actually filled with white. When D2D tries to do subpixel AA over transparent pixels things go all wrong. This is likely somehow related to nsDisplayButton incorrectly thinking it has an opaque background? I'm guessing here though, do you know anything about this Markus?

Comment 18

[Markus Stange [:mstange]]

What you're saying sounds plausible. I can't tell you where the bug is without actually debugging it, though.

(In reply to Bas Schouten (:bas.schouten) from comment #16)
> as moving the mouse over the content
> area seems to cause a horizontal scrollbar to appear.. this is really
> weird.. I have no idea where that is coming from.

During a transform animation we suppress updating the scrollable overflow rect to a low frequency. But mouse move events force a layout update, which recalculates the scrollable overflow rect and makes the scrollbars appear.

If you tweak the testcase to always have scrollbars, the problem might be more easily reproducible.

Comment 19

[Bas Schouten (:bas.schouten)]

Attachment: Disable subpixel AA in list numbers for transparent destinations

There's probably a more correct change in layout to fix this, but this change will fix the bug and the tiny quality regression of losing subpixel AA for list numbers is probably only going to occur in very rare cases.

Comment 20

[Markus Stange [:mstange]]

Comment on attachment 8757709 [details] [diff] [review]
Disable subpixel AA in list numbers for transparent destinations

Review of attachment 8757709 [details] [diff] [review]:
-----------------------------------------------------------------

Ah, I was confused as to how DrawTargetAutoDisableSubpixelAntialiasing is supposed to be used. It turns out it's really only used in the places where we actually draw the text, and not further up the stack (which is what I thought would happen). So you're using it in the right place here. But all the other users of DrawTargetAutoDisableSubpixelAntialiasing don't check the DT's opaqueness; instead, they carry along a flag from their display item, which is updated in the display item's DisableComponentAlpha() override. Doing the same here shouldn't be much more work so I think you should just do that.

Comment 21

[Milan Sreckovic [:milan] (needinfo for best results)]

Bas, if this is too "layout-ish", just unassign yourself and I can help find somebody in the layout team to sort it out.

Comment 22

[Bas Schouten (:bas.schouten)]

(In reply to Markus Stange [:mstange] from comment #20)
> Comment on attachment 8757709 [details] [diff] [review]
> Disable subpixel AA in list numbers for transparent destinations
> 
> Review of attachment 8757709 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Ah, I was confused as to how DrawTargetAutoDisableSubpixelAntialiasing is
> supposed to be used. It turns out it's really only used in the places where
> we actually draw the text, and not further up the stack (which is what I
> thought would happen). So you're using it in the right place here. But all
> the other users of DrawTargetAutoDisableSubpixelAntialiasing don't check the
> DT's opaqueness; instead, they carry along a flag from their display item,
> which is updated in the display item's DisableComponentAlpha() override.
> Doing the same here shouldn't be much more work so I think you should just
> do that.

I find it a little confusing and it seems to conflict/weirdly override the flags the layers system(higher up in the stack as you suggested) sets. And that function doesn't make as much sense to me as I'd like either. I feel we could be handling this type of thing better.

Comment 23

[Markus Stange [:mstange]]

Attachment: MozReview Request: Bug 1270023 - Make nsDisplayBullet respect DisableComponentAlpha(). r?mattwoodrow

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

Comment 24

[Markus Stange [:mstange]]

(In reply to Bas Schouten (:bas.schouten) from comment #22)
> I find it a little confusing and it seems to conflict/weirdly override the
> flags the layers system(higher up in the stack as you suggested) sets. And
> that function doesn't make as much sense to me as I'd like either. I feel we
> could be handling this type of thing better.

I'm not sure what you mean here. I'd be interested to hear your ideas for a simpler appreach that still allows us to draw with subpixel AA to transparent DrawTargets if the pixels under the text are opaque (which is what our current system allows us to do).

Comment 25

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8758393 [details]
MozReview Request: Bug 1270023 - Make nsDisplayBullet respect DisableComponentAlpha(). r?mattwoodrow

https://reviewboard.mozilla.org/r/56624/#review53366

Comment 26

[Bas Schouten (:bas.schouten)]

(In reply to Markus Stange [:mstange] from comment #24)
> (In reply to Bas Schouten (:bas.schouten) from comment #22)
> > I find it a little confusing and it seems to conflict/weirdly override the
> > flags the layers system(higher up in the stack as you suggested) sets. And
> > that function doesn't make as much sense to me as I'd like either. I feel we
> > could be handling this type of thing better.
> 
> I'm not sure what you mean here. I'd be interested to hear your ideas for a
> simpler appreach that still allows us to draw with subpixel AA to
> transparent DrawTargets if the pixels under the text are opaque (which is
> what our current system allows us to do).

I guess the tricky bit is when -part- of the display items are over a transparent area and part of the display items are over an opaque area. But the main oddity I find is the additional SubpixelAA logic in Layers.cpp:2508. I'm not entirely certain how it's supposed to interact with this and it doesn't seem to be documented anywhere.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a25f9f94e89
Make nsDisplayBullet respect DisableComponentAlpha(). r=mattwoodrow

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0a25f9f94e89

Comment 29

[Markus Stange [:mstange]]

Bas, can you check whether this is actually fixed?

(In reply to Bas Schouten (:bas.schouten) from comment #26)
> the main oddity I find is the additional SubpixelAA logic in
> Layers.cpp:2508.

I did not know about this code. This is very confusing to me too.

Comment 30

[Gerry Chang [:gchang]]

Can we have QE to check if this is fixed in nightly?

Comment 31

[Jet Villegas (inactive)]

I think comment 29 meant to needinfo Bas. 

The severity doesn't seem to warrant a late beta uplift. I'll mark 'wontfix' for 48 but can be convinced otherwise.

Comment 32

[Bas Schouten (:bas.schouten)]

(In reply to Jet Villegas (:jet) from comment #31)
> I think comment 29 meant to needinfo Bas. 
> 
> The severity doesn't seem to warrant a late beta uplift. I'll mark 'wontfix'
> for 48 but can be convinced otherwise.

I agree with this assessment.

(In reply to Markus Stange [:mstange] from comment #29)
> Bas, can you check whether this is actually fixed?
> 
> (In reply to Bas Schouten (:bas.schouten) from comment #26)
> > the main oddity I find is the additional SubpixelAA logic in
> > Layers.cpp:2508.
> 
> I did not know about this code. This is very confusing to me too.

We should probably reevaluate and sanity check this logic then. Do you want to take a bug for this or do you think someone else would be the right person?

Comment 33

[Cornel Ionce [:noni] [Hubs QA]]

Reproduced the initial issue on Windows 10x64 using Fx 48.0.1.

Confirming the fix using Firefox 49 beta 6, build ID 20160822111414.