1098266 - impress.js - Elements switching between visible and not

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Elbart]

Attachment: bad_1.png

Nightly 2014-11-12


Open URL and press right-arrow to go to the next element ("Have you noticed it's in 3D*?")

The "one more thing"-text is disappearing, although it shouldn't.

Actually when going through that tour, a lot of elements are switching between visible and not although they should be visible in the background at all times.

Regression window

m-c:
Last good revision: 0894d2cdb16d (2014-07-20)
First bad revision: 5b64c42742cd (2014-07-21)
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0894d2cdb16d&tochange=5b64c42742cd

m-i:
Last good revision: 9350909a3401
First bad revision: 24a69de91baa
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=9350909a3401&tochange=24a69de91baa

Bug 1022612 - Remove ComputeVisibility pass when painting

Comment 1

[Elbart]

Attachment: bad_2.png

Comment 2

[Elbart]

Attachment: good_1.png

Comment 3

[Elbart]

Attachment: good_2.png

Comment 4

[Boris Zbarsky [:bzbarsky]]

[Tracking Requested - why for this release]: Things not painting in web pages.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Comment 6

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8526531 [details] [diff] [review]
fix

I think you will also need to change the reference frame of the opacity item too, since we are moving it from inside the transform to outside the transform effectively here.

(I was just about to post the same patch but with the reference frame change to bug 1091000, and indeed the reference frame change is needed to make that testcase not break even further.)

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Timothy Nikkel (:tn) from comment #6)
> I think you will also need to change the reference frame of the opacity item
> too, since we are moving it from inside the transform to outside the
> transform effectively here.
> 
> (I was just about to post the same patch but with the reference frame change
> to bug 1091000, and indeed the reference frame change is needed to make that
> testcase not break even further.)

If you've already got a tested patch, can you attach it here, with my new reftest? :-)

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v2

Comment 9

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8527383 [details] [diff] [review]
fix v2

Yeah, this is what I had in mind. Thanks. Sorry I didn't get time to post my patch before (my) Monday.

I also realized that we should probably be updating mToReferenceFrame as well when changing the reference frame.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v3

Comment 11

[Lawrence Mandel [:lmandel] (use needinfo)]

It's pretty much too late to take this in the 34 release unless this is really a release blocker. I am tracking to consider this for a ride-along for any point releases.

Comment 12

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8527385 [details] [diff] [review]
fix v3

> void
> nsDisplayTransform::Init(nsDisplayListBuilder* aBuilder)
> {
>   mStoredList.SetClip(aBuilder, DisplayItemClip::NoClip());
>-  mStoredList.SetVisibleRect(mChildrenVisibleRect);
>+  mStoredList.SetVisibleRect(mReferenceFrame, mChildrenVisibleRect);

Isn't the reference frame for the contained list mFrame and not mReferenceFrame?

>         case nsDisplayItem::TYPE_OPACITY: {
>           if (!aTemp->IsEmpty()) {
>+            // Flush current aTemp contents
>             aOutput->AppendToTop(new (aBuilder) nsDisplayTransform(aBuilder,
>                 aFrame, aTemp, aTemp->GetVisibleRect(), aIndex++));
>           }
>           nsDisplayOpacity *opacity = static_cast<nsDisplayOpacity*>(item);
>           nsDisplayList output;
>           // Call GetChildren, not GetSameCoordinateSystemChildren, because
>           // the preserve-3d children of 'opacity' are temporarily not in the
>           // same coordinate system as the opacity --- until this wrapping is done.
>           rv = WrapPreserve3DListInternal(aFrame, aBuilder,
>               opacity->GetChildren(), &output, aIndex, aTemp);
>           if (!aTemp->IsEmpty()) {
>             output.AppendToTop(new (aBuilder) nsDisplayTransform(aBuilder,
>                 aFrame, aTemp, aTemp->GetVisibleRect(), aIndex++));
>+            opacity->GetChildren()->AppendToTop(&output);
>+            opacity->SetVisibleRect(aTemp->GetBottom()->ReferenceFrame(),
>+                                    aTemp->GetVisibleRect());
>+            opacity->UpdateBounds(aBuilder);
>+            aOutput->AppendToTop(item);
>+          } else {
>+            opacity->~nsDisplayOpacity();
>           }
>-          opacity->GetChildren()->AppendToTop(&output);
>-          opacity->UpdateBounds(aBuilder);
>-          aOutput->AppendToTop(item);
>           break;
>         }

If aTemp is empty but output is non-empty won't be be losing the contents of output here?

Comment 13

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8527385 [details] [diff] [review]
fix v3

r- for now but feel free to explain why I'm wrong.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Timothy Nikkel (:tn) from comment #12)
> Isn't the reference frame for the contained list mFrame and not
> mReferenceFrame?

Yeah OK.

> >         case nsDisplayItem::TYPE_OPACITY: {
> >           if (!aTemp->IsEmpty()) {
> >+            // Flush current aTemp contents
> >             aOutput->AppendToTop(new (aBuilder) nsDisplayTransform(aBuilder,
> >                 aFrame, aTemp, aTemp->GetVisibleRect(), aIndex++));
> >           }
> >           nsDisplayOpacity *opacity = static_cast<nsDisplayOpacity*>(item);
> >           nsDisplayList output;
> >           // Call GetChildren, not GetSameCoordinateSystemChildren, because
> >           // the preserve-3d children of 'opacity' are temporarily not in the
> >           // same coordinate system as the opacity --- until this wrapping is done.
> >           rv = WrapPreserve3DListInternal(aFrame, aBuilder,
> >               opacity->GetChildren(), &output, aIndex, aTemp);
> >           if (!aTemp->IsEmpty()) {
> >             output.AppendToTop(new (aBuilder) nsDisplayTransform(aBuilder,
> >                 aFrame, aTemp, aTemp->GetVisibleRect(), aIndex++));
> >+            opacity->GetChildren()->AppendToTop(&output);
> >+            opacity->SetVisibleRect(aTemp->GetBottom()->ReferenceFrame(),
> >+                                    aTemp->GetVisibleRect());
> >+            opacity->UpdateBounds(aBuilder);
> >+            aOutput->AppendToTop(item);
> >+          } else {
> >+            opacity->~nsDisplayOpacity();
> >           }
> >-          opacity->GetChildren()->AppendToTop(&output);
> >-          opacity->UpdateBounds(aBuilder);
> >-          aOutput->AppendToTop(item);
> >           break;
> >         }
> 
> If aTemp is empty but output is non-empty won't be be losing the contents of
> output here?

If aTemp is empty, how can 'output' be non-empty? I'll make this more clear in my next patch

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Oh I guess I see the problem...

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v4

Comment 17

[Timothy Nikkel (:tnikkel)]

roc, can I go ahead and land this patch? I'd like to land it before the merge.

Comment 18

[Timothy Nikkel (:tnikkel)]

I pushed to try
https://tbpl.mozilla.org/?tree=Try&rev=e460cdc4a75f
I'll land it if that looks good.

Comment 19

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b291989a1656

Comment 20

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8528812 [details] [diff] [review]
fix v4

It's too late for beta, but assuming we don't find any regressions we should uplift this to aurora (or beta after merge day).

Approval Request Comment
[Feature/regressing bug #]: fixes bugs caused by both bug 1022612 and bug 1042772
[User impact if declined]: pages using preserve3d transforms rendered wrongly, given that there are four separate bugs that this fixes (bug 1098266, bug 1106081, bug 1091000, bug 1103165) it's likely to be a fairly frequent occurrence to trip over this bug
[Describe test coverage new/current, TBPL]: this patch adds a test
[Risks and why]: the code was pretty broken before, any regressions caused by this should be less severe. a full beta period should be more than enough time to shake out any regressions that are worse.
[String/UUID change made/needed]: none

Comment 21

[Phil Ringnalda (:philor)]

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

Comment 22

[IU]

This has fixed visibility, but Firefox is now obviously slow and jerky in rendering it.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to IU from comment #22)
> This has fixed visibility, but Firefox is now obviously slow and jerky in
> rendering it.

Are you saying it regressed something, or was it always slow and jerky but this bug obscured the fact?

Comment 25

[IU]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #24)
> Are you saying it regressed something, or was it always slow and jerky but
> this bug obscured the fact?

Probably this bug obscured it.  Now that it's visually accurate, the poor rendering performance is obvious.  Although, Firefox dynamic content performance in general has increasingly gone in the toilet the past few weeks, so who knows.

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to IU from comment #25)
> Probably this bug obscured it.  Now that it's visually accurate, the poor
> rendering performance is obvious.  Although, Firefox dynamic content
> performance in general has increasingly gone in the toilet the past few
> weeks, so who knows.

Please file a bug if you have a specific testcase I can look at that has regressed. Thanks!

Comment 27

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8528812 [details] [diff] [review]
fix v4

After merge, aurora is now beta. Switching to beta approval.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/506962a16ce8
https://hg.mozilla.org/releases/mozilla-beta/rev/b791be231dda

Comment 30

[Timothy Nikkel (:tnikkel)]

I'm confused, I thought we had passed merge day, so this should have been fixed on aurora already.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

This landed on trunk after the uplift to Aurora.

Comment 33

[Craig]

Does not seem to be fixed on firefox35 Beta

Comment 34

[Elbart]

I cannot reproduce the bug in 35.0b2.

Comment 35

[Craig]

My bad. You are right. Fixed in 35.0b2