Closed Bug 26650 Opened 25 years ago Closed 22 years ago

Align attribute not working inside Button element

Categories

(Core :: Layout, defect, P3)

defect

Tracking

()

RESOLVED FIXED
Future

People

(Reporter: chrispetersen, Assigned: foss)

References

Details

(Keywords: css1, testcase, Whiteboard: [CSS1-5.5.25])

Attachments

(3 files, 3 obsolete files)

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.
After talking to rick, I will reassign to steve.
Assignee: rickg → buster
Target Milestone: M16
post-beta2 issue.
Status: NEW → ASSIGNED
Target Milestone: M16 → M18
redistributing bugs across future milestones, sorry for the spam
Target Milestone: M18 → M19
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.
Target Milestone: M19 → Future
Blocks: html4.01
Keywords: css1
SPAM. HTML Element component deprecated, changing component to Layout. See bug 88132 for details.
Component: HTML Element → Layout
Keywords: testcase
Build reassigning Buster's bugs to Marc.
Assignee: buster → attinasi
Status: ASSIGNED → NEW
what's the deal with the testcase being <!-- CONFIDENTIAL AND PROPRIETARY TEST SCRIPT OF NETSCAPE COMMUNICATIONS CORPORATION ???
Whiteboard: [CSS1-5.5.25]
*** Bug 156852 has been marked as a duplicate of this bug. ***
Attached patch Demonstrates how a resize reflow helps. (obsolete) (deleted) — Splinter Review
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).
Attachment #111824 - Flags: superreview?(bzbarsky)
Stealing, but attinasi won't mind.
Assignee: attinasi → arunan_bala
Oops, bug 138403 was on my candidate list but is not actually fixed by this.
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. ;)
Attachment #111824 - Flags: superreview?(bzbarsky)
Attachment #111824 - Flags: superreview+
Attachment #111824 - Flags: review?(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 :)
Attachment #111824 - Flags: review?(jkeiser) → review+
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.
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.
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.
Attached patch Patch v2 (void return) (obsolete) (deleted) — Splinter Review
As previous patch but not returning anything.
Attachment #111824 - Attachment is obsolete: true
Attached file Regression test failures (deleted) —
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.
Um... that patch is identical to the previous one, both on visual inspection and according to 'diff'. ;)
Attached patch Real v2! (obsolete) (deleted) — Splinter Review
That's what you get for working past midnight :) Must sleep more ...
Attachment #113642 - Attachment is obsolete: true
Comment on attachment 113672 [details] [diff] [review] Real v2! + //reflowState.computedWidth = availSize; Take that out, and sr=bzbarsky... ;)
Attachment #113672 - Flags: superreview+
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?
Attachment #113672 - Attachment is obsolete: true
Attachment #113695 - Flags: superreview+
Arunan, please remind me to check this in once the tree reopens for 1.4a, ok?
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).
Severity: normal → trivial
Thanks for the reminder. ;) John's r= still applies. I just checked in the patch to the 1.4a trunk. fixed.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Blocks: 45670
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: