Closed Bug 725981 Opened 8 years ago Closed 7 years ago

Over-invalidation in former gaia settings screen

Categories

(Core :: Layout, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: cjones, Assigned: mattwoodrow)

References

Details

Attachments

(2 files, 2 obsolete files)

Attached file test case (not small)
Single-page test case included below.  No attempt made to minimize it yet.

I'm not quite sure what's up here.  I don't fully understand all the magic in the HTML/CSS yet, but it's trying to animate transforms between "pages" with a translation transition, and the pages themselves are window-sized.  So shouldn't be repainted, at first blush.
In a little more detail: STR
 (0) Turn on paint flashing
 (1) Load the page
 (2) Click on e.g. "Display" button
 (3) Transition to detailed settings for display

The main settings menu is a window-sized (w=100%,h=100%) absolutely-positioned div of CSS class "page".  The detailed settings for display is the same class, "page", also window-sized.  Until the detailed settings for display is activated, that div is display:none.  When the detailed settings page loads, the main settings menu div goes display:none.

With paint flashing on, you can see both the main settings menu div and the detailed display settings div repainting all during the transition.  That's not expected.

A small test case that cuts out all the irrelevant choices, down to one line per page, and automatically transitions after load to show the flashing, would be fabulous.  Slowing down the transition duration to 5 or 10seconds would also help with debugging.
Andreas can you set the other bugs you filed to block this one?
No longer blocks: b2g-demo-phone
Yuren, this bug has a similar symptom to what you're seeing on the new lock screen.
Summary: Over-invalidation in gaia settings screen → Over-invalidation in former gaia settings screen
(DLBI didn't fix this.)
Matt should take a look.
Assignee: nobody → matt.woodrow
This fixes the majority of the painting issues.

We still repaint the text while animating though. This is because the overflow area of the document is changing, which triggers reflow. Reflowing the text always causes it to invalidate, since we can't (easily) figure out if the reflow actually changed rendering or not.

Adding overflow:hidden; to the body element fixes that though.
Attachment #719309 - Flags: review?(roc)
Comment on attachment 719309 [details] [diff] [review]
Invalidate only the changed area of solid colors

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

::: layout/base/nsDisplayList.h
@@ +882,5 @@
> +
> +    nsRegion borderDifference;
> +    if (!geometry->mBounds.IsEqualInterior(border)) {
> +      borderDifference.Xor(geometry->mBorderRect, border);
> +    }

Why do we need both? Isn't GetBounds enough, because all these items fill GetBounds with a solid color?
Fair enough, I was checking both as this was a shared function that might potentially need that.

Lets just not store the border rect for these types, then we don't need to worry about checking it.
Attachment #721397 - Flags: review?(roc)
Forgot to qref.
Attachment #719309 - Attachment is obsolete: true
Attachment #721397 - Attachment is obsolete: true
Attachment #719309 - Flags: review?(roc)
Attachment #721397 - Flags: review?(roc)
Attachment #721405 - Flags: review?(roc)
Comment on attachment 721405 [details] [diff] [review]
Invalidate only the changed area of solid colors

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

::: layout/base/nsDisplayList.h
@@ +871,5 @@
> +                                           nsRegion* aInvalidRegion)
> +  {
> +    bool snap;
> +    nsRect bounds = GetBounds(aBuilder, &snap);
> +    nsRect border = GetBorderRect();

Remove this line!
Attachment #721405 - Flags: review?(roc) → review+
https://hg.mozilla.org/mozilla-central/rev/d10b1ac51ece
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.