Closed Bug 78820 Opened 23 years ago Closed 22 years ago

Right alignment of IMG element is ignored.

Categories

(Core :: Layout: Tables, defect)

x86
Windows 2000
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.1alpha

People

(Reporter: kirkpp, Assigned: alexsavulov)

References

Details

(Keywords: testcase, topembed, Whiteboard: DIGBug [see BS18864])

Attachments

(8 files, 1 obsolete file)

From Bugzilla Helper: User-Agent: Mozilla/4.0 (compatible; MSIE 5.5; Windows NT 5.0) BuildID: 2001043004 Reproducible: Always Steps to Reproduce: 1.Put an image element inside a table, with a single table cell. 2.Use the nowrap attribute in the TD element. Actual Results: Image element ignores right alignment, and is placed at the left side of the cell. Expected Results: Image should be placed at right edge of cell. If the nowrap attribute is not included in the TD tag, the image is correctly placed in the right hand side of the cell. Tested on Windows 2000, PIII 600 machine.
This could be related to bug #46885, but I'm not sure that it's the same bug.
Whiteboard: DIG Bug
Keywords: nsbeta1
Whiteboard: DIG Bug → DIGBug
Might be related to bug 45621. Sending to HTMLTables, DOM HTML is for, you know, well, DOM :) Confirmed btw.
Assignee: jst → karnaze
Status: UNCONFIRMED → NEW
Component: DOM HTML → HTMLTables
Ever confirmed: true
QA Contact: desale → amar
I'm seeing what appears to be the same problem at http://search.sussex.ac.uk/USIS/ (the inktomi search icon should be aligned right) When I copy the page and remove `nowrap' on the relevant cell the problem is fixed. But in this example it's also the case that the wrongly-aligned image gets overwritten by text and form controls, which is particularly ugly.
DIG would like a fix for this on the nsBranch. Don't know your schedule, Chris, but if this one is gettable and not too risky, the DIG team would love to have it.
I can confirm the problem with build id: 2001080104 on linux. If nowrap is removed then mozilla displays fine. Please look at the attached test case.
Keywords: testcase
Reassigning to alexsavulov since nowrap is involved.
Assignee: karnaze → alexsavulov
Blocks: 104166
Target Milestone: --- → mozilla0.9.7
-> 0.9.9 target milestone (hope i come to this)
Target Milestone: mozilla0.9.7 → mozilla0.9.9
retargeting
Target Milestone: mozilla0.9.9 → mozilla1.1
nowrap/floater problem
Depends on: 100317
nsbeta1+
Keywords: nsbeta1+
Attached patch fix for the bug (obsolete) — Splinter Review
we need to mark floaters containing lines in blocks that have white-space:nowrap dirty for resize reflows particularly be cause otherwise initially the block might a) get unconstrained size in the initial reflow if is a cell, thus right floaters will be placed left most possible (see nsBlockReflowState::FlowAndPlaceFloater/ region.x = mAvailSpaceRect.x;) b) since is not wrapping, a resize reflow won't affect the lines (thinks the reflow logic) but it will be affected if there is a nowrap involved. happens with div's, p's and you name it. Now in a LTR block the only the right floaters are not working correctly. In a RTL block the left ones are broken. The patch could be eventually optimized if we test for LTR && Right Floaters or RTL && Left Floaters. I don't know yet how much that costs us. Testing for regressions. I don't think that there are some, since the change consists of marking of a line dirty, but who knows.
*** Bug 100317 has been marked as a duplicate of this bug. ***
forget point b) from comment 14. (what the hell was that?) b)if the block is a div or p (or you name it), the reflow logic will think a resize reflow won't affect the lines, but it will be affected if there is a floater and white-space:nowrap is involved.
Blocks: 99097
Blocks: 83330
No longer depends on: 100317
a new issue for white-space:nowrap blocks that contain floated elemets arises after applying the proposed patch (attachment 95209 [details] [diff] [review]): <p style="white-space:nowrap"> <img src="test.gif" height="31" width="88" align="right"> qwerqwer qwerqwer qwerqwer qwerqwer</p> now that the block gets reflown in a resize reflow, if the available space shrinks the image will at some point get overlapped by the text. however, fixing this problem could be made a subject for another bug since IE has this problem too. We might endup not fixing it at all for compatibility reasons. (i know it sounds very harsh, but who can say if the web authors do not missuse that bug already. until now i saw a lot of this cases where authors missuse IE quirks)
Blocks: 92756
Blocks: 82313
*** Bug 109273 has been marked as a duplicate of this bug. ***
Blocks: 133846
looks like i was wrong about the RTL stuff. we do have problems with the left aligned floaters in RTL blocks, but is of different nature so checking for the side of the floaters would save us an unnecessary reflow since left floaters work ok without the patch too. as concerning the text overlapping issue from comment 17, there is a problem in the way the min size is calculated when the floater is right, cause is working for left floaters.
Attached file new enhanced testcase
Attached patch improved patchSplinter Review
so this is the patch. test case to follow to show why the line have to be marked dirty for both type of floaters (L/R) and why in the BlockReflowState we need to prevent advancing to next line in case of nowrap.
Attachment #95209 - Attachment is obsolete: true
Comment on attachment 95732 [details] [diff] [review] improved patch >RCS file: /cvsroot/mozilla/layout/html/base/src/nsBlockFrame.cpp,v >@@ -2204,7 +2204,15 @@ > // reflow this line if it's line wrapped and any of the continuing lines > // are dirty. If we are printing (constrained height), always reflow > // the line. >+ // If the line has floaters and we have white-space:nowrap mark the line >+ // dirty to reflow it again in order to position the floaters correctly. >+ // problem cases: >+ // a) in the initial reflow the block got unconstrained width (that's why >+ // we reflow a second time) >+ // b) the block had a constrained width, but we resize the window. >+ // ( fix for bug 78820 and dependents ) > if ((NS_UNCONSTRAINEDSIZE != aState.mReflowState.availableHeight) || >+ (aState.GetFlag(BRS_NOWRAP) && line->HasFloaters()) || > (!line->IsDirty() && > aState.GetFlag(BRS_COMPUTEMAXWIDTH) && > ::WrappedLinesAreDirty(line, line_end))) { There's no reason this case is specific to nowrap. However, the bug doesn't occur if nowrap isn't present. That suggests there's a bogus check for nowrap elsewhere that also needs a HasFloaters check. Perhaps it's the condition inside the loop in nsBlockFrame::PrepareResizeReflow? (That condition looks wrong. It seems the HasFloaters should be moved out of the (wrapping && (...))) It also seems like PrepareResizeReflow might be a more logical place for this code. >RCS file: /cvsroot/mozilla/layout/html/base/src/nsBlockReflowState.cpp,v >+ // don't let floaters wrap >+ // ( required by fix for bug 78820 and dependents ) >+ if (GetFlag(BRS_NOWRAP)) >+ break; This is incorrect. The 'white-space' property doesn't affect the rules for positioning of floaters. See CSS2 sections 9.5.1 and 16.6.
>This is incorrect. The 'white-space' property doesn't affect the rules for >positioning of floaters. See CSS2 sections 9.5.1 and 16.6. I introduced that in the patch be cause otherwise i get a regression (that also makes us work different than IE). I promised a testcase, i didn't attached it cause I don't like to work during the weekend a lot (there are other things to do too). I still have to test if this is needed in strict mode too. >That suggests there's a bogus check for nowrap elsewhere that also needs a >HasFloaters check. I doubt. From what I debugged, there is no such check. The second reflow is not preformed since in the previous reflow the block already delivered the correct size. Please note that in the testcases with tables (e.g attachment 47961 [details]) the table has the right size in the unpatched version (text + floater). The floater is placed incorrectly be cause of the following code in the block reflow state: nsBlockReflowState::FlowAndPlaceFloater(nsFloaterCache* aFloaterCache, .... if (NS_STYLE_FLOAT_LEFT == floaterDisplay->mFloats) { .... else { okToAddRectRegion = PR_FALSE; region.x = mAvailSpaceRect.x; }
This is the patch I mentioned, and it fixes the testcases in this bug by correcting existing code rather than adding new code.
Attached file regression testcase
this is the test case i was talking about. test it with your patch or with the first one i attached. shrink the window until the two images wrap. that does not happen in the unpatched verision (also IE does not wrap them). that is why i added the code in the block reflow state. i think that your patch marks the line dirty at a better moment than mine. have to test. unfortuantelly there is no usable regression testing yet (need to modify the nmake binaries object directory paths in our rtest files) so i had to imagine all kind of combinations tha might regress.
IE's float support is horribly broken. Do you really propose that we imitate bugs in it even if web pages don't depend on those bugs?
Also, what happens in the testcase if you load it initially with the window narrow rather than resizing it? Do you really want to create that kind of inconsistency?
Er, never mind about creating it -- you'd be removing the inconsistency -- but do note that our old behavior would be the correct one in the normal case of loading the page without resizing the window, and the incorrect one only if you narrowed the window. Thus I think the change you'd cause to the case of loading the page in an already narrow window (or, more realistically, loading a wider version of the same testcase) would be a much more serious regression that the "regression" you claim my patch causes.
This is a variant of attachment 95874 [details] with larger (and percentage-sized, so they don't depend on the window size) images.
sorry, i was not able to respond. got troubles with my monitor. i see what you mean. damn, this only complicates the things i agree, we need consistency. if we consider that the way mozilla handles the 'regression' testcase before was buggy (resize didn't affected the floaters) then, ok we don't have to patch the block reflow state. if we want to imitate IE, then looks like we have to do something else, still i doubt that we have to do this, since your're latest testcase reveals that in the unpatched verion if the floaters are big enough they split and true there are no rules specifying floater behaviour for nowrap. well, let me get a reliable regression test working and see if there are any issues.
david, i did regression testing with the version of the patch you suggested and it passed so far. as soon as i get a fresh build i will do the printing tests although i think there are no problems either, so let's get it in. if you agree, i can get the necessary reviews and chek it in. ok?
Comment on attachment 95848 [details] [diff] [review] patch suggested by dbaron r= alexsavulov let's get this in
Attachment #95848 - Flags: review+
Comment on attachment 95848 [details] [diff] [review] patch suggested by dbaron sr=kin@netscape.com
Attachment #95848 - Flags: superreview+
fixed on trunk. use attachment 95501 [details] to test.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
for the additional issues: bug 83330 - floaters overlap text bug 165062 - a different behaviour than IE. this is not really an important issue yet, might eventually become one. see attachment 95874 [details] and attachment 95876 [details].
BS 18864
*** Bug 148209 has been marked as a duplicate of this bug. ***
When will this get fixed on the branch?
this one is not going on any branch anymore. trunk is the last branch afaik.
Whiteboard: DIGBug → DIGBug [see BS18864]
Keywords: topembed
Blocks: grouper
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: