1233135 - "Assertion failure: fieldsetContentDisplay->mDisplay == 1 (bug in nsRuleNode::ComputeDisplayData?)"

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: testcase

Assertion failure: fieldsetContentDisplay->mDisplay == 1 (bug in nsRuleNode::ComputeDisplayData?), at layout/base/nsCSSFrameConstructor.cpp:3180

This assertion is part of code added in https://hg.mozilla.org/mozilla-central/rev/76a5cbbc0600

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Mats Palmgren (inactive)]

The problem is caused by this code block:
http://hg.mozilla.org/mozilla-central/annotate/0babaa3edcf9/layout/style/nsStyleContext.cpp#l708

ShouldSuppressLineBreak returns true because we have 'display:ruby':
http://hg.mozilla.org/mozilla-central/annotate/0babaa3edcf9/layout/style/nsStyleContext.cpp#l514
and then it modifies 'display' from 'block' (from forms.css) to 'inline-block'
on the inner ::-moz-fieldset-content frame.

It seems dangerous to modify 'display' like this on inner anonymous frames.
I think we need to avoid that somehow.

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Attachment: MozReview Request: Bug 1233135 - Do not touch display value of anonymous box for ruby.

Review commit: https://reviewboard.mozilla.org/r/28235/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28235/

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8699307 [details]
MozReview Request: Bug 1233135 - Do not touch display value of anonymous box for ruby.

https://reviewboard.mozilla.org/r/28235/#review28863

::: layout/style/nsStyleContext.cpp:724
(Diff revision 1)
> +    if (GetPseudoType() != nsCSSPseudoElements::ePseudo_AnonBox) {

Instead of putting this here, I think it would be a lot clearer if you passed aStyleContext into ShouldSuppressLineBreak, and put this condition next to the IsOutOfFlowStyle check there.

Could you also add some tests with various form controls (fieldset, <select>, <select size=2>) and an element with overflow:scroll (and display:inline-block), with, and inside of, display:ruby?  It would also be good to test the overflow:scroll case with stuff inside the overflow:scroll element that would cause problems if linebreak suppression failed in a more normal case.


I'm not crazy about the assumptions this makes about what's in the frame tree, but I guess this seems OK, with that change.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron [:dbaron] ⌚️UTC+13 from comment #4)
> Comment on attachment 8699307 [details]
> MozReview Request: Bug 1233135 - Do not touch display value of anonymous box
> for ruby.
> 
> https://reviewboard.mozilla.org/r/28235/#review28863
> 
> ::: layout/style/nsStyleContext.cpp:724
> (Diff revision 1)
> > +    if (GetPseudoType() != nsCSSPseudoElements::ePseudo_AnonBox) {
> 
> Instead of putting this here, I think it would be a lot clearer if you
> passed aStyleContext into ShouldSuppressLineBreak, and put this condition
> next to the IsOutOfFlowStyle check there.

As comment above this line said, we still need to set the SUPPRESS_LINEBREAK flag on some anonbox e.g. text frame, so I think this condition probably should be here.

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8699307 [details]
MozReview Request: Bug 1233135 - Do not touch display value of anonymous box for ruby.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28235/diff/1-2/

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

OK -- I suspect you should just exclude text frames, then?

My guess is that currently a select inside of ruby is pretty broken because of the linebreak suppression on options, for example.  Although I suppose it's possible that what I was suggesting wouldn't even fix that.

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Hmmm... and ruby anonymous boxes need to be excluded as well.

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron [:dbaron] ⌚️UTC+13 from comment #7)
> My guess is that currently a select inside of ruby is pretty broken because
> of the linebreak suppression on options, for example.  Although I suppose
> it's possible that what I was suggesting wouldn't even fix that.

It seems so if the select has an inline display value. So anything I should do in this bug for this issue?

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8699307 [details]
MozReview Request: Bug 1233135 - Do not touch display value of anonymous box for ruby.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28235/diff/2-3/

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #9)
> It seems so if the select has an inline display value. So anything I should
> do in this bug for this issue?

Does the revised patch fix the problem?  I would expect that it does.

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Hmmm, partially. It seems to fix the combobox case, but not the listbox case (with size>1 or multiple set).

From code in CSS frame constructor, it seems listbox doesn't have any anonymous box in-between.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8699307 [details]
MozReview Request: Bug 1233135 - Do not touch display value of anonymous box for ruby.

https://reviewboard.mozilla.org/r/28235/#review28895

I think it's worth filing a followup bug on the remaining issue with listboxes, but I don't think it's worth spending effort on now.

::: layout/style/crashtests/crashtests.list:136
(Diff revision 3)
> +load 1233135-1.html
> +load 1233135-2.html

Seems like you might want to construct a reftest here rather than just a crashtest.

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron [:dbaron] ⌚️UTC+13 from comment #13)
> ::: layout/style/crashtests/crashtests.list:136
> (Diff revision 3)
> > +load 1233135-1.html
> > +load 1233135-2.html
> 
> Seems like you might want to construct a reftest here rather than just a
> crashtest.

It doesn't seem to me we are testing any rendering effect here. I'm just testing that form controls won't crash even if inside ruby or have ruby display value.

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

Filed bug 1242344.

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f993400fcbd963512579cad8eb8a8a4b38c1cf3
Bug 1233135 - Do not touch display value of anonymous box for ruby. r=dbaron

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9f993400fcbd

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

Too late for assertion fixes in 46.