913759 - Percentage heights on children of a <button> don't work (because of the anonymous auto-sized -moz-button-content block between the <button> and its children)

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Boris Zbarsky [:bzbarsky]]

See testcase in URL field...

Daniel, do you have time to look into this?

Comment 1

[Daniel Holbert [:dholbert]]

So we've got a frame tree that looks like this:
 button (fixed-height)
   anonymous "-moz-button-content" block frame
     block (percentage-height)

In an identical testcase with a div instead of a button, an unstyled div to mimic the anonymous -moz-button-content frame, we end up figuring out the basis for our percent value to resolve against in CalcQuirkContainingBlockHeight(). That returns 30000 (500px in app units).

BUT: when we hit CalcQuirkContainingBlockHeight call in the testcase (with a button), and when "rs" reaches the button in the loop there, we break out of the loop without even inspecting the button-height, here:
 http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsHTMLReflowState.cpp?rev=63a55ac51f7f#1660
and return the variable 'result', which was initialized to NS_AUTOHEIGHT and which we haven't changed away from that value.

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #1)
> In an identical testcase with a div instead of a button, an unstyled div to
> mimic the anonymous -moz-button-content frame, we end up figuring out the

er I meant to say, "*and* an unstyled div to mimic the anonymous -moz-button-content frame"

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: strawman patch

This isn't a complete fix (for one thing, this would let us continue walking up the ancestor chain, past a button frame, if it happened to have NS_AUTOHEIGHT, to find a higher-up containing block, which we don't currently allow and we probably don't want to allow).

But this fixes the testcase, demonstrating why this works for blocks-in-blocks but not for blocks-in-buttons.

Comment 4

[Daniel Holbert [:dholbert]]

(I wonder what IE does here? If they match our current behavior, it might be better not to bother fixing this, since this is quirks-mode code anyway.)

A few other data points, from some standards-mode code:

Gecko & Webkit agree that the yellow region is tall here:
> data:text/html,<!DOCTYPE%20html><div%20style="height:%20500px;"><div%20style="height:%20100%;%20background:%20yellow">Text
...but that it's short if we add an unstyled <div> in between the fixed-height and percent-height divs: 
> data:text/html,<!DOCTYPE%20html><div%20style="height:%20500px;"><div><div%20style="height:%20100%;%20background:%20yellow">Text

Our <button> code is like the latter example there -- with an anonymous div in between the <button> and the author's provided button-contents -- so in any standards-mode content, we'll be doomed to not resolve percentages.  We could fix it in quirks-mode, via CalcQuirkContainingBlockHeight, but I'm not sure it's worth it.

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #4)
> (I wonder what IE does here?

Answering my own question: IE10 (on win8) matches WebKit on the original testcase -- the button's contents are tall & yellow. (both in quirks-mode and standards-mode.) (I also couldn't get the testcase to load in IE as a data URL, so I made a standalone HTML file.)

So I guess we're the odd ones out on this. I'm not sure what the best way is to make it work; the attached strawman patch isn't sufficient, since it'll only help in quirks mode.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Should we be setting the computed height of the button's anonymous block to something based on the button?  But that would break the vertical centering...

Ideally, the anonymous block would not be the containing block for the kids; the button would.

Comment 7

[Boris Zbarsky [:bzbarsky]]

As in, maybe nsIFrame::IsBlockWrapper should return true for this block, so that GetNearestBlockContainer will skip over it?

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: strawman patch #2

Ah, yes -- that makes sense, and that seems to work.

One problem, though: the yellow region ends up running off the bottom of the button, because it's positioned at an offset within the button, and yet it has the same height as the button, so it necessarily sticks out.

It's offset due to the button's theme-specific border / padding.  That border/padding is set aside here...
> 188   nsMargin focusPadding = mRenderer.GetAddedButtonBorderAndPadding();
http://mxr.mozilla.org/mozilla-central/source/layout/forms/nsHTMLButtonControlFrame.cpp#188
...and it's subtracted out in ReflowButtonContents(), but it's not subtracted from the button's reflow state's mComputedHeight variable, which is what our 100% ends up resolving against.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Hrm.  That's annoying.  Maybe we should adjust the reflow state...

Comment 10

[Daniel Holbert [:dholbert]]

Bug 875275 will likely depend on the ability to have a "height:100%" thing inside of a button (technically a subclass of nsHTMLButtonControlFrame), so it depends on this being fixed.

I'll try Comment 9 (probably just creating a mutableReflowState like we do elsewhere) and try to get this fixed in the next day or so, I think.

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: fix v1

This has the IsBlockWrapper tweak from comment 7, plus a chunk to give the child's reflow state an adjusted parent-state (with a smaller computedheight/computedwidth), per comment 8 - 9.

(We could also directly & edit the (nominally "const") passed-in reflow state, but that seems hackier & more prone to possible hard-to-debug/footgun issues down the line, which is why I went the "clone-and-tweak" route.)

This renders this bug's testcase (the data URL in URL field) nicely. Reftests coming in a second patch.

Comment 12

[Daniel Holbert [:dholbert]]

Attachment: fix v1a

(fixed accidental double-blank-line in previous patch)

(Also: for the record, this patch layers on top of bug 921174's reflow-cleanup patches.)

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 810959 [details] [diff] [review]
fix v1a

>+// Helper-funtion that lets us clone the button's reflow state, but with its

s/funtion/function/

>+    adjustedHeight -= aFocusPadding.TopBottom();

I think you should do the std::max() here.  Applying it to NS_INTRINSICSIZE is pretty fishy.

r=me with that.

Comment 14

[Daniel Holbert [:dholbert]]

Attachment: fix v2 (addressed review comments)

Thanks! Fixed. (I bumped both std::max() invocations up to be adjacent to the expression they're clamping, for consistency.)

Comment 15

[Daniel Holbert [:dholbert]]

Attachment: reftests patch

Here's a patch with 4 reftests for this.

Each reftest checks both an 80% length and a 100% length on content inside of a fixed-size button and (separately) an intrinsic-size button.

[where "length" = height | width]

The -1* tests have no focus-border/padding on the button, whereas the -2* tests have focus-border/padding.

Comment 16

[Daniel Holbert [:dholbert]]

Comment on attachment 811376 [details] [diff] [review]
reftests patch

[sorry, reposting patch; that one had "hg cp" info from another testcase, vertical-centering, but I meant to undo the "hg cp" since I ended up stomping on a lot of the stuff in the file that I'd initially copied from, making the 'hg cp' more confusing than useful.]

Comment 17

[Daniel Holbert [:dholbert]]

Attachment: reftests patch

Comment 18

[Daniel Holbert [:dholbert]]

Try run w/ both patches: https://tbpl.mozilla.org/?tree=Try&rev=eb4d253797cb

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 811378 [details] [diff] [review]
reftests patch

You could style the buttons with box-sizing to use the same nice round width/height as the divs use in the references, right?

r=me

Comment 20

[Daniel Holbert [:dholbert]]

I could use box-sizing, yeah.

I opted not to, just to preserve as much of the button's integrity as I can. :) (I'm already ripping out its background, border, and appearance... I figured I'd let it keep its box-sizing.)

Comment 21

[Daniel Holbert [:dholbert]]

Looks like the tests are fuzzy on mac, due to the extreme corner-pixel of each button's (black, square) border being rgb(1,1,1) instead of rgb(0,0,0) as they are in the equivalent div.

B2G has the same problem, but worse -- instead of just the corner-pixel being off, there's a 3x3 grid of pixels at each corner that doesn't match. (though in most parts of the test, that grid is covered up by the pink or yellow buttonContent frame's background).

So it looks like those platforms are trying to put some "fancy" styling on the button's border (and background, in the case of B2G), despite my efforts to suppress the fanciness with CSS.  (I wonder if that's a bug?)

Anyway, I'll rip out the border, since it's not important; that should fix the mac fuzziness. And then I'll just use a fuzzy-if() annotation to allow for fancy-corners on B2G.

Comment 22

[Daniel Holbert [:dholbert]]

Landed with button-borders removed in the testcase, and with fuzzy-if(B2G) annotations for the fancy corner-shading:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/a831cc6240b2
  https://hg.mozilla.org/integration/mozilla-inbound/rev/d28e8f43b09e

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a831cc6240b2
https://hg.mozilla.org/mozilla-central/rev/d28e8f43b09e