Closed Bug 531461 Opened 11 years ago Closed 11 years ago

position fixed visibility hidden table causes flickering when window scrolls.

Categories

(Core :: Layout, defect)

1.9.2 Branch
defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1
Tracking Status
status1.9.2 --- final-fixed

People

(Reporter: berupon, Assigned: roc)

References

Details

Attachments

(4 files, 2 obsolete files)

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.
please see this example. hope the problem reproduces with your firefox.
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.
Attached 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.
Attached 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.
Attached 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.
Attachment #415568 - Flags: review?(dbaron) → review+
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
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Keywords: checkin-needed
Whiteboard: [needs landing] → [needs 192 landing]
Attached patch fix assertionsSplinter Review
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.