Closed Bug 1098266 Opened 10 years ago Closed 9 years ago

impress.js - Elements switching between visible and not

Categories

(Core :: Layout, defect)

x86_64
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37
Tracking Status
firefox34 + wontfix
firefox35 + fixed
firefox36 + fixed
firefox37 --- fixed

People

(Reporter: elbart, Assigned: roc)

References

()

Details

(Keywords: regression)

Attachments

(5 files, 3 obsolete files)

Attached 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
Blocks: 1022612
Attached image bad_2.png
Attached image good_1.png
Attached image good_2.png
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)
Attached 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.)
Blocks: 1091000
Blocks: 1103165
(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)
Attached 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)
Attached 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...
Attached 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: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
This has fixed visibility, but Firefox is now obviously slow and jerky in rendering it.
(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)
(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+
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)
Does not seem to be fixed on firefox35 Beta
I cannot reproduce the bug in 35.0b2.
My bad. You are right. Fixed in 35.0b2
You need to log in before you can comment on or make changes to this bug.