78820 - Right alignment of IMG element is ignored.

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Kirk Parsons]

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.

Comment 1

[Kirk Parsons]

Attachment: A zip file with an HTML test case, and an image used in the test case.

Comment 2

[Kirk Parsons]

This could be related to bug #46885, but I'm not sure that it's the same bug.

Comment 3

[Fabian Guisset]

Might be related to bug 45621.
Sending to HTMLTables, DOM HTML is for, you know, well, DOM :)
Confirmed btw.

Comment 4

[Ben Roberts]

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.

Comment 5

[clayton]

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.

Comment 6

[ron]

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.

Comment 7

[ron]

Attachment: html file showing that nowrap => right aligned img is ignored

Comment 8

[ron]

Attachment: nowrap removed => mozilla displays right

Comment 9

[karnaze (gone)]

Reassigning to alexsavulov since nowrap is involved.

Comment 10

[Alexandru Savulov]

-> 0.9.9 target milestone
(hope i come to this)

Comment 11

[Alexandru Savulov]

retargeting

Comment 12

[Alexandru Savulov]

nowrap/floater problem

Comment 13

[Kevin McCluskey (gone)]

nsbeta1+

Comment 14

[Alexandru Savulov]

Attachment: fix for the bug

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.

Comment 15

[Alexandru Savulov]

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

Comment 16

[Alexandru Savulov]

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.

Comment 17

[Alexandru Savulov]

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)

Comment 18

[Alexandru Savulov]

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

Comment 19

[Alexandru Savulov]

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.

Comment 20

[Alexandru Savulov]

Attachment: new enhanced testcase

Comment 21

[Alexandru Savulov]

Attachment: improved patch

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.

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 23

[Alexandru Savulov]

>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;
    }

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch suggested by dbaron

This is the patch I mentioned, and it fixes the testcases in this bug by
correcting existing code rather than adding new code.

Comment 25

[Alexandru Savulov]

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

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 29

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: more realistic regression testcase

This is a variant of attachment 95874 [details] with larger (and percentage-sized, so
they don't depend on the window size) images.

Comment 30

[Alexandru Savulov]

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.

Comment 31

[Alexandru Savulov]

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 32

[Alexandru Savulov]

Comment on attachment 95848 [details] [diff] [review]
patch suggested by dbaron

r= alexsavulov
let's get this in

Comment 33

[kinmoz]

Comment on attachment 95848 [details] [diff] [review]
patch suggested by dbaron

sr=[e-mail]

Comment 34

[Alexandru Savulov]

fixed on trunk. use attachment 95501 [details] to test.

Comment 35

[Alexandru Savulov]

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

Comment 36

[Alexandru Savulov]

BS 18864

Comment 37

[Alexandru Savulov]

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

Comment 38

[Christine Hoffman]

When will this get fixed on the branch?

Comment 39

[Alexandru Savulov]

this one is not going on any branch anymore. trunk is the last branch afaik.

Comment 40

[Marek Z. Jeziorek]

batch: adding topembed per Gecko2 document
http://rocknroll.mcom.com/users/marek/publish/Gecko/Gecko2Tasks.html