1426760 - placeholders for absolutely positioned elements or floats can affect line-height

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Tobi Reif]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20171128222554

Steps to reproduce:

Open https://tobireif.com/non_site_stuff/test_case_for_firefox_spacing_issue/ in Firefox.


Actual results:

There's a large space between the heading and the paragraph.


Expected results:

The space should be smaller, as in eg Chrome.

Comment 1

[Tobi Reif]

Attachment: screenshot_chrome_vs_firefox.png

Comment 2

[Tobi Reif]

When I open
https://tobireif.com/non_site_stuff/test_case_for_firefox_spacing_issue/
in Firefox, and (using the dev tools) set the h1 :before and :after "content" property to "none", the oversized-spacing issue is gone.

By the way, the real URL (as opposed to the above reduced test page) is
https://tobireif.com/demos/grid/ .
Please also compare the spacing in this page in Chrome (etc) vs in Firefox.

Comment 3

[Flore Allemandou [:flore]]

Confirm that the space is larger in latest Nightly

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Testcase

This isn't related to pseudo-elements at all, any absolutely positioned descendant (the <span> in this test-case) does it.

Comment 5

[Tobi Reif]

Thanks for investigating!

Comment 6

[Emilio Cobos Álvarez (:emilio)]

This is an attempt that fixes this:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=a61f8149957788109193dfc73afcc2485e66218d

But I don't know the nsLineLayout code well enough to know whether it's correct or not, or whether it just wallpapers an underlying issue.

Comment 7

[Tobi Reif]

Would that code change also fix the space issue on https://tobireif.com/demos/grid/ ?
(oversized space below the h1 in Firefox vs eg Chrome)

Comment 8

[Jonathan Kew [:jfkthame]]

Just FYI -- I think you could work around the problem on your site by removing the "line-height: 59.5px;" property from the h1 styles (perhaps replacing with "line-height: 1" to ensure consistent behavior), and instead using negative margin-top and margin-bottom values, and/or reducing/removing the vertical padding that is currently applied. AFAICT the problem of the abs-pos content disrupting the spacing only occurs when line-height is artificially small, and I don't see any real need for that.

(This doesn't excuse the bug, which we still need to fix! But in the specific case of your site, I believe you could avoid it without too much difficulty.)

Comment 9

[Tobi Reif]

If I remember correctly, the metrics of that font are so extreme that I had applied all that CSS to make the font manageable, and it works - except in Firefox.

I'll still try what you suggested - thanks!

You wrote "line-height: 1" - did you mean "line-height: 1em"? Or something else?

And yes, I hope that the bug in Firefox will get fixed in any case :)

Comment 10

[Tobi Reif]

I did what you suggested, it works. Thanks!

Sincerely: Good luck with the bug :)

Comment 11

[Emilio Cobos Álvarez (:emilio)]

(In reply to Tobi Reif from comment #7)
> Would that code change also fix the space issue on
> https://tobireif.com/demos/grid/ ?
> (oversized space below the h1 in Firefox vs eg Chrome)

Perhaps! Though the patch in there is pretty orange, so needs work. Part of it was because of another commit in my tree, which makes it a bit easier, but still... :)

Comment 12

[Tobi Reif]

In reply to Emilio:

I did what Jonathan suggested, it works, so my page is OK now, even without the fix.

But there still is that bug in Firefox - good luck!

Comment 13

[Emilio Cobos Álvarez (:emilio)]

I'm debugging this a bit more, will unassign if I get stuck :)

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1426760: Don't let placeholders affect line height.

Review commit: https://reviewboard.mozilla.org/r/209278/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/209278/

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Kinda sad that I had to add a test for this, I would've expected the CSS2 WPT test-suite to catch this :(

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Probably dbaron is a better reviewer for this looking at the blame log, though not sure how backlogged he is right now. Feel free to redirect to him if you wish Jonathan :).

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Here's a much greener try run:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1f68a183aff19dbf5af091517336fe61e081c35

Comment 18

[Emilio Cobos Álvarez (:emilio)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #16)
> Probably dbaron is a better reviewer for this looking at the blame log,
> though not sure how backlogged he is right now. Feel free to redirect to him
> if you wish Jonathan :).

Oh, looks like you were also involved in bug 942017, which is the last time this code was touched, so nvm :)

Comment 19

[Jonathan Kew [:jfkthame]]

Comment on attachment 8938683 [details]
Bug 1426760: Don't let placeholders affect line height.

https://reviewboard.mozilla.org/r/209278/#review215082

Thanks for looking into this so promptly!

::: layout/generic/nsLineLayout.cpp:2217
(Diff revision 1)
> +      bool canUpdate = !pfd->mIsTextFrame && !pfd->mIsPlaceholder;
> +
>        // Only consider non empty text frames when line-height=normal
> -      bool canUpdate = !pfd->mIsTextFrame;
> +      if (pfd->mIsTextFrame && pfd->mIsNonWhitespaceTextFrame) {
> -      if (!canUpdate && pfd->mIsNonWhitespaceTextFrame) {
>          canUpdate =
>            frame->StyleText()->mLineHeight.GetUnit() == eStyleUnit_Normal;
>        }

It makes sense this should fix the problem, but I found it a bit confusing to read in this form. Maybe consider rewriting this to handle the text and non-text cases in two clearly-separate branches, as:

      bool canUpdate;
      if (pfd->mIsTextFrame) {
        // Only consider text frames if they are non-empty and have
        // line-height=normal
        canUpdate = pfd->mIsNonWhitespaceTextFrame &&
          frame->StyleText()->mLineHeight.GetUnit() == eStyleUnit_Normal;
      } else {
        canUpdate = !pfd->mIsPlaceholder;
      }

AFAICT this should be functionally equivalent, and it seems easier to understand in my opinion. WDYT?

Comment 20

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8938683 [details]
Bug 1426760: Don't let placeholders affect line height.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/209278/diff/1-2/

Comment 21

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

r=dbaron as well, fwiw

Comment 22

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8938683 [details]
Bug 1426760: Don't let placeholders affect line height.

https://reviewboard.mozilla.org/r/209278/#review215082

> It makes sense this should fix the problem, but I found it a bit confusing to read in this form. Maybe consider rewriting this to handle the text and non-text cases in two clearly-separate branches, as:
> 
>       bool canUpdate;
>       if (pfd->mIsTextFrame) {
>         // Only consider text frames if they are non-empty and have
>         // line-height=normal
>         canUpdate = pfd->mIsNonWhitespaceTextFrame &&
>           frame->StyleText()->mLineHeight.GetUnit() == eStyleUnit_Normal;
>       } else {
>         canUpdate = !pfd->mIsPlaceholder;
>       }
> 
> AFAICT this should be functionally equivalent, and it seems easier to understand in my opinion. WDYT?

Yeah, agreed.

I thought a bit about it and the cleanest IMO would be some sort of static function like:

  static bool FrameCanUpdateLineBSize(const PerFrameData&);

But I wasn't quite sold on the name. If you think that's better or think of another name happy to push that as a followup.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eca6aca5819f
Don't let placeholders affect line height. r=jfkthame

Comment 24

[Emilio Cobos Álvarez (:emilio)]

(In reply to David Baron :dbaron: ⌚️UTC-8 (don't expect responses/reviews until early January) from comment #21)
> r=dbaron as well, fwiw

Thanks David! :)

Comment 25

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/eca6aca5819f

Comment 26

[Tobi Reif]

Thanks!