Last Comment Bug 527459 - Padding specified in percent ignored on text inputs
: Padding specified in percent ignored on text inputs
Status: RESOLVED FIXED
[mentor=bz] [lang=c++]
:
Product: Core
Classification: Components
Component: Layout: Form Controls (show other bugs)
: Trunk
: All All
: P3 normal with 25 votes (vote)
: mozilla17
Assigned To: Charly Molter :lahabana
:
Mentors:
http://jsbin.com/odabo/
: 711030 791386 (view as bug list)
Depends on: 716875
Blocks:
  Show dependency treegraph
 
Reported: 2009-11-09 08:06 PST by Bohdan Ganický
Modified: 2012-11-18 15:35 PST (History)
23 users (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Reproduction of percentage problem (1.01 KB, text/html)
2010-02-12 00:51 PST, Dobiatowski
no flags Details
reftest (161 bytes, text/html)
2010-08-22 21:17 PDT, Boris Zbarsky [:bz]
no flags Details
Reference (155 bytes, text/html)
2010-08-22 21:18 PDT, Boris Zbarsky [:bz]
no flags Details
Reftest appropriate to the bug (1.79 KB, patch)
2012-08-10 17:48 PDT, Dan Shearmur
no flags Details | Diff | Splinter Review
Ammended file structure (1.09 KB, text/plain)
2012-08-10 18:28 PDT, Dan Shearmur
no flags Details
Ref test for bug 527459 with fixes to previous patch (1.79 KB, patch)
2012-08-12 06:48 PDT, Dan Shearmur
no flags Details | Diff | Splinter Review
Ref test for bug (1.73 KB, patch)
2012-08-13 11:16 PDT, Dan Shearmur
bzbarsky: review+
mounir: checkin+
Details | Diff | Splinter Review

Description Bohdan Ganický 2009-11-09 08:06:32 PST
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.6pre) Gecko/20091108 Shiretoko/3.5.6pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.6pre) 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 2.0.0.20 on Windows and FF 3.5.6pre on Linux.
I suppose it happens everywhere in between as well.
Comment 1 Bohdan Ganický 2009-11-09 08:08:21 PST
Ooops, sorry, padding IS applied but the text is displayed stuck to the top left corner.
Comment 2 philippe (part-time) 2009-11-09 15:39:33 PST
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
Comment 3 Bohdan Ganický 2009-12-21 08:47:36 PST
Is anyone looking into this? :)
Comment 4 Dobiatowski 2010-02-12 00:51:38 PST
Created attachment 426643 [details]
Reproduction of percentage problem

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.
Comment 5 Boris Zbarsky [:bz] 2010-05-04 13:58:30 PDT
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.
Comment 6 Boris Zbarsky [:bz] 2010-05-06 00:22:36 PDT
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?
Comment 7 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-05-06 00:35:17 PDT
I think as long as the frame has zero size, GetUsedPadding should return zero size.
Comment 8 Boris Zbarsky [:bz] 2010-05-06 00:40:41 PDT
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.
Comment 9 Boris Zbarsky [:bz] 2010-05-06 00:40:58 PDT
Or we could stop inheriting from nsStackFrame.  Why _do_ we do that anyway?
Comment 10 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-05-06 01:06:45 PDT
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?
Comment 11 Boris Zbarsky [:bz] 2010-05-06 01:19:27 PDT
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.
Comment 12 Boris Zbarsky [:bz] 2010-08-22 21:17:52 PDT
Created attachment 468217 [details]
reftest
Comment 13 Boris Zbarsky [:bz] 2010-08-22 21:18:22 PDT
Created attachment 468218 [details]
Reference
Comment 14 Mat Marquis 2011-11-02 08:33:24 PDT
Requesting a status update on this. Thanks!
Comment 15 umnothankyou 2011-11-11 03:18:49 PST
FF8, still not fixed.
Comment 16 Boris Zbarsky [:bz] 2011-12-16 20:04:50 PST
*** Bug 711030 has been marked as a duplicate of this bug. ***
Comment 17 Boris Zbarsky [:bz] 2011-12-16 20:07:31 PST
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.
Comment 18 ralph 2012-08-10 05:56:56 PDT
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....
Comment 19 Charly Molter :lahabana 2012-08-10 06:04:56 PDT
(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!
Comment 20 Dan Shearmur 2012-08-10 17:48:26 PDT
Created attachment 651063 [details] [diff] [review]
Reftest appropriate to the bug

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
Comment 21 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2012-08-10 18:15:44 PDT
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.
Comment 22 Dan Shearmur 2012-08-10 18:28:02 PDT
Created attachment 651071 [details]
Ammended file structure

Ammended file structure to fit with other tests, and files.

Also hopefully fixed the indetation issues on the last couple lines
Comment 23 Charly Molter :lahabana 2012-08-11 11:34:01 PDT
I think you forgot to add the reference file in your patch. Otherwise it looks good
Comment 24 Dan Shearmur 2012-08-12 06:48:02 PDT
Created attachment 651193 [details] [diff] [review]
Ref test for bug 527459 with fixes to previous patch

Actually included the ref this time! (Third time lucky perhaps)
Comment 25 Mounir Lamouri (:mounir) 2012-08-13 01:28:27 PDT
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.
Comment 26 Dan Shearmur 2012-08-13 11:16:28 PDT
Created attachment 651477 [details] [diff] [review]
Ref test for bug

Renamed to input-percentage-padding and removed excess whitespace.
Comment 27 Charly Molter :lahabana 2012-08-15 06:31:45 PDT
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!
Comment 28 Charly Molter :lahabana 2012-08-15 07:25:11 PDT
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 29 Boris Zbarsky [:bz] 2012-08-27 21:16:24 PDT
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?
Comment 30 Mounir Lamouri (:mounir) 2012-08-27 21:22:35 PDT
(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!)
Comment 31 Mounir Lamouri (:mounir) 2012-08-28 05:41:36 PDT
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
Comment 32 Mounir Lamouri (:mounir) 2012-08-28 05:43:05 PDT
Dan, thank you for this contribution and congratulations for your first patch! :)
Comment 33 Ryan VanderMeulen [:RyanVM] 2012-08-28 17:13:20 PDT
https://hg.mozilla.org/mozilla-central/rev/cd551da155fb
Comment 34 Boris Zbarsky [:bz] 2012-09-20 13:15:07 PDT
*** Bug 791386 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.