440149 - whitespace following border-left on inline (but nothing else) is an allowed break point, but should not be

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, defect, P3)

Description

[lasdka88+mozbugs]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9) Gecko/2008052906 Firefox/3.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9) Gecko/2008052906 Firefox/3.0

The following xhtml code:

<h1 id="logoBlock"><a id="logo" href="index.xhtml" title="UKリビングinfo: イギリスの生活情報">\r\n<img src="http://uklivinginfo.webhop.net/assets/images/uklivinginfo/UKlivinginfo_square_120x120.jpg" width="120" height="120" alt="UK リビング info, イギリスの生活情報" /></a>\r\n</h1>

where I have identified windows end-of-line characters by \r\n, displays as if it were:

<h1 id="logoBlock"><a id="logo" href="index.xhtml" title="UKリビングinfo: イギリスの生活情報"><br /><img src="http://uklivinginfo.webhop.net/assets/images/uklivinginfo/UKlivinginfo_square_120x120.jpg" width="120" height="120" alt="UK リビング info, イギリスの生活情報" /></a></h1>

Here the first end-of-line has been replaced by an <br /> and the second has been removed. So, the image is shifted down by one line relative to the surrounding elements when it shouldn't be. IE, Opera, Safari and the previous version of Firefox all display this correctly without the shift down the page. This bug only exists in the just released Firefox 3. I submitted the bug using the 'broken website' facility during beta testing, but it clearly didn't get picked up.

Reproducible: Always

Steps to Reproduce:
See the code above. Insert it into an XHTML 1.0 transitional document, served as application/xhtml+xml so that it is interpreted as XML. (I am not sure if this also occurs when it is served as text/html.) I encountered this bug at http://uklivinginfo.webhop.net/ in the square logo at the top left of the page, but I have since removed the superfluous end-of-line characters to prevent the image from getting shifted down the page. In case you believe this is a css or styling problem, you can check this page to see the environment in which the bug occurred.
Actual Results:  
When the offending end-of-line characters are in place, the image is shifted down by one line, thus interfering with the content below.

Expected Results:  
I expected the end-of-line characters to be ignored, and for the image to be placed inline with the surrounding elements.

If you need any more information about the bug, feel free to contact me.

Comment 1

[lasdka88+mozbugs]

I just specified this bug to be specific to Firefox 3, since I know that with exactly the same environment I didn't encounter this bug in Firefox 2 releases.

Comment 2

[Ria Klaassen (not reading all bugmail)]

Could you attach a testcase with the "Add an attachment" link on this page?

Comment 3

[lasdka88+mozbugs]

Attachment: A test case composed of an XHTML document and two png images, zipped up.

I claim that the incorrect.png image is not correctly placed, but the correct.png image is. See the XHTML source for comments.

In putting together this test case, I realised that it only occurs in a very specific case. Removing either of the two end-of-lines in the 'incorrect placement' case makes it work correctly, as does making minor modifications to the CSS styling.

Comment 4

[Ria Klaassen (not reading all bugmail)]

Thank you! Regression range is:
 http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1196998500&maxdate=1197020039
-> Bug 407111

Comment 5

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

A testcase that can be loaded just by clicking on an attachment would be nice.

Comment 6

[lasdka88+mozbugs]

David.

I don't think that's possible, because the bug involves more than one file; one XHTML source and one image. I have included two images, so as to demonstrate the expected outcome as well. So, you'll have to click twice; once to unzip the archive and once on the XHTML file to launch it in your browser and see the bug demonstrated using the images.

Paul

Comment 7

[philippe (part-time)]

Attachment: stand-alone test case

Comment 8

[lasdka88+mozbugs]

Philippe: I am not familiar with the bugzilla interface so I didn't know you could create an attachment like that. Thanks.

David: Sorry, Philippe clearly did what you were requesting. I just didn't understand what you meant by an attachment that could be clicked on.

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: testcase 3

Here's a further-reduced version of the stand-alone testcase.

Comment 10

[Daniel Holbert [:dholbert]]

(In reply to comment #4)
> Thank you! Regression range is:
> 
> http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1196998500&maxdate=1197020039
> -> Bug 407111

I think you're mistaken about this regression range & source.  The range you give is at around 2007-12-07 01:00:00, but from testing nightlies, I actually get a regression range 1 day earlier, between these nightly builds:

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b2pre) Gecko/2007120504 Minefield/3.0b2pre
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b2pre) Gecko/2007120604 Minefield/3.0b2pre

That yields this regression range:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=mozilla%2Flayout+mozilla%2Fgfx&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-12-05++03%3A00%3A00&maxdate=2007-12-06++05%3A00%3A00&cvsroot=%2Fcvsroot

which points to bug 405577.

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: hacky patch v1

This patch "fixes" the issue by restoring a line that bug 405577 removed from in nsInlineFrame.cpp.

The restored line clamps availableWidth to be non-negative.  And in this bug's testcases, that prevents us from toggling "breakAfter" here in nsTextFrameThebes.cpp:
5992     if (textMetrics.mAdvanceWidth - trimmableWidth > availWidth) {
5993       breakAfter = PR_TRUE;

(The "availWidth" variable used in line 5992 is based on the "availableWidth" value that this patch tweaks.)

Of course, I doubt that this patch is the _right_ fix -- that line was probably removed in bug 405577 for a reason.

Comment 12

[Daniel Holbert [:dholbert]]

(In reply to comment #11)
> Of course, I doubt that this patch is the _right_ fix

(Yeah -- FWIW, "hacky patch v1" regresses the reftest 405577-1.html)

Comment 13

[Ria Klaassen (not reading all bugmail)]

A bit curious I retested this, found out that it regressed between 2007120619 and 2007120701 and when I make that range extra wide (2 hours before the first build), bug 405577 is still not in this range. I have seen this kind of problem before and couldn't explain it. Maybe nightlies are built differently.

Comment 14

[Daniel Holbert [:dholbert]]

(In reply to comment #13)

Weird -- it sounds like the times in those build IDs don't match up with time at the CVS server.  Maybe there's a timezone difference, or a delay between CVS checkout time & build time?

In any case, bug 405577's checkin definitely caused this.

Comment 15

[Daniel Holbert [:dholbert]]

Attachment: testcase 4

Comment 16

[Daniel Holbert [:dholbert]]

Attachment: reference 4

Comment 17

[Daniel Holbert [:dholbert]]

So to sum up my understanding of what's going wrong here:
 - Bug 405577's patch lets us break after zero-width text frames that are beyond the available space.
 - That bug was specifically concerned with situations in which there were other elements to the left of the text frame (which pushed it beyond the available space).  In that situation, such breaks are desired.
 - This bug here is about situations where the zero-width text frame is the *first thing on the line* (and is pushed beyond the available width by a large left margin, rather than by other elements on the line). In this situation, we *don't* want to break after the text frame.

Comment 18

[Daniel Holbert [:dholbert]]

Attachment: reftests patch v1

Here's a series of reftests that test combinations of margin + padding + border on this sort of bug, in both standards and quirks mode, in both rtl and ltr contexts.

Each reference case differs from its testcase only by the removal of "<!--space--> ".

All of these reftests fail using current mozilla-central.

Comment 19

[Daniel Holbert [:dholbert]]

Attachment: patch v2

This patch's strategy is to clamp negative availableWidth values to zero, but *only* when the margin / border / padding pushes it into negative territory.

I'm not yet sure if this strategy is correct.  However, it passes reftests, including the new ones I attached in "reftests patch v1".

Comment 20

[Daniel Holbert [:dholbert]]

Attachment: testcase 5 (breaks after applying patch v2 on Linux)

Here's a testcase that shows where this patch causes problems (at least on Linux -- the widths may need to be tweaked slightly on other platforms, to accommodate differences in fonts)

Expected rendering:
  -----------------------------------
  | [image]T [orange block]         |
  | [image]T [orange block]         |
  -----------------------------------

Bad rendering:
  -----------------------------------
  | [image]T [orange block][image]T | [orange block]
  -----------------------------------

mozilla-central shows expected rendering.
mozilla-central+patchv2 shows bad rendering.

Comment 21

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

Just looking at this briefly -- I'm wondering whether nsLineLayout::LineIsEmpty should be returning true when nsLineLayout::ReflowFrame is reflowing the text frame -- could it be the case that this bug is related to it returning true when it perhaps ought to be returning false?  (Note that I haven't tested what it's returning.)

More generally, have you looked into why nsLineLayout::CanPlaceFrame is returning false (which I presume has to happen in order for the break to happen)?  Are we doing the reflow-twice codepath (LL_NEEDSBACKUP, mLastOptional*, mForceBreakContent)?

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Instead of messing with the available width, we can just fix this directly by refusing to take a break opportunity after empty text at the start of a line.

This patch passes all the tests in this bug, and reftests too.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #21)
> More generally, have you looked into why nsLineLayout::CanPlaceFrame is
> returning false (which I presume has to happen in order for the break to
> happen)?

Actually it's not. The break is caused by the textframe for the empty text returning NS_INLINE_LINE_BREAK_AFTER, because it sees a break opportunity beyond the available width.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Pushed ef3871089bd7

Comment 25

[Daniel Holbert [:dholbert]]

We should make a 1.9.0.x patch for this, too -- it's wanted1.9.0.x+, and the crasher bug 435664 (which is fixed by this bug's patch) is wanted1.9.0.x+ as well.

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The patch should apply to 1.9.0.x quite easily.

Comment 27

[Daniel Holbert [:dholbert]]

Attachment: fix backported to 1.9.0.x

Yup -- roc's fix applies on 1.9.0.x, with fuzz 8 (due to contextual changes in nsTextFrameThebes.cpp and reftest.list).  Here's the resulting patch for CVS trunk (1.9.0.x)

Comment 28

[Daniel Veditz [:dveditz]]

Comment on attachment 356109 [details] [diff] [review]
fix backported to 1.9.0.x

Approved for 1.9.0.7 (primarily because of the crash fix), a=dveditz for release-drivers.

Comment 29

[Daniel Holbert [:dholbert]]

Patch pushed to 1.9.1 on roc's behalf:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/92866cc73bde

Comment 30

[Daniel Holbert [:dholbert]]

attachment 356109 [details] [diff] [review] checked into CVS 1.9.0.x:

Checking in layout/generic/nsTextFrameThebes.cpp;
/cvsroot/mozilla/layout/generic/nsTextFrameThebes.cpp,v  <--  nsTextFrameThebes.cpp
new revision: 3.185; previous revision: 3.184
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/440149-1-ref.html,v
done
Checking in layout/reftests/bugs/440149-1-ref.html;
/cvsroot/mozilla/layout/reftests/bugs/440149-1-ref.html,v  <--  440149-1-ref.html
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/layout/reftests/bugs/440149-1.html,v
done
Checking in layout/reftests/bugs/440149-1.html;
/cvsroot/mozilla/layout/reftests/bugs/440149-1.html,v  <--  440149-1.html
initial revision: 1.1
done
Checking in layout/reftests/bugs/reftest.list;
/cvsroot/mozilla/layout/reftests/bugs/reftest.list,v  <--  reftest.list
new revision: 1.466; previous revision: 1.465
done

Comment 31

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.9.0.7 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.7pre) Gecko/2009021704 GranParadiso/3.0.7pre using testcases.

Comment 32

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.9.1 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090213 Shiretoko/3.1b3pre.