Padding specified in percent ignored on text inputs

RESOLVED FIXED in mozilla17

Status

()

Core
Layout: Form Controls
P3
normal
RESOLVED FIXED
8 years ago
5 years ago

People

(Reporter: Bohdan Ganický, Assigned: lahabana)

Tracking

Trunk
mozilla17
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=bz] [lang=c++], URL)

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

8 years ago
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.
(Reporter)

Comment 1

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

Comment 2

8 years ago
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
(Reporter)

Comment 3

8 years ago
Is anyone looking into this? :)

Comment 4

7 years ago
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.
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: helpwanted, student-project
Summary: Text input behaves differently when CSS padding is set in px and percents. → Padding specified in percent ignored on text inputs
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?
Keywords: student-project
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.
Assignee: nobody → bzbarsky
Priority: -- → P2
Created attachment 468217 [details]
reftest
Created attachment 468218 [details]
Reference
Priority: P2 → P3

Comment 14

6 years ago
Requesting a status update on this. Thanks!

Comment 15

6 years ago
FF8, still not fixed.
Duplicate of this bug: 711030
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.
Whiteboard: [mentor=bz]

Updated

5 years ago
Whiteboard: [mentor=bz] → [mentor=bz] [lang=c++]
Depends on: 716875

Comment 18

5 years ago
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....
(Assignee)

Comment 19

5 years ago
(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

5 years ago
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

Updated

5 years ago
Attachment #651063 - Flags: checkin+
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.

Updated

5 years ago
Attachment #651063 - Flags: checkin+

Comment 22

5 years ago
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
Attachment #651063 - Attachment is obsolete: true
(Assignee)

Comment 23

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

Comment 24

5 years ago
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)
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.
Attachment #651193 - Flags: review?(dbaron)

Comment 26

5 years ago
Created attachment 651477 [details] [diff] [review]
Ref test for bug

Renamed to input-percentage-padding and removed excess whitespace.
Attachment #651193 - Attachment is obsolete: true
Attachment #651193 - Flags: review?(dbaron)
Attachment #651477 - Flags: review?(dbaron)
(Assignee)

Comment 27

5 years ago
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!
(Assignee)

Comment 28

5 years ago
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
Assignee: bzbarsky → charly.molter
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
Flags: in-testsuite+
Keywords: helpwanted
Target Milestone: --- → mozilla18
https://hg.mozilla.org/mozilla-central/rev/cd551da155fb
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Duplicate of this bug: 791386

Updated

5 years ago
Target Milestone: mozilla18 → mozilla17
You need to log in before you can comment on or make changes to this bug.