Closed
Bug 157846
Opened 22 years ago
Closed 11 years ago
Incorrect implementation of padding on textarea elements (scrollbars/resizer wrongly positioned)
Categories
(Core :: Layout: Form Controls, defect)
Core
Layout: Form Controls
Tracking
()
RESOLVED
FIXED
mozilla29
Tracking | Status | |
---|---|---|
blocking2.0 | --- | - |
People
(Reporter: bugzilla, Assigned: lahabana)
References
()
Details
(6 keywords, Whiteboard: [parity-IE][parity-webkit][parity-Opera][polish-visual][post-2.0])
Attachments
(11 files, 14 obsolete files)
574 bytes,
text/html
|
Details | |
10.78 KB,
image/png
|
Details | |
1.33 KB,
text/html
|
Details | |
12.10 KB,
image/png
|
Details | |
24.02 KB,
image/png
|
Details | |
14.26 KB,
patch
|
MatsPalmgren_bugz
:
review+
|
Details | Diff | Splinter Review |
3.28 KB,
patch
|
MatsPalmgren_bugz
:
review+
|
Details | Diff | Splinter Review |
1.41 KB,
patch
|
MatsPalmgren_bugz
:
review+
|
Details | Diff | Splinter Review |
22.99 KB,
patch
|
Details | Diff | Splinter Review | |
2.50 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•22 years ago
|
||
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•22 years ago
|
||
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
Comment 3•22 years ago
|
||
-> jkeiser
Assignee: rods → jkeiser
Priority: -- → P3
Target Milestone: --- → Future
Comment 4•22 years ago
|
||
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•21 years ago
|
||
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•20 years ago
|
||
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.
Comment 8•17 years ago
|
||
*** 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•17 years ago
|
||
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?
Reporter | ||
Comment 10•17 years ago
|
||
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-
Flags: blocking1.9-
Updated•16 years ago
|
Assignee: john → nobody
Priority: P3 → --
QA Contact: tpreston → layout.form-controls
Target Milestone: Future → ---
Comment 12•16 years ago
|
||
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•16 years ago
|
||
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
Updated•15 years ago
|
OS: Windows XP → All
Hardware: x86 → All
Comment 14•15 years ago
|
||
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•15 years ago
|
||
bz: can you hint which files to look at?
Comment 16•15 years ago
|
||
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
Comment 17•15 years ago
|
||
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•15 years ago
|
||
(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•15 years ago
|
||
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•15 years ago
|
||
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•15 years ago
|
||
> 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•15 years ago
|
||
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•15 years ago
|
||
> I noticed that mozilla has a ::-moz-anonymous-block pseudo-element
That can only be used in a UA stylesheet.
Comment 24•15 years ago
|
||
damn!
Comment 25•15 years ago
|
||
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
Updated•15 years ago
|
Keywords: polish
Whiteboard: [parity-IE] [parity-webkit] [parity-Opera] → [parity-IE][parity-webkit][parity-Opera][polish-visual]
Comment 27•14 years ago
|
||
Yep, since 2002 still not fixed. Very strange.
http://translate.google.com/ now looks very ugly in Fx 4.0b.
Updated•14 years ago
|
Summary: Incorrect implementation of padding on textarea elements (scrollbars wrongly positioned) → Incorrect implementation of padding on textarea elements (scrollbars/resizer wrongly positioned)
Updated•14 years ago
|
blocking2.0: --- → ?
blocking2.0: ? → -
Comment 29•14 years ago
|
||
(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•14 years ago
|
||
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•14 years ago
|
||
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 33•14 years ago
|
||
(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.
Comment 35•14 years ago
|
||
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]
Comment 36•13 years ago
|
||
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•13 years ago
|
||
Ehsan, are you still planning to work on this?
Comment 39•13 years ago
|
||
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #37)
> Ehsan, are you still planning to work on this?
Not really.
Assignee: ehsan → nobody
Assignee | ||
Comment 41•12 years ago
|
||
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.
Assignee | ||
Comment 42•12 years ago
|
||
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)
Assignee | ||
Comment 43•12 years ago
|
||
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)
Updated•12 years ago
|
Assignee: nobody → bellot.zoe
Assignee | ||
Comment 44•12 years ago
|
||
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...
Assignee | ||
Comment 45•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
Just to make clear what's the expected display I created a mockup of how the test case should be rendered.
Sebastian
Assignee | ||
Comment 48•12 years ago
|
||
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
Assignee | ||
Comment 49•12 years ago
|
||
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 51•12 years ago
|
||
Comment on attachment 656008 [details] [diff] [review]
first patch with a problem when you type text
What roc said. ;)
Attachment #656008 -
Flags: feedback?(bzbarsky)
Assignee | ||
Comment 52•12 years ago
|
||
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)
Assignee | ||
Comment 53•12 years ago
|
||
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•12 years ago
|
||
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-
Assignee | ||
Comment 56•12 years ago
|
||
(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?
Assignee | ||
Comment 57•12 years ago
|
||
(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.
Assignee | ||
Comment 59•12 years ago
|
||
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?
Assignee | ||
Comment 61•12 years ago
|
||
(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.
Assignee | ||
Comment 63•12 years ago
|
||
: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
Assignee | ||
Comment 64•12 years ago
|
||
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.
Assignee | ||
Comment 66•12 years ago
|
||
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.
Comment 68•12 years ago
|
||
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...
Comment 70•12 years ago
|
||
Mounir was arguing against it in bug 737786 is the problem. :(
Comment 71•12 years ago
|
||
(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•12 years ago
|
||
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•12 years ago
|
||
(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•12 years ago
|
||
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.
Updated•12 years ago
|
Attachment #659729 -
Flags: review?(bzbarsky)
Comment 75•12 years ago
|
||
2002-07-16, today 2013-05-13 -_-
Reporter | ||
Updated•12 years ago
|
Comment 76•11 years ago
|
||
I hope this bug is fixed. Current behavior is really unexpected...
Comment 77•11 years ago
|
||
11 years and counting. That must be one helluva bug. Come on Mozilla. This needs to be addressed for crying out loud.
Comment 79•11 years ago
|
||
Comment 80•11 years ago
|
||
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.
Reporter | ||
Comment 81•11 years ago
|
||
(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•11 years ago
|
||
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.
Reporter | ||
Comment 83•11 years ago
|
||
> 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•11 years ago
|
||
...
> 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•11 years ago
|
||
Any chance for fix?
Comment 86•11 years ago
|
||
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)
Comment 87•11 years ago
|
||
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-
Comment 89•11 years ago
|
||
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">.
Comment 92•11 years ago
|
||
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?
Updated•11 years ago
|
Keywords: checkin-needed
Keywords: checkin-needed
Comment on attachment 8351592 [details] [diff] [review]
Proposed patch (try 3)
Review of attachment 8351592 [details] [diff] [review]:
-----------------------------------------------------------------
Lovely!
Attachment #8351592 -
Flags: checkin? → checkin+
Comment 96•11 years ago
|
||
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/2a09a9842202 for mochitest (https://tbpl.mozilla.org/php/getParsedLog.php?id=32408791&tree=Mozilla-Inbound) and reftest (https://tbpl.mozilla.org/php/getParsedLog.php?id=32409000&tree=Mozilla-Inbound) and Windows mochitest-chrome (https://tbpl.mozilla.org/php/getParsedLog.php?id=32408958&tree=Mozilla-Inbound) failures.
Oops, should have done a try run. For some reason I thought we had.
Some of the Windows tests show definite positioning issues:
https://tbpl.mozilla.org/?rev=7273dbeaeb88&tree=Mozilla-Inbound
Comment 99•11 years ago
|
||
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
Comment 100•11 years ago
|
||
Flagging roc to either help or redirect to someone else.
Flags: needinfo?(roc)
I'll try to help.
Flags: needinfo?(roc)
Comment 102•11 years ago
|
||
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...
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.
Trying a new patch with a fix for that: https://tbpl.mozilla.org/?tree=Try&rev=d8a555c7062a
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)
Depends on: 958999
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•11 years ago
|
||
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.
Comment 117•11 years ago
|
||
(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)
Comment 118•11 years ago
|
||
(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•11 years ago
|
||
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!
Attachment #8357525 -
Attachment is obsolete: true
Attachment #8358059 -
Attachment is obsolete: true
Updated with some Mac-related fixes:
https://tbpl.mozilla.org/?tree=Try&rev=2e2032b6ff70
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
Fix stupid error: https://tbpl.mozilla.org/?tree=Try&rev=035cfc4ca73d
Green!
Attachment #8359592 -
Attachment is obsolete: true
Attachment #8360291 -
Flags: review?(matspal)
Attachment #8360293 -
Flags: review?(matspal)
Attachment #8360294 -
Flags: review?(matspal)
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+.
Updated•11 years ago
|
Attachment #8360291 -
Flags: review?(matspal) → review+
Updated•11 years ago
|
Attachment #8360293 -
Flags: review?(matspal) → review+
Comment 132•11 years ago
|
||
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+
Comment 133•11 years ago
|
||
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•11 years ago
|
||
(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!
Comment 139•11 years ago
|
||
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 140•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/259c34b488f7
https://hg.mozilla.org/mozilla-central/rev/f94ada7507f4
https://hg.mozilla.org/mozilla-central/rev/358f4a9aac18
https://hg.mozilla.org/mozilla-central/rev/36e4fcbd07d3
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
(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•11 years ago
|
||
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)
Attachment #8361367 -
Flags: review?(roc) → review+
Assignee | ||
Comment 143•11 years ago
|
||
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.
Updated•11 years ago
|
Attachment #8360296 -
Attachment is patch: true
Comment 144•11 years ago
|
||
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.
Updated•11 years ago
|
Keywords: checkin-needed
Comment 146•11 years ago
|
||
To clarify: I'm requesting checkin for attachment 8361367 [details] [diff] [review]
Comment 147•11 years ago
|
||
>> 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•11 years ago
|
||
Keywords: checkin-needed
Comment 149•11 years ago
|
||
Comment 150•11 years ago
|
||
Could these patches have caused bug 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.
Comment 152•11 years ago
|
||
> 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 154•11 years ago
|
||
This has somehow slipped under my radar... Added to the site compat doc, tracker and timelines.
https://developer.mozilla.org/en-US/Firefox/Releases/29/Site_Compatibility#CSS
https://docs.google.com/spreadsheets/d/1048AVpvbP3O2atsV3i--xLpHCr4PR7TGXyFtgMrvt6Q/edit
https://twitter.com/FxSiteCompat/status/467499340408160256
https://www.facebook.com/FxSiteCompat/posts/642095355882307
https://plus.google.com/115208555324773750121/posts/hFZh5qREoMT
Keywords: dev-doc-complete,
site-compat
Comment 155•10 years ago
|
||
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)
Comment 157•4 years ago
|
||
(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.
You need to log in
before you can comment on or make changes to this bug.
Description
•