Closed Bug 257612 Opened 20 years ago Closed 20 years ago

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

Categories

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

Product:
Core
Component:
Layout: Block and Inline
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: jasonb, Assigned: roc)

References

(
URL
)

Details

(Keywords: regression)

Attachments

(4 files, 1 obsolete file)

Testcase
452 bytes, application/xhtml+xml
Details
Like this
772 bytes, patch
Details | Diff | Splinter Review
testcase showing dynamic margin changes don't work
214 bytes, text/html
Details
oops
8.55 KB, patch
dbaron
: review+
dbaron
: superreview+
Details | Diff | Splinter Review 
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.
Keywords: regression
A plain HTML testcase for this would be very helpful.
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...
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...
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.)
> 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.
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".)
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....
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.
> 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?
Yeah, and I'd like to fix it.
Attached file 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.
Attached patch Like this — — Splinter Review 
Like this, but off course it would be better if the general problem would be
solved (the testcase) :)
Martjin, you rock! I should be able to fix this now.
Assignee: nobody → roc
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.
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.
Attached patch fix (obsolete) — — Splinter Review 
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 on attachment 159286 [details] [diff] [review]
fix

Another incremental reflow fix.
Attachment #159286 - Flags: superreview?(dbaron)
Attachment #159286 - Flags: review?(dbaron)
Comment on attachment 159286 [details] [diff] [review]
fix

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

Eh?

(Patch not read in detail.)
Attachment #159286 - Flags: superreview?(dbaron)
Attachment #159286 - Flags: superreview-
Attachment #159286 - Flags: review?(dbaron)
Attachment #159286 - Flags: review-
Attached patch oops — — Splinter Review 
that stray line was just a thinko
Attachment #159287 - Flags: superreview?(dbaron)
Attachment #159287 - Flags: review?(dbaron)
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.
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.
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 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
Attachment #159287 - Flags: superreview?(dbaron)
Attachment #159287 - Flags: superreview+
Attachment #159287 - Flags: review?(dbaron)
Attachment #159287 - Flags: review+
fix checked in
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Martijn and Robert - you guys both rock!
Martjin rocks more. I bet he spent more time on that testcase than I did writing
the code :-)
This looks like it caused a ~0.75% pageload regression on btek.
Nice!  This fixed a bunch of known incremental-reflow bugs with margins (adding
them as dependencies here).
Blocks: 129981, 138142, 189414, 189852, 229674, 230924
bz: thanks!

Luna and monkey look basically unchanged, so I'm not going to worry about btek.
This also fixed bug 254793.
(Thanks Robert! But you're too kind. You made the fix, not me.)
Blocks: 254793
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)
File that as a separate bug, please, and CC me ... and a minimized testcase
would be nice too :-)
Blocks: 261196
Verified FIXED using the
https://bugzilla.mozilla.org/attachment.cgi?id=159096&action=view# testcase on
build 2004-10-15-05 under Windows XP.
Status: RESOLVED → VERIFIED
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
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
(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? 


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 :(
Status: REOPENED → RESOLVED
Closed: 20 years ago → 20 years ago
Resolution: --- → FIXED
*** Bug 266992 has been marked as a duplicate of this bug. ***
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: