1306590 - percent "text-indent" values resolve against the wrong container box, for overflow:hidden elements (causing stray character to be visible at reddit)

Status: RESOLVED INVALID

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

Description

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

Attachment: Reddit issue screenshot.png

[Affected versions]:
- latest Nightly 52.0a1 (2016-09-29)
- latest Aurora 51.0a2 (2016-09-29)
- 50.0b3 build1 (20160929120120)
- 49.0.1 build3 20160922113459

[Affected platforms]:
- Windows 10 x86
- Windows 7 x64
- Mac OS X 10.10.5
- Ubuntu 14.04 x64

[Steps to reproduce]:
1. Launch Firefox
2. Go to https://www.reddit.com/
3. Choose a random post and click the "share" button
  - inspect the Share window

[Expected result]:
- The Share window is properly open and no visual issues come out

[Actual result]:
- Some visual issues can be notice in the Share window, near the "X" button (see the attached screenshot)

[Regression range]:
- I will investigate this as soon as possible

Comment 1

[Alice0775 White]

Attachment: Stand alone testcase

Comment 2

[Alice0775 White]

At least, I can reproduce the problem on very old Firefox4.0.

Comment 3

[Daniel Holbert [:dholbert]]

The issue seems to be with "text-indent: 100%".

In Chrome, that pushes the text ("close this window") entirely outside of the container. In Firefox, it does not (quite).

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: (ignore, better testcase coming up next)

Here's a reduced testcase with various percentage & pixel values.

I *think* what Firefox is doing makes sense -- we're resolving "text-indent: 100%" against the content-box area of the containing block.  So we end up positioning the text at the right edge of the content box -- and since there's some padding as well (outside the content box), that means a sliver of text still shows up.

I also think Chrome has a bug -- it looks to me like they're interpreting percentages to be a factor of 10 too large.  In Chrome, "text-indent: 10%" renders like how Firefox renders "text-indent: 100%".  That explains why they're positioning the text out of view -- they're positioning it *WAY* out of view, by a factor of 10 too far.

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8796639 [details]
(ignore, better testcase coming up next)

(ah, disregard previous comment/testcase; another better version coming up shortly)

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: testcase 2

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: semi-reference case 2 (removed "overflow:hidden")

Comment 8

[Daniel Holbert [:dholbert]]

So per https://developer.mozilla.org/en-US/docs/Web/CSS/text-indent , "percentages refer to the width of the containing block".

In Firefox, when we have "overflow:hidden", the containing block is the scrollable frame (the overflow:hidden thing).  In Chrome, they're apparently interpreting "containing block" to mean the *container* of the scrollable frame.

I'm not sure offhand who's correct... BUT:
 - given that "overflow:hidden" creates two nested boxes (the outer one with the specified size & the inner one that gets clipped), I tend to think it makes sense that we consider the outer box the containing block of the inner one.
 - Reddit's "100%" here seems like it likely wants to refer to "100% of the width of this box".  In Firefox, it does (though there's padding which is why there's a glitch).  In Chrome, it refers to 100% of the width of some ancestor box which is probably way huger.

So while we sort out whether or not this is a Firefox bug (and in the fastest case, ship a fix which will take a few months to make it to release), I think Reddit probably really wants to either:
 (a) change their .c-hide-text CSS rule to use "text-indent: 200%" (instead of 100%), for good measure (which will work as long as the extra padding isn't larger than the content-box size)

...and/or possibly:
 (b) use "color: transparent" to make the text invisible, if it does happen to show up. (I'm assuming they have it there for screen-reader purposes, and hopefully screen readers will read transparent text, though I'm not entirely sure.)

Adam, could you reach out to someone at Reddit and suggest that they make change (a) here?

Comment 9

[Daniel Holbert [:dholbert]]

[er, meant to direct end of comment 8 at @astevenson]

Comment 10

[Jeff Muizelaar [:jrmuizel]]

(In reply to Daniel Holbert [:dholbert] from comment #6)
> Created attachment 8796651 [details]
> testcase 2

It looks like Edge renders this the same as Chrome and Safari.

Comment 11

[Daniel Holbert [:dholbert]]

Opera 12.6 (Presto) renders testcase 2 like Chrome, too -- so it looks like we indeed are the odd one out here.

Comment 12

[Daniel Holbert [:dholbert]]

This shouldn't be too hard to fix. Wherever we resolve this percentage, we need to just add a special-case for scrolled frames, to step over the outer scroll frame and look at *its* containing block as our percent-basis, when we resolve percent values for this property during layout.

SPECIFICALLY:
I think the relevant place where we resolve the percentage is here:
https://dxr.mozilla.org/mozilla-central/rev/9baec74b3db1bf005c66ae2f50bafbdb02c3be38/layout/generic/nsLineLayout.cpp#223-229
...though we should audit other usages of mTextIndent, too, and see if any of them attempt to resolve the percent value.

And to test whether we're a scrolled frame there, I think we'd check whether mBlockReflowInput->mFrame->StyleContext()->GetPseudo() == nsCSSAnonBoxes::scrolledContent.  And if that's true, we'd want to look outside of that frame for the containing block. (get its parent's containing block, or something like that)

Comment 13

[Daniel Holbert [:dholbert]]

(Actually, it might be cleaner to fix this by changing the internals of the ReflowInput::GetContainingBlockContentISize(), so that it recognizes whether we're scrolled-content and steps out appropriately...  This is the only caller of that method, so that wouldn't break assumptions in other code.)

One open question here, which we should figure out before/while fixing this:
 - How/why do we get this correct for percent "padding-left" values?  Both "padding-left" and "text-indent" seem to be resolved using information from ReflowInput::mCBReflowInput.  But for "padding-left", we do seem to resolve the percentages against the outer containing block, rather than against the size of the scroll-frame.  Whatever functionality we use to do that, we should probably share or mimic for this text-indent resolution.  (BTW, percent "padding" values get resolved in ReflowInput::InitOffsets, below here:
 https://dxr.mozilla.org/mozilla-central/rev/9baec74b3db1bf005c66ae2f50bafbdb02c3be38/layout/generic/ReflowInput.cpp#2445

Comment 14

[Daniel Holbert [:dholbert]]

Attachment: testcase 3 (comparing padding-left to text-indent)

Comment 15

[Adam Stevenson [:adamopenweb]]

Daniel: I think this is what we are looking for.
https://github.com/reddit/reddit/blob/862a7e0fc5142058a88044f9254a8ec5db977a77/r2/r2/public/static/css/components/utils.less#L17

Comment 16

[Daniel Holbert [:dholbert]]

Nice sleuthing! I think you're right. I think if they replace "100%" with "200%" there, that should avoid the issue in Firefox (without affecting the rendering anywhere else).

If you file a github issue, maybe link it from here? Meanwhile, I think Jet is going to find a rendering-team engineer to work on this.

Comment 17

[Adam Stevenson [:adamopenweb]]

Reported: https://github.com/reddit/reddit/issues/1700

Comment 18

[David Baron :dbaron:]

This sounds like a duplicate of bug 908706.

Comment 19

[David Baron :dbaron:]

Bug 1453298 makes this invalid.

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Clearly ni on Jet as he no longer works at Mozilla.

Comment 21

[Firefox Bug Husbandry Bot]

Migrating Webcompat whiteboard priorities to project flags. See bug 1547409.

Comment 22

[Firefox Bug Husbandry Bot]

See bug 1547409. Migrating whiteboard priority tags to program flags.

Comment 23

[Mike Taylor [:miketaylr]]

(In reply to David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-7 from comment #19)

Bug 1453298 makes this invalid.

reddit.com was redesigned, so the reported issue no longer exists. Unsure if we should just close this as INVALID, given dbaron's comment?

Comment 24

[Ting-Yu Lin [:TYLin] (UTC-8)]

Close per comment 19.