impress.js - Elements switching between visible and not

RESOLVED FIXED in Firefox 35

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: elbart, Assigned: roc)

Tracking

({regression})

Trunk
mozilla37
x86_64
All
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox34+ wontfix, firefox35+ fixed, firefox36+ fixed, firefox37 fixed)

Details

()

Attachments

(5 attachments, 3 obsolete attachments)

Reporter

Description

5 years ago
Posted image 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
Reporter

Updated

5 years ago
Blocks: 1022612
Reporter

Comment 1

5 years ago
Posted image bad_2.png
Reporter

Comment 2

5 years ago
Posted image good_1.png
Reporter

Comment 3

5 years ago
Posted image good_2.png

Updated

5 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
OS: Windows 7 → All
[Tracking Requested - why for this release]: Things not painting in web pages.
Flags: needinfo?(roc)
Posted patch fix (obsolete) — Splinter Review
Assignee: nobody → roc
Flags: needinfo?(roc)
Attachment #8526531 - Flags: review?(matt.woodrow)
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.)
(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? :-)
Flags: needinfo?(tnikkel)
Posted patch fix v2 (obsolete) — Splinter Review
Attachment #8526531 - Attachment is obsolete: true
Attachment #8526531 - Flags: review?(matt.woodrow)
Attachment #8527383 - Flags: review?(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.
Flags: needinfo?(tnikkel)
Posted patch fix v3 (obsolete) — Splinter Review
Attachment #8527383 - Attachment is obsolete: true
Attachment #8527383 - Flags: review?(tnikkel)
Attachment #8527385 - Flags: review?(tnikkel)
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 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 on attachment 8527385 [details] [diff] [review]
fix v3

r- for now but feel free to explain why I'm wrong.
Attachment #8527385 - Flags: review?(tnikkel) → review-
(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
Oh I guess I see the problem...
Posted patch fix v4Splinter Review
Attachment #8527385 - Attachment is obsolete: true
Attachment #8528812 - Flags: review?(tnikkel)
Attachment #8528812 - Flags: review?(tnikkel) → review+
Attachment #8528812 - Attachment is patch: true
roc, can I go ahead and land this patch? I'd like to land it before the merge.
I pushed to try
https://tbpl.mozilla.org/?tree=Try&rev=e460cdc4a75f
I'll land it if that looks good.
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
Attachment #8528812 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/b291989a1656
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37

Comment 22

5 years ago
This has fixed visibility, but Firefox is now obviously slow and jerky in rendering it.
Duplicate of this bug: 1103165
(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?
Flags: needinfo?(fehe)

Comment 25

5 years ago
(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.
Flags: needinfo?(fehe)
(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 on attachment 8528812 [details] [diff] [review]
fix v4

After merge, aurora is now beta. Switching to beta approval.
Attachment #8528812 - Flags: approval-mozilla-beta?
Attachment #8528812 - Flags: approval-mozilla-beta?
Attachment #8528812 - Flags: approval-mozilla-beta+
Attachment #8528812 - Flags: approval-mozilla-aurora?
Attachment #8528812 - Flags: approval-mozilla-aurora+
Flags: in-testsuite+

Updated

5 years ago
Duplicate of this bug: 1106998
I'm confused, I thought we had passed merge day, so this should have been fixed on aurora already.
Flags: needinfo?(ryanvm)
This landed on trunk after the uplift to Aurora.
Flags: needinfo?(ryanvm)

Updated

5 years ago
Duplicate of this bug: 1107403

Comment 33

5 years ago
Does not seem to be fixed on firefox35 Beta
Reporter

Comment 34

5 years ago
I cannot reproduce the bug in 35.0b2.

Comment 35

5 years ago
My bad. You are right. Fixed in 35.0b2

Updated

5 years ago
Duplicate of this bug: 1109661
Duplicate of this bug: 1109236

Updated

5 years ago
Duplicate of this bug: 1112138

Updated

5 years ago
Duplicate of this bug: 1114327
You need to log in before you can comment on or make changes to this bug.