Closed
Bug 408656
Opened 17 years ago
Closed 17 years ago
Buttons in Backbase spinner demo are at the wrong spot
Categories
(Core :: Layout, defect, P2)
Tracking
()
VERIFIED
FIXED
People
(Reporter: martijn.martijn, Assigned: dholbert)
References
()
Details
(Keywords: regression, testcase)
Attachments
(4 files, 1 obsolete file)
(deleted),
text/html
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
See url, the spinbuttons of the spinner are at the wrong spot, they should appear at the right hand side of the text input.
See the testcase, you should at least have 100px space from the top and the left of the page, with the black bordered box.
This regressed between 2007-02-26 and 2007-02-27:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-02-26+04&maxdate=2007-02-27+09&cvsroot=%2Fcvsroot
Regression from bug 371536, I think.
Laurens, I saw your post at:
http://fredericiana.com/2007/12/04/gmail-clickable-colored-tags-good-but-not-good-enough/
where you talked of tons of rendering issues in Firefox 3.
Could you post the links of all the pages where you see rendering issues somewhere? Or file new bugs for them? Or mail them directly to me? Thanks!
Flags: blocking1.9?
They got fixed in the meanwhile, I’ll have to check out the SVN log to see how, I’ll get back on that.
Only the spinner’s still broken now afaik, as reported here, and we didn’t look into the cause for this yet. This testcase’ll probably be useful :).
The changes to importNode also broke a couple of things (slider, panelSet, Node.lookupPrefix, etc; all the same cause, we fixed it yesterday but it’s not in our 4.1.2 release). I’m not sure whether that was Firefox’s fault though, I think we used (or didn’t use) importNode correctly.
And there was also something with getClientRect that caused a little bit of havoc to us. I’ll ask about that too.
~Grauw
> I think we used (or didn’t use) importNode correctly.
Incorrectly, I meant to say ;p.
Comment 3•17 years ago
|
||
Daniel can you take this (or re-assign)
Assignee: nobody → dholbert
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Assignee | ||
Comment 4•17 years ago
|
||
(In reply to comment #3)
> Daniel can you take this (or re-assign)
Sure.
First thin I'm noticing:
Both the testcase and URL seem to fix themselves when Firefox window is resized.
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•17 years ago
|
||
Testcase seems to have regressed between these nightlies:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a5pre) Gecko/20070501 Minefield/3.0a5pre
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a5pre) Gecko/20070502 Minefield/3.0a5pre
Regression window:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-05-01+03%3A00%3A00&maxdate=2007-05-02+06%3A00%3A00&cvsroot=%2Fcvsroot
Reporter | ||
Comment 6•17 years ago
|
||
Well, weird that you get a different regression range.
With your regression range, I guess it's a regression from bug 372703.
(thanks for the info, Laurens)
Assignee | ||
Comment 7•17 years ago
|
||
(In reply to comment #6)
> Well, weird that you get a different regression range.
Yeah -- that's what I'm seeing from the testing I've done. Let me know if you still see it broken in builds before 2007-05-01, though.
> With your regression range, I guess it's a regression from bug 372703.
> (thanks for the info, Laurens)
Hrm, backing out bug 372703 doesn't seem to fix it. Looking for other possible causes...
Assignee | ||
Comment 8•17 years ago
|
||
maybe bug 378975?
Reporter | ||
Comment 9•17 years ago
|
||
I just retested with a 2007-04-29 build, so yes, I still see it broken in builds before 2007-05-01.
Assignee | ||
Comment 10•17 years ago
|
||
This is the testcase I was testing with -- it's a modified form of attachment 293493 [details]. My regression range stands wrt this testcase, as far as I can tell.
However, testing with the orig testcase and the URL (which I should've done while looking at regression range but didn't -- my bad), I *do* get buggy behavior before 05-01.
So, maybe there are multiple things involved, one of which broke in each regression range.
Assignee | ||
Comment 11•17 years ago
|
||
Forgot to mention -- as stated in the <title>, testcase 2 behaves as follows:
- Correct: Green block renders 50px from top of page
- Incorrect: Green block renders at top of page, and then snaps down 50px when browser is resized.
Every build I've tested up to 05-01 is correct on testcase 2, and everything after is incorrect on testcase 2.
Assignee | ||
Comment 12•17 years ago
|
||
(In reply to comment #0)
> See the testcase, you should at least have 100px space from the top and the
> left of the page, with the black bordered box.
>
> This regressed between 2007-02-26 and 2007-02-27:
Yup, I get the same range using first testcase.
Assignee | ||
Comment 13•17 years ago
|
||
This fixes both testcases; not sure if it's correct.
Assignee | ||
Comment 14•17 years ago
|
||
(In reply to comment #6)
> With your regression range, I guess it's a regression from bug 372703.
I tried backing out that bug's patch, and it doesn't fix testcase 2, so I'm guessing it's not from that one.
I'm guessing it's a regression from bug 378975 ("Gut InitialReflow"), because the *initial* reflow seems to be the only one that's broken -- the bug fixes itself when you resize the window.
(Can't cleanly back that bug's patch out of current trunk to test this theory, though, because it's a pretty far-reaching patch.)
Blocks: 378975
Comment 15•17 years ago
|
||
Hmm. More likely, it's an incremental reflow bug that the initial reflow changes exposed. In this case, the view for the scrollframe is being badly mispositioned. The view dump says for it:
0x84dc118 {480,480,6120,6120} z=0 vis=1 clientData=0x873f340 <
which is wrong, since it's parented to the view for the CanvasFrame.
Comment 16•17 years ago
|
||
The original "regression" is from bug 371536.
But the real issue seems to be that nsLineLayout::VerticalAlignFrames and nsLineLayout::RelativePositionFrames reposition inline frames but don't actually reposition the child views. In particular, in nsLineLayout::RelativePositionFrames pfd->mSpan is true, so we don't call PositionChildViews() on the rel pos inline. As a result, the abs pos frame's view is never positioned. That's the real bug. This code handles positioning all the in-flow descendants, one way or another, but not the out-of-flows.
So the right fix might in fact be what's attached to this bug, at least in the short term. And in the long term, we'll kill views, right? ;)
That would work, yes.
Assignee | ||
Comment 18•17 years ago
|
||
Comment on attachment 295294 [details] [diff] [review]
fix
Patch summary: Make nsPositionedInlineFrame::NeedsView return true (instead of falling back on nsIFrame method, which returns false)
My understanding is that this fixes the problem as Boris described it in Comment 16 for this reason: The rel pos inline's nsPositionedInlineFrame will now get its own view, and so that gets positioned and will ensure that its child view gets positioned.
Attachment #295294 -
Attachment description: fix? → fix
Attachment #295294 -
Flags: superreview?(bzbarsky)
Attachment #295294 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 19•17 years ago
|
||
(In reply to comment #14)
> I'm guessing it's a regression from bug 378975 ("Gut InitialReflow"),
(In reply to comment #16)
> The original "regression" is from bug 371536.
Oops -- you're totally right. "Gut InitialReflow" just exposed another way for the original regression to rear its head.
Comment 20•17 years ago
|
||
Comment on attachment 295294 [details] [diff] [review]
fix
Let's make sure to get a reftest in too?
Attachment #295294 -
Flags: superreview?(bzbarsky)
Attachment #295294 -
Flags: superreview+
Attachment #295294 -
Flags: review?(bzbarsky)
Attachment #295294 -
Flags: review+
Assignee | ||
Comment 21•17 years ago
|
||
Here are three reftests based on the original testcase.
Note: I changed "overflow: scroll" to "overflow: hidden", because scrollframes get tweaked slightly by the inclusion of "<script>document.body.offsetHeight</script>" -- this seems to increase the scrolled-frame height, making the scrollbar appear. I'm filing a separate bug on that issue.
408656-1a.html is basically the original testcase -- it fails after 2007-02-27
408656-1b.html is like testcase2 -- it fails after 2007-05-01.*
408656-1c.html is a semi-reference case, as a sanity check. (It's the same as tests a & b, but without the <script> tag). This test passes in all builds I've tried.
408656-1-ref.html is a very simplified reference case.
* It turns out the key difference that caused different regression ranges between "testcase" and "testcase2" is the presence of whitespace between the </script> and the subsequent <div>. If there's no whitespace, as in testcase2 and 408656-1b.html, then we don't see the bug for any builds up until the "Gut InitialReflow" change. (see Comment 14)
Attachment #295429 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 22•17 years ago
|
||
(In reply to comment #21)
> Note: I changed "overflow: scroll" to "overflow: hidden", because scrollframes
> get tweaked slightly by the inclusion of
> "<script>document.body.offsetHeight</script>" -- this seems to increase the
> scrolled-frame height, making the scrollbar appear. I'm filing a separate bug
> on that issue.
Filed this issue as bug 410849.
OS: Windows XP → All
Assignee | ||
Comment 23•17 years ago
|
||
Fix checked in:
/cvsroot/mozilla/layout/generic/nsInlineFrame.h,v <-- nsInlineFrame.h
new revision: 1.71; previous revision: 1.70
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Assignee | ||
Comment 24•17 years ago
|
||
oops -- first reftests post was missing the 408656-1c.html file
Attachment #295429 -
Attachment is obsolete: true
Attachment #295429 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•17 years ago
|
Attachment #295463 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 25•17 years ago
|
||
Comment on attachment 295463 [details] [diff] [review]
reftests v2
>+== 408656-1a.html 408656-1-ref.html
>+== 408656-1b.html 408656-1-ref.html
reftest.list needs a line for 408656-1c.html as well:
== 408656-1c.html 408656-1-ref.html
(I'll add that before checking in the reftests)
Comment 26•17 years ago
|
||
Comment on attachment 295463 [details] [diff] [review]
reftests v2
r= with that addition
Attachment #295463 -
Flags: review?(bzbarsky) → review+
Comment 28•17 years ago
|
||
Verified in Firefox 3 Beta 3. Thanks!
Comment 29•17 years ago
|
||
verified fixed using Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9pre) Gecko/2008050804 Minefield/3.0pre and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008050614 Minefield/3.0pre
Also verified per comment #28
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•