Closed Bug 408656 Opened 15 years ago Closed 15 years ago

Buttons in Backbase spinner demo are at the wrong spot

Categories

(Core :: Layout, defect, P2)

x86
All
defect

Tracking

()

VERIFIED FIXED

People

(Reporter: martijn.martijn, Assigned: dholbert)

References

()

Details

(Keywords: regression, testcase)

Attachments

(4 files, 1 obsolete file)

Attached file testcase
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.
Daniel can you take this (or re-assign)
Assignee: nobody → dholbert
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
(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
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
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)
(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...
I just retested with a 2007-04-29 build, so yes, I still see it broken in builds before 2007-05-01.
Attached file testcase 2
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.
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.
(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.
Attached patch fixSplinter Review
This fixes both testcases; not sure if it's correct.
(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
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.
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?  ;)
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)
(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 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+
Attached patch reftests (obsolete) — Splinter Review
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)
(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
Fix checked in:
  /cvsroot/mozilla/layout/generic/nsInlineFrame.h,v  <--  nsInlineFrame.h
  new revision: 1.71; previous revision: 1.70
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Attached patch reftests v2Splinter Review
oops -- first reftests post was missing the 408656-1c.html file
Attachment #295429 - Attachment is obsolete: true
Attachment #295429 - Flags: review?(bzbarsky)
Attachment #295463 - Flags: review?(bzbarsky)
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 on attachment 295463 [details] [diff] [review]
reftests v2

r= with that addition
Attachment #295463 - Flags: review?(bzbarsky) → review+
Reftests checked in.
Flags: in-testsuite? → in-testsuite+
Depends on: 412623
Verified in Firefox 3 Beta 3. Thanks!
Depends on: 429315
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.