527459 - Padding specified in percent ignored on text inputs

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect, P3)

Description

[Bohdan Ganický]

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ý]

Ooops, sorry, padding IS applied but the text is displayed stuck to the top left corner.

Comment 2

[philippe (part-time)]

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ý]

Is anyone looking into this? :)

Comment 4

[Dobiatowski]

Attachment: 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 [:bzbarsky]]

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 [:bzbarsky]]

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) (email my personal email if necessary)]

I think as long as the frame has zero size, GetUsedPadding should return zero size.

Comment 8

[Boris Zbarsky [:bzbarsky]]

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 [:bzbarsky]]

Or we could stop inheriting from nsStackFrame.  Why _do_ we do that anyway?

Comment 10

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

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 [:bzbarsky]]

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 [:bzbarsky]]

Attachment: reftest

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Reference

Comment 14

[Mat Marquis]

Requesting a status update on this. Thanks!

Comment 15

[umnothankyou]

FF8, still not fixed.

Comment 17

[Boris Zbarsky [:bzbarsky]]

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]

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]

(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]

Attachment: 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-4, no longer working on Mozilla)]

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]

Attachment: 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]

I think you forgot to add the reference file in your patch. Otherwise it looks good

Comment 24

[Dan Shearmur]

Attachment: 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)]

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]

Attachment: Ref test for bug

Renamed to input-percentage-padding and removed excess whitespace.

Comment 27

[Charly Molter :lahabana]

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]

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 [:bzbarsky]]

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)]

(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)]

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)]

Dan, thank you for this contribution and congratulations for your first patch! :)

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cd551da155fb