Closed Bug 1242781 Opened 4 years ago Closed 4 years ago

last word of line ending in <br> wraps inside a container sized at its max-content width (intrinsic preferred width)

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox49 --- fixed

People

(Reporter: charles.bourasseau, Assigned: shinglyu)

References

(Depends on 1 open bug, )

Details

(Keywords: testcase)

Attachments

(3 files, 4 obsolete files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/48.0.2564.82 Safari/537.36

Steps to reproduce:

The following code renders on 3 lines instead of 2 lines.

<p style="display:inline-block">some <span>te<br/>xt</span></p>


Actual results:

Firefox renders like that:

some
te
xt


Expected results:

It should render this way:

some te
xt
OS: Unspecified → All
Hardware: Unspecified → x86_64
WFM with FF43 on Win 7, I see:

some te
xt

Is it specific to OSX?
Component: Untriaged → Layout: Block and Inline
Flags: needinfo?(charles.bourasseau)
Product: Firefox → Core
I have the same issue with Firefox 43.0.4 on Windows 7.

The funny thing is that with your link data:text/html;charset=utf-8,<p style%3D"display%3Ainline-block">some <span>te<br%2F>xt<%2Fspan><%2Fp> it's rendered correctly on Mac and Windows.

From jsbin.com it's broken on both: http://output.jsbin.com/kiqumicopu
Flags: needinfo?(charles.bourasseau)
WFM with the tescase I joined to the bug report. Are you sure jsbin doesn't inject some CSS in the source code?
The index.html is working.

If you add "<!DOCTYPE html>" at the beginning of your file it's broken.
This is broken too for me.
Attachment #8712114 - Attachment description: index.html → index.html (without <!DOCTYPE html>)
Is it already broken for you Loic?
Yes, it's broken with !DOCTYPE specified.
Is there any chance this could be fixed?
Version: 43 Branch → 44 Branch
Regression range: [2007-01-27, 2007-01-28]

However, lack of the pushlog, because it is "mozilla-1.9.0" (Firefox 3.0) in retired CVS...
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression, testcase
Hardware: x86_64 → All
Version: 44 Branch → Trunk
(In reply to YF (Yang) from comment #10)
> Regression range: [2007-01-27, 2007-01-28]
> 
> However, lack of the pushlog, because it is "mozilla-1.9.0" (Firefox 3.0) in
> retired CVS...

That's when bug 9458, which initially implemented 'inline-block', landed.  So it's not a regression.
Keywords: regression
This also happens with floats; it's not specific to inline-block.
Summary: Inline elements with <br /> behaves strangely inside a inline-block → last word of line ending in <br> wraps inside a container sized at its max-content width (intrinsic preferred width)
One fix for this is probably relatively simple; add the 1 twip width in BRFrame::AddInlinePrefISize just like we do in Reflow.
I have the one-line fix and reftest ready. But since I'm not that familiar with the MozReview tool and the process so it might take me some time to submit it.
Comment on attachment 8727230 [details] [diff] [review]
Patch - Add 1 twip BRFrame::AddInlinePrefISize and added a reftest

>fixed br bug; added tests

This commit message should be better at explaining what the patch changes.  It should probably be something more like:

Bug 1242781 - Add 1 appunit in BRFrame::AddInlinePrefISize to match the 1 appunit that is added during reflow.  r?dbaron

> /* virtual */ void
> BRFrame::AddInlinePrefISize(nsRenderingContext *aRenderingContext,
>                             nsIFrame::InlinePrefISizeData *aData)
> {
>   if (!GetParent()->StyleContext()->ShouldSuppressLineBreak()) {
>+    aData->currentLine += 1;

This should have a brief comment pointing to the code above in BRFrame::Reflow (e.g., "match the 1 appunit width given in Reflow above"), and you should also add a comment to the code in Reflow pointing to this code (e.g., "the code below in AddInlinePrefISize also adds one appunit of width").

>diff --git a/layout/reftests/line-breaking/1242781-ref.html b/layout/reftests/line-breaking/1242781-ref.html
>new file mode 100644
>--- /dev/null
>+++ b/layout/reftests/line-breaking/1242781-ref.html
>@@ -0,0 +1,6 @@
>+<!DOCTYPE HTML>
>+<html>
>+  <body>
>+    <p style="display:inline-block">some <span>te<br/>xt&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;</span></p>

I think the reference could be clearer if you had an "&nbsp;" after "some" instead of a space, and dropped the other "&nbsp;"s.  The current reference isn't even necessarily reliable since it depends on the width of the space character in the default font.

>diff --git a/layout/reftests/line-breaking/reftest.list b/layout/reftests/line-breaking/reftest.list
>+== 1242781.html 1242781-ref.html

I don't think this is really a line-breaking test.  It would be better to put this in layout/reftests/bugs/ (which is a catch-all directory for things that don't fit elsewhere).



With those things fixed, I think this looks good, but I'd like to have a look at the revised patch, so I'm marking this one as review-.
Attachment #8727230 - Flags: review?(dbaron) → review-
Thank you for the detailed review. I'll update my patch ASAP.
Attached patch Patch v2 (obsolete) — Splinter Review
I've addressed the review comments. Thank you.
Attachment #8727230 - Attachment is obsolete: true
Attachment #8727703 - Flags: review?(dbaron)
Assignee: nobody → slyu
Attachment #8727703 - Flags: review?(dbaron) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bd6d33ad12886757bb9407f5408aee590abaf24
Bug 1242781 - Add 1 appunit in BRFrame::AddInlinePrefISize to match the 1 appunit added during reflow. r=dbaron
backed out for testfailures in reftests like https://treeherder.mozilla.org/logviewer.html#?job_id=23209336&repo=mozilla-inbound
Flags: needinfo?(slyu)
Backed out for reftest failures.

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/4dca9582c0a9

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=2bd6d33ad128
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=23208846&repo=mozilla-inbound

01:25:18     INFO -  REFTEST TEST-START | file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/bugs/465574-1.html
01:25:18     INFO -  REFTEST TEST-LOAD | file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/bugs/465574-1.html | 3991 / 6677 (59%)
01:25:18     INFO -  REFTEST TEST-LOAD | file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/bugs/465574-1-ref.html | 3991 / 6677 (59%)
01:25:18     INFO -  REFTEST TEST-UNEXPECTED-PASS | file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/bugs/465574-1.html | image comparison (==)

Seems like the test needs an update and might be fine.
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ad367f04970060f9d1d6c5fce18865186c7cbe1
Bug 1242781 - Add 1 appunit in BRFrame::AddInlinePrefISize to match the 1 appunit added during reflow. r=dbaron
I relanded with the failure annotation removed.
Flags: needinfo?(slyu)
Backed out for reftest failures.

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/65d3a747feb3

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=3ad367f04970
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=23217592&repo=mozilla-inbound

02:59:56     INFO -  REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/bugs/465574-1.html | image comparison (==), max difference: 116, number of differing pixels: 72

There is also a failure on Android: https://treeherder.mozilla.org/logviewer.html#?job_id=23217707&repo=mozilla-inbound
Flags: needinfo?(slyu)
Sorry about that; I clearly should have done a try run first.
I'm so sorry, let me check that.
Flags: needinfo?(slyu)
Attached image Reftest 465574-1 diff
Reftest 465574-1.html fails on OSX with this diff. (It passes on Linux) The wierd thing is that there is a small shift in the middle of an Hebrew word. I thought it was caused by the 1 appunit hack but that should cause a shift in the end of the line, not in the middle. Do you have any suggestion?
Flags: needinfo?(dbaron)
(In reply to Shing Lyu [:slyu] from comment #27)
> Created attachment 8728288 [details]
> Reftest 465574-1 diff
> 
> Reftest 465574-1.html fails on OSX with this diff. (It passes on Linux) The
> wierd thing is that there is a small shift in the middle of an Hebrew word.
> I thought it was caused by the 1 appunit hack but that should cause a shift
> in the end of the line, not in the middle. Do you have any suggestion?

I think it is because of the 1 appunit hack; the shift probably only shows up for one character because that is the only character where the tiny shift in position causes a visible difference.

The right thing to do is probably to annotate the test as failing on certain platforms, with the comment still pointing to the bug about removing the 1-appunit hack.
Flags: needinfo?(dbaron)
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla48
The test are failing on Linux after I rebase, so I disable it on all platforms again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4292b8cba295&selectedJob=18069789
I also loosen the fuzzy match threshold for a test on Android, which is also caused by the <br> width issue.

Does that test result looks OK? There are some timeout failures but they doesn't seem to be related to this patch. If its OK I'll squash the commit and ask for r? again.
Flags: needinfo?(dbaron)
That try run doesn't appear to include the actual patch, which is presumably why the test starts failing again on all platforms.

You should either look at the try results from when it landed (I believe it passed on Linux, failed on OS X and Win8 and Android, unclear about other Windows versions) or do a try run to figure out which platforms the test now passes on, and then do a try run with the annotation to check that you've annotated it correctly.
Flags: needinfo?(dbaron)
Sorry, this should be the correct one: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd9dd96e0e60&selectedJob=18206302

There are some timeouts and crashes, but no more reftest errors. If it looks OK I'll submit a new patch.
Flags: needinfo?(dbaron)
I think that looks fine, except that you should use fails-if() rather than skip-if(), so that we'll notice if it starts passing.
Flags: needinfo?(dbaron)
Attachment #8727703 - Attachment is obsolete: true
Attachment #8736567 - Attachment is obsolete: true
From the try result on comment 37, looks like the same oranges appeared on windows 8 x64 opt&debug.
Could you help to address that ?
Might adding a left margin of 0.016667px to the table in the reference (maybe only the rtl ones??), where the test has a <br> and the reference doesn't, fix the slight errors in the test?  They could be from subpixel positioning differences related to the same 1-appunit <br> issue.

If that works, please both:
 (1) check that without the patch, the test still fails in the original way
 (2) add a clear comment explaining why the margin is there and that it should be removed when we actually fix bug 421436.
Attachment #8737117 - Attachment is obsolete: true
The failing test, which used to fail on Windows, Android and OSX, now passes on all platform after rebasing, so I marked it as PASSING.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf0b54d1f05e&selectedJob=20442970

Do I need to request for another full review? Thank you.
Flags: needinfo?(dbaron)
(In reply to Shing Lyu [:shinglyu] from comment #43)
> Do I need to request for another full review? Thank you.

No.
Flags: needinfo?(dbaron)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9d85d87c1179
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Duplicate of this bug: 1274793
You need to log in before you can comment on or make changes to this bug.