531461 - position fixed visibility hidden table causes flickering when window scrolls.

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[katsuhisa yuasa]

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.

Comment 1

[katsuhisa yuasa]

Attachment: html file that reproduces the bug with my firefox (3.6b4)

please see this example. hope the problem reproduces with your firefox.

Comment 2

[katsuhisa yuasa]

Attachment: fixed the html so layout is OK with InternetExplorer.

fixed and simplified html. so the page layout would be the same on InternetExplorer.
anywayz, the problem also reproduces with Firefox3.5.5.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

On Mac I actually see a permanent rendering artifact.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Oh, that's probably a regression from my patch for bug 530686.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Yep, I saw it on Linux (and the original report is on Windows).

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Avoid creating an nsDisplayTableBorderBackground if every part of the table is hidden.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 ?)

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v2

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v3

You're right again!

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.)

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/41a0a9b84b53

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: fix assertions

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.

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Assertion fix landed: http://hg.mozilla.org/mozilla-central/rev/89a4f5a6525a

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

assertion fix landed: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/79f925475a39