Closed Bug 157846 Opened 22 years ago Closed 10 years ago

Incorrect implementation of padding on textarea elements (scrollbars/resizer wrongly positioned)

Categories

(Core :: Layout: Form Controls, defect)

Product:
Core
Component:
Layout: Form Controls
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla29
Tracking Flags:
Tracking Status
blocking2.0 --- -

People

(Reporter: bugzilla, Assigned: lahabana)

References

(
URL
)

Details

(6 keywords, Whiteboard: [parity-IE][parity-webkit][parity-Opera][polish-visual][post-2.0])

Attachments

(11 files, 14 obsolete files)

Testcase: div, textarea, select
574 bytes, text/html
Details
Expected result of the test case
10.78 KB, image/png
Details
future reftests that cause a bug
1.33 KB, text/html
Details
screenshot of the reftest
12.10 KB, image/png
Details
Comparison between Firefox and other browsers
24.02 KB, image/png
Details
Part 0: Make tests more robust to padding changes and clipping of overflowing glyph edges
14.26 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
Part 0.1: When reflowing a non-rootframe reflow root, preserve its used padding in case that differs from its CSS computed padding for some reason
3.28 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
Part 0.2: Relax heuristic used to detect 'monospace' font for <input> sizing so that it at least includes 'monospace' font-family in Windows 7
1.41 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
main patch
22.99 KB, patch
Details | Diff | Splinter Review
Remove padding from textbox.plain
2.50 KB, patch
roc
: review+
Details | Diff | Splinter Review
Missing padding
16.13 KB, image/png
Details 
Actual results: the padding on textarea elements are "located" between the
scrollbars (if present) and the borders of the textarea.

Expected results: the padding on textarea elements should be between the content
(text content) and the borders (or scrollbars if present).

Reproducible: always. Windows XP Pro. Mozilla 1.1a+ 20020707

Test case following.
Attached file Self-explanatory test case (obsolete) — 
Instructions

If needed, fill in the textarea with more "X"s so that vertical and horizontal
scrollbars are displayed.

Actual results: there is a 19 pixels padding between the scrollbars and the red
borders.

Expected results: there should be no space, no yellow pixel between the
scrollbars and the red border of the textarea. The 19 pixels padding should be
located between the content (the "X"s) and the scrollbars.

Note: MSIE 6 do not display any space between the scrollbars and the red
border.

Best is to view this test case with MSIE 6.
thanks for the clear report.

having just spoken to some folks on IRC, this isn't really "incorrect" as such -
the standards don't cover where the scrollbar is supposed to be. however, it's
obviously true that we're not handling this the same way MSIE is, and there's
something to be said for different browsers doing the same thing, even when
there's nothing in the standard.

marking confirmed so that people can have a look at this.
Status: UNCONFIRMED → NEW
Ever confirmed: true
-> jkeiser
Assignee: rods → jkeiser
Priority: -- → P3
Target Milestone: --- → Future
Attached file Div with same style (obsolete) — 
A div with the same style paints the scrollbars *overlapping* the padding area.
 This is a low priority deal, but it kinda makes sense to do the same thing a
<div> does when we can.
The same also goes for select boxes. The drop down arrow has extra padding
around it, as well as the rest of the element, even though the padding should be
on the *inside* of the element.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5) Gecko/20031007 Firebird/0.7
Reproducible in Firefox 0.9
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7) Gecko/20040614 Firefox/0.9
This bug has persisted for almost two years now.
Firefox 2.0.0.6 on Windows XP -- this is still broken.  Seriously, "padding-right: 5px" does not go between the scrollbar and the right border; it belongs on the left side of the scrollbar (not the right).  Right now, it looks retarded.  And yeah, select boxes are broken too (still).  Get it fixed.  It's been over three years.
*** BUMP ***
This is still alive and kickin'... What's the deal?

Mozilla/5.0 (Windows; U; Windows NT 5.1; de-DE; rv:1.9b2) Gecko/2007121120 Firefox/3.0b2

Given that Firefox 3 is going to support off-line Web applications, I think form design should be as polished as possible.
Flags: blocking1.9?
Folks,

If you want to submit a patch, go ahead. 
"The only person who has any obligation to fix the bugs you want fixed is you. Never act as if you expect someone to fix a bug by a particular date or release."
https://bugzilla.mozilla.org/page.cgi?id=etiquette.html

If you want to vote for the bug (Morgan R., Rex G.), then go ahead.
"If you agree the bug should be fixed, vote for it."
https://bugzilla.mozilla.org/page.cgi?id=etiquette.html

Besides that, there is very little else/more or recommendable to do.

Regards, Gérard
Flags: blocking1.9? → blocking1.9-
Assignee: john → nobody
Priority: P3 → --
QA Contact: tpreston → layout.form-controls
Target Milestone: Future → ---
For what it's worth, as of today's webkit nightly, Firefox is the only major browser to still put the scrollbar on the inner div of a textarea. WebKit, IE and Opera all agree now.
Sure.  This is not really a high-priority item, though, since the exact sizing of the scrollable area of a textarea doesn't seem like something that would be as likely to cause interoperability issues as other things are...

If someone writes a patch I'd be happy to review it, of course.
Keywords: helpwanted
OS: Windows XP → All
Hardware: x86 → All
Attached file testcase (obsolete) — 
I tested this with other browsers.
IE, Chrome, Safari and Opera all are doing the same. The scrollbars are "inside" the padding only in Firefox.
Keywords: testcase
Whiteboard: [parity-IE] [parity-webkit] [parity-Opera]
bz: can you hint which files to look at?
Interestingly, <select multiple> acts as expected, while simple <select> doesn't. Though I had to add a -moz-box-sizing rule for it to actually work. Perhaps -moz-box-sizing should be defaulted to content-box for all elements?
Attachment #91591 - Attachment is obsolete: true
Attachment #113283 - Attachment is obsolete: true
Attachment #420802 - Attachment is obsolete: true
Rimas, the text control implementation is all in layout/forms/nsTextControlFrame.cpp.  This is used for both textareas and single line text controls (text and password inputs).

The reason <select multiple> works is that that particular class just inherits from nsHTMLScrollFrame, so it acts just like a <div style="overflow: auto">.  In the case of <textarea> the overflow styling, and the scrollbars, are on an anonymous <div> that's a child of the <textarea>; hence the observed behavior.

> Perhaps -moz-box-sizing should be defaulted to content-box for all elements?

It's explicitly not that way for certain form controls due to IE compat and breaking sites otherwise (at least the last time the issue was investigated).
(In reply to comment #17)
> Rimas, the text control implementation is all in
> layout/forms/nsTextControlFrame.cpp.  This is used for both textareas and
> single line text controls (text and password inputs).

OK, I must admit that I don't know C++ enough.

> The reason <select multiple> works is that that particular class just inherits
> from nsHTMLScrollFrame, so it acts just like a <div style="overflow: auto">. 
> In the case of <textarea> the overflow styling, and the scrollbars, are on an
> anonymous <div> that's a child of the <textarea>; hence the observed behavior.

I don't know why this div is needed at all, but since it exists: perhaps it's possible to delegate padding styling to that div too? 

> > Perhaps -moz-box-sizing should be defaulted to content-box for all elements?
> 
> It's explicitly not that way for certain form controls due to IE compat and
> breaking sites otherwise (at least the last time the issue was investigated).

Yeah, well, at least I can redefine that in CSS when making a website. I just don't like the fact that I have to...

On the other hand (and back on topic), this bug is about IE compat too. ;)
The <div> serves two functions:

1)  It's the scrollable thing in this case.
2)  It's the root of the editable subtree.

It'd significantly complicate text control reflow (e.g. text controls would actually have to implement their own reflow method, which they don't have to right now).  It might also be somewhat difficult to get it to play nice with the rest of the world.  But it might be doable.

I realize this is IE compat too.  See comment 13.  ;)
OK, and what about delegating textarea's padding styling to the div? Is it possible?

I tried to create a style rule for textarea > .anonymous-div, but it didn't seem to work...
> OK, and what about delegating textarea's padding styling to the div?

That's what the part of comment 19 after the 2-item list was about.

> I tried to create a style rule for textarea > .anonymous-div

In what sort of stylesheet?  That sort of rule would only work in a UA sheet.
It was a website stylesheet, of course...

bz: on devmo[1] I noticed that mozilla has a ::-moz-anonymous-block pseudo-element. Unfortunately, there's no description for this pseudo-element. Perhaps that anonymous div should match it?

In other words: if we had a workaround for this problem, that would be much better than nothing at all...

[1] https://developer.mozilla.org/en/CSS_Reference/Mozilla_Extensions
> I noticed that mozilla has a ::-moz-anonymous-block pseudo-element

That can only be used in a UA stylesheet.
are mozilla still no closer to solving this problem, does anyone know?

i know to some people this wouldn’t be the biggest problem ever, but the little things always count as i found out with a simple comment form i made, it looks bad in FF because of the padding

screenshot (useful for anyone who hasn’t seen this problem yet):
http://i44.tinypic.com/1sxxsk.png
Keywords: polish
Whiteboard: [parity-IE] [parity-webkit] [parity-Opera] → [parity-IE][parity-webkit][parity-Opera][polish-visual]
Yep, since 2002 still not fixed. Very strange.
http://translate.google.com/ now looks very ugly in Fx 4.0b.
Summary: Incorrect implementation of padding on textarea elements (scrollbars wrongly positioned) → Incorrect implementation of padding on textarea elements (scrollbars/resizer wrongly positioned)
blocking2.0: --- → ?
(In reply to comment #2)
> having just spoken to some folks on IRC, this isn't really "incorrect" as such -
> the standards don't cover where the scrollbar is supposed to be. 

This is apparently wrong, and we're actually violating CSS2.1, which says:

In the case of a scrollbar being placed on an edge of the element's box, it should be inserted between the inner border edge and the outer padding edge. Any space taken up by the scrollbars should be taken out of (subtracted from the dimensions of) the containing block formed by the element with the scrollbars.
URL: http://www.w3.org/TR/2010/WD-CSS2-201...
The title of this issue already covers it, but I wanted to explicitly mention again, that the grabber/resizer added in FF4 also looks misplaced when applying a padding to the text area. So this should be fixed before FF4 final ships.
As this bug isn't a blocking, and there release of ff4 is meant to be next month. That's just not going to happen.

Firefox 5 isn't far away though so might happen there.
(In reply to comment #19)
> The <div> serves two functions:
> 
> 1)  It's the scrollable thing in this case.
> 2)  It's the root of the editable subtree.
> 
> It'd significantly complicate text control reflow (e.g. text controls would
> actually have to implement their own reflow method, which they don't have to
> right now).  It might also be somewhat difficult to get it to play nice with
> the rest of the world.  But it might be doable.
> 
> I realize this is IE compat too.  See comment 13.  ;)

Wouldn't that also require making nsTextContralFrame something else than nsStackFrame?
Making nsTextControlFrame not be an nsStackFrame would make the code nicer IMHO.
So, skimming over this quickly, what's needed here is comment 19, right?  (Not to say that it doesn't make me sad!)

I will most likely get to this post 2.2 (ignore the whiteboard flag I'm using, I'm effectively the only one now who knows what it means!)
Assignee: nobody → ehsan
Whiteboard: [parity-IE][parity-webkit][parity-Opera][polish-visual] → [parity-IE][parity-webkit][parity-Opera][polish-visual][post-2.0]
I was about to report the same issue, but luckily I found this first (one of the few times my search for potential existing bug reports has been fruitful, yay). Here's my testcase, in case it helps in any way: http://jsfiddle.net/leaverou/rLwVL/show

Also, every other browser I tested places the scrollbar in the padding-box, so it's inconsistent with pretty much every other engine around.
Ehsan, are you still planning to work on this?
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #37)
> Ehsan, are you still planning to work on this?

Not really.
Assignee: ehsan → nobody
Depends on: 716875
Blocks: 576976
We are working (with :alexdmt) and close to the end on bug 716875. So we decided while it's running server tests to look at that bug.
Does it still look logical to just propagate the padding to the HTMLScrollFrame and ignore it in the nsTextControlFrame? If not what looks like the best way to do that?
The first option looks now much easier as nsTextControlFrame has it's own reflow method now.
Attached patch Patch that pass all tests (obsolete) — — Splinter Review 
Hello,
This is a patch that finally passes all tests on all platform as you can see on:
https://tbpl.mozilla.org/?tree=Try&rev=4c52175978b2

Therefore we would like a review
Thanks
Attachment #633068 - Flags: review?(roc)
Attachment #633068 - Flags: review?(bzbarsky)
Comment on attachment 633068 [details] [diff] [review]
Patch that pass all tests

That was an epic error of mine submitting the patch to the wrong bug
Sorry
Attachment #633068 - Attachment is obsolete: true
Attachment #633068 - Flags: review?(roc)
Attachment #633068 - Flags: review?(bzbarsky)
Assignee: nobody → bellot.zoe
We are working on that from the patch submitted to https://bugzilla.mozilla.org/show_bug.cgi?id=716875
We think that the problem is in ReflowTextControlChild
what we did for the moment is ignore the padding present in the parameter aReflowState (by changing the size and offset of the kid)
What we would like to do now is to force the padding when we build reflowState so that 
      reflowState.mComputedPadding == aReflowState.mComputedPadding.
We changed it directly in the code everything is working well until you actually type something and the padding is removed.
We think it's because of mComputedWidth being just a cache version and therefore not updated correctly...
So we identified the problem:
It wasn't working when we were changing the properties of the reflowState cause the reflow is done directly on the ScrollBarFrame when you type.
Is there a way to change the reflowState all the time?
Should it be done at the construction of the frames?
You might want to take a look at how nsHTMLScrollFrame::ReflowScrolledFrame handles padding by propagating it to the scrolled frame...  It seems like you want something similar here, perhaps.
Just to make clear what's the expected display I created a mockup of how the test case should be rendered.

Sebastian
Just so you know I'm still on that I just add a more important regression bug to fix first.
Assignee: bellot.zoe → charly.molter
Hey I've been working on this and I've got a beginning that starts to work quite well. It works until you start focusing the textarea then the whole padding just disappear. I've been looking for a bit and it's because the nsTextControlFrame is not reflowed but directly the text frame. That doesn't seem to know it has a padding. I looked at nsGfxScrollFrame to see but it doesn't seem to have to deal with that.
Do you have any idea how to fix that?
Attachment #656008 - Flags: feedback?(roc)
Attachment #656008 - Flags: feedback?(bzbarsky)
That's because the anonymous div's nsHTMLScrollFrame is marked as a reflow root by nsTextControlFrame::SetInitialChildList. So editing changes start reflowing at the nsHTMLScrollFrame and nsTextControlFrame::SetInitialChildList isn't called.

I think you need to make the "textarea > .anonymous-div" rule in forms.css have "padding:inherit" so the style system knows the anonymous DIV has the padding. (Move the existing padding to textarea/input directly.) Then we need to make sure the padding doesn't get applied to the nsTextControlFrame. Maybe by adding its frame type to the checks in nsCSSOffsetState::ComputePadding.
Comment on attachment 656008 [details] [diff] [review]
first patch with a problem when you type text

What roc said.  ;)
Attachment #656008 - Flags: feedback?(bzbarsky)
So I followed your advices and put the div's padding in inherit there was a rule for IE compability that I put in textarea instead should be ok this way no? Is that padding actually still usefull?
It wasn't necessary to change anything in nsCSSOffsetState::ComputePadding to make sure the input forgot the padding as it was taken into account in nsTextControlFrame::reflowChild. Forcing the padding to 0 was just making the nsTextControlFrame too small as it didn't have any space for the kid's padding.
I sent it to try at some point and the only error was: 
REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/editor/reftests/xul/plain-1.xul | image comparison (==)
it uses -moz-margin-start don't really get the test tbh.
I'm going to add reftests to ensure correctness.
The select still doesn't work ftm can somebody confirm that the attached mockup is actually the wanted result? Cause Chrome for eg just ignores padding on selects... 
Hope this is good!
Attachment #656437 - Flags: feedback?(roc)
Attachment #656437 - Flags: feedback?(bzbarsky)
I was preparing reftests and ran into a bug (I think). I merged the two files in one. As you can see the first tag (the text area) has one less " a" than the div. 
It seems to be because in the textarea the scrollbar is counted twice. Any idea why?
Comment on attachment 656437 [details] [diff] [review]
Prepatch that seems to work for inputs

Seems plausible at first glance.  Have you tested what happens when intrinsic sizing is used?  Explicit sizing?  I would assume this patch is not changing the size of the control as long as the page doesn't style padding?

Also, I doubt you need the NS_MAX of computed width/height against 0: those should always be nonnegative.
Attachment #656437 - Flags: feedback?(bzbarsky) → feedback+
Comment on attachment 656437 [details] [diff] [review]
Prepatch that seems to work for inputs

Review of attachment 656437 [details] [diff] [review]:
-----------------------------------------------------------------

I think you really should make sure that nsCSSOffsetState::ComputePadding computes a zero padding for nsTextControlFrame. It makes things more consistent.

::: layout/style/forms.css
@@ +121,5 @@
>  input > .anonymous-div {
>    white-space: pre;
>    overflow: auto;
>    border: 0px !important;
> +  padding: inherit;

Don't you need to add the 1px left/right padding to the default padding on <input>?
Attachment #656437 - Flags: feedback?(roc) → feedback-
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #55)
> Comment on attachment 656437 [details] [diff] [review]
> Prepatch that seems to work for inputs
> 
> Review of attachment 656437 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think you really should make sure that nsCSSOffsetState::ComputePadding
> computes a zero padding for nsTextControlFrame. It makes things more
> consistent.
Ok but then I'll need a way to extract the kids padding to find the right size of the nsTextControlFrame
> ::: layout/style/forms.css
> @@ +121,5 @@
> >  input > .anonymous-div {
> >    white-space: pre;
> >    overflow: auto;
> >    border: 0px !important;
> > +  padding: inherit;
> 
> Don't you need to add the 1px left/right padding to the default padding on
> <input>?
Yes indeed I forgot that. But only on input[type="text"] though. Cause are we ok that nsTextControlFrame only deal with these kind of inputs?
(In reply to Charly Molter :lahabana from comment #56)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #55)
> > Comment on attachment 656437 [details] [diff] [review]
> > Prepatch that seems to work for inputs
> > 
> > Review of attachment 656437 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I think you really should make sure that nsCSSOffsetState::ComputePadding
> > computes a zero padding for nsTextControlFrame. It makes things more
> > consistent.
> Ok but then I'll need a way to extract the kids padding to find the right
> size of the nsTextControlFrame
> > ::: layout/style/forms.css
> > @@ +121,5 @@
> > >  input > .anonymous-div {
> > >    white-space: pre;
> > >    overflow: auto;
> > >    border: 0px !important;
> > > +  padding: inherit;
> > 
> > Don't you need to add the 1px left/right padding to the default padding on
> > <input>?
> Yes indeed I forgot that. But only on input[type="text"] though. Cause are
> we ok that nsTextControlFrame only deal with these kind of inputs?
Ok I'm just stupid don't pay attention to that last sentence about input[type="text"] sorry
(In reply to Charly Molter :lahabana from comment #56)
> Ok but then I'll need a way to extract the kids padding to find the right
> size of the nsTextControlFrame

That's no problem, because nsTextControlFrame::ReflowTextControlChild constructs the kidReflowState which will contain the child's computed padding.
Hmm ok but what do I do when there's more than one child? 
And about Comment 53 and the possible reftest? Is it normal?
(In reply to Charly Molter :lahabana from comment #59)
> Hmm ok but what do I do when there's more than one child?

OK then, I guess it does get a bit complicated to do it that way. So leave it the way you were doing it.

> And about Comment 53 and the possible reftest? Is it normal?

Is that an existing bug or a new bug with your patch?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #60)
> (In reply to Charly Molter :lahabana from comment #59)
> > Hmm ok but what do I do when there's more than one child?
> 
> OK then, I guess it does get a bit complicated to do it that way. So leave
> it the way you were doing it.
> 
Ok
> > And about Comment 53 and the possible reftest? Is it normal?
> 
> Is that an existing bug or a new bug with your patch?
It's kind of an existing bug. I mean before the scrollbar where inside the padding so it was logical whereas.
IMHO now the size of the scrollbar should be subtracted to the padding. So that the content seems centered. Should I fix that in another bug? If yes that means I should do temporary reftests and replace them later with the right ones?
I'm not really sure what the bug in comment #53 is. On trunk both halves of your testcase have the same number of a's per line whether there's padding or not.
:roc you don't get something like that? I've checked that's what I get on macOS with both 15 and Nightly. Shouldn't both container have the same number of letters/line? 
I'm sorry if I'm wrong
Hey so this a patch that is halfway through completion.
The reftests I added don't work, I think it is because of what I said just before.
and I've got an issue with 2 other reftests I'm looking into it. I think it is due to some changes in the padding.
Here is the try link:
https://tbpl.mozilla.org/?tree=Try&rev=f2a1b1ffcdab
Attachment #656008 - Attachment is obsolete: true
Attachment #656437 - Attachment is obsolete: true
Attachment #656008 - Flags: feedback?(roc)
Attachment #658480 - Flags: feedback?(roc)
Attachment #658480 - Flags: feedback?(bzbarsky)
Comment on attachment 658480 [details] [diff] [review]
patch with reftests but still some fail in try

Review of attachment 658480 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/forms/nsTextControlFrame.cpp
@@ +563,5 @@
>                                             nsHTMLReflowMetrics& aParentDesiredSize)
>  {
>    // compute available size and frame offsets for child
>    nsSize availSize(aReflowState.ComputedWidth(), 
>                     aReflowState.ComputedHeight());

Seems like this is wrong now. ComputedWidth is computed by subtracting border and padding from this frame's border-box. We need to adjust availSize for the fact that we're going to treat our padding as zero, so you need to add the padding to availSize.

@@ +570,5 @@
>                                     aKid, availSize);
>  
>    // Set computed width and computed height for the child
> +  kidReflowState.SetComputedWidth(availSize.width);
> +  kidReflowState.SetComputedHeight(availSize.height); 

This seems wrong. SetComputedWidth sets the content width for the child, which should not be affected by this change.
Attached patch corrected availSize (obsolete) — — Splinter Review 
So this is an updated patch.

I'm not convinced by that comment:
@@ +570,5 @@
>                                     aKid, availSize);
>  
>    // Set computed width and computed height for the child
> +  kidReflowState.SetComputedWidth(availSize.width);
> +  kidReflowState.SetComputedHeight(availSize.height); 
Firstly, because if I remove it. It fails when the text is shorter than a full line. (The ScrollFrame is smaller than the actual TextControlFrame)
Secondly, it seems logical to me that has we've got a padding in our parent frame that is not actually rendered we have to adapt the kids frame too.

Maybe there is something I'm missing too.

- I've looked at the reftests and there seems to be a problem with the padding of xul textboxes (the test /editor/reftests/xul/plain-1.xul fails)

- the reftest textarea-margin-scrollbar-placement-2.html fails because it aims to test intrinsic sizing of textareas with padding though I don't know how to make a reference for it working on all platform. Any idea?
Attachment #658480 - Attachment is obsolete: true
Attachment #658480 - Flags: feedback?(roc)
Attachment #658480 - Flags: feedback?(bzbarsky)
Attachment #659729 - Flags: review?(roc)
Attachment #659729 - Flags: review?(bzbarsky)
(In reply to Charly Molter :lahabana from comment #66)
> I'm not convinced by that comment:
> @@ +570,5 @@
> >                                     aKid, availSize);
> >  
> >    // Set computed width and computed height for the child
> > +  kidReflowState.SetComputedWidth(availSize.width);
> > +  kidReflowState.SetComputedHeight(availSize.height); 
> Firstly, because if I remove it. It fails when the text is shorter than a
> full line. (The ScrollFrame is smaller than the actual TextControlFrame)
> Secondly, it seems logical to me that has we've got a padding in our parent
> frame that is not actually rendered we have to adapt the kids frame too.

You changed it to kidReflowState.SetComputedHeight(aReflowState.ComputedHeight()) etc which looks good to me. So the patch overall looks good to me.

> - I've looked at the reftests and there seems to be a problem with the
> padding of xul textboxes (the test /editor/reftests/xul/plain-1.xul fails)

I don't have a specific suggestion here. I guess it just needs debugging.

> - the reftest textarea-margin-scrollbar-placement-2.html fails because it
> aims to test intrinsic sizing of textareas with padding though I don't know
> how to make a reference for it working on all platform. Any idea?

I don't have any ideas here. You could just remove this test.
A question.  The attached patch seems to treat the main text and placeholder identically.  Should that be the case?

Mounir, in particular this would change the "sticking out" behavior you saw with the padding....
(In reply to Boris Zbarsky (:bz) from comment #68)
> A question.  The attached patch seems to treat the main text and placeholder
> identically.  Should that be the case?

I would have thought so...
Mounir was arguing against it in bug 737786 is the problem.  :(
(In reply to Boris Zbarsky (:bz) from comment #68)
> A question.  The attached patch seems to treat the main text and placeholder
> identically.  Should that be the case?
> 
> Mounir, in particular this would change the "sticking out" behavior you saw
> with the padding....

The sticking out behaviour is when the padding is set on ::-moz-placeholder not on the input.

That means
input {
  padding: 5px;
}
should treat the same way the editor and the placeholder.

But
input::-moz-placeholder {
  padding: 20px;
}
will move the placeholder outside of the input field.

Is that an incorrect behaviour? IIRC, <progress> and ::-moz-progressbar has a similar behaviour.
wow, this must be one of the oldest unresolved firefox bugs in history?

no padding between text and scrollbar of a textarea since ten years? are you kidding me? guys, get your act together and fix it already!
(In reply to info from comment #72)
> wow, this must be one of the oldest unresolved firefox bugs in history?
> 
> no padding between text and scrollbar of a textarea since ten years? are you
> kidding me? guys, get your act together and fix it already!

You’re not helping.
There are, as of today, 6509 open bugs that are older than this one.

Which means nothing, because age tells you nothing about how important a bug is.
Attachment #659729 - Flags: review?(bzbarsky)
2002-07-16, today 2013-05-13 -_-
I hope this bug is fixed. Current behavior is really unexpected...
11 years and counting.  That must be one helluva bug.  Come on Mozilla.  This needs to be addressed for crying out loud.
I'm a developer for the Wikimedia Foundation and can vouch for the fact that this bug has been a pain in the butt for us. We ran into this summer while working on our Visual Editor and then again recently while working on our mobile editing interface. It sucks having to build workarounds for something as basic as textarea display, especially on mobile where we try to keep the loading footprint as small as possible. Is there anything we can do to get more traction on this bug? (Unfortunately, we don't have C++ developers here.) I attached a minimal test case with screenshots if that helps.
(In reply to kaldari from comment #80)
> I'm a developer for the Wikimedia Foundation and can vouch for the fact that
> this bug has been a pain in the butt for us.

kaldari,

I am the original bug reporter of this bug and I do not understand why people seem to be very annoyed by the bug. Objectively speaking, in terms of bug severity, bug gravity, bug importance, bug relevance, this bug is not serious: no dataloss, no loss of functionality, no accessibility-to-content weakening or loss or disturbance, no crash, no application hang, no performance incidence, no increased memory footprint, no negative memory management incidence. Only a difference of layout: the position of padding area versus the position of vertical scrollbar (if its rendering is required). And textarea are, by default in Firefox, resizeable thanks to resizing grippy at bottom-right corner of textarea.

> Is there anything we can do to get
> more traction on this bug? (Unfortunately, we don't have C++ developers
> here.)

More traction?
- Did you vote for this bug?
- You can donate and indicate that your donation is to help fix this bug
- You can learn C++ and then submit/propose patches. (Some people may think this may be weird but, personally, having known in advance that some of my own bug reports would not have been fixed after 10 years would have inticed/motivated me to take a course in C++ or Rust language.)

> I attached a minimal test case with screenshots if that helps.

I believe we do not need any reduced test case for this bug report.

Gérard
Gérard,

Some web applications require pixel-perfect alignment. For example, the in-browser syntax highlighter EditArea includes a hack for this Firefox bug on lines 213-218 of http://sourceforge.net/p/editarea/code/33/tree/trunk/edit_area/edit_area.js#l213 but even this hack does not fix the problem in all circumstances or on all platforms.

I was told at https://bugzilla.mozilla.org/show_bug.cgi?id=712128 that that problem is a symptom of this bug. This would mean that https://bugzilla.mozilla.org/show_bug.cgi?id=753662 is also a symptom of this bug.

The bottom line is that there some cool but unusual things we want to do that this bug hampers.
> the
> in-browser syntax highlighter EditArea includes a hack for this Firefox bug
> on lines 213-218 of
> http://sourceforge.net/p/editarea/code/33/tree/trunk/edit_area/edit_area.
> js#l213 but even this hack does not fix the problem in all circumstances or
> on all platforms.

I do not know exactly what all that code does and what's the related CSS and HTML for those functions but:

1)
t.lineHeight= 16;
http://sourceforge.net/p/editarea/code/33/tree/trunk/edit_area/edit_area.js#l57
This appears wrong. See
http://www.w3.org/TR/2000/REC-DOM-Level-2-Style-20001113/css.html#CSS-CSS2Properties-lineHeight
as lineHeight is not an integer but a string like "16px" or "1.5em" or "14pt" or "140%"

2)
t.execCommand("change_highlight", s["start_highlight"]);
http://sourceforge.net/p/editarea/code/33/tree/trunk/edit_area/edit_area.js#l192
This execCommand(), if I recall correctly, is IE-only code and is not supported by Firefox.

3)
The default monospace font-size in Firefox is 12px under Linux while it is 13px under Windows. This is not widely known and I wonder if this could affect those scripts.

4)
8.6.3 Best practices for in-page editors
http://www.whatwg.org/specs/web-apps/current-work/multipage/editing.html#best-practices-for-in-page-editors
states
"
Authors are encouraged to set the 'white-space' property on editing hosts and on markup that was originally created through these editing mechanisms to the value 'pre-wrap'.
"

5)
I see other possible issues (like use of eval()) with the code at http://sourceforge.net/p/editarea/code/33/tree/trunk/edit_area/edit_area.js
http://www.javascripttoolbox.com/bestpractices/#eval

Anyway... I checked your attachment 582950 [details] and I see what you mean.

And I see that kaldari and yourself have already voted for this bug.

Gérard
...
> I do not understand why people seem to be very annoyed by the bug. 
> Objectively speaking, in terms of bug severity, bug gravity, bug 
> importance, bug relevance, this bug is not serious: no dataloss,
> no loss of functionality, no accessibility-to-content weakening or 
> loss or disturbance, no crash, no application hang, no performance 
> incidence, no increased memory footprint, no negative memory management 
> incidence. Only a difference of layout: the position of padding area
> versus the position of vertical scrollbar (if its rendering is 
> required). And textarea are, by default in Firefox, resizeable thanks
> to resizing grippy at bottom-right corner of textarea.
...

Would you then say that all design/display/layout bugs are irrelevant. 'cause if those are ignored IE6 is quite a neat browser (:evilgrin:)

I ran into this bug a few years ago while making a plaintext editor using textarea.
The bug is not subtle, in Firefox alone, trying to apply design to a textarea results in something that (to the end user) looks broken. Since Firefox is one of the leading browsers I opted out and ended up using an editable-div solution from somebody else.

For the purpose i needed, editable-div is an overly (and unnecessarily) complicated solution, but alas it is finite. On the other hand, this bug in Firefox has no solution: no hack, cheat or approach will allow you to put the padding on the correct side of the scrollbar.

reductio ad absurdum arguments:

"So don't design padding on the textareas, and problem solved" - sure, that's what I do now.. but that logic could be applied to any individual CSS property as implemented on any individual DOM element, and what's the point of standards and design in that case.

"But this is not exactly a hot design feature that's being used and sought after" - you can't really say that: if it doesn't work, people will not use it (duh). 

Anyway, when I started monitoring this thread Firefox was holding a third of the browser market and going strong. Nowadays, word-of-mouth is suggesting clients and designers to ignore Firefox, install Chrome and move on. And I noticed that I (more and more often) switch back to Firefox only because of the familiarity of Firebug. It might have nothing to do with this issue, but might eventually provide a solution to it.

This bug will be a teenager soon, suggesting designers and frontend developers learn C++ to solve it is really not a solution.

Apologies for illiteracy, venom and exaggeration,
Barney
Any chance for fix?
Attached patch Proposed patch (obsolete) — — Splinter Review 
I've rebased and updated Chris's patch, correcting the test failures. This new patch fixes the bug while matching pixel for pixel Firefox's existing padding on <input>, <textarea>, and <textbox>. The only exceptions are 4 tests (including the 1 new test) that now have an imperceptible difference in the subpixel antialiasing. Maybe we should open a bug report for the subpixel antialiasing differences, but they are so extremely minor that I wouldn't worry about it.

The XUL tests were failing because they expected <textbox> to have 1px padding on the left and right, just like <textarea> has 1px padding on the left and right. Making the extra padding explicit in the default XUL stylesheet and in a few other places resolved this and similar problems. The margin scrollbar placement test was failing due to an unrelated bug in text wrapping; I added "white-space: pre-wrap" to the div and it now works fabulously. I left out the second margin scrollbar placement test because I think it is redundant.

I hope that all makes sense. If anything is not clear, please let me know.
Attachment #8351471 - Flags: review?(roc)
Merry Christmas and Happy New Year!
Comment on attachment 8351471 [details] [diff] [review]
Proposed patch

Review of attachment 8351471 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you very much for your help!!!

I think there is one problem left, which was also in Charly's patch: nsTextControlFrame::CalcIntrinsicSize adds the padding of the anonymous child (around "add in the padding of our value div child"). This returns the intrinsic width of the element's content-box, so effectively any padding set on the element will be counted twice. For example I expect that <input style="padding-left:200px"> will be > 400px wide. I think we should just stop adding that stuff, and add a test.

::: layout/reftests/forms/textarea/margin-scrollbar-placement-ref.html
@@ +12,5 @@
> +                border: 5px solid red;
> +                margin: 10px;
> +                overflow: scroll;
> +                font-family: verdana;
> +                white-space: pre-wrap; /* problem: the test fails if you remove this line */

Take out this comment. It makes sense to use white-space:pre-wrap, since that's what <textarea> uses. Thanks for catching it.

::: layout/style/forms.css
@@ +54,4 @@
>    /* The sum of border-top, border-bottom, padding-top, padding-bottom
>       must be the same here, for buttons, and for <select> (including its
>       internal padding magic) */
> +  padding: 1px 1px;

Could just be "1px"
Attachment #8351471 - Flags: review?(roc) → review-
Attached patch Proposed patch (try 2) (obsolete) — — Splinter Review 
The intrinsic size problem only happens with monospace fonts, but I see what you mean. How's this? It could probably be improved further but I'm trying to make the minimum number of changes required to fix the scrollbar positioning problem.
Attachment #8351471 - Attachment is obsolete: true
Attachment #8351539 - Flags: review?(roc)
Comment on attachment 8351539 [details] [diff] [review]
Proposed patch (try 2)

Review of attachment 8351539 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/reftests/forms/input/text/intrinsic-size.html
@@ +4,5 @@
> +        <title>Intrinsic Size Test</title>
> +    </head>
> +    <body>
> +        <div>
> +            <input style="font-family:'Courier New'; font-size:12pt; padding-left:200px"/>

I don't think this test will necessarily pass everywhere as written, since the width of this <input> will depend on the average char width of the font, which is probably but not necessarily the same across platforms, but the reference <input>'s width does not depend on the average char width of the font.

Therefore this is difficult to reliably reftest I guess. Maybe make the test <input style="border:0; padding-left:200px">, and the reference <input style="border:0"> wrapped in a <span style="padding-left:200px">?

If that doesn't work, feel free to drop this test. I don't want to lump you with extra work :-).
Attachment #8351539 - Flags: review?(roc) → review-
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #90)
> Therefore this is difficult to reliably reftest I guess. Maybe make the test
> <input style="border:0; padding-left:200px">, and the reference <input
> style="border:0"> wrapped in a <span style="padding-left:200px">?

Of course, for that test to actually test anything you'd need to make sure it actually renders something based on the width of the <input>s. E.g. by wrapping them in a <span style="border:2px">.
Attached patch Proposed patch (try 3) (obsolete) — — Splinter Review 
Done.
Attachment #8351539 - Attachment is obsolete: true
Attachment #8351592 - Flags: review?(roc)
Attachment #659729 - Attachment is obsolete: true
Attachment #659729 - Flags: review?(roc)
Comment on attachment 8351592 [details] [diff] [review]
Proposed patch (try 3)

Review of attachment 8351592 [details] [diff] [review]:
-----------------------------------------------------------------

Lovely!
Attachment #8351592 - Flags: review?(roc)
Attachment #8351592 - Flags: review+
Attachment #8351592 - Flags: checkin?
Comment on attachment 8351592 [details] [diff] [review]
Proposed patch (try 3)

Review of attachment 8351592 [details] [diff] [review]:
-----------------------------------------------------------------

Lovely!
Attachment #8351592 - Flags: checkin? → checkin+
Oops, should have done a try run. For some reason I thought we had.
Attached patch Proposed patch (try 4) (obsolete) — — Splinter Review 
New test results: https://tbpl.mozilla.org/?tree=Try&rev=4bb4e03ef3c6

I've worked out most of the issues, but I can't test Mac OS, I'm not set up to test Android, and Windows is doing something funky. For example, layout/reftests/forms/input/number/number-selected-ref.html is off on Windows 7. It's like the padding on the input element is ignored when drawing the rectangle; I tried changing the input padding in layout/style/forms.css to 100px and the text was rendered way outside the textbox. If I manually specify "padding:2px" then it renders perfectly, but forms.css says that input is only supposed to have 1px padding.

I'm stumped. I really need a Mozilla developer to work with me on these last problems.
Attachment #8351592 - Attachment is obsolete: true
Flagging roc to either help or redirect to someone else.
Flags: needinfo?(roc)
I'll try to help.
Flags: needinfo?(roc)
after my last (venomous) comment I just wanted to say that I'm sincerely glad someone is working on this, and would like to thank you for the effort
I've tracked down one problem. On Windows, <input> is themed, so the theme borders and padding override UA style sheet CSS on the <input>. In this case the theme border is 1 device pixel and the theme padding is 2 device pixels. So with the patch, a regular text <input> gets a 1 device pixel border, and positions its scrollframe child 1 device pixel inset from top-left. Then the patch expects the padding to be applied to the scrollframe child via CSS inheritance. Unfortunately that doesn't work since we inherit the CSS 1px padding, where we need to inherit the 2 device pixel theme padding. It's extra confusing when 1px != 1 device pixel (e.g. when zooming).
Fortunately we have one weird trick to handle exactly that situation...
Attached patch fix v5 (obsolete) — — Splinter Review 
I've changed the initialization of kidReflowState in nsTextControlFrame::ReflowTextControlChild.

I've also removed the height constraint in availSize. The available height is only used for vertical breaking which we never want here.
More bugs to look into tomorrow :-)
OK, the latest results show a problem with the "reflow root" optimization. nsTextControlFrame makes its first anonymous child (the scrollframe) a reflow root, which means that changes under that frame don't require reflowing all the way up the frame tree; the new reflow can start at the reflow root. But now we're inheriting computed padding from the parent into the scrollframe during normal reflow, but we treat it as a reflow root that doesn't happen because nsTextControlFrame::Reflow is not called. Instead we get the CSS padding in our nsHTMLReflowState.
Getting closer, but still significant failures.
Looks like intrinsic size calculation is a little off.
It looks like two main classes of failures:
-- Failures where we display a bit more text than we used to. This actually makes sense since, having moved the padding inside the scrolled area, we now render text overlapping the padding where before we would have clipped it out. We probably need to update tests to take account of this.
-- Positioning and sizing in some XUL inputs is slightly broken. This is presumably just a bug.
Jim, the only patch you need is here: https://hg.mozilla.org/try/rev/dbd693da9fbe
Flags: needinfo?(jmathies)
Some of the Android and B2G failures are due to bug 958999. We can work around those for now.

A couple of the changes to input.css were incorrect; some of the 0 padding needs to stay 0.

I'm adding padding:0 to the size-1 and anonymous-block tests to work around the issues in comment #112.

Jim Mathies is going to help us with the Metro failure.

Some of the failures were due to fuzzy(...) being added after fails-if. This causes those platforms to revert to the fuzzy behavior when they actually still fail. Put fuzzy(...) first on the line.

https://tbpl.mozilla.org/?tree=Try&rev=f57483e86db2
I just wanted to say thanks for working on this. You're so close!

It looks like padding-scrollbar-placement.html needs to be increased to fuzzy(65,11347) to take into account antialiasing differences on Android and B2G. I don't think I can offer much help with the other test failures, but I'm confident you'll work it out.

By the way, I don't care who gets their name on the final patch. You, me, and Charly Molter have all contributed.
(In reply to Alex Henrie from comment #115)
> I just wanted to say thanks for working on this. You're so close!

Thank you too!

> It looks like padding-scrollbar-placement.html needs to be increased to
> fuzzy(65,11347) to take into account antialiasing differences on Android and
> B2G. I don't think I can offer much help with the other test failures, but
> I'm confident you'll work it out.

I'm making the elements overflow:hidden so the scrollbars don't cause problems.

I don't know why the fonts are looking different. Maybe a layerization issue, may be affected by overflow:hidden. We'll see.
https://tbpl.mozilla.org/?tree=Try&rev=06f60fb23723

The monospace font detection in nsTextControlFrame::CalcIntrinsicSize is broken. On my Windows 7 laptop it detects font-family:monospace as not monospace :-). (AveWidth is > MaxAdvance :-(.) Tweaking that heuristic a bit should fix size-1.html.

After I've got the tests green I'll have to break the patch up a bit more and get review for the new pieces.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #113)
> Jim, the only patch you need is here:
> https://hg.mozilla.org/try/rev/dbd693da9fbe

https://tbpl.mozilla.org/?tree=Try&showall=0&rev=e0bdd19012c0

Pushed a fix to try for some mc retriggers looking for new sporadic failures in our selection tests.
Flags: needinfo?(jmathies)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #116)
> I'm making the elements overflow:hidden so the scrollbars don't cause
> problems.

Umm, isn't the point of padding-scrollbar-placement.html to test where the scrollbar is drawn? How can you test that if you hide the scrollbars?
Actually I just realized that you can also know the fix is working because the bottom padding won't be applied. If that test is enough, great, although if the presence of scrollbars throws the text out of alignment then we haven't really fixed this bug.
(In reply to Alex Henrie from comment #118)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment
> #116)
> > I'm making the elements overflow:hidden so the scrollbars don't cause
> > problems.
> 
> Umm, isn't the point of padding-scrollbar-placement.html to test where the
> scrollbar is drawn? How can you test that if you hide the scrollbars?

Haha yes, good point.

The problem is that on B2G, Android and modern Macs, scrollbars don't take up any width *and* they fade in and out a little bit unpredictably for our purposes. So this test doesn't really test anything on those platforms and it can unpredictably fail. The simplest solution is just to skip the test on B2G/Android. Another solution would be to vary the text content so it's "1 2 3 4 ..." instead of all "a"s, and cover the right-hand-side of the element with something opaque so the drawing of the scrollbar doesn't affect the results of the test.

The two remaining issues in the try push are padding-scrollbar-placement.html on B2G/Android and editor/reftests/xul failures still happening on Mac, where the text in the test is either one pixel to the left or right of the text in the reference depending on the test.
I discovered the reason for the text looking different in padding-scrollbar-placement.html on B2G/Android. It was also coming up on my Linux box. On some platforms <textarea>s have a default text color that's not quite black!
Attached patch latest patch (obsolete) — — Splinter Review 

        Attachment #8357525 -
        Attachment is obsolete: true

        Attachment #8358059 -
        Attachment is obsolete: true

    
    

    
  
I screwed up and lost some changes. Fixed that, plus some other fixes: https://tbpl.mozilla.org/?tree=Try&rev=84956900b3e7

In this push I've removed all the fuzzy() annotations that were added because I want to reexamine the results. We may be able to avoid the errors in other ways.

    
    

    
  
Various hacks around those failures. Looks like we don't need fuzzy() if we're a little bit creative in the test :-).
https://hg.mozilla.org/try/rev/cd73eb1f6362

    
    

    
  

      
      
      
      

        Attached patch
          
          
        main patch
        — 
          — Splinter Review
      

    


  
This is substantially the same as the patch I reviewed before (just with less test fuzzing and a few stylesheet changes) so I'm carrying forward the r+.

        Attachment #8360291 -
        Flags: review?(matspal) → review+

        Attachment #8360293 -
        Flags: review?(matspal) → review+

    
    

    
  
Comment on attachment 8360294 [details] [diff] [review]
Part 0.2: Relax heuristic used to detect 'monospace' font for <input> sizing so that it at least includes 'monospace' font-family in Windows 7

You need to update "charMaxAdvance != charWidth" in the comment too.

We have our own implementation of std::abs in mfbt/MathAlgorithms.h
for some reason I don't understand.  You should probably use that.

I'm not sure what the style guide would say about that indentation.
I would probably make charMaxAdvance and nsPresContext start at the
same offset (or +2) because they are part of the same expression.
Perhaps just move the whole std::max expression to its own
line though, in case it fits on one line there?

        Attachment #8360294 -
        Flags: review?(matspal) → review+

    
    

    
  
Another option is to drop the heuristic we have and instead do what the current whatwg spec says here, which is "(size-1)*avg + max".  If avg == max, that gives the same thing we have now; in the other case right now we produce size*avg + (max-4), so the difference would be avg-4.  I guess that's actually several pixels; I see an avg of 7-8 for my default text control font...

    
    

    
  
(In reply to comment #132)
> We have our own implementation of std::abs in mfbt/MathAlgorithms.h
> for some reason I don't understand.  You should probably use that.

<https://groups.google.com/forum/#!topic/mozilla.dev.platform/ikf9YVuPc4M>

    
    

    
  
(In reply to Boris Zbarsky [:bz] from comment #133)
> Another option is to drop the heuristic we have and instead do what the
> current whatwg spec says here, which is "(size-1)*avg + max".  If avg ==
> max, that gives the same thing we have now; in the other case right now we
> produce size*avg + (max-4), so the difference would be avg-4.  I guess
> that's actually several pixels; I see an avg of 7-8 for my default text
> control font...

That sounds like a good idea but it's a bigger change than I want to do in this bug.

    
    

    
  
Alex, Charly, thanks very much!

    
    

    
  
I'm not sure that the padding changes to textbox.plain were desirable; this class was designed to be used to create pseudo-labels whose content needs to be arbitrarily long yet copyable.

    
  
Depends on: 960664

    
    

    
  
(In reply to neil@parkwaycc.co.uk from comment #139)
> I'm not sure that the padding changes to textbox.plain were desirable; this
> class was designed to be used to create pseudo-labels whose content needs to
> be arbitrarily long yet copyable.

We can probably revert them then.

    
    

    
  


  
Robert, thanks for making this bugfix happen.

Neil, the extra padding on textbox.plain was needed to preserve Firefox's previous rendering pixel-for-pixel. If you want to remove it, that's fine, but removing it wasn't necessary to fix this bug: https://tbpl.mozilla.org/?tree=Try&rev=0f2f95a4fa98

If we don't remove the textbox.plain padding, we should add it to the faststripe theme too to be consistent.

        Attachment #8361367 -
        Flags: review?(roc)

    
    

    
  
Thank Alex for finishing this. Happy to see someone took the patch where I didn't and finally fixed this great work!
Thanks Robert and Jim for the help.

    
  

        Attachment #8360296 -
        Attachment is patch: true

    
    

    
  

      
      
      
      

        Attached image
          
        Missing padding
        — 
      

    


  
Good work on this! Though it's still not fixed completely. Two points are wrong:
1. There is no bottom padding on the text area, the div and the multi-select list.
2. The arrow of the drop-down list should not be affected by the padding. (See my mockup of comment 47)

Sebastian

    
    

    
  
(In reply to Sebastian Zartner from comment #144)
> Good work on this! Though it's still not fixed completely. Two points are
> wrong:
> 1. There is no bottom padding on the text area, the div and the multi-select
> list.

This is just the way padding works with scrollable elements in Firefox, IE and the CSS spec. Namely, 'overflow' doesn't affect the size or position of the padding box. So in your example, the content just overlaps the bottom padding.

> 2. The arrow of the drop-down list should not be affected by the padding.
> (See my mockup of comment 47)

Yeah maybe, but that's a different bug.

    
  
Keywords: checkin-needed

    
    

    
  
To clarify: I'm requesting checkin for attachment 8361367 [details] [diff] [review]

    
    

    
  
>> Good work on this! Though it's still not fixed completely. Two points are
>> wrong:
>> 1. There is no bottom padding on the text area, the div and the multi-select
>> list.

> This is just the way padding works with scrollable elements in Firefox, IE and the CSS spec. Namely, 
> 'overflow' doesn't affect the size or position of the padding box. So in your example, the content 
> just overlaps the bottom padding.

Well, if 'overflow' shouldn't affect the size of the padding box, why isn't the bottom padding there then while the top padding is? 
Blink and Webkit obviously work different here (and behave more expected from a web author's view, IMO).

>> 2. The arrow of the drop-down list should not be affected by the padding.
>> (See my mockup of comment 47)

> Yeah maybe, but that's a different bug.

Created bug 963970 to cover that.

Sebastian

    
    

    
  
Could these patches have caused bug 961347?

    
  
Depends on: 961347

    
    

    
  
(In reply to Sebastian Zartner from comment #147)
> >> Good work on this! Though it's still not fixed completely. Two points are
> >> wrong:
> >> 1. There is no bottom padding on the text area, the div and the multi-select
> >> list.
> 
> > This is just the way padding works with scrollable elements in Firefox, IE and the CSS spec. Namely, 
> > 'overflow' doesn't affect the size or position of the padding box. So in your example, the content 
> > just overlaps the bottom padding.
> 
> Well, if 'overflow' shouldn't affect the size of the padding box, why isn't
> the bottom padding there then while the top padding is? 

The bottom-padding *is* there, but the text overlaps it.

> Blink and Webkit obviously work different here (and behave more expected
> from a web author's view, IMO).

For some things, yes. But if you add a background image to the scrolled element, with "background-position:bottom; background-repeat:no-repeat; background-origin:padding-box", you'll see that Webkit/Blink is confused about where the padding-box really is.

There was a thread in www-style about this recently ("padding lost in overflow"). If you want to pursue this issue, please read that thread carefully and contribute there.

    
    

    
  
> The bottom-padding *is* there, but the text overlaps it.

> For some things, yes. But if you add a background image to the scrolled element, with "background-
> position:bottom; background-repeat:no-repeat; background-origin:padding-box", you'll see that 
> Webkit/Blink is confused about where the padding-box really is.

Obviously the only difference to the behavior of the other engines is that they don't let the text overlap the bottom-padding.

> There was a thread in www-style about this recently ("padding lost in overflow"). If you want to 
> pursue this issue, please read that thread carefully and contribute there.

Thanks for letting me know. I'll read through it.

Sebastian
Depends on: 965237
Depends on: 971955

    
  
Depends on: 997921
Depends on: 1012202
Depends on: 1014005

    
    

    
  
It appears that this issue has been prematurely marked as resolved. Although the the implementation of padding has improved, it is still inconsistent with the implementation of chrome, safari and IE.

What has been fixed is the relation of the padding compared to the scrollbars - the padding used to be located outside the scrollbars, between the scrollbars and the border, and has now been moved so that it correctly appears inside of the scrollbars.

However, what is still incorrect is the relation of the padding compared to the text when scrolling. In firefox, the text scrolls behind the padding, in all other browsers the text scrolls over the padding and disappears behind the edge/border of the textarea.

In short:

actual behaviour: text scrolls behind the padding
expected behaviour: text scrolls over the padding

You can easily see what I mean by comparing the behaviour of the textarea in which I am writing this comment in firefox and chrome - in chrome text disappears behind the border, in firefox you can see it disappear behind the padding. 

Alternatively, please see this jsFiddle which was kindly prepared by Paul Adam Davis: http://jsfiddle.net/af47whzf/1/

This is a particularly gnarly bug because there is no way to work around the issue without having to modify the textarea's height with JS. 

I am not sure if this is already tracked on another issue that I can't find (it is particularly hard to search for this issue, as talk about the original padding-outside-issue is so prevalent), or if I need to raise it as a separate new issue, or if this issue ought to be reopened as the fix is incomplete.
Flags: needinfo?(charly.molter)

    
    

    
  
File a new bug, please.
Flags: needinfo?(charly.molter)

    
  
See Also:  → 963970

    
    

    
  
(In reply to :Ms2ger (he/him; ⌚ UTC+1/+2) from comment #156)




File a new bug, please.




Hey you there, you that searched for "firefox textarea padding bug" or something similar.


Perhaps you even saw Hannah Wolfe reply above, and saw that the jsfiddle link in it perfectly showcased the issue you're experiencing right now on your textarea.


And perhaps you were also let down that it seemingly stopped there.


At least let me point you to that bug that actually was filed: https://bugzilla.mozilla.org/show_bug.cgi?id=1099204


The story continues.


See Also:  → 1099204

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: