position fixed visibility hidden table causes flickering when window scrolls.

RESOLVED FIXED in mozilla1.9.3a1

Status

()

defect
--
minor
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: berupon, Assigned: roc)

Tracking

1.9.2 Branch
mozilla1.9.3a1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.2 -
wanted1.9.2 +

Firefox Tracking Flags

(status1.9.2 final-fixed)

Details

Attachments

(4 attachments, 2 obsolete attachments)

Reporter

Description

10 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2b4) Gecko/20091124 Firefox/3.6b4 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2b4) Gecko/20091124 Firefox/3.6b4 (.NET CLR 3.5.30729)

position fixed visibility hidden table causes flickering when window scrolls.


Reproducible: Always

Steps to Reproduce:
1.scroll
2.see glitches.
3.back to 1
Actual Results:  
i see flickering (rendering delay) around hidden table.

Expected Results:  
position fixed visibility hidden elements should not affect rendering result.
Reporter

Comment 1

10 years ago
please see this example. hope the problem reproduces with your firefox.
Reporter

Comment 2

10 years ago
fixed and simplified html. so the page layout would be the same on InternetExplorer.
anywayz, the problem also reproduces with Firefox3.5.5.
Component: Tabbed Browser → Layout
Product: Firefox → Core
QA Contact: tabbed.browser → layout
Version: unspecified → 1.9.2 Branch
The out-of-sync-ness is very easy to see when smooth scrolling is enabled.

I wonder if display lists could optimize away the whole position:fixed element?
Status: UNCONFIRMED → NEW
Ever confirmed: true
On Mac I actually see a permanent rendering artifact.
Assignee: nobody → roc
Flags: blocking1.9.2?
Oh, that's probably a regression from my patch for bug 530686.
Ok, I found the regression from bug 530686 and submitted a new patch there.

This bug isn't visible on Mac, but it is on Linux.
Yep, I saw it on Linux (and the original report is on Windows).
The scroll analysis has an nsDisplayTableBorderBackground in it, because of this comment:
http://mxr.mozilla.org/mozilla-central/source/layout/tables/nsTableFrame.cpp#1338

I guess we should just traverse the frames for the table and avoid adding nsDisplayTableBorderBackground if they're all hidden.
Posted patch fix (obsolete) — Splinter Review
Avoid creating an nsDisplayTableBorderBackground if every part of the table is hidden.
Attachment #415305 - Flags: review?(dbaron)
Whiteboard: [needs review]
Flags: wanted1.9.2+
Flags: blocking1.9.2?
Flags: blocking1.9.2-
Comment on attachment 415305 [details] [diff] [review]
fix

>-  NS_ASSERTION(currentItem, "No current table item!");
>-  currentItem->UpdateForFrameBackground(aFrame);
>+  // currentItem may be null if none of the table parts are visible
>+  if (currentItem) {
>+    currentItem->UpdateForFrameBackground(aFrame);
>+  }

I don't like this change.  It seems that:

 * if we're doing appropriate visibility checks, we wouldn't need this change at all
 * if we actually do need this change, we might notify an nsDisplayTableItem for a containing visible table instead, which seems bad

(I also wonder whether table *cells* should clear the current table item so it's no longer set while processing their descendants.)

>@@ -1307,16 +1309,32 @@ nsTableFrame::DisplayGenericTablePart(ns
>     // before everything else (cells and their blocks)
>     separatedCollection.BorderBackground()->Sort(aBuilder, CompareByTablePartRank, nsnull);
>     separatedCollection.MoveTo(aLists);
>   }
>   
>   return aFrame->DisplayOutline(aBuilder, aLists);
> }
> 
>+static PRBool
>+AnyTablePartVisible(nsIFrame* aFrame)
>+{

Can you assert at the start of this that the frame is some table part?  (Perhaps through aFrame->GetStyleDisplay->mDisplay ?)
Attachment #415305 - Flags: review?(dbaron) → review-
(In reply to comment #10)
>  * if we're doing appropriate visibility checks, we wouldn't need this change
> at all

I'm not sure what you mean. I still want to call DisplayGenericTablePart even if every part of the table is hidden, otherwise we'd need a different code path that recursively calls BuildDisplayListForChild on the children. And if we call 
DisplayGenericTablePart from nsTableFrame::BuildDisplayList and pass in a null aDisplayItem, we'll crash without this change.

>  * if we actually do need this change, we might notify an nsDisplayTableItem
> for a containing visible table instead, which seems bad

Indeed...

> (I also wonder whether table *cells* should clear the current table item so
> it's no longer set while processing their descendants.)

That's a good idea, I'll do that.

> Can you assert at the start of this that the frame is some table part? 

Sure.
Posted patch fix v2 (obsolete) — Splinter Review
Attachment #415305 - Attachment is obsolete: true
Attachment #415523 - Flags: review?(dbaron)
I meant that that the call to currentItem->UpdateForFrameBackground ought to be within a check that the frame is visible (and perhaps some other code in that function?), and if it were, you wouldn't need to null-check currentItem.

Also, your assertion should allow column groups and columns.
Posted patch fix v3Splinter Review
You're right again!
Attachment #415523 - Attachment is obsolete: true
Attachment #415568 - Flags: review?(dbaron)
Attachment #415523 - Flags: review?(dbaron)
I can't seem to write a testcase where a box-shadow shows up on a table-related frame with visibility:hidden, though.  In what cases would we reach and then fail that IsVisibleForPainting check?
I'm not sure what you're asking. On trunk, a box-shadow on a visibility:hidden table part won't show up because hasBoxShadow checks IsVisibleForPainting.

Here is a testcase that will reach and fail the IsVisibleForPainting check in "fix v3".

<table border="1" style="visibility:hidden">
<tr style="visibility:visible"><td>Hello
<tr><td>Kitty</table>

The check will fail when DisplayGenericTablePart is called for the hidden table, table-row-groups, and table-row.
Comment on attachment 415568 [details] [diff] [review]
fix v3

Oops, missed that check.  Sorry.

RequestSpecialHeightReflow should still assert if it's given a colgroup or col frame; you could either check for those specifically or just undo the change.


r=dbaron with that
Whiteboard: [needs review] → [needs landing]
OS: Windows Vista → All
Hardware: x86_64 → All
Landed:
http://hg.mozilla.org/mozilla-central/rev/9af5c4367b00
and then addressed the review comment in:
http://hg.mozilla.org/mozilla-central/rev/8bc6e7e44946

This will need to land on 1.9.2 because it blocks bug 530686.  (In other words, it's basically the first two thirds of the patch to bug 530686.)
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Keywords: checkin-needed
Whiteboard: [needs landing] → [needs 192 landing]
The above patch caused large numbers of assertions during reftest because we're finding nsScrollbarFrames.  This fixes those assertions, and I think it's the right thing to do.
Attachment #417323 - Flags: review?(roc)
Attachment #417323 - Flags: review?(roc) → review+
You need to log in before you can comment on or make changes to this bug.