26650 - Align attribute not working inside Button element

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[Chris Petersen]

Version: Apprunner
Build: 2000020108
Platforms: All

Expected Results: In a Button element, alignment values should be applied to the 
assigned element

What I got: In a Button element, align values such as "center" or "right" which 
have been applied to P elements are not functioning.



Steps to reproduce:

1) Open the attached test case in Apprunner.

2) The Button element contains several P elements:

 *The first P element has no aligned and is the width is the longest among the P 
elements. 

* The second P has been assigned "right" align.

* The third P has been assigned "center" align.

3) The second and third elements are not rendering with the assigned alignment.

Comment 1

[Chris Petersen]

Attachment: A button element containing two P elements which have a assigned alignment value.

Comment 2

[Chris Petersen]

After talking to rick, I will reassign to steve.

Comment 3

[buster]

post-beta2 issue.

Comment 4

[buster]

redistributing bugs across future milestones, sorry for the spam

Comment 5

[buster]

This bug has been marked "future" because we have determined that it is not 
critical for netscape6.0. 
If you feel this is an error, or if it blocks your work in some way -- please 
attach your concern to the bug for reconsideration.

Comment 6

[Heikki Toivonen (remove -bugzilla when emailing directly)]

SPAM. HTML Element component deprecated, changing component to Layout. See bug
88132 for details.

Comment 7

[Kevin McCluskey (gone)]

Build reassigning Buster's bugs to Marc.

Comment 8

[basic]

what's the deal with the testcase being 
<!-- CONFIDENTIAL AND PROPRIETARY TEST SCRIPT OF NETSCAPE COMMUNICATIONS
CORPORATION 

???

Comment 9

[Boris Zbarsky [:bzbarsky]]

*** Bug 156852 has been marked as a duplicate of this bug. ***

Comment 10

[Arunan Balasubramaniam]

Attachment: Demonstrates how a resize reflow helps.

I was poking around this and think the problem with buttons is that they only
perform the first half of a shrink-wrap reflow.  Having done the first "size
finding" reflow they don't do the follow-up resize reflow.  This patch shows
what happens when you do one - good things :)

This problem causes this bug, bug 111183, bug 138403 and bug 26644.

This patch is probably not entirely correct, but it demonstrates how the fix
could work, hence I have not performed the layout regression tests with it.  If
anybody would like to review it or see the regression results, I'll happily
take this bug and see it through (as part of my ongoing attempt to understand
Gecko).

Comment 11

[Arunan Balasubramaniam]

Stealing, but attinasi won't mind.

Comment 12

[Arunan Balasubramaniam]

Oops, bug 138403 was on my candidate list but is not actually fixed by this.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 111824 [details] [diff] [review]
Demonstrates how a resize reflow helps.

>+  NS_IMETHOD ReflowButtonContents(nsIPresContext* aPresContext,

Why not make this return void?	You never check the return value; it always
returns NS_OK.

Other than that, looks pretty good to me.  ;)

Comment 14

[John Keiser (jkeiser)]

Comment on attachment 111824 [details] [diff] [review]
Demonstrates how a resize reflow helps.

Wow.  Most impressive.	It is very sad that we have to actually completely
reflow this guy a second time to handle alignment issues, but that's a more
fundamental architecture issue that we won't address here :)

Comment 15

[Arunan Balasubramaniam]

Oh goody.  Thanks for looking at the patch.  Would you like me to:

a) perform the regression tests?

b) attach a new patch with the prototype changed to void so it doesn't look like
I just copy and pasted the prototype (*cough*) ?  I have no CVS access so
somebody else will have to check this in and would have to change this
themselves if I did not.

Comment 16

[John Keiser (jkeiser)]

Regression tests: yes, please.  You can check against
http://www.johnkeiser.com/mozilla/select/init.html and
http://www.johnkeiser.com/mozilla/select/js.html (do Hide and Unhide a few
times) to start with--just make sure they all look right and work, I don't
expect huge problems but it's good to check.  This is unlikely to go in the tree
until 1.4alpha (in 4 weeks or so).  When that time comes we'll check it in and
run with it ourselves for a bit--please bug me or bz if it branches and we don't
check it in right away.

I am impressed you have this much knowledge of our reflow system already.  Kudos.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Adding some deps that should be retested once this lands.

Arunan, please do attach the updated patch with the changed function signature
at some point.

Comment 18

[Arunan Balasubramaniam]

Attachment: Patch v2 (void return)

As previous patch but not returning anything.

Comment 19

[Arunan Balasubramaniam]

Attachment: Regression test failures

This is the list of regression test failures.  I didn't run the ones in
table/printing as they just kept freezing the viewer and I got bored of
commenting out one and trying the next repeatedly.

Visually, the test for bug 79863 (duped to bug 55285) has the multi-line text
in the buttons now centred rather than left aligned.

The frame state change is an addition of NS_FRAME_OUTSIDE_CHILDREN.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Um... that patch is identical to the previous one, both on visual inspection and
according to 'diff'.  ;)

Comment 21

[Arunan Balasubramaniam]

Attachment: Real v2!

That's what you get for working past midnight :)  Must sleep more ...

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 113672 [details] [diff] [review]
Real v2!

+  //reflowState.computedWidth = availSize;

Take that out, and sr=bzbarsky... ;)

Comment 23

[Arunan Balasubramaniam]

Attachment: Removed commented out line, updated comments a bit.

I think you're just trying to see if I attach the wrong patch again ;)

The comments have changed slightly, hope that's OK.  I don't know the
mozilla.org policy on minor corrections to comments such as capitalisation - do
you prefer not to for CVS or to do so for correctness?

Comment 24

[Boris Zbarsky [:bzbarsky]]

Arunan, please remind me to check this in once the tree reopens for 1.4a, ok?

Comment 25

[Arunan Balasubramaniam]

John: I should have mentioned that the tests you pointed me at seem fine with
this patch.

Boris: The tree is open for 1.4a and has gained lots of pretty colours.  Oh
look, you're just checking in other things on tinderbox now.  Does John need to
mark the patch r+ or can it be assumed to be carried forward as the changes you
asked for were minor?  (And oddly I did not get your last message in this bug -
don't know if that was Bugzilla or Hotmail).

Comment 26

[Boris Zbarsky [:bzbarsky]]

Thanks for the reminder.  ;)

John's r= still applies.  I just checked in the patch to the 1.4a trunk.

fixed.