REGRESSION (r116331): RSS Headlines/links are missing (-webkit-box-flex broken?)

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Vincent]

Choose RSS->View all RSS articles -> the headlines/links don't show up (see attachment)

[Vincent]

Created attachment 140939 [details]
screen RSS example

[Alexey Proskuryakov]

*** Bug 86053 has been marked as a duplicate of this bug. ***

[Alexey Proskuryakov]

Eric, Pravin, is this something that can be fixed quickly, or should this change be just reverted?

[Eric Seidel (no email)]

Do you have any html example?

[Alexey Proskuryakov]

Created attachment 141240 [details]
An archive of such a page

This is not a reduced case, but shows the issue in browser.

[Eric Seidel (no email)]

Reducing now...

[Eric Seidel (no email)]

Created attachment 141270 [details]
mostly reduced test case

[Eric Seidel (no email)]

Created attachment 141272 [details]
more reduced test case

[Eric Seidel (no email)]

I don't know anything about deprecated flex box.  It's not clear to me what broke.  If it isn't obvious to pravind, we'll likely have to roll it out.

[Eric Seidel (no email)]

A link to the change:
http://trac.webkit.org/changeset/116331/trunk/Source/WebCore/rendering/RenderBlock.cpp

[Eric Seidel (no email)]

The change was a single character change. :)  The question is how Safari's RSS layout is supposed to work here.  Not understanding deprecated flex-boxes, it's unclear to me if the new rendering is "correct" or not.

[Tony Chang]

I think the new behavior is correct.  Here's the CSS of the flex item:
#inner {
    max-width: intrinsic;
    width: 0px;
    overflow: hidden;
    -webkit-box-flex: 60;
}

The max-width is intrinsic, which means it's 0px.  Thus, #inner can't flex bigger than 0.

The main problem is that width is trying to be used both to determine the intrinsic size and it's being used as the starting size for flexing (that's why the author set it to 0 here).  Unfortunately, I'm not sure how to get the same behavior as prior to r116331.

FWIW, new flexbox avoids this by having the starting size for flexing be a part of the flex property rather than overloading 'width' for both.

[Eric Seidel (no email)]

Tony: Do you have a suggestion as to how one might change the markup to get what the RSS author intended?

Or would we need to add a quirk to this check that if the box is using the deprecated flexbox model width: 0 needs to be special?

[Tony Chang]

Created attachment 141289 [details]
Patch

[Tony Chang]

The above fixes the old flexbox behavior.  It's kind of conceptually correct-- I bet the old spec is underdefined here.

You could construct other cases where this would differ from before r116331, but the new behavior is probably closer to the author's intent.

Maybe Hyatt or Ojan have an opinion.

[Ojan Vafai]

(In reply to comment #15)
> The above fixes the old flexbox behavior.  It's kind of conceptually correct-- I bet the old spec is underdefined here.
> 
> You could construct other cases where this would differ from before r116331, but the new behavior is probably closer to the author's intent.
> 
> Maybe Hyatt or Ojan have an opinion.

Tony's fix seems fine to me. The new behavior is more correct, but if there's content depending on old bugs, it's fine to special case deprecated flex items to do something dumb since we're not fixing bugs in old flexbox.

[Pravin D]

Sorry guys... I'm on the otherside of the Globe, so could not reply earlier. I'll  try and check for a alternative fix...

[Eric Seidel (no email)]

I don't think there is anything for you to do Pravin.  The new behavior is correct.  We just need to check in a work-around for deprecated-flexbox, like Tony suggested.  Someone just needs to take tony's patch an a variant (ideally text-only) of my test case, and check it in. :)

[Pravin D]

hmmm maybe... :) 
I was wondering about the patch that Tony had submitted. Will deprecated flex boxes behave as expected when the width > 0 ??

[Pravin D]

(In reply to comment #14)
> Created an attachment (id=141289) [details]
> Patch

> if (!isTableCell() && styleToUse->logicalWidth().isFixed() 
  && styleToUse->logicalWidth().value() >= 0 && style()->marqueeBehavior() !=      MALTERNATE && !isDeprecatedFlexItem())

Hey Tony you need to add a check for width:0 also in the deprecated flex box case. 
Behavior before r116331
When the width > 0 then the width is taken
otherwise calculated width of the content is taken.

So !isDeprecatedFlexItem() will become 
!(isDeprecatedFlexItem() && styleToUse->logicalWidth().value()==0) shuld restore the behavior in case of flex box prior to r116331.

[Ojan Vafai]

(In reply to comment #20)
> (In reply to comment #14)
> > Created an attachment (id=141289) [details] [details]
> > Patch
> 
> > if (!isTableCell() && styleToUse->logicalWidth().isFixed() 
>   && styleToUse->logicalWidth().value() >= 0 && style()->marqueeBehavior() !=      MALTERNATE && !isDeprecatedFlexItem())
> 
> Hey Tony you need to add a check for width:0 also in the deprecated flex box case. 
> Behavior before r116331
> When the width > 0 then the width is taken
> otherwise calculated width of the content is taken.
> 
> So !isDeprecatedFlexItem() will become 
> !(isDeprecatedFlexItem() && styleToUse->logicalWidth().value()==0) shuld restore the behavior in case of flex box prior to r116331.

Yup. That's more correct. Pravin, if you have the time, you could upload a patch with this fix and with an appropriate layout test.

[Pravin D]

(In reply to comment #21)
> (In reply to comment #20)
> > (In reply to comment #14)
> > > Created an attachment (id=141289) [details] [details] [details]
> > > Patch
> > 
> > > if (!isTableCell() && styleToUse->logicalWidth().isFixed() 
> >   && styleToUse->logicalWidth().value() >= 0 && style()->marqueeBehavior() !=      MALTERNATE && !isDeprecatedFlexItem())
> > 
> > Hey Tony you need to add a check for width:0 also in the deprecated flex box case. 
> > Behavior before r116331
> > When the width > 0 then the width is taken
> > otherwise calculated width of the content is taken.
> > 
> > So !isDeprecatedFlexItem() will become 
> > !(isDeprecatedFlexItem() && styleToUse->logicalWidth().value()==0) shuld restore the behavior in case of flex box prior to r116331.
> 
> Yup. That's more correct. Pravin, if you have the time, you could upload a patch with this fix and with an appropriate layout test.

Sure...

[Pravin D]

Created attachment 141361 [details]
Patch

[Pravin D]

(In reply to comment #23)
> Created an attachment (id=141361) [details]
> Patch

Hi Eric , Ojan ,

The uploaded patch is for making the behavior of deprecated flex boxes prior to r116331.

There seems to be another issue to this. When width is fixed, we seem to totally ignore the max-width or min-width and take 
m_minPreferredLogicalWidth = m_maxPreferredLogicalWidth = width. 

The max-width:intrinsic , min-width:intrinsic is not handled at all...These cases have to handled atleast in flex box cases. Not sure of the other use cases...
Please let me know your opinions

[Tony Chang]

Actually, I don't think you should include the check for 0 width.

Here's an example of where it would matter:

#inner {
    max-width: intrinsic;
    width: 50px;
    overflow: hidden;
    -webkit-box-flex: 60;
}

In the old code, max-width would be 50px.  But that's probably not the author's intent because by setting width, they probably were trying to set the initial flexing size while allowing #inner to flex as large as the intrinsic size.

The new behavior would be have max-width: intrinsic always resolve to the intrinsic size when we're dealing with a flex item.

It's possible that there's some content on the web that this may change the results of, but I doubt it.  The combination of flexbox + intrinsic min/max sizing + width != 0 sounds very rare (0 is a common width for flexbox items because it lets the boxes flex in proportion to box-flex).

[Pravin D]

(In reply to comment #25)
> Actually, I don't think you should include the check for 0 width.
> 
> Here's an example of where it would matter:
> 
> #inner {
>     max-width: intrinsic;
>     width: 50px;
>     overflow: hidden;
>     -webkit-box-flex: 60;
> }
> 
> In the old code, max-width would be 50px.  But that's probably not the author's intent because by setting width, they probably were trying to set the initial flexing size while allowing #inner to flex as large as the intrinsic size.
> 

I also had the same doubt... See comment #24... However that's not the case here. 

The behavior before r116331:
When max-width:instrinsic; width: Xpx then width has to be taken as X pixels when X > 0 , intrinsic size when X <= 0 . 
width:0 is a special case or a bug and in my opinion it was exploited/used by webpage author to make flex box take the size of its content.


> The new behavior would be have max-width: intrinsic always resolve to the intrinsic size when we're dealing with a flex item.
> 

This would mean changing the behavior of deprecated flex boxes, which may not a good idea. 

The behavior on other browsers FF, Opera, and IE-8 is same as the behavior on current webkit i.e all ignore max-width:intrinsic when width:Xpx is specified, irrespective of whether width:0 or not. 

Also non of the attached test cases work on other browsers. 


As Ojan said in comment #16 we just need special case webkit to make the behavior of deprecated flex box prior to r116331 so that we don't end up breaking old webpages.

[Tony Chang]

(In reply to comment #26)
> The behavior on other browsers FF, Opera, and IE-8 is same as the behavior on current webkit i.e all ignore max-width:intrinsic when width:Xpx is specified, irrespective of whether width:0 or not. 
> 
> Also non of the attached test cases work on other browsers. 

'intrinsic' and 'max-intrinsic' are webkit specific keywords.  Firefox supports -moz-max-content.  IE10 Preview and Opera 11 don't seem to support max-content/min-content.

> > The new behavior would be have max-width: intrinsic always resolve to the intrinsic size when we're dealing with a flex item.
> > 
> 
> This would mean changing the behavior of deprecated flex boxes, which may not a good idea. 
> 
> As Ojan said in comment #16 we just need special case webkit to make the behavior of deprecated flex box prior to r116331 so that we don't end up breaking old webpages.

Yes, this would change the behavior, but I think this is closer to the author's intent (i.e., we're fixing a bug) and there are probably no websites that actually hit this case so it should be safe.  The benefit of making the change is that the code is simpler.

[Tony Chang]

(In reply to comment #27)
> (In reply to comment #26)
> > The behavior on other browsers FF, Opera, and IE-8 is same as the behavior on current webkit i.e all ignore max-width:intrinsic when width:Xpx is specified, irrespective of whether width:0 or not. 
> > 
> > Also non of the attached test cases work on other browsers. 
> 
> 'intrinsic' and 'max-intrinsic' are webkit specific keywords.  Firefox supports -moz-max-content.  IE10 Preview and Opera 11 don't seem to support max-content/min-content.

I meant to add that setting max-width to -moz-max-content doesn't seem to apply in display: -moz-box contexts.  Which is to say, this is a WebKit specific bug.

[Pravin D]

(In reply to comment #27)

> Yes, this would change the behavior, but I think this is closer to the author's >intent (i.e., we're fixing a bug) and there are probably no websites that >actually hit this case so it should be safe.  The benefit of making the change >is that the code is simpler.
>

So this change for max-width:intrinsic will only apply for deprecated flex boxes or for all flex boxes ?

[Tony Chang]

(In reply to comment #29)
> (In reply to comment #27)
> 
> > Yes, this would change the behavior, but I think this is closer to the author's >intent (i.e., we're fixing a bug) and there are probably no websites that >actually hit this case so it should be safe.  The benefit of making the change >is that the code is simpler.
> >
> 
> So this change for max-width:intrinsic will only apply for deprecated flex boxes or for all flex boxes ?

Only deprecated.  New flexbox avoids this by using a separate CSS property.

[Ojan Vafai]

I don't feel strongly about this either way. I think it's unlikely to break content either way, so I'm happy to defer to Tony's "simpler code" argument.

[Eric Seidel (no email)]

Comment on attachment 141361 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=141361&action=review

> Source/WebCore/rendering/RenderBlock.cpp:5171
> +       && style()->marqueeBehavior() != MALTERNATE && !(isDeprecatedFlexItem() && styleToUse->logicalWidth().value() == static_cast<int>(0)))

YOu can use intValue() of the logicalWidth to do the case for you.  But otehrwies this loolks great.

[Pravin D]

(In reply to comment #32)
> (From update of attachment 141361 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141361&action=review
> 
> > Source/WebCore/rendering/RenderBlock.cpp:5171
> > +       && style()->marqueeBehavior() != MALTERNATE && !(isDeprecatedFlexItem() && styleToUse->logicalWidth().value() == static_cast<int>(0)))
> 
> YOu can use intValue() of the logicalWidth to do the case for you.  But otehrwies this loolks great.
>

I'll upload another patch with the IntValue changes... Thanks for the review comments

[Pravin D]

Created attachment 142433 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 142433 [details]
Patch

Thanks.

[Pravin D]

(In reply to comment #35)
> (From update of attachment 142433 [details])
> Thanks.

Thanks a lot for the review

[WebKit Review Bot]

Comment on attachment 142433 [details]
Patch

Clearing flags on attachment: 142433

Committed r117413: <http://trac.webkit.org/changeset/117413>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.