1209994 - fieldset height is calculated incorrect when set to "auto" if parent form has fixed height

Status: RESOLVED FIXED

(Core :: Layout, defect, P4)

Description

[gfx-freeek]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:41.0) Gecko/20100101 Firefox/41.0
Build ID: 20150917150946

Steps to reproduce:

https://jsfiddle.net/moo24/Lvtr64cy/

Note: Issue only occurs when using fieldset element and if parent form has set a fixed height. Otherwise everything is working fine.


Actual results:

Height of fieldset element is not re-caluclated after it is set to "auto" by Javascript. Therefore input field in fieldset is not visible.

Note: Everything is working fine in FireFox 40 and other browsers.


Expected results:

Height of fieldset element should be calculated correctly after it is set to "auto" by Javascript.

http://jsfiddle.net/moo24/Lvtr64cy/2/

Comment 1

[Boris Zbarsky [:bzbarsky]]

Regression range on nightlies: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b0b3dcfa5557&tochange=d3228c82badd

Note that this is during development of Firefox 42, which means that whatever caused this got backported to Firefox 41.

Looking at that checking range, the change in bug 1172239 looks pretty suspicious: it deals with vertical sizes (at least for purposes of this testcase), and was backported to Firefox 41.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Minimalish testcase

Comment 3

[Boris Zbarsky [:bzbarsky]]

So we land in nsHTMLReflowState::InitResizeFlags where we take this branch in the if/else cascade for the BResize flag:

  } else if (mCBReflowState && !nsLayoutUtils::IsNonWrapperBlock(frame)) {
...
    SetBResize(mCBReflowState->IsBResize());

mCBReflowState is the reflow state for the fixed-height <form> so it sort of make sense that after bug 1172239 it doesn't have IsBResize() true, though I would have expected it to not be true even before bug 1172239.

In any case, if we tested false for !nsLayoutUtils::IsNonWrapperBlock(frame), we'd end up in the case that does:

  } else if (ComputedBSize() == NS_AUTOHEIGHT) {
...
    SetBResize(IsBResize() || NS_SUBTREE_DIRTY(frame));

which would set BResize to true, because frame has NS_FRAME_HAS_DIRTY_CHILDREN set.

This part kinda looks like a regression from bug 10209 at heart: before that the test that now does IsNonWrapperBlock was !frame->IsContainingBlock(), and fieldset had IsContainingBlock() == true.

I think the right fix involves changing this condition to not make fieldsets copy their parent BResize flag.  I bet we have the same issue with buttons, <select>, possibly tables...

What I'm not sure is what the right condition is.  We clearly want to exclude anonymous blocks here.  Do we want to exclude non-replaced inlines?  Anything else that should be excluded?

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Boris Zbarsky [:bz] from comment #3)
> This part kinda looks like a regression from bug 10209 at heart: before that
> the test that now does IsNonWrapperBlock was !frame->IsContainingBlock(),
> and fieldset had IsContainingBlock() == true.
> 
> I think the right fix involves changing this condition to not make fieldsets
> copy their parent BResize flag.  I bet we have the same issue with buttons,
> <select>, possibly tables...
> 
> What I'm not sure is what the right condition is.  We clearly want to
> exclude anonymous blocks here.  Do we want to exclude non-replaced inlines? 
> Anything else that should be excluded?

I don't think it matters for inlines because they don't check the resize flag, nor should they.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Boris Zbarsky [:bz] from comment #3)
> I think the right fix involves changing this condition to not make fieldsets
> copy their parent BResize flag.  I bet we have the same issue with buttons,
> <select>, possibly tables...

I can only see this for fieldset, probably because of the curious situation where the scrollframe is a child of the fieldset frame instead of wrapping it like normal.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5292ba65099d

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=14577e551b73

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: MozReview Request: Bug 1209994. Fix a couple of issues that make the percentage-height-calculation.html test unreliable. r=bz

Bug 1209994. Take block-wrapper path only for blocks that are wrappers. r=bz

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8677231 [details]
MozReview Request: Bug 1209994. Fix a couple of issues that make the percentage-height-calculation.html test unreliable. r=bz

https://reviewboard.mozilla.org/r/22899/#review20609

r=me, but I have low confidence that I didn't miss some edge case.  :(

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a8adf1f9d366b936e1f25dbd3deb7a83b8cfa0cd
Bug 1209994. Take block-wrapper path only for blocks that are wrappers. r=bz

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/a59b9742c81e for

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=a8adf1f9d366
05:16:35     INFO - TEST-UNEXPECTED-FAIL | /quirks-mode/percentage-height-calculation.html | The percentage height calculation quirk, <img id=test src="{png}" height=100%> - assert_equals: quirks mode expected "184px" but got "169px"
05:16:35     INFO - onload/</<@http://web-platform.test:8000/quirks-mode/percentage-height-calculation.html:92:1
05:16:35     INFO - Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1380:20
05:16:35     INFO - test@http://web-platform.test:8000/resources/testharness.js:496:9
05:16:35     INFO - onload/<@http://web-platform.test:8000/quirks-mode/percentage-height-calculation.html:82:1
05:16:35     INFO - onload@http://web-platform.test:8000/quirks-mode/percentage-height-calculation.html:81:9
05:16:35     INFO - EventHandlerNonNull*@http://web-platform.test:8000/quirks-mode/percentage-height-calculation.html:16:5
05:16:35     INFO - TEST-UNEXPECTED-PASS | /quirks-mode/percentage-height-calculation.html | The percentage height calculation quirk, <img id=test src="{png}" height=100% border=10> - expected FAIL

The accidentally backed out bug 1215699 relanded as https://hg.mozilla.org/integration/mozilla-inbound/rev/b6bf653873b5

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Mac only, sigh.

Comment 14

[Boris Zbarsky [:bzbarsky]]

That test is just broken, imo.  The reason it's broken is that it uses iframes with "width:20px; height: 200px;" to do its testing.  Then it puts in a 1x1 image and styles it with 100% height, and maybe a 10px border.  This causes overflow to the right and a horizontal scrollbar, which can then change the height of the viewport.  And on Mac this likely depends on what's going on with the magic auto-hiding scrollbars or something.

I think the test should be changed to use a width of at least (220 + default body padding + vertical scrollbar width) for its iframes, to avoid issues with horizontal overflow and scrollbars appearing...

Comment 15

[Boris Zbarsky [:bzbarsky]]

Actually, there is another source of bustage in this test.  The test is assuming the image loads will be sync, and nothing requires that as far as I can tell.  That's why in almost-standards mode it measures 24px instead of 1px: it's measuring the size of the "loading image" placeholder, which has a quite nonzero auto height.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8677231 [details]
MozReview Request: Bug 1209994. Fix a couple of issues that make the percentage-height-calculation.html test unreliable. r=bz

Bug 1209994. Fix a couple of issues that make the percentage-height-calculation.html test unreliable. r=bz

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: MozReview Request: Bug 1209994. Take block-wrapper path only for blocks that are wrappers. r=bz

Bug 1209994. Take block-wrapper path only for blocks that are wrappers. r=bz

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Boris Zbarsky [:bz] from comment #15)
> Actually, there is another source of bustage in this test.  The test is
> assuming the image loads will be sync, and nothing requires that as far as I
> can tell.  That's why in almost-standards mode it measures 24px instead of
> 1px: it's measuring the size of the "loading image" placeholder, which has a
> quite nonzero auto height.

I figured all that out after you wrote your comments but before I read them :-)

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8677231 [details]
MozReview Request: Bug 1209994. Fix a couple of issues that make the percentage-height-calculation.html test unreliable. r=bz

r=me though technically the HTML spec only requires the preload thing to work on a per-document basis, so this test is still making assumptions that aren't supported by the spec.  Fixing that will require more surgery (e.g. making it into an async test altogether), so probably OK as a separate bug report on the wpt tests on github.

You're going to need to update the .ini file for this test, I assume, to remove a bunch of failure annotations.  Do a try run?

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e7c2ff87b559a305658877b9fb903d304af105f1
Bug 1209994. Fix a couple of issues that make the percentage-height-calculation.html test unreliable. r=bz

https://hg.mozilla.org/integration/mozilla-inbound/rev/beb4cf7d167970d5c774bfe903211e8d58633829
Bug 1209994. Take block-wrapper path only for blocks that are wrappers. r=bz

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e7c2ff87b559
https://hg.mozilla.org/mozilla-central/rev/beb4cf7d1679

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/e7c2ff87b559
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/beb4cf7d1679

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8679354 [details]
MozReview Request: Bug 1209994. Take block-wrapper path only for blocks that are wrappers. r=bz

Approval Request Comment
[Feature/regressing bug #]: 1172239
[User impact if declined]: Incorrect layout in some cases
[Describe test coverage new/current, TreeHerder]: lots of layout test coverage
[Risks and why]: reasonably low risk given our test coverage and small size of the patch, but nonzero
[String/UUID change made/needed]: none

(Need approval for both patches in this bug)

Comment 24

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8679354 [details]
MozReview Request: Bug 1209994. Take block-wrapper path only for blocks that are wrappers. r=bz

Early in the cycle, plenty of tests, let's take it.
Should be in 43 beta 2.

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/973bf2f68f2f

Comment 26

[Carsten Book [:Tomcat]]

this failed to apply to beta : grafting 313475:973bf2f68f2f "Bug 1209994 - Take block-wrapper path only for blocks that are wrappers. r=bz, a=sylvestre"
merging layout/generic/nsHTMLReflowState.cpp
merging layout/reftests/bugs/reftest.list
warning: conflicts during merge.
merging layout/reftests/bugs/reftest.list incomplete! (edit conflicts, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue)


could you take a look, thanks !

Comment 27

[Carsten Book [:Tomcat]]

(In reply to Carsten Book [:Tomcat] from comment #25)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/973bf2f68f2f

that got backed out because it seems it needed both parts to land (found this out after checkin), so since this was a test only changed landed both parts again as

https://hg.mozilla.org/releases/mozilla-aurora/rev/cb49672a8a4d
https://hg.mozilla.org/releases/mozilla-aurora/rev/cafe4b776cac

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/cb49672a8a4d
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/cafe4b776cac

Comment 29

[Carsten Book [:Tomcat]]

Hi,

seems this cause problems during uplift to beta:
grafting 313545:cb49672a8a4d "Bug 1209994 -  Fix a couple of issues that make the percentage-height-calculation.html test unreliable. r=bz, a=sylvestre"
merging testing/web-platform/meta/quirks-mode/percentage-height-calculation.html.ini
warning: conflicts during merge.
merging testing/web-platform/meta/quirks-mode/percentage-height-calculation.html.ini incomplete! (edit conflicts, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue)

could you take a look, thanks!

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/releases/mozilla-beta/rev/26ef30e81468
https://hg.mozilla.org/releases/mozilla-beta/rev/278b5140a696

Comment 31

[Carsten Book [:Tomcat]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #30)
> https://hg.mozilla.org/releases/mozilla-beta/rev/26ef30e81468
> https://hg.mozilla.org/releases/mozilla-beta/rev/278b5140a696

setting flags for roc