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

RESOLVED FIXED in Firefox 43

Status

()

defect
P4
minor
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: gfx-freeek, Assigned: robert)

Tracking

(Depends on 1 bug)

41 Branch
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 wontfix, firefox42 wontfix, firefox43 fixed, firefox44 fixed, firefox45 fixed, b2g-v2.5 fixed)

Details

Attachments

(3 attachments)

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/
Severity: normal → minor
Priority: -- → P4
Component: Untriaged → Layout
Product: Firefox → Core
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.
Blocks: 1172239
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(roc)
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?
(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.
Flags: needinfo?(roc)
(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 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.  :(
Attachment #8677231 - Flags: review?(bzbarsky) → review+
No longer blocks: 1217831
Duplicate of this bug: 1217831
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
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...
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 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
Attachment #8677231 - Attachment description: MozReview Request: Bug 1209994. Take block-wrapper path only for blocks that are wrappers. r=bz → MozReview Request: Bug 1209994. Fix a couple of issues that make the percentage-height-calculation.html test unreliable. r=bz
(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 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?
Attachment #8677231 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/e7c2ff87b559
https://hg.mozilla.org/mozilla-central/rev/beb4cf7d1679
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
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)
Attachment #8679354 - Flags: approval-mozilla-beta?
Attachment #8679354 - Flags: approval-mozilla-aurora?
Assignee: nobody → robert
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.
Attachment #8679354 - Flags: approval-mozilla-beta?
Attachment #8679354 - Flags: approval-mozilla-beta+
Attachment #8679354 - Flags: approval-mozilla-aurora?
Attachment #8679354 - Flags: approval-mozilla-aurora+
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 !
Flags: needinfo?(robert)
(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
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!
Depends on: 1240361
Depends on: 1243623
Depends on: 1263845
You need to log in before you can comment on or make changes to this bug.