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)
Tracking
()
VERIFIED
FIXED
People
(Reporter: jasonb, Assigned: roc)
References
()
Details
(Keywords: regression)
Attachments
(4 files, 1 obsolete file)
452 bytes,
application/xhtml+xml
|
Details | |
772 bytes,
patch
|
Details | Diff | Splinter Review | |
214 bytes,
text/html
|
Details | |
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.
Reporter | ||
Updated•20 years ago
|
Keywords: regression
Assignee | ||
Comment 1•20 years ago
|
||
A plain HTML testcase for this would be very helpful.
Reporter | ||
Comment 2•20 years ago
|
||
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...
Reporter | ||
Comment 3•20 years ago
|
||
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...
Reporter | ||
Comment 4•20 years ago
|
||
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•20 years ago
|
||
> 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.
Reporter | ||
Comment 6•20 years ago
|
||
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".)
Assignee | ||
Comment 7•20 years ago
|
||
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....
Reporter | ||
Comment 8•20 years ago
|
||
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.
Reporter | ||
Comment 9•20 years ago
|
||
> 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?
Assignee | ||
Comment 10•20 years ago
|
||
Yeah, and I'd like to fix it.
Comment 11•20 years ago
|
||
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   character entity, instead of the empty space character which is used currently.
Comment 12•20 years ago
|
||
Like this, but off course it would be better if the general problem would be solved (the testcase) :)
Assignee | ||
Comment 13•20 years ago
|
||
Martjin, you rock! I should be able to fix this now.
Assignee: nobody → roc
Assignee | ||
Comment 14•20 years ago
|
||
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.
Assignee | ||
Comment 15•20 years ago
|
||
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.
Assignee | ||
Comment 16•20 years ago
|
||
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.
Assignee | ||
Comment 17•20 years ago
|
||
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-
Assignee | ||
Comment 19•20 years ago
|
||
that stray line was just a thinko
Assignee | ||
Updated•20 years ago
|
Attachment #159286 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
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.
Assignee | ||
Comment 22•20 years ago
|
||
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+
Assignee | ||
Comment 24•20 years ago
|
||
fix checked in
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 25•20 years ago
|
||
Martijn and Robert - you guys both rock!
Assignee | ||
Comment 26•20 years ago
|
||
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.
Comment 28•20 years ago
|
||
Nice! This fixed a bunch of known incremental-reflow bugs with margins (adding them as dependencies here).
Assignee | ||
Comment 29•20 years ago
|
||
bz: thanks! Luna and monkey look basically unchanged, so I'm not going to worry about btek.
Comment 30•20 years ago
|
||
This also fixed bug 254793. (Thanks Robert! But you're too kind. You made the fix, not me.)
Blocks: 254793
Comment 31•20 years ago
|
||
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)
Assignee | ||
Comment 32•20 years ago
|
||
File that as a separate bug, please, and CC me ... and a minimized testcase would be nice too :-)
It's been filed as bug 261153.
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
Comment 35•20 years ago
|
||
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 → ---
Comment 36•20 years ago
|
||
(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•20 years ago
|
||
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
Comment 38•20 years ago
|
||
*** 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.
Description
•