Closed Bug 527459 Opened 12 years ago Closed 9 years ago
Padding specified in percent ignored on text inputs
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:18.104.22.168pre) Gecko/20091108 Shiretoko/3.5.6pre Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:22.214.171.124pre) Gecko/20091108 Shiretoko/3.5.6pre When I set padding on text input in pixels, input behaves as I expect (sets the padding in all directions correctly). When I use percents, padding isn't applied at all. Is it a feature or a bug? Seems as a bug to me. Other browsers gets this right. See: http://jsbin.com/odabo/ Reproducible: Always Happens in FF 126.96.36.199 on Windows and FF 3.5.6pre on Linux. I suppose it happens everywhere in between as well.
Ooops, sorry, padding IS applied but the text is displayed stuck to the top left corner.
So, the padding is applied on all platforms (and all versions of Gecko/Firefox I can test). percentage based vertical padding is tricky, though: it is computed based on the width of containing block. http://www.w3.org/TR/CSS21/box.html#propdef-padding (resize the window horizontally to see how the text field changes) ref comment1: That doesn't happen on Firefox 3.0+ on OS X, but I can confirm it on Linux (3.0.15, 3.5). The issue is fixed however on trunk builds: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a1pre) Gecko/20091109 Minefield/3.7a1pre
Is anyone looking into this? :)
In the file you can see a CSS class and three tags: div, textarea, input. It shows different behave of textarea and input tags when percentage value of padding is set.
Component: General → Layout: Form Controls
Product: Firefox → Core
QA Contact: general → layout.form-controls
This happens because text controls use an nsStackFrame for their layout and this handles padding via nsIFrame::GetBorderAndPadding (called from GetClientRect, called from nsStackLayout::Layout). And this doesn't use the reflow state; it just gets padding directly off the style struct. So it ignored percentage paddings. This happens for both vertical and horizontal paddings, really, except that the vertical one ends up looking like it works due to vertical alignment. The layout isn't quite correct, though.
I'd really like to make nsTextControlFrame::GetBorderAndPadding return GetUsedBorderAndPadding, except during the intrinsic size computation for initial reflow we force GetUsedPadding to 0 (because NS_FRAME_IN_REFLOW is not set yet at that point, and NS_FRAME_FIRST_REFLOW _is_ set). I suppose I could add a member in which I stash the current reflow state (if any) and use that to return the right thing from GetBorderAndPadding. But that seems odd, since GetUsedPadding should in fact do what I want it to do. I suppose I could manually twiddle the NS_FRAME_IN_REFLOW bit in nsTextControlFrame::GetBorderAndPadding, but that's a bit hacky too. :( Maybe we should set NS_FRAME_IN_REFLOW earlier? Or use some other way of detecting that GetUsedPadding should return 0?
I think as long as the frame has zero size, GetUsedPadding should return zero size.
Hmm. OK, but the situation here is that we're in the middle of reflow for the frame. Anyway, I guess I'll just add a backdoor for getting at the reflow state from the box layout gunk.... It'll mean that GetBorderAndPadding on the frame is different under Reflow() and not, but that might be ok.
Or we could stop inheriting from nsStackFrame. Why _do_ we do that anyway?
That is a very good question, and I don't know the answer. Karl, you hacked on textcontrol layout, did you notice any particular need for nsStackFrame/XUL box layout?
Actually, I just recalled why it's a box. It's so it'll work in XUL (e.g. inside <xul:textbox>). But I think we should change nsTextControlFrame::Reflow to just do its own reflow instead of going through the box layout rigmarole.
Requesting a status update on this. Thanks!
FF8, still not fixed.
The status, since Mat asked, is that for me this is lower-priority than the other things I'm working on. Specifically, somewhere in the 40+ range in priority ranking (as in, about 40 bugs I plan to fix before worrying about this one, assuming nothing else important comes up, which is a bad assuption). So this bug could really use a different owner; in particular if someone wants to learn about reflow internals this is a good bug to do it with in some ways.
Guys, Given the emphasis on responsive web design that we're getting these days, all the padding, margins and borders on all elements in a design are going to need to be expressed as percentages. This surely bumps up the importance of fixing a bug like this. I've got a pig ugly form in firefox now as I can't put in truly proportional values for the padding. I might just have to go back to using background images on the input's parent div. I'd hate to see Firefox go the way of IE6, needing hacks to work around this issue....
(In reply to ralph from comment #18) > Guys, > Given the emphasis on responsive web design that we're getting these days, > all the padding, margins and borders on all elements in a design are going > to need to be expressed as percentages. This surely bumps up the importance > of fixing a bug like this. I've got a pig ugly form in firefox now as I > can't put in truly proportional values for the padding. I might just have to > go back to using background images on the input's parent div. I'd hate to > see Firefox go the way of IE6, needing hacks to work around this issue.... You know firefox has many bugs and needs people to contribute if it is so important to you. Firefox is an open source software and you can fix it! Anyway this bug has been fixed by Bug 716875 though no reftests has been added yet so that's why this bug isn't closed yet. I'm planning to do it, though I'm already working on Bug 776265 which is another "really important bug" (just like any other bug). If you want writing a reftests doesn't need any C++ knowledge or what so ever so I can recommend you: https://developer.mozilla.org/en-US/docs/Creating_reftest-based_unit_tests If you can contribute that would be awesome!
Sorry if I've put this reftest in the wrong place, or messed up the format. I'm really a mercurial and firefox beginner Also it's a patch that contains just a test not any actual code. So hopefully this format works :S
I think the patch format is mostly ok, except that at some point you pasted it into an editor that does auto-indentation, which messed up (overindented) a bunch of the last few lines. You also shouldn't mark it "checkin+" -- that's for a patch that's been checked in to a mozilla repository, which this hasn't. As far as the test, it looks reasonable at first glance (though I didn't actually download the patch to look at the HTML), except the file naming convention is wrong. Normally the file names would be such that the first test would be foo-1.html and its reference foo-1-ref.html; then the second test (rather than the reference for the first test) would be foo-2.html, etc.
Ammended file structure to fit with other tests, and files. Also hopefully fixed the indetation issues on the last couple lines
Attachment #651063 - Attachment is obsolete: true
I think you forgot to add the reference file in your patch. Otherwise it looks good
Actually included the ref this time! (Third time lucky perhaps)
Attachment #651071 - Attachment is obsolete: true
Comment on attachment 651193 [details] [diff] [review] Ref test for bug 527459 with fixes to previous patch Review of attachment 651193 [details] [diff] [review]: ----------------------------------------------------------------- Let's have David reviewing this. Two notes from me: - I prefer to have explicit tests name like "input-percentage-padding.html" instead of "123456.html"; - You have a lot of useless empty lines in your HTML files.
Renamed to input-percentage-padding and removed excess whitespace.
Hey I've just pushed your patch to try servers. You can follow the results there: https://tbpl.mozilla.org/?tree=Try&rev=ca3db8c3d48c What I would do to your patch is add as a comment the bug number it refers to in reftest.list eg: == input-percentage-padding.html input-percentage-padding-ref.html #test for bug 527459 It helps future people to identify really quickly why this test has been added... Also I'm not sure you chose the perfect place to add the previous line in reftest.list. However, I'm really unsure about that so let's see what :dbarron or :roc say! Thanks for writting that test! That bug really need to be totally fixed!
Ok so for a change I just gave the wrong link this is the right one: https://tbpl.mozilla.org/?tree=Try&rev=83a6cabe5c0d Sorry
Comment on attachment 651477 [details] [diff] [review] Ref test for bug r=me. Mounir, are you going to land this? If not, please post patch with From line and whatnot?
Attachment #651477 - Flags: review?(dbaron) → review+
(In reply to Boris Zbarsky (:bz) from comment #29) > Comment on attachment 651477 [details] [diff] [review] > Ref test for bug > > r=me. > > Mounir, are you going to land this? If not, please post patch with From > line and whatnot? That's not my patch. However, I just sent this to try: https://tbpl.mozilla.org/?tree=Try&rev=6f7a273168d3 (I realize I could have save cycles by only requesting reftests... shame on me!)
OS: Linux → All
Hardware: x86 → All
Version: unspecified → Trunk
Comment on attachment 651477 [details] [diff] [review] Ref test for bug I pushed this to mozilla-inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/cd551da155fb
Attachment #651477 - Flags: checkin+
Dan, thank you for this contribution and congratulations for your first patch! :)
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla18
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.