Closed Bug 1306590 Opened 4 years ago Closed 8 days ago
percent "text-indent" values resolve against the wrong container box, for overflow:hidden elements (causing stray character to be visible at reddit)
19.54 KB, image/png
2.13 KB, text/html
682 bytes, text/html
688 bytes, text/html
515 bytes, text/html
[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
At least, I can reproduce the problem on very old Firefox4.0.
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).
Component: Layout → Layout: Text
Comment on attachment 8796639 [details] (ignore, better testcase coming up next) (ah, disregard previous comment/testcase; another better version coming up shortly)
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?
[er, meant to direct end of comment 8 at @astevenson]
(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.
Opera 12.6 (Presto) renders testcase 2 like Chrome, too -- so it looks like we indeed are the odd one out here.
Summary: Visual issues on https://www.reddit.com/ → percent "text-indent" values resolve against the wrong box, for overflow:hidden elements (causing stray character to be visible at reddit)
Summary: percent "text-indent" values resolve against the wrong box, for overflow:hidden elements (causing stray character to be visible at reddit) → percent "text-indent" values resolve against the wrong container box, for overflow:hidden elements (causing stray character to be visible at reddit)
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)
(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
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
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.
This sounds like a duplicate of bug 908706.
Bug 1453298 makes this invalid.
Clearly ni on Jet as he no longer works at Mozilla.
Webcompat Priority: --- → ?
Webcompat Priority: ? → ---
Status: NEW → RESOLVED
Closed: 8 days ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.