Closed Bug 1236400 Opened 8 years ago Closed 8 years ago

Layout problem on mobile version of Bing.com (from emulating -webkit-box with modern flexbox), with layout.css.prefixes.webkit = true

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: sunhaitao, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

Attachments

(9 files, 3 obsolete files)

34.54 KB, image/png
Details
35.82 KB, image/png
Details
90.45 KB, image/png
Details
109.39 KB, image/png
Details
1.60 KB, text/html
Details
18.42 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
13.11 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
10.57 KB, patch
Details | Diff | Splinter Review
7.12 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
Several elements get wrong 'display' property values ('block' instead of 'inline').
Flags: needinfo?(dholbert)
Summary: Enabling layout.css.prefixes.webkit creates layout problem on Bing.com → Enabling layout.css.prefixes.webkit creates layout problem on mobile version of Bing.com
(In reply to SUN Haitao from comment #0)
> Several elements get wrong 'display' property values ('block' instead of
> 'inline').

They're inside of a "display: -webkit-box; -webkit-box-orient: vertical;" element (the <p>).

We promote the "display:-webkit-box" with modern "display:flex", which causes each of its children to have their display blockified. In contrast, "true" -webkit-box (in WebKit-derived browsers) does not blockify each child like that. I'm not 100% sure what it does do -- maybe it wraps all inline-level children in an anonymous block, or something.

For this to work, we'll need to add some extra hacks for "display: -webkit-box", to make it behave differently from "display:flex" with respect to inline-level children.
Flags: needinfo?(dholbert)
Summary: Enabling layout.css.prefixes.webkit creates layout problem on mobile version of Bing.com → Enabling layout.css.prefixes.webkit creates layout problem on mobile version of Bing.com (from emulating -webkit-box with modern flexbox)
Blocks: 1238668
Summary: Enabling layout.css.prefixes.webkit creates layout problem on mobile version of Bing.com (from emulating -webkit-box with modern flexbox) → Layout problem on mobile version of Bing.com (from emulating -webkit-box with modern flexbox), with layout.css.prefixes.webkit = true
(In reply to Daniel Holbert [:dholbert] from comment #3)
> For this to work, we'll need to add some extra hacks for "display:
> -webkit-box", to make it behave differently from "display:flex" with respect
> to inline-level children.

I think this change is non-trivial -- until we've got a better idea of how many sites are broken due to this same problem, there are perhaps higher-priority thinks for us to focus on from an implementation & risk/reward perspective.

For the time being, I'd suggest that we reach out to Bing and ask if they could adjust their code here to do one of the following things:
 (1) Just drop the -webkit-box styling entirely, if their standards-based display:block fallback code works everywhere. (That's the code that we render, when we lack -webkit-box support, and it seems to work great.)

...OR...
 (2) Add an explicit <div> wrapper around the inline-level elements here (inside of the <p>).  That shouldn't impact the behavior in old-school "-webkit-box" model --they'll still have a single flex item there. But it'll prevent them from ending up with multiple vertically-stacked flex items, with modern-flexbox-emulation of "-webkit-box".
The same problem also appears on Baidu (e.g. `http://m.baidu.com/s?word=Firefox`). It may be too late for an easy way.
Could you attach a screenshot of the Baidu behavior (and indicate which lines are broken)?  I don't see anything that looks obviously-broken there (but it might just be that I can't read Chinese).
Flags: needinfo?(sunhaitao)
(I do see considerable breakage at Baidu if I *disable* the 'layout.css.prefixes.webkit' pref. e.g. text superimposed, line wrapping where it looks like the line isn't supposed to wrap, and search button not showing up. But I don't see any obvious problems in a default Nightly profile, with the pref at its default enabled state.)
Actually, this page breaks on *BOTH* settings. Several highlight terms in red turn into blocks (scrolling down a bit to see) while the pref is set to true. But there ARE more problems while set to false.
Flags: needinfo?(sunhaitao)
Attached image Expected Result (obsolete) —
Attachment #8712242 - Attachment is obsolete: true
Depends on: 1257661
Blocks: 1257688
bz: frame-constructor question for you.

So, to match Chrome on the just-attached testcase, I think I need to add a special case in our FrameConstructionItem::NeedsAnonFlexOrGridItem() impl* -- if we're in a "display:-webkit-box" flex container there, and the fc item is an inline-level thing, I want to return "true".

Do you know what piece of state I should be checking for, to cover inline content (including replaced elements)? I tried "mIsAllInline", which mostly works, but its documentation suggests that it's not perfect, and also it ends up putting an entire block-in-inline subtree into an anonymous flex item, which is bad.  (To match chrome, we need to be able to do the block-in-inline split, and then wrap each of the inline pieces in an anonymous flex container.)


*That impl is here:
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp?rev=1fbadb0b2f44#12488
Flags: needinfo?(bzbarsky)
Wrapping the separate pieces of an {ib} split into different anonymous flex container is likely to be bad no matter what, unfortunately.  I'm pretty sure that there's code around that depends on the parents of the various pieces being either ib siblings or continuations...  _Maybe_ you can make those flex containers be continuations of each other, I guess.

Either way, doesn't sound like NeedsAnonFlexOrGridItem will really do what you want, since it operates before we've performed the ib split, right?
Flags: needinfo?(bzbarsky)
I'm willing to bet there's not much content that depends on IB splits directly inside of a -webkit-box, in practice, so I'm OK having imperfect compatibility there.

So: supposing we're OK disregarding IB splits (letting the whole inline/block/inline get wrapped into a single anonymous flex item): would mIsAllInline be a decent thing to check for inside of NeedsAnonFlexOrGridItem, to catch inline content? (including replaced & non-replaced elements)
mIsAllInline will be true for everything inline except for an inline with a block inside id (ib split).  In that case it will be false.

mHasInlineEnds will be true for anything that's an actual non-replaced inline or would not cause an ib split.  Notably, it will be true for things that are display:block but floated or absolutely positioned, since the _placeholder_ doesn't cause an ib split.

If you really just care about the CSS display type, nsStyleDisplay::IsInlineOutsideStyle() is what you want.
These new enum values are added with same behavior as their modern flexbox
equivalents -- they're hooked up to NS_NewFlexContainerFrame, and they're
listed alongside the modern flexbox enums in 'switch' & 'if' statements.

There's one exception, which I call out with a comment at the end of the patch:
we don't treat -webkit-box the same as flexbox in IsFlexOrGridDisplayType(),
because that method is used to determine whether we should blockify
inline-level children of a flex/grid container, and we don't want to blockify
any children of a -webkit-box. (Instead, we want to wrap them in an anonymous
flex item. That happens in the next patch.)
Attachment #8733107 - Flags: review?(mats)
(Note that the WebkitPrefixEnabledPrefChangeCallback boilerplate in nsLayoutUtils.cpp is copypasted from the existing equivalent display:grid/display:inline-grid function there.)
This patch adds an "aIsWebkitBox" arg to the needs-anonymous-flex-or-grid-item related functions in nsCSSFrameConstructor, and makes us check the container & pass in the correct value at all of the callsites.

Internally, it makes us just check nsStyleDisplay::IsInlineOutsideStyle() for the child fc-item, which seems to be basically what we need to check.*

* (except for block-in-inline splits, but I'm going to assume we can handle those incorrectly [aggressively wrapping them] inside of a -webkit-box for the time being, since it seems that matching Blink/WebKit there would complicate things a good bit.  We've only hit a few instances where there's inline stuff directly inside of a webkit-box in the first place, so I suspect that cases with a block-in-inline-split directly inside of a webkit-box are quite rare.)
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #8733110 - Flags: review?(mats)
(Note: the only actual behavior-change in part 2 is the new "return true" inside of the aIsWebkitBox && IsInlineOutsideStyle() check.

Everything else in that patch is just related to adding the new aIsWebkitBox parameter to the API, and determining what value it should have.)
Currently, mozilla-central has a hack where we catch "display:-webkit-box" *in the parser when we parse the keyword* and we immediately pretend we actually parsed "display: flex".  Due to this hack, none of the code from the previous patches is *actually active* yet, because the parser never actually reports that it's parsed "-webkit-box".

This patch changes that -- it relaxes this hack a bit to let us actually parse -webkit-box.

(We do have to leave the hack partially in place -- specifically, we still have to notice when we parse display:-webkit-box so that we can suppress any "display: -moz-box" that comes after it -- see code-comment in the patch for more info. But we don't need to return eCSSKeyword_flex anymore.)
Attachment #8733117 - Flags: review?(mats)
With these patches, I get EXPECTED BEHAVIOR on the sites mentioned here -- no weird line-breaking in Bing search results with a mobile UA, and no weird line-breaking at http://m.baidu.com/s?word=Firefox (we look like the expected results screenshot that Sun posted).

Moreover, we behave like Chrome on my attached "testcase 1", except for the ib split part, which I'm not caring about as noted above in comment 15 & comment 19.

(I'll write reftests before landing; just getting the code patches up for review at this point, as I'm pretty confident they're ready.)
Comment on attachment 8733107 [details] [diff] [review]
part 1: Add internal enum values to represent "display: -webkit-box" & "display: -webkit-inline-box"

r=mats with some nits:

>nsCSSFrameConstructor::ConstructDocEleme
>-  } else if (display->mDisplay == NS_STYLE_DISPLAY_FLEX) {
>+  } else if (display->mDisplay == NS_STYLE_DISPLAY_FLEX ||
>+             display->mDisplay == NS_STYLE_DISPLAY_WEBKIT_BOX) {

Does WebKit/Blink actually support webkit-box layout on :root ?
If not, then I don't think we should either.

>   switch (fieldsetContentDisplay->mDisplay) {
>     case NS_STYLE_DISPLAY_FLEX:
>+    case NS_STYLE_DISPLAY_WEBKIT_BOX:

Does WebKit/Blink actually support webkit-box layout on a <fieldset> ?
If not, then I don't think we should either.

>layout/style/nsStyleStruct.h

I think NS_STYLE_DISPLAY_WEBKIT_BOX should be listed in
IsBlockOutsideStyle().

>+WebkitPrefixEnabledPrefChangeCallback(const char* aPrefName, void* aClosure)

There are four instances of the word "grid" in this function
(in comments/strings).

>+  // Note that this function does not return true for "-webkit-box" or
>+  // "-webkit-inline-box", even though we implement those as a flavor of
>+  // flexbox.  This (intentionally) prevents us from blockifying the "display"
>+  // values of flex items inside of "-webkit-box".
>   bool IsFlexOrGridDisplayType() const {
>     return NS_STYLE_DISPLAY_FLEX == mDisplay ||
>            NS_STYLE_DISPLAY_INLINE_FLEX == mDisplay ||
>            NS_STYLE_DISPLAY_GRID == mDisplay ||
>            NS_STYLE_DISPLAY_INLINE_GRID == mDisplay;
>   }

I think this makes the method name ambiguous.  I suspect some people
(including me) might assume the -webkit-box "alias" for flex is included.
It looks like the only use of this method is the call in ApplyStyleFixups,
so we should probably make it static helper function near that method
and name it ShouldBlockifyChildren or something like that.

(Having IsFlexOrGridDisplayType in the header was a bit unfortunate in
the first place since it's very likely the wrong thing to use in layout
code.)
Attachment #8733107 - Flags: review?(mats) → review+
Attachment #8733110 - Flags: review?(mats) → review+
Comment on attachment 8733117 [details] [diff] [review]
part 3: Remove CSS Parser code that parses "display: -webkit-box" into "display: flex"

>   if (aKeywordTable == nsCSSProps::kDisplayKTable) {
>     if ((keyword == eCSSKeyword__webkit_box ||
>          keyword == eCSSKeyword__webkit_inline_box) &&
>         (sWebkitPrefixedAliasesEnabled || ShouldUseUnprefixingService())) {

If you get one of these 'display' values, then "sWebkitPrefixedAliasesEnabled"
is always true now, right?  (since we clobber the display table in part 1)
Attachment #8733117 - Flags: review?(mats) → review+
(In reply to Mats Palmgren (:mats) from comment #23)
> Comment on attachment 8733107 [details] [diff] [review]
> Does WebKit/Blink actually support webkit-box layout on :root ?
> If not, then I don't think we should either.

Good question -- yes, they do support it, based on this testcase:
data:text/html,<html style="display:-webkit-box;-webkit-box-pack: center">AmICentered</div>

So I'll leave this part of the patch as-is.

> >   switch (fieldsetContentDisplay->mDisplay) {
> >     case NS_STYLE_DISPLAY_FLEX:
> >+    case NS_STYLE_DISPLAY_WEBKIT_BOX:
> 
> Does WebKit/Blink actually support webkit-box layout on a <fieldset> ?
> If not, then I don't think we should either.

No, they don't support that (though FWIW they also don't support "display:flex" on a <fieldset> -- I included it here for consistency with our flexbox impl, but I suppose consistency with WebKit/Blink is more important here, all things being equal).

I agreed that I should remove webkit-box from this case; I'll do that.

> >layout/style/nsStyleStruct.h
> 
> I think NS_STYLE_DISPLAY_WEBKIT_BOX should be listed in
> IsBlockOutsideStyle().

Yup, thanks -- looks like I missed that one. Good catch.

> >+WebkitPrefixEnabledPrefChangeCallback(const char* aPrefName, void* aClosure)
> 
> There are four instances of the word "grid" in this function
> (in comments/strings).

Oops -- I must've used "Grid" (capitalized) in my search-and-replace, and missed these lowercase instances. Thanks!

> It looks like the only use of this method is the call in ApplyStyleFixups,
> so we should probably make it static helper function near that method
> and name it ShouldBlockifyChildren or something like that.

Good suggestion -- I agree. I'll fix that.
(In reply to Mats Palmgren (:mats) from comment #24)
> If you get one of these 'display' values, then
> "sWebkitPrefixedAliasesEnabled"
> is always true now, right?  (since we clobber the display table in part 1)

Good question, but no, I don't think that's true.  At this place in the code (in CSSParserImpl::LookupKeywordPrefixAware), we haven't actually checked the display property's keyword-table yet.  We've simply parsed some keyword (which could be *any* recognized CSS keyword), and we have yet to discover if it exists in our property's keyword-table.
Attached patch part 4: reftestsSplinter Review
Here's a reftests patch, based on testcase 1 here.

(The 1st reftest covers common cases -- continuous runs of inline content. The 2nd and 3rd are marked as "fails", because they cover edge-casey things where we don't match WebKit/Blink right now [but we probably could if it's worth the extra implementation work]. The 2nd reftest covers ib splits, discussed in several comments above, and the 3rd covers flex items that consist entirely of whitespace (which I believe can only happen with non-default values of the "white-space" property) -- modern flexbox gets rid of them, -webkit-box apparently preserves them.)

Note also that these reftests use "-webkit-box-pack: justify;" (to visualize flex item boundaries) *along with* its modern equivalent, "justify-content: space-between".  The latter is just a polyfill for us until bug 1231682 is fixed -- I've included an XXX comment with the bug number as a reminder to eventually get rid of that.

Feedback welcome, but I won't bother requesting review. I'll run this through Try and then land.
(In reply to Daniel Holbert [:dholbert] from comment #27)
> Created attachment 8734148 [details] [diff] [review]
> part 4: reftests
> the 3rd covers flex items that
> consist entirely of whitespace (which I believe can only happen with
> non-default values of the "white-space" property) -- modern flexbox gets rid
> of them, -webkit-box apparently preserves them.)

Sorry, I meant to say "...covers *anonymous* flex items..."
Blocks: 1259345
My Try run revealed 2 minor issues:
 (1) Trivial: I neglected to add these newly-legit display values to the hardcoded list in layout/inspector/tests/test_bug877690.html  [--> Done, in this patch -- I also added some strategic newlines to make the end of the list a bit more readable -- putting all the flex values on one line, and all the grid values on the next line]

 (2) Slightly less trivial: turns out we need to keep the "unprefix-all-the-way-to-modern-flexbox" codepath around, *if* we're relying on the (hacky, temporary) CSS Unprefixing Service (which we currently ship default-enabled, for a whitelist of sites -- and that whitelist depends on this for correct rendering).  See comments in the updated patch.  So, this code needs to be a bit more complicated in the short term, but it'll get simpler once we can rip out the CSS Unprefixing Service in bug 1259348.
Attachment #8734252 - Flags: review?(mats)
(I forgot to update the commit message to reflect the updates to part 3. I've fixed that in this version.)
Attachment #8733117 - Attachment is obsolete: true
Attachment #8734252 - Attachment is obsolete: true
Attachment #8734252 - Flags: review?(mats)
Attachment #8734253 - Flags: review?(mats)
Comment on attachment 8734253 [details] [diff] [review]
part 3, v2a: Skip CSS Parser code that parses "display: -webkit-box" into "display: flex", UNLESS we're relying on the CSS Unprefixing Service

r=mats
Attachment #8734253 - Flags: review?(mats) → review+
Comment on attachment 8734253 [details] [diff] [review]
part 3, v2a: Skip CSS Parser code that parses "display: -webkit-box" into "display: flex", UNLESS we're relying on the CSS Unprefixing Service

[In the interests of transparency, I'll explain one final change I'm making to this patch before landing.]

The latest Try run had one assertion failure, which makes sense & was caused by me simplifying something tangential (& which I should not have simplified).

Specifically, the following change (removing the "if" check) was responsible for the assertion failure:
>+++ b/layout/style/nsCSSParser.cpp
>-      if (mWebkitBoxUnprefixState == eHaveNotUnprefixed) {
[...]
>         mWebkitBoxUnprefixState = eHaveUnprefixed;
>-      }

I was removing the "if" check simply because I thought it was unnecessary, but it turns out it *is necessary* -- I'd forgotten that there's a 3rd state that this flag can take, and we *only* want to modify it here when it's eHaveNotUnprefixed.

(In particular, we only want to toggle this flag [and start stomping on subsequent -moz-box styles] when we're parsing a block of declarations, because that the only time we can meaninfully watch out for later "-moz-box" styling and be confident that it's still part of the same CSS rule. And "eHaveNotUnprefixed" is the flag-state that indicates we're parsing a block of declarations and have yet to unprefix.)

So, anyway, I'm restoring this "if" check before landing, and I've verified locally that it fixes the assertions from the Try run & that my attached reftests still pass.
Blocks: 1309119
You need to log in before you can comment on or make changes to this bug.