Closed Bug 1270023 Opened 4 years ago Closed 4 years ago

transform animation make numbers of ordered list disappear

Categories

(Core :: Layout, defect)

35 Branch
x86_64
Windows 10
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla49
Tracking Status
firefox47 - wontfix
firefox48 + wontfix
firefox49 + verified

People

(Reporter: email, Assigned: mstange, NeedInfo)

References

Details

(Keywords: regression, testcase, Whiteboard: [gfx-noted])

Attachments

(3 files, 1 obsolete file)

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.
OS: Unspecified → Windows 10
Hardware: Unspecified → x86_64
Comment on attachment 8748522 [details] [review]
[gaia] albertopq:1270022-history > mozilla-b2g:kanikani

Unrelated bug report.
Attachment #8748522 - Attachment is obsolete: true
Attached file 1270023.html
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".
Blocks: 1046550
Status: UNCONFIRMED → NEW
Component: Untriaged → Graphics
Ever confirmed: true
Flags: needinfo?(matt.woodrow)
Flags: needinfo?(bas)
Product: Firefox → Core
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.
Whiteboard: [gfx-noted]
[Tracking Requested - why for this release]: This is a content rendering regression
(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?
Assignee: nobody → bas
Status: NEW → ASSIGNED
Flags: needinfo?(bas) → needinfo?(alice0775)
Hrm, okay, I can randomly sometimes repro this outside of jsfiddle as well, good.
Flags: needinfo?(alice0775)
(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.
Marking affected. Regression that would be nice to fix.
Flags: needinfo?(matt.woodrow)
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.
(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.
(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.
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.
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?
Flags: needinfo?(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.
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.
Attachment #8757709 - Flags: feedback?(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.
Attachment #8757709 - Flags: feedback?(mstange)
Bas, if this is too "layout-ish", just unassign yourself and I can help find somebody in the layout team to sort it out.
Component: Graphics → Layout
(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.
(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).
Assignee: bas → mstange
Flags: needinfo?(mstange)
Comment on attachment 8758393 [details]
MozReview Request: Bug 1270023 - Make nsDisplayBullet respect DisableComponentAlpha(). r?mattwoodrow

https://reviewboard.mozilla.org/r/56624/#review53366
Attachment #8758393 - Flags: review?(matt.woodrow) → review+
(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.
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a25f9f94e89
Make nsDisplayBullet respect DisableComponentAlpha(). r=mattwoodrow
https://hg.mozilla.org/mozilla-central/rev/0a25f9f94e89
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
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.
Can we have QE to check if this is fixed in nightly?
Flags: qe-verify+
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.
Flags: needinfo?(bas)
(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?
Flags: needinfo?(bas) → needinfo?(mstange)
Reproduced the initial issue on Windows 10x64 using Fx 48.0.1.

Confirming the fix using Firefox 49 beta 6, build ID 20160822111414.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.