257612 - Clicking on "Try again" from error page causes the link to jump down and not do anything the first time.

Status: VERIFIED FIXED

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

Description

[Jason Bassford]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a3) Gecko/20040828
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a3) Gecko/20040828

When getting an error page after going to a URL that doesn't exist, or is having
problems, and clicking the "Try again" link, the links jumps down the page what
looks like one line of text without actually doing anything.  If you click on it
in its new location, then it works as expected.

Reproducible: Always
Steps to Reproduce:
1. Make sure that "browser.xul.error_pages.enabled" is set to "true".
2. Go to the URL link given which doesn't exist.
3. You'll get the error page.
4. Click on the "Try again" link.

Actual Results:  
The link jumps down about a line of text and nothing else happens.  Only upon
clicking on it a second time does the link remain in its (new) position, and the
browser attempt to contact the URL again.

Expected Results:  
The link should stay where it is, and the URL should be attempted again immediately.

This is a follow-up to bug 255584, so I'm filing this under the same component.

Comment 1

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

A plain HTML testcase for this would be very helpful.

Comment 2

[Jason Bassford]

I don't know how to get a plain HTML version - if I do a view source code of an
error page and simply copy the HTML I see there, it doesn't show the same thing
at all (opening that file) as Mozilla does when an error is generated.  There
must be some process going on that isn't amenable to just a view source.  (If
the whole source code I get actually worked, I would then have started stripping
things down to a minimal testcase - but it doesn't.)  Somebody else who knows
how the error pages work is going to have to contribute something I'm missing
here...

Comment 3

[Jason Bassford]

At best I can say that it's referencing the built-in
"chrome://global/locale/netError.dtd" and is using javascript.  But since I
can't get the whole thing to work the same way, I can't start stripping pieces
off to see where it's having the problem...

Comment 4

[Jason Bassford]

Ah...and the JavaScript console shows me "Error: cyclic __proto__ value"
whenever the error page comes up.  (Although I don't know if this is actually
related to the problem with clicking the link or something else.)

Comment 5

[Boris Zbarsky [:bzbarsky]]

> and the JavaScript console shows me "Error: cyclic __proto__ value"
> whenever the error page comes up.

Pull a new build, please -- that was fixed earlier today.

Comment 6

[Jason Bassford]

Okay - with the current latest build (8/31-08 under XP) I'm no longer seeing the
javascript errors.

However, the problem reported here persists.  (The link moves down a line and
nothing else happens the first time you click on "Try again".)

Comment 7

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

You can bring up DOM Inspector on the error page window. Unfortunately cutting
and pasting the XML doesn't make a usable testcase because some styles need to
be brought over as well, but looking at the DOM and the CSS style rules applying
to the elements one should be able to make an HTML testcase. Bringing up the
chrome error page in the layout debugger does not show the bug so we really need
the HTML testcase....

Comment 8

[Jason Bassford]

Right.  I actually installed the DOM Inspector for the first time tonight and
used it to look at the error page.  I couldn't determine anything useful from
this - then again, I've never used the tool before so that's not too suprising.

If there's going to be an HTML testcase, it appears that somebody else is going
to have to create it.  If you want to mark this bug as INVALID because we don't
have one, that's fine too.

Comment 9

[Jason Bassford]

> Bringing up the chrome error page in the layout debugger
> does not show the bug

Troubleshooting aside, the real question is - do you see the bug when following
the steps to reproduce it?

Comment 10

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

Yeah, and I'd like to fix it.

Comment 11

[Martijn Wargers (dead)]

Attachment: Testcase

The problem seems to be that when the (sort of) empty textnode is changed into
a not empty textnode, the margin-bottom of the <p> isn't added in the reflow
somehow.
When you focus the link underneath, then the extra margin-bottom gets applied.

A quick and easy fix would be to add a &#160; character entity, instead of the
empty space character which is used currently.

Comment 12

[Martijn Wargers (dead)]

Attachment: Like this

Like this, but off course it would be better if the general problem would be
solved (the testcase) :)

Comment 13

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

Martjin, you rock! I should be able to fix this now.

Comment 14

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

Attachment: testcase showing dynamic margin changes don't work

It turns out that dynamic margin changes don't work at all. This simple
testcase shows that.

This testcase should be fixable in ReflowBlockFrame by detecting when the
block's carried-out margin has changed.

Comment 15

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

I've fixed the bug here by changing ReflowDirtyLines so that when we reflow a
dirty block line, if the block line could have been and should still be involved
in a subsequent line's ShouldApplyTopMargin calculation and its Empty status
might have changed, then the next line gets marked dirty. We implement this
check by observing that a) a block line can only be involved in a subsequent
line's ShouldApplyTopMargin if its Y-position is zero and b) it can only be
empty if its height is zero.

I don't quite have a complete patch yet because my fix for the dynamic margin
change bug doesn't work when the block whose margin changes is followed by an
empty line. The previousMarginDirty state needs to be carried through the empty
line but isn't.

Comment 16

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

Attachment: fix

This patch fixes both issues.

It does the following:
1) Propagate IsPreviousMarginDirty through empty lines
2) Reflow any line whose previous margin is dirty. The "fast path" for
non-block lines doesn't look right if the line is empty. Let's do the simple
thing since I can't see that this would be a performance issue.
3) If a reflowed line could have been tested in a subsequent line's
ShouldApplyTopMargin both before and after reflow, and might have been empty
before or after reflow, then make the next line's previous margin dirty.
4) If reflowing a block changes its carried-out margin, then mark the next
line's previous margin dirty.

Comment 17

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

Comment on attachment 159286 [details] [diff] [review]
fix

Another incremental reflow fix.

Comment 18

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

Comment on attachment 159286 [details] [diff] [review]
fix

>+      changed = aValue != mBlockData->mCarriedOutBottomMargin;
>       mBlockData->mCarriedOutBottomMargin = aValue;
>+      changed = PR_TRUE;

Eh?

(Patch not read in detail.)

Comment 19

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

Attachment: oops

that stray line was just a thinko

Comment 20

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

So what's the difference between this and getting rid of the previous margin
dirty bit entirely and just using the dirty bit?

I recall there was a reason I added this, but maybe it was premature optimization.

Comment 21

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

Ah, bug 86947 comment 54 bullet 2 says that my change for adding it was to make
it so we didn't do the margin work for every non-dirty line -- which doesn't
imply that doing the dirtyness work for every line needing margin work would be
a problem.

Comment 22

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

Actually I was going to remove the previous-margin-dirty bit and just dirty the
lines, but I realized that it still has a use, in conjunction with this patch:
we want to propagate previous-margin-dirty through empty lines, but we *don't*
necessarily need or want to propagate plain dirty-ness through empty lines. 

Comment 23

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

Comment on attachment 159287 [details] [diff] [review]
oops

>+      PRBool dirtyMargin = PR_FALSE;
>+      if (line->mBounds.height == 0 && previousMarginWasDirty) {
>+        // This means that the line might be empty, so the previous dirty
>+        // margin might carry through to the next line.
>+        dirtyMargin = PR_TRUE;
>+      } else if (oldY == 0 && deltaY != line->mBounds.y) {
>         // This means the current line was just reflowed for the first
>+        // time.
>+        dirtyMargin = PR_TRUE;
>+      } else if (oldY == 0 && line->mBounds.y == 0) {
>+        // This means the current line might have been tested in
>+        // a subsequent line's ShouldApplyTopMargin
>+        if (oldYMost != 0 && line->mBounds.height != 0) {
>+          // It wasn't empty before and it wasn't empty now, so
>+          // nothing has changed that could affect ShouldApplyTopMargin
>+        } else {
>+          // Maybe ShouldApplyTopMargin will change
>+          dirtyMargin = PR_TRUE;
>         }
>+      }
>+

[ - lines removed in above snippet ]

It seems like this should be simplified into a single expression.  It'll be a
big boolean expression with comments in the middle, but I think that's probably
cleaner than this.


>Index: layout/html/base/src/nsLineBox.cpp
>-              IsPreviousMarginDirty() ? "prevmargindirty" : "prevmarginclean",
>+              IsPreviousMarginDirty()  ? "prevmargindirty" : "prevmarginclean",

No need for more whitespace.

r+sr=dbaron with that

Comment 24

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

fix checked in

Comment 25

[Jason Bassford]

Martijn and Robert - you guys both rock!

Comment 26

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

Martjin rocks more. I bet he spent more time on that testcase than I did writing
the code :-)

Comment 27

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

This looks like it caused a ~0.75% pageload regression on btek.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Nice!  This fixed a bunch of known incremental-reflow bugs with margins (adding
them as dependencies here).

Comment 29

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

bz: thanks!

Luna and monkey look basically unchanged, so I'm not going to worry about btek.

Comment 30

[Martijn Wargers (dead)]

This also fixed bug 254793.
(Thanks Robert! But you're too kind. You made the fix, not me.)

Comment 31

[Anton van Bohemen]

This still doesn't fix http://white.sakura.ne.jp/~piro/xul/_tabextensions.html.en
Try clicking on "download" for instance, it jumps. Or should that be another bug? 

I'm using Mozilla/5.0 (Windows; U; Windows NT 5.0; @AB_CD@; rv:1.8a4)
Gecko/20040922 Firefox/0.9.1+ (BlueFyre)

Comment 32

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

File that as a separate bug, please, and CC me ... and a minimized testcase
would be nice too :-)

Comment 33

[Stephen Donner [:stephend] Not actively reading bugmail]

It's been filed as bug 261153.

Comment 34

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using the
https://bugzilla.mozilla.org/attachment.cgi?id=159096&action=view# testcase on
build 2004-10-15-05 under Windows XP.

Comment 35

[Hermann Schwab]

Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8a5) Gecko/20041018

though comment 16 was stating "This patch fixes both issues" the HTML testcase
still doesn´t work:

attachment (id=159160) testcase showing dynamic margin changes don't work

Comment 36

[Martijn Wargers (dead)]

(In reply to comment #35)
> attachment (id=159160) testcase showing dynamic margin changes don't work
It works for me, using:
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a5) Gecko/20041020
Firefox/0.9.1+
The bottom margin of 100px gets applied directly after I clicked on the button.
In Mozilla builds before this patch only the 100px top margin got applied
directly, not the 100px bottom margin (focusing the "other text" link would cure
that bug).
Is that not what you are seeing? 

Comment 37

[Hermann Schwab]

Sorry for the bugspam, it´s been my error. I was so used to see margins jumping
all around for the last two hours when looking at testcases and websites, that I
didn´t expect that this was the correct behaviour over here. I promise I won´t
reopen a bug which blocks 8 other bugs, but I assume a lot of people know will
know my name :(

Comment 38

[Joaquin Cuenca Abela]

*** Bug 266992 has been marked as a duplicate of this bug. ***

Comment 39

[Stephen Donner [:stephend] Not actively reading bugmail]

I already verified this on 10-15, and it was erroneously reopened, so verifying
again, this time with build 2004-11-12-04 on Windows XP.