Closed
Bug 26650
Opened 25 years ago
Closed 22 years ago
Align attribute not working inside Button element
Categories
(Core :: Layout, defect, P3)
Core
Layout
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)
(deleted),
text/html
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•25 years ago
|
||
Reporter | ||
Comment 2•25 years ago
|
||
After talking to rick, I will reassign to steve.
Assignee: rickg → buster
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
Updated•24 years ago
|
Updated•24 years ago
|
Keywords: mozilla1.0
SPAM. HTML Element component deprecated, changing component to Layout. See bug
88132 for details.
Component: HTML Element → Layout
Comment 7•23 years ago
|
||
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
???
Updated•23 years ago
|
Whiteboard: [CSS1-5.5.25]
Comment 9•22 years ago
|
||
*** Bug 156852 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 10•22 years ago
|
||
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).
Updated•22 years ago
|
Attachment #111824 -
Flags: superreview?(bzbarsky)
Assignee | ||
Comment 11•22 years ago
|
||
Stealing, but attinasi won't mind.
Assignee: attinasi → arunan_bala
Assignee | ||
Comment 12•22 years ago
|
||
Oops, bug 138403 was on my candidate list but is not actually fixed by this.
Comment 13•22 years ago
|
||
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 14•22 years ago
|
||
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+
Assignee | ||
Comment 15•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
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.
Assignee | ||
Comment 18•22 years ago
|
||
As previous patch but not returning anything.
Attachment #111824 -
Attachment is obsolete: true
Assignee | ||
Comment 19•22 years ago
|
||
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•22 years ago
|
||
Um... that patch is identical to the previous one, both on visual inspection and
according to 'diff'. ;)
Assignee | ||
Comment 21•22 years ago
|
||
That's what you get for working past midnight :) Must sleep more ...
Attachment #113642 -
Attachment is obsolete: true
Comment 22•22 years ago
|
||
Comment on attachment 113672 [details] [diff] [review]
Real v2!
+ //reflowState.computedWidth = availSize;
Take that out, and sr=bzbarsky... ;)
Attachment #113672 -
Flags: superreview+
Assignee | ||
Comment 23•22 years ago
|
||
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
Updated•22 years ago
|
Attachment #113695 -
Flags: superreview+
Comment 24•22 years ago
|
||
Arunan, please remind me to check this in once the tree reopens for 1.4a, ok?
Assignee | ||
Comment 25•22 years ago
|
||
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
Comment 26•22 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•