1567098 - img tag within p tags not separated by a space from the text is not rendered correctly

Status: RESOLVED FIXED

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

Description

[bunglegrind]

Attachment: html example reproducing the issue

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:68.0) Gecko/20100101 Firefox/68.0

Steps to reproduce:

Just open the html file herein attached. Not sure about CSP, if the picture does not appear, please download it and edit the img src accordingly.

The issue appears in Firefox 68 and above (in 69.0b5 for instance), Win64 and OS X platforms. At least. Not present in 67.0.4.

Actual results:

You should note that the text in the first div is at the right of the picture, whereas in the second div is (correctly) under the image.

Please note that the picture is about the same size of the div.

Expected results:

Chrome correctly shows the text under the picture in both cases.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

I bet this is a regression from bug 1283222, but will confirm ASAP.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Yeah, https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=e51a022e039fce5f3e30079d6700a7575913cfbe&tochange=121fd81292cbff53feb33d739c2432b902a36675

Jonathan, sounds like maybe floats should be excluded from the check? Or we should allow breaking at the beginning of the line unconditionally? Or something of that sort?

Comment 3

[Jonathan Kew [:jfkthame]]

Interestingly, if I open the DevTools inspector, it magically fixes the rendering for me. (It's not immediately obvious why; seems like maybe a devtools bug?)

We should probably allow a break after a float if the line is (otherwise) empty. Building with a patch to check how it behaves...

Comment 4

[Emilio Cobos Álvarez (:emilio)]

(In reply to Jonathan Kew (:jfkthame) from comment #3)

Interestingly, if I open the DevTools inspector, it magically fixes the rendering for me. (It's not immediately obvious why; seems like maybe a devtools bug?)

Devtools causes the whole page to reframe, and causes the creation of whitespace-only text-nodes. Probably the whitespace that is before the image in the test-case, which usually is optimized away in the frame constructor, is acting as a soft break opportunity.

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1567098 - Allow line-break after a float placeholder if the line is empty. r=emilio

Comment 6

[Jonathan Kew [:jfkthame]]

I think something like this ^^ should resolve the issue; pushed a try run to check if anything breaks. https://treeherder.mozilla.org/#/jobs?repo=try&revision=0399cf6c715b1a50a4269fdec8703dc24eb3bb6a

Assuming it looks good, we should also add some tests.

(In reply to Emilio Cobos Álvarez (:emilio) from comment #4)

Devtools causes the whole page to reframe, and causes the creation of whitespace-only text-nodes. Probably the whitespace that is before the image in the test-case, which usually is optimized away in the frame constructor, is acting as a soft break opportunity.

Right, I think that's what is happening; but it seems like a bug to me. Simply opening the Inspector should not modify the thing being inspected!

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1567098 - Add a WPT reftest where no content fits onto the shortened line beside a float. r=emilio

Comment 8

[Emilio Cobos Álvarez (:emilio)]

(In reply to Jonathan Kew (:jfkthame) from comment #6)

Right, I think that's what is happening; but it seems like a bug to me. Simply opening the Inspector should not modify the thing being inspected!

Yes, inspector should probably not call getBoxQuads() on all nodes in the page, which is what causes the creation of these. I'll file an inspector bug.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/77f2a37b794a
Allow line-break after a float placeholder if the line is empty. r=emilio
https://hg.mozilla.org/integration/autoland/rev/3169928e34dc
Add a WPT reftest where no content fits onto the shortened line beside a float. r=emilio

Comment 10

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/17919 for changes under testing/web-platform/tests

Comment 11

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 12

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/77f2a37b794a
https://hg.mozilla.org/mozilla-central/rev/3169928e34dc

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Is this worth a Beta backport? Not sure about ESR68 either.

Comment 14

[Jonathan Kew [:jfkthame]]

Yes, probably worth backporting this, as it could manifest as a layout regression on real sites (I assume the reporter here observed the bug on a site, and then created the reduced testcase to illustrate it), and the patch is really small and simple.

Comment 15

[Jonathan Kew [:jfkthame]]

Comment on attachment 9079075 [details]
Bug 1567098 - Allow line-break after a float placeholder if the line is empty. r=emilio

Beta/Release Uplift Approval Request

• User impact if declined: Incorrect float placement in some cases
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: none
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Very minor patch that partially reverts to earlier behavior around floats
• String changes made/needed: none

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Bug can result in broken layout, e.g. in cases where there is a full-width float at the start of a block
• User impact if declined: Bad layout on affected sites
• Fix Landed on Version: 70
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Very minor patch that partially reverts to earlier behavior around floats
• String or UUID changes made by this patch: none

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9079075 [details]
Bug 1567098 - Allow line-break after a float placeholder if the line is empty. r=emilio

Simple fix with automated test for incorrect float placement in some circumstances. Approved for 69.0b8 and 68.1esr.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/f8df999f8ed3
https://hg.mozilla.org/releases/mozilla-esr68/rev/4566fd9bd50f

Comment 18

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/releases/mozilla-beta/rev/34fca225d15c
https://hg.mozilla.org/releases/mozilla-beta/rev/b665d4c48980

Comment 19

[bunglegrind]

(In reply to Jonathan Kew (:jfkthame) from comment #14)

Yes, probably worth backporting this, as it could manifest as a layout regression on real sites (I assume the reporter here observed the bug on a site, and then created the reduced testcase to illustrate it), and the patch is really small and simple.

Yes, indeed.