Last Comment Bug 157846 - Incorrect implementation of padding on textarea elements (scrollbars/resizer wrongly positioned)
: Incorrect implementation of padding on textarea elements (scrollbars/resizer ...
Status: RESOLVED FIXED
[parity-IE][parity-webkit][parity-Ope...
: css2, dev-doc-complete, helpwanted, polish, site-compat, testcase
Product: Core
Classification: Components
Component: Layout: Form Controls (show other bugs)
: Trunk
: All All
: -- normal with 63 votes (vote)
: mozilla29
Assigned To: Charly Molter :lahabana
:
Mentors:
http://www.w3.org/TR/CSS2/visufx.html...
: 451574 484257 576956 585107 646530 712128 712605 900719 (view as bug list)
Depends on: 958999 1012202 716875 960664 961347 965237 971955 997921 1014005
Blocks: 576976
  Show dependency treegraph
 
Reported: 2002-07-16 20:16 PDT by Gérard Talbot
Modified: 2015-07-16 23:58 PDT (History)
60 users (show)
roc: blocking1.9-
roc: blocking1.9-
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
Self-explanatory test case (1.90 KB, text/html)
2002-07-16 20:20 PDT, Gérard Talbot
no flags Details
Div with same style (752 bytes, text/html)
2003-01-31 23:59 PST, John Keiser (jkeiser)
no flags Details
testcase (287 bytes, text/html)
2010-01-08 12:59 PST, Emil Ivanov
no flags Details
Testcase: div, textarea, select (574 bytes, text/html)
2010-01-10 02:33 PST, Rimas Kudelis
no flags Details
Patch that pass all tests (20.98 KB, patch)
2012-06-14 01:55 PDT, Charly Molter :lahabana
no flags Details | Diff | Splinter Review
Expected result of the test case (10.78 KB, image/png)
2012-07-11 22:38 PDT, Sebastian Zartner [:sebo]
no flags Details
first patch with a problem when you type text (3.02 KB, patch)
2012-08-28 08:20 PDT, Charly Molter :lahabana
no flags Details | Diff | Splinter Review
Prepatch that seems to work for inputs (3.94 KB, patch)
2012-08-29 07:32 PDT, Charly Molter :lahabana
bzbarsky: feedback+
roc: feedback-
Details | Diff | Splinter Review
future reftests that cause a bug (1.33 KB, text/html)
2012-08-29 08:11 PDT, Charly Molter :lahabana
no flags Details
screenshot of the reftest (12.10 KB, image/png)
2012-09-05 02:14 PDT, Charly Molter :lahabana
no flags Details
patch with reftests but still some fail in try (9.95 KB, patch)
2012-09-05 07:02 PDT, Charly Molter :lahabana
no flags Details | Diff | Splinter Review
corrected availSize (10.52 KB, patch)
2012-09-10 08:51 PDT, Charly Molter :lahabana
no flags Details | Diff | Splinter Review
Comparison between Firefox and other browsers (24.02 KB, image/png)
2013-08-01 16:24 PDT, kaldari
no flags Details
Proposed patch (13.75 KB, patch)
2013-12-25 23:04 PST, Alex Henrie
roc: review-
Details | Diff | Splinter Review
Proposed patch (try 2) (15.81 KB, patch)
2013-12-26 20:17 PST, Alex Henrie
roc: review-
Details | Diff | Splinter Review
Proposed patch (try 3) (15.94 KB, patch)
2013-12-27 17:02 PST, Alex Henrie
roc: review+
roc: checkin+
Details | Diff | Splinter Review
Proposed patch (try 4) (24.84 KB, patch)
2014-01-08 18:14 PST, Alex Henrie
no flags Details | Diff | Splinter Review
fix v5 (25.04 KB, patch)
2014-01-09 15:43 PST, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details | Diff | Splinter Review
latest patch (29.13 KB, patch)
2014-01-13 20:22 PST, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details | Diff | Splinter Review
Part 0: Make tests more robust to padding changes and clipping of overflowing glyph edges (14.26 KB, patch)
2014-01-15 01:51 PST, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
mats: 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)
2014-01-15 01:52 PST, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
mats: 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)
2014-01-15 01:53 PST, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review
main patch (22.99 KB, patch)
2014-01-15 01:56 PST, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details | Diff | Splinter Review
Remove padding from textbox.plain (2.50 KB, patch)
2014-01-16 15:53 PST, Alex Henrie
roc: review+
Details | Diff | Splinter Review
Missing padding (16.13 KB, image/png)
2014-01-18 17:11 PST, Sebastian Zartner [:sebo]
no flags Details

Description Gérard Talbot 2002-07-16 20:16:52 PDT
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.
Comment 1 Gérard Talbot 2002-07-16 20:20:05 PDT
Created attachment 91591 [details]
Self-explanatory test case

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.
Comment 2 Michael Lefevre 2002-12-03 09:45:27 PST
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.
Comment 3 Kevin McCluskey (gone) 2003-01-22 15:35:45 PST
-> jkeiser
Comment 4 John Keiser (jkeiser) 2003-01-31 23:59:11 PST
Created attachment 113283 [details]
Div with same style

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.
Comment 5 Brian Earley 2003-12-19 16:28:25 PST
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
Comment 6 Morgan Roderick 2004-06-30 06:07:05 PDT
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.
Comment 7 Rex Gozar 2007-09-18 14:16:54 PDT
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.
Comment 8 James Barrante 2007-12-30 06:17:20 PST
*** 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
Comment 9 Andrés Delfino 2008-02-21 09:39:54 PST
Given that Firefox 3 is going to support off-line Web applications, I think form design should be as polished as possible.
Comment 10 Gérard Talbot 2008-02-21 13:28:33 PST
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
Comment 11 Ojan Vafai 2009-03-20 00:06:47 PDT
*** Bug 484257 has been marked as a duplicate of this bug. ***
Comment 12 Ojan Vafai 2009-03-20 12:08:51 PDT
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.
Comment 13 Boris Zbarsky [:bz] 2009-03-20 12:18:42 PDT
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.
Comment 14 Emil Ivanov 2010-01-08 12:59:08 PST
Created attachment 420802 [details]
testcase

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.
Comment 15 Rimas Kudelis 2010-01-10 02:21:53 PST
bz: can you hint which files to look at?
Comment 16 Rimas Kudelis 2010-01-10 02:33:25 PST
Created attachment 420934 [details]
Testcase: div, textarea, select

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?
Comment 17 Boris Zbarsky [:bz] 2010-01-10 08:21:02 PST
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).
Comment 18 Rimas Kudelis 2010-01-10 09:31:47 PST
(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. ;)
Comment 19 Boris Zbarsky [:bz] 2010-01-10 14:35:06 PST
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.  ;)
Comment 20 Rimas Kudelis 2010-01-11 00:26:35 PST
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...
Comment 21 Boris Zbarsky [:bz] 2010-01-11 05:02:17 PST
> 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.
Comment 22 Rimas Kudelis 2010-02-03 06:53:10 PST
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
Comment 23 Boris Zbarsky [:bz] 2010-02-03 08:15:26 PST
> I noticed that mozilla has a ::-moz-anonymous-block pseudo-element

That can only be used in a UA stylesheet.
Comment 24 Rimas Kudelis 2010-02-03 12:24:22 PST
damn!
Comment 25 jacko310592 2010-05-13 15:09:29 PDT
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
Comment 26 Neil Deakin 2010-08-08 10:10:54 PDT
*** Bug 585107 has been marked as a duplicate of this bug. ***
Comment 27 Oleg 2010-08-12 03:21:15 PDT
Yep, since 2002 still not fixed. Very strange.
http://translate.google.com/ now looks very ugly in Fx 4.0b.
Comment 28 Kevin Brosnan 2010-08-19 10:24:30 PDT
*** Bug 576956 has been marked as a duplicate of this bug. ***
Comment 29 Rimas Kudelis 2010-12-12 09:38:17 PST
(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.
Comment 30 Sebastian Zartner 2011-01-28 05:12:34 PST
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.
Comment 31 Cork 2011-01-28 08:58:42 PST
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.
Comment 32 Mats Palmgren (vacation) 2011-03-30 13:19:44 PDT
*** Bug 646530 has been marked as a duplicate of this bug. ***
Comment 33 Mounir Lamouri (:mounir) 2011-03-31 03:32:31 PDT
(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?
Comment 34 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-03-31 14:14:47 PDT
Making nsTextControlFrame not be an nsStackFrame would make the code nicer IMHO.
Comment 35 :Ehsan Akhgari 2011-03-31 20:27:09 PDT
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!)
Comment 36 Lea Verou 2011-11-29 19:16:35 PST
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.
Comment 37 Mounir Lamouri (:mounir) 2011-12-01 02:58:15 PST
Ehsan, are you still planning to work on this?
Comment 38 John P Baker 2011-12-21 05:25:53 PST
*** Bug 712605 has been marked as a duplicate of this bug. ***
Comment 39 :Ehsan Akhgari 2012-01-03 15:58:12 PST
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #37)
> Ehsan, are you still planning to work on this?

Not really.
Comment 40 :Ehsan Akhgari 2012-01-03 16:10:07 PST
*** Bug 712128 has been marked as a duplicate of this bug. ***
Comment 41 Charly Molter :lahabana 2012-06-07 07:40:45 PDT
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.
Comment 42 Charly Molter :lahabana 2012-06-14 01:55:29 PDT
Created attachment 633068 [details] [diff] [review]
Patch that pass all tests

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
Comment 43 Charly Molter :lahabana 2012-06-14 02:02:21 PDT
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
Comment 44 Charly Molter :lahabana 2012-06-20 08:18:51 PDT
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...
Comment 45 Charly Molter :lahabana 2012-06-22 06:20:22 PDT
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?
Comment 46 Boris Zbarsky [:bz] 2012-06-22 13:34:20 PDT
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.
Comment 47 Sebastian Zartner [:sebo] 2012-07-11 22:38:25 PDT
Created attachment 641363 [details]
Expected result of the test case

Just to make clear what's the expected display I created a mockup of how the test case should be rendered.

Sebastian
Comment 48 Charly Molter :lahabana 2012-08-15 05:43:22 PDT
Just so you know I'm still on that I just add a more important regression bug to fix first.
Comment 49 Charly Molter :lahabana 2012-08-28 08:20:03 PDT
Created attachment 656008 [details] [diff] [review]
first patch with a problem when you type text

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?
Comment 50 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-28 19:36:06 PDT
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 51 Boris Zbarsky [:bz] 2012-08-28 23:11:14 PDT
Comment on attachment 656008 [details] [diff] [review]
first patch with a problem when you type text

What roc said.  ;)
Comment 52 Charly Molter :lahabana 2012-08-29 07:32:32 PDT
Created attachment 656437 [details] [diff] [review]
Prepatch that seems to work for inputs

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!
Comment 53 Charly Molter :lahabana 2012-08-29 08:11:58 PDT
Created attachment 656455 [details]
future reftests that cause a bug

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 54 Boris Zbarsky [:bz] 2012-08-29 13:41:03 PDT
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.
Comment 55 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-29 15:01:11 PDT
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>?
Comment 56 Charly Molter :lahabana 2012-08-30 02:50:57 PDT
(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?
Comment 57 Charly Molter :lahabana 2012-08-30 04:07:05 PDT
(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
Comment 58 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-30 17:20:25 PDT
(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.
Comment 59 Charly Molter :lahabana 2012-09-03 07:35:40 PDT
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?
Comment 60 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-03 23:02:52 PDT
(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?
Comment 61 Charly Molter :lahabana 2012-09-04 00:59:31 PDT
(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?
Comment 62 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-04 21:33:41 PDT
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.
Comment 63 Charly Molter :lahabana 2012-09-05 02:14:42 PDT
Created attachment 658419 [details]
screenshot of the reftest

: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
Comment 64 Charly Molter :lahabana 2012-09-05 07:02:09 PDT
Created attachment 658480 [details] [diff] [review]
patch with reftests but still some fail in try

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
Comment 65 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-05 16:52:47 PDT
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.
Comment 66 Charly Molter :lahabana 2012-09-10 08:51:04 PDT
Created attachment 659729 [details] [diff] [review]
corrected availSize

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?
Comment 67 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-10 15:03:09 PDT
(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.
Comment 68 Boris Zbarsky [:bz] 2012-10-12 19:34:08 PDT
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....
Comment 69 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-10-14 20:25:56 PDT
(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...
Comment 70 Boris Zbarsky [:bz] 2012-10-14 21:04:21 PDT
Mounir was arguing against it in bug 737786 is the problem.  :(
Comment 71 Mounir Lamouri (:mounir) 2012-10-16 04:20:35 PDT
(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.
Comment 72 info 2012-11-08 05:49:58 PST
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!
Comment 73 Lea Verou 2012-11-08 06:42:55 PST
(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.
Comment 74 Boris Zbarsky [:bz] 2012-11-08 08:44:53 PST
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.
Comment 75 belyanskii 2013-05-13 00:51:33 PDT
2002-07-16, today 2013-05-13 -_-
Comment 76 Iñaki Baz Castillo 2013-06-04 04:56:26 PDT
I hope this bug is fixed. Current behavior is really unexpected...
Comment 77 Domenic Polsoni 2013-06-11 09:07:08 PDT
11 years and counting.  That must be one helluva bug.  Come on Mozilla.  This needs to be addressed for crying out loud.
Comment 78 kaldari 2013-08-01 16:22:50 PDT
*** Bug 900719 has been marked as a duplicate of this bug. ***
Comment 79 kaldari 2013-08-01 16:24:48 PDT
Created attachment 784647 [details]
Comparison between Firefox and other browsers
Comment 80 kaldari 2013-11-06 10:52:36 PST
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.
Comment 81 Gérard Talbot 2013-11-06 13:54:42 PST
(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
Comment 82 Alex Henrie 2013-11-06 14:15:21 PST
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.
Comment 83 Gérard Talbot 2013-11-06 17:26:11 PST
> 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
Comment 84 Vedran Aberle Tokić 2013-11-07 01:32:55 PST
...
> 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
Comment 85 Xerol 2013-11-19 20:13:45 PST
Any chance for fix?
Comment 86 Alex Henrie 2013-12-25 23:04:13 PST
Created attachment 8351471 [details] [diff] [review]
Proposed patch

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.
Comment 87 Alex Henrie 2013-12-25 23:05:23 PST
Merry Christmas and Happy New Year!
Comment 88 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-12-26 00:59:06 PST
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"
Comment 89 Alex Henrie 2013-12-26 20:17:38 PST
Created attachment 8351539 [details] [diff] [review]
Proposed patch (try 2)

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.
Comment 90 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-12-27 13:43:00 PST
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 :-).
Comment 91 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-12-27 13:52:54 PST
(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">.
Comment 92 Alex Henrie 2013-12-27 17:02:41 PST
Created attachment 8351592 [details] [diff] [review]
Proposed patch (try 3)

Done.
Comment 93 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-12-27 17:30:12 PST
Comment on attachment 8351592 [details] [diff] [review]
Proposed patch (try 3)

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

Lovely!
Comment 94 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-12-28 03:04:11 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/7273dbeaeb88
Comment 95 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-12-28 03:04:28 PST
Comment on attachment 8351592 [details] [diff] [review]
Proposed patch (try 3)

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

Lovely!
Comment 97 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-12-29 02:23:07 PST
Oops, should have done a try run. For some reason I thought we had.
Comment 98 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-12-29 02:25:19 PST
Some of the Windows tests show definite positioning issues:
https://tbpl.mozilla.org/?rev=7273dbeaeb88&tree=Mozilla-Inbound
Comment 99 Alex Henrie 2014-01-08 18:14:12 PST
Created attachment 8357525 [details] [diff] [review]
Proposed patch (try 4)

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.
Comment 100 Josh Matthews [:jdm] 2014-01-08 20:51:08 PST
Flagging roc to either help or redirect to someone else.
Comment 101 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2014-01-09 02:23:09 PST
I'll try to help.
Comment 102 Vedran Aberle Tokić 2014-01-09 02:58:26 PST
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
Comment 103 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2014-01-09 15:29:22 PST
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).
Comment 104 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2014-01-09 15:35:05 PST
Fortunately we have one weird trick to handle exactly that situation...
Comment 105 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2014-01-09 15:43:15 PST
Created attachment 8358059 [details] [diff] [review]
fix v5

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.
Comment 106 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2014-01-09 15:45:41 PST
https://tbpl.mozilla.org/?tree=Try&rev=069d4369e0a6
Comment 107 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2014-01-09 18:02:26 PST
More bugs to look into tomorrow :-)
Comment 108 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2014-01-10 01:59:22 PST
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.
Comment 109 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2014-01-10 02:44:31 PST
Trying a new patch with a fix for that: https://tbpl.mozilla.org/?tree=Try&rev=d8a555c7062a
Comment 110 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2014-01-10 10:36:52 PST
Getting closer, but still significant failures.
Comment 111 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2014-01-11 03:46:08 PST
Looks like intrinsic size calculation is a little off.
Comment 112 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2014-01-11 04:03:36 PST
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.
Comment 113 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2014-01-12 11:50:56 PST
Jim, the only patch you need is here: https://hg.mozilla.org/try/rev/dbd693da9fbe
Comment 114 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2014-01-12 15:22:37 PST
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
Comment 115 Alex Henrie 2014-01-12 22:56:44 PST
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.
Comment 116 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2014-01-13 05:03:58 PST
(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.
Comment 117 Jim Mathies [:jimm] 2014-01-13 05:18:52 PST
(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.
Comment 118 Alex Henrie 2014-01-13 11:04:39 PST
(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?
Comment 119 Alex Henrie 2014-01-13 11:22:35 PST
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.
Comment 120 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2014-01-13 12:16:52 PST
(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.
Comment 121 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2014-01-13 20:19:28 PST
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!
Comment 122 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2014-01-13 20:22:53 PST
Created attachment 8359592 [details] [diff] [review]
latest patch
Comment 123 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2014-01-13 21:37:25 PST
Updated with some Mac-related fixes:
https://tbpl.mozilla.org/?tree=Try&rev=2e2032b6ff70
Comment 124 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2014-01-14 02:40:48 PST
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.
Comment 125 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2014-01-14 18:17:58 PST
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
Comment 126 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2014-01-14 21:16:05 PST
Fix stupid error: https://tbpl.mozilla.org/?tree=Try&rev=035cfc4ca73d
Comment 127 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2014-01-15 00:10:46 PST
Green!
Comment 128 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2014-01-15 01:51:49 PST
Created attachment 8360291 [details] [diff] [review]
Part 0: Make tests more robust to padding changes and clipping of overflowing glyph edges
Comment 129 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2014-01-15 01:52:56 PST
Created attachment 8360293 [details] [diff] [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
Comment 130 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2014-01-15 01:53:22 PST
Created 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
Comment 131 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2014-01-15 01:56:53 PST
Created attachment 8360296 [details] [diff] [review]
main patch

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+.
Comment 132 Mats Palmgren (vacation) 2014-01-15 04:48:51 PST
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?
Comment 133 Boris Zbarsky [:bz] 2014-01-15 07:29:02 PST
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...
Comment 134 :Ehsan Akhgari 2014-01-15 07:48:53 PST
(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>
Comment 135 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2014-01-15 14:43:17 PST
(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.
Comment 137 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2014-01-15 19:28:17 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/36e4fcbd07d3
Comment 138 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2014-01-15 20:57:29 PST
Alex, Charly, thanks very much!
Comment 139 neil@parkwaycc.co.uk 2014-01-16 00:49:56 PST
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.
Comment 141 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2014-01-16 12:40:22 PST
(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.
Comment 142 Alex Henrie 2014-01-16 15:53:02 PST
Created attachment 8361367 [details] [diff] [review]
Remove padding from textbox.plain

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.
Comment 143 Charly Molter :lahabana 2014-01-17 01:12:53 PST
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.
Comment 144 Sebastian Zartner [:sebo] 2014-01-18 17:11:55 PST
Created attachment 8362169 [details]
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
Comment 145 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2014-01-24 03:13:08 PST
(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.
Comment 146 Alex Henrie 2014-01-24 12:05:37 PST
To clarify: I'm requesting checkin for attachment 8361367 [details] [diff] [review]
Comment 147 Sebastian Zartner [:sebo] 2014-01-25 13:29:57 PST
>> 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
Comment 148 Ryan VanderMeulen [:RyanVM] 2014-01-27 05:23:01 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/f33ac356cd85
Comment 149 Ryan VanderMeulen [:RyanVM] 2014-01-27 12:08:58 PST
https://hg.mozilla.org/mozilla-central/rev/f33ac356cd85
Comment 150 Christian Ascheberg 2014-01-27 14:08:26 PST
Could these patches have caused bug 961347?
Comment 151 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2014-01-27 19:00:04 PST
(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.
Comment 152 Sebastian Zartner [:sebo] 2014-01-27 23:21:33 PST
> 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
Comment 153 Alex Henrie 2014-02-13 12:50:53 PST
*** Bug 451574 has been marked as a duplicate of this bug. ***
Comment 155 Hannah Wolfe 2014-11-14 07:56:25 PST
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.
Comment 156 :Ms2ger 2014-11-14 07:58:04 PST
File a new bug, please.

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