261064 - Clicking unnumbered list (ul) causes page to shift in some cases

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, defect)

Description

[Anton van Bohemen]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; @AB_CD@; rv:1.8a4) Gecko/20040922 Firefox/0.9.1+ (BlueFyre)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; @AB_CD@; rv:1.8a4) Gecko/20040922 Firefox/0.9.1+ (BlueFyre)

Clicking a link in a list causes the page to shift, and not follow the link
because the mouse cursor is not on it anymore. Clicking the link for the second
time works. Not all ul's are effected though, part of this bug is already fixed. 

Reproducible: Always
Steps to Reproduce:
1. Go to site mentioned above
2. Click one of the links in the right half of the page (under "XUL applications")
3.

Actual Results:  
Page shifts up a little, missing the link you just tried to click.

Expected Results:  
Do not shift and hit the link the first time.

Follow-up to bug 257612 and bug 255584. This entire site shows the bug, not only
this page, so feel free to click around.

Comment 1

[Anton van Bohemen]

Robert, you requested a minimal testcase. I'm sorry, my knowledge of html is too
limited for that. If anyone can provide one, feel free to do so.

Comment 2

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

Can you jump on irc.mozilla.org #mozilla and find someone to help you?

Comment 3

[Martijn Wargers (dead)]

Attachment: Minimal testcase

Well, this is a minimal testcase derived from that site.
In case you wonder, the empty css rule "body::first-line {}" is also somehow
needed to trigger the bug.

Comment 4

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

Attachment: another testcase

Thanks for that testcase again, Martjin! After some debugging I've localized
the problem and I've produced this testcase, which is simpler because it
doesn't involve ::first-line (which is really odd code).

Comment 5

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

Attachment: partial fix

This patch fixes the assertion that fires in the testcase. Basically when we're
checking for empty frames before the absolute placeholder we need to scan the
deep frame tree not just the direct line children, because the placeholder can
be inside an inline (or in the earlier testcase, the first-line psuedo-inline).


This patch helps a lot; deleting the text moves the absolute up to the top of
its placeholder's block. But it's not totally fixed; margin collapsing should
cause all the margins to collapse out and leave the absolute frame's top edge
adjacent to the top of the content window. But we're not detecting the margin
collapse until a reflow is forced again.

Comment 6

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

The remaining problem is this: we get an incremental reflow targeted at the
absolute frame and at the line containing its placeholder. In
nsBlockFrame::Reflow we incrementally reflow the absolute frame. Then we reflow
the line, moving the placeholder. Then we skip reflowing the incremental frames
again, because we already reflowed them incrementally and the block's size
didn't change.

Clearly we need to reflow again any absolute frames whose placeholders moved and
which have auto left or top. I wonder what the best way is to do this. One quite
simple approach would be to modfiy nsAbsoluteContainingBlock::IncrementalReflow
to detect if any absolute frames have auto left or top. Then we would reflow the
absolute frames again at the end of nsBlockFrame::Reflow if any such were found
during the incremental pass. I think this would perform quite well because I
believe that auto left or top is fairly uncommon and not the interesting case
for DHTML.

One question is, why reflow the absolute frames twice, anyway? The only reason I
can see is if an incremental reflow is targeted only at absolute frames, then we
can skip all the work of line reflow (even if no lines are dirty, it's still a
lot of work). So I think the *best* fix, if not the smallest, would be to change
the call to nsAbsoluteContainingBlock::IncrementalReflow to first check whether
"handled" will be true (all the incremental reflow is to absolute frames)
without reflowing anything, which is fairly cheap (just inspecting the reflow
path). If "handled" is not true then we don't do any incremental reflow of the
absolute frames until after all the lines are reflowed, then we reflow all the
absolute frames. How does that sound?

Comment 7

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

Attachment: complete fix

This fixes the bug completely.

I've tested this against a variety of absolute, fixed and relative positioning
testcases and it seems to work OK. I don't know about performance but I don't
think it will get worse.

Comment 8

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

BTW we could add back the optimization which skips the reflow of absolute frames
when the block was reflowed incrementally and its size didn't change; we'd just
have to also scan the list of absolute frames and ensure that none of them have
auto top or left. I haven't done that since I'm not sure it matters.

Comment 9

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

(In reply to comment #8)
> BTW we could add back the optimization which skips the reflow of absolute frames
> when the block was reflowed incrementally and its size didn't change; we'd just

If I understand what you're saying, that's important.  The containing block for
most absolutely positioned elements is the root, and the root is involved in
pretty much every incremental reflow.

Comment 10

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

Attachment: fix including size-change optimization

Okay, this patch adds the size-change optimization. We do have to scan for
frames whose position depends on the placeholder since placeholders might have
moved.

This would be much improved by folding the incremental reflow and the normal
reflow together, but I'm not qutie sure how the reflows should combine.

Comment 11

[Ronald Tilby]

Mozilla Windows Trunk Nightly Build Regression Window
Pass: 2004071609
Fail: 2004071808

Comment 12

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

Comment on attachment 160116 [details] [diff] [review]
fix including size-change optimization

r+sr=dbaron, rubber-stamp.

I didn't look at the absolute stuff -- I'm hoping to rewrite that when
refactoring reflow.

Please file a bug on making nsLineLayout::VerticalAlignFrames use IsFrameEmpty
for its quirks mode stuff.

I don't like the name IsFrameEmpty.  IsSelfEmpty might be a little more
consistent with one other thing in the tree (PaintSelf), but I don't much like
that either.

Comment 13

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

But before landing please profile a DHTML testcase that has a lot of incremental
reflows not in absolutely positioned content *and* has a lot of absolutely
positioned elements (non-auto, but with the root (or is it ICB?) as a containing
block) to make sure that this doesn't hurt the previously-known performance problem.

Comment 14

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

OK. I'll change it to IsSelfEmpty.

Comment 15

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

Attachment: DHTML performance testcase

This is the testcase I will use to check incremental reflow performance.

Comment 16

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

times in milliseconds:
normal run: 18347 18356 18381
patched: 24542 24570 24713
so I guess I'm going to have to work harder

Comment 17

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

Attachment: better fix

With this patch I got these times: 18879 18909 18943
Much better, just a 3% regression on this annoying testcase.

The problem was that in the testcase I posted, we always reflowed the absolute
frames because the block height was changing. There are two reasons why we
don't need to do that:
-- All the frames had fixed left, top, width and height, and thus couldn't be
affected by changes in the size of the containing block
-- The containing block height for the initial containing block is not the
actual block height, but the viewport height, which isn't changing during
incremental reflow.

So this patch improves the previous patch by detecting both of those cases and
optimizing for them. Checking to see whether the absolute frame's size or
position depends on the containing block's size is unfortunately complex. But
this logic could be reused to avoid reflowing absolute frames in a Resize
reflow of their containing block. I'll post a followup patch for that once I've
tested it.

The existing code avoided reflowing the absolute frames only because it has a
major bug. There's a check 'mRect != oldRect' that's supposed to detect a block
size change. But it doesn't work because where mRect is tested, it hasn't yet
been updated (the new size is only in aMetrics). I will post a testcase that
demonstrates the bug. That bug is fixed by this patch.

David, if you intend to carry over your rubber-stamp, just do it :-)

Comment 18

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

Attachment: testcase

This is the bug in the current code I was referring to. When you click the
button, the bottom-positioned DIV should move down to keep a constant distance
to the bottom of its containing block, but it doesn't move ... until you force
a reflow e.g. by resizing, which pops it into the right place.

Comment 19

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

Checked in. Waiting to see Tp impact

Comment 20

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

No apparent Tp impact --- maybe a 1% loss on creature only, hard to tell with
only one number.

Seems to have cut Tdhtml on luna by about 1% :-)

Comment 21

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

Yeah, the creature number was just noise. There was no Tp impact at all.

Comment 22

[Boris Zbarsky [:bzbarsky]]

This did cause some dhtml-type regressions -- see bug 277760.