408656 - Buttons in Backbase spinner demo are at the wrong spot

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Martijn Wargers (dead)]

Attachment: 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!

Comment 1

[u81239]

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

Comment 2

[u81239]

> I think we used (or didn’t use) importNode correctly.

Incorrectly, I meant to say ;p.

Comment 3

[Mike Schroepfer]

Daniel can you take this (or re-assign)

Comment 4

[Daniel Holbert [:dholbert]]

(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.

Comment 5

[Daniel Holbert [:dholbert]]

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

Comment 6

[Martijn Wargers (dead)]

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)

Comment 7

[Daniel Holbert [:dholbert]]

(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...

Comment 8

[Daniel Holbert [:dholbert]]

maybe bug 378975?

Comment 9

[Martijn Wargers (dead)]

I just retested with a 2007-04-29 build, so yes, I still see it broken in builds before 2007-05-01.

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: 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.

Comment 11

[Daniel Holbert [:dholbert]]

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.

Comment 12

[Daniel Holbert [:dholbert]]

(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.

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: fix

This fixes both testcases; not sure if it's correct.

Comment 14

[Daniel Holbert [:dholbert]]

(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.)

Comment 15

[Boris Zbarsky [:bzbarsky]]

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

[Boris Zbarsky [:bzbarsky]]

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 17

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

That would work, yes.

Comment 18

[Daniel Holbert [:dholbert]]

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.

Comment 19

[Daniel Holbert [:dholbert]]

(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

[Boris Zbarsky [:bzbarsky]]

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

Let's make sure to get a reftest in too?

Comment 21

[Daniel Holbert [:dholbert]]

Attachment: reftests

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)

Comment 22

[Daniel Holbert [:dholbert]]

(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.

Comment 23

[Daniel Holbert [:dholbert]]

Fix checked in:
  /cvsroot/mozilla/layout/generic/nsInlineFrame.h,v  <--  nsInlineFrame.h
  new revision: 1.71; previous revision: 1.70

Comment 24

[Daniel Holbert [:dholbert]]

Attachment: reftests v2

oops -- first reftests post was missing the 408656-1c.html file

Comment 25

[Daniel Holbert [:dholbert]]

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

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 295463 [details] [diff] [review]
reftests v2

r= with that addition

Comment 27

[Daniel Holbert [:dholbert]]

Reftests checked in.

Comment 28

[u81239]

Verified in Firefox 3 Beta 3. Thanks!

Comment 29

[Carsten Book [:Tomcat]]

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