Closed Bug 1426760 Opened 2 years ago Closed 2 years ago

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

Categories

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

57 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: tobi, Assigned: emilio)

Details

Attachments

(3 files)

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.
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.
Confirm that the space is larger in latest Nightly
Status: UNCONFIRMED → NEW
Component: Untriaged → Layout
Ever confirmed: true
Product: Firefox → Core
Attached file Testcase
This isn't related to pseudo-elements at all, any absolutely positioned descendant (the <span> in this test-case) does it.
Component: Layout → Layout: Text
Summary: Oversized spacing with CSS content:"foo" → Absolutely positioned descendants affect line-height.
Attachment #8938637 - Attachment mime type: text/plain → text/html
Thanks for investigating!
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.
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)
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.)
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 :)
I did what you suggested, it works. Thanks!

Sincerely: Good luck with the bug :)
(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... :)
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!
I'm debugging this a bit more, will unassign if I get stuck :)
Assignee: nobody → emilio
Kinda sad that I had to add a test for this, I would've expected the CSS2 WPT test-suite to catch this :(
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 :).
(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 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?
Attachment #8938683 - Flags: review?(jfkthame) → review+
Summary: Absolutely positioned descendants affect line-height. → placeholders for absolutely positioned elements or floats can affect line-height
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.
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/eca6aca5819f
Don't let placeholders affect line height. r=jfkthame
(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! :)
https://hg.mozilla.org/mozilla-central/rev/eca6aca5819f
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Thanks!
You need to log in before you can comment on or make changes to this bug.