Closed Bug 1567098 Opened 4 months ago Closed 4 months ago

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

Categories

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

68 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- fixed
firefox68 --- wontfix
firefox69 --- fixed
firefox70 --- fixed

People

(Reporter: bunglegrind, Assigned: jfkthame)

References

(Regression)

Details

(Keywords: regression)

Attachments

(3 files)

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.

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

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?

Status: UNCONFIRMED → NEW
Component: Graphics: WebRender → Layout: Block and Inline
Ever confirmed: true
Flags: needinfo?(jfkthame)
Keywords: regression
Priority: -- → P2
Regressions: 1283222

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

Flags: needinfo?(jfkthame)

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

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!

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

Pushed by jkew@mozilla.com:
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
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/17919 for changes under testing/web-platform/tests
See Also: → 1567310
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Status: NEW → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
Assignee: nobody → jfkthame

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

Flags: needinfo?(jfkthame)
Flags: in-testsuite+
Regressed by: 1283222
No longer regressions: 1283222

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.

Flags: needinfo?(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
Attachment #9079075 - Flags: approval-mozilla-esr68?
Attachment #9079075 - Flags: approval-mozilla-beta?
Attachment #9079114 - Flags: approval-mozilla-beta? approval-mozilla-esr68?

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.

Attachment #9079075 - Flags: approval-mozilla-esr68?
Attachment #9079075 - Flags: approval-mozilla-esr68+
Attachment #9079075 - Flags: approval-mozilla-beta?
Attachment #9079075 - Flags: approval-mozilla-beta+
Attachment #9079114 - Flags: approval-mozilla-esr68?
Attachment #9079114 - Flags: approval-mozilla-esr68+
Attachment #9079114 - Flags: approval-mozilla-beta?
Attachment #9079114 - Flags: approval-mozilla-beta+

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

You need to log in before you can comment on or make changes to this bug.