Last Comment Bug 40828 - cellpadding disappears when changing style with dom, and when changing text size
: cellpadding disappears when changing style with dom, and when changing text size
Status: VERIFIED WORKSFORME
[rtm-] fix-in-hand a=buster, r=dcone
: css1, testcase
Product: Core
Classification: Components
Component: Layout: Tables (show other bugs)
: Trunk
: x86 All
: P2 normal with 1 vote (vote)
: M18
Assigned To: karnaze (gone)
: Amarendra Hanumanula
:
Mentors:
http://www.mozillazine.org/
: 54118 56725 59625 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2000-05-27 11:19 PDT by Péter Bajusz
Modified: 2001-06-04 17:14 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (120 bytes, text/html)
2000-05-27 11:21 PDT, Péter Bajusz
no flags Details
patch to fix the bug (1.58 KB, patch)
2000-06-05 18:12 PDT, karnaze (gone)
no flags Details | Diff | Splinter Review
lose cellpadding when setting absolutely positioned table to visible (642 bytes, text/html)
2000-09-21 19:29 PDT, Jesse Ruderman
no flags Details
more up to date patch (1.41 KB, patch)
2000-10-05 20:28 PDT, karnaze (gone)
no flags Details | Diff | Splinter Review
This bug also manifests if you change the display property under some circumstances, as in this attachment. I sure hope a fix is in 6.01, 'cause my page-in-development looks pretty bad in Mozilla at the moment. (1.92 KB, text/html)
2000-11-07 12:17 PST, jsp
no flags Details

Description Péter Bajusz 2000-05-27 11:19:41 PDT
Version tested:
2000-05-26-20 on Win98SE

Reproduction:
* Load the attached testcase
it contains a table with only one cell
* Change the textsize with any of the following:
View/Enlarge text size, View/Reduce text size, Ctrl+MouseWheel

What happens (and shouldn't):
The cellpadding disappears from the table.

Note:
Changing back the fontsize doesn't gives back the cellpadding.
Comment 1 Péter Bajusz 2000-05-27 11:21:18 PDT
Created attachment 9243 [details]
Testcase
Comment 2 karnaze (gone) 2000-06-05 18:12:45 PDT
Created attachment 9669 [details] [diff] [review]
patch to fix the bug
Comment 3 Jesse Ruderman 2000-09-21 19:29:07 PDT
I ran across a similar problem with cellpadding disappearing while I was 
splitting bug 51037: an position:absolute table will lose implicit or explicit 
cellpadding when its style.visible is set to "visible".  Does the patch fix 
this problem as well?
Comment 4 Jesse Ruderman 2000-09-21 19:29:51 PDT
Created attachment 15273 [details]
lose cellpadding when setting absolutely positioned table to visible
Comment 5 karnaze (gone) 2000-10-05 20:28:06 PDT
Created attachment 16298 [details] [diff] [review]
more up to date patch
Comment 6 karnaze (gone) 2000-10-05 20:28:42 PDT
*** Bug 54118 has been marked as a duplicate of this bug. ***
Comment 7 karnaze (gone) 2000-10-05 20:33:19 PDT
Adding keywords from bug 54118 and marking pdt+ (the code reviews should happen 
before PDT sees this). 

PDT, this is low hanging fruit, the fix has been around quite a while, and Ian 
thinks it is very important.
Comment 8 buster 2000-10-05 21:16:28 PDT
I don't think that you are 100% guaranteed that kidFrame ISA nsTableCellFrame.
For example, nsTableRowFrame has code that guards against the possibility that
it is not:

nsTableRowFrame::CalcTallestCell()
{
  [snip]
  for (nsIFrame* kidFrame = mFrames.FirstChild(); kidFrame;
       kidFrame->GetNextSibling(&kidFrame)) {
    nsCOMPtr<nsIAtom> frameType;
    kidFrame->GetFrameType(getter_AddRefs(frameType));
    if (nsLayoutAtoms::tableCellFrame == frameType.get())
    {
      nscoord availWidth = ((nsTableCellFrame *)kidFrame)->GetPriorAvailWidth();
      etc....

You need to add this kind of check before you do the cast.
Comment 9 karnaze (gone) 2000-10-05 21:43:28 PDT
Buster, you can't see it in the patch, but the new code is actually inside an if 
statement that checks the frame type.
Comment 10 buster 2000-10-05 21:53:43 PDT
in that case, a=buster
Comment 11 Phil Peterson 2000-10-06 16:25:47 PDT
PDT marking [rtm need info] since only one reviewer is listed. Please mark
[rtm+] after peer review.
Comment 12 rods (gone) 2000-10-09 04:55:50 PDT
I tried out the patch and it worked fine r=rods
Comment 13 dcone (gone) 2000-10-09 07:07:58 PDT
Looks good to me.  r=dcone
Comment 14 karnaze (gone) 2000-10-09 07:57:12 PDT
Marking rtm+ again.
Comment 15 selmer (gone) 2000-10-10 17:08:29 PDT
rtm-, wouldn't pull it off the wire for this.
Comment 16 rob cherny 2000-10-24 20:10:26 PDT
this strikes me as pretty serious.

#1 - if the user changes their text size the tables lose their padding
#2 - if a script sets visibility on a table (and how DHTML/Layout work isn't 
ina table these days? CSS isn't that widely supported, lets be realistic) then 
the cellpadding is lost.
#3 - if a previously hidden table is shown the table padding is lost.
#4 - if you animate the table (slide it accross the screen or anything) the 
padding is lost. 

bug 56725 *is* looking like a dup ....

i'm confused by "wouldn't pull it off the wire" .... please help a (realively 
speaking) newbie to mozilla out ... ok, so it doesn't crash the browser but it 
does render a VERY high number of any DHTML/DOM scripting painful to the nth 
degree. the only work around i can isolate is toggling the display property, 
but if the item is being animated, you simply can't do that.

this has the potential to impact other things. if the table has lost it's 
padding, the dimensions and the positioning could (as reflected in the scripts) 
be impacted as well.  this can break all kinds of DHTML code.

is it really too late for RTM on this?

Some New Testcases demonstrating all these behaviors are here: 
http://www.cherny.com/mozilla/padding/

This seems like a big deal to my little mind.
Comment 17 karnaze (gone) 2000-10-27 07:20:27 PDT
Changing rtm- to rtm+ due to low risk. Sorry PDT, but my conscience is bothering 
me. Our simple minds are happy to ship a product, while web developers want 
their pages to look right.
Comment 18 Phil Peterson 2000-10-27 15:47:02 PDT
PDT still thinks this is rtm- given the existing test cases. If we had an
example of a frequently visited web page which was severely damaged by this bug,
we might reconsider.
Comment 19 rob cherny 2000-10-28 06:54:43 PDT
>> If we had an example of a frequently visited web page which was severely
>> damaged by this bug, we might reconsider.

Yes, this is bordering on a flame ... My apologies ... it's early and I'm tired.
 But the comment above is bothersome ... and I just spent almost an entire day
explaining to all my coworkers in a presentation how "DHTML IS ALIVE AND WELL IN
MOZILLA" ... well maybe not.

#1 - There *WILL NEVER BE PAGES THAT ARE FREQUENTLY VISITED WITH DHTML* if
Netscape keeps putting out entirely TOO BUGGY A PRODUCT FOR IT TO BE DEPENDED
ON.  So your comment that we need examples of frequently visited pages is a weak
arguement. #2 - There are VERY FEW IF ANY DHTML pages that work in Mozilla yet
anyway because people don't design for beta browsers ... how can that be made an
argument with a technology that isn't supported effectively yet?  So again,
that's a weak argument.

Mozilla WAS SUPPOSED TO BE that solution.

You've been able to effectively do DHTML in INTERNET EXPLORER SINCE 1997!!!.

Developers (I'm one, so are all my coworkers and they can't believe this) can't
build with it cuz both browsers don't effectively support it.  When we do, we
add days and hours to any project just to get it to work in a Netscape Browser.

Mozilla WAS SEEN as our last hope ... now I'm losing confidence.  This was the
Web Standards Project's and developers AROUND THE WORLD'S challenge: Put out a
browser that is as bug free as possible, standards compliant and the behavior of
which can be depended on instead of having broken implementations of certain
features.  Guess what? You're not doing it by not fixing things like this (I
realize it will be fixed later ... but upon the FIRST RELEASE after TWO years is
when reviewers and developers will be watching, NOT 6.01 - they won't care about
patches).  More and more recently in the decisions of what gets fixed by when
I'm seeing the corporate influence of a release date that borders on a Microsoft
like decision -- let the people who use the product be our beta testers, never
mind that it is a release version ... we'll issue a service patch anyway.  So
make life harder for developers, they'll have to branch their scripts again and
do a detection for version 6.0 vs. 6.01 ... [exasperated sigh]

Web builders build pages and we're constantly having to explain to our project
managers how so much of our time is spent working on work arounds and kludges to
thing that should just work.  Mozilla.org CONSTANTLY argues how they want
developers to start using Standards complaint HTML 4 and yet (in this case,
which is pretty hypocritical) we'll be forced to use spacers and the like just
to get a layout to stick if we want to make a table dynamic in *ANY WAY* (change
font sizes, animate, or hide it) ... when there may not even be room for a
spacer (and I haven't even checked if that will work to hold the spacing).

The basics of this are:  Here we have a DOM Feature, which is great to see it
implemented, essentially breaking an HTML 3.2 feature (cellpadding).  If that
isn't a regression, I don't know what is.

Think about it - the TABLE, like it or not, is the building block of every
designer/developers page out there.  This would include any positioned element
(because Netscape 4.x still won't do padding/borders right) and now if we try to
animate it, show it, hide it, or do ANYTHING to it, the padding is destroyed?
With a fix in hand?

Forgive me for my passion here, after 4 years of web building I'm just growing
tired of all the work arounds ... I hope I haven't offended anyone, I know
you're all busy, I feel like this is a stunning example of what's been wrong
with web browsers for a very long time.  And it won't be fixed for release.

Comment 20 Henri Sivonen (:hsivonen) (Not reading bugmail or doing reviews until 2016-10-03) 2000-10-29 13:57:40 PST
It might be a good idea to change the summary to reflect the DOM-related severeness.
Comment 21 rob cherny 2000-10-29 14:25:47 PST
Agreed.  Who can do this?  I tried ... 
Comment 22 karnaze (gone) 2000-10-29 15:45:50 PST
Changing rtm- to rtm+ again based on recent comments.
Comment 23 selmer (gone) 2000-10-30 18:00:56 PST
rtm-, this is still not a pull-off-the-wire issue and the train has left.
Comment 24 rob cherny 2000-10-30 19:18:49 PST
a pity.  the train left too early.  and i think the rest of us missed it.
Comment 25 Asa Dotzler [:asa] 2000-11-02 19:59:05 PST
*** Bug 56725 has been marked as a duplicate of this bug. ***
Comment 26 Jeffrey Baker 2000-11-06 19:56:45 PST
Hey PDT, you suck.  Jam another buggy browser down my throat.  Fix in hand for 5
months.  PDT: the Mozilla anti-contributor.
Comment 27 Stephan Jaensch 2000-11-07 09:36:50 PST
Seconded. The fix was posted on 2000-06-05 (!!), unbelievable that it does not
make it into rtm.

I don't mind developing around bugs if all known severe ones were fixed, no
product is bugfree. But having to implement a special NS6 workaround because an
available fix HAS NOT BEEN APPLIED FOR FIVE MONTHS just sucks.

With this attitude I really hope that NS marketshare is down to some 1% or what
in a year, at least you don't have to work around totally broken features in
three browsers (IE, NS4, NS6) but only check that the page looks good on IE.

This all is really a shame. I sincerely hoped I would finally get a really good
Linux browser, and have to discover that I just get a revamped 4.5 because some
PDT guys don't get the picture (they obviously didn't change attitude since the
NS4 days).
Comment 28 jsp 2000-11-07 12:17:33 PST
Created attachment 18881 [details]
This bug also manifests if you change the display property under some circumstances, as in this attachment. I sure hope a fix is in 6.01, 'cause my page-in-development looks pretty bad in Mozilla at the moment.
Comment 30 Johnny Stenback (:jst, jst@mozilla.com) 2000-11-08 19:12:43 PST
Could this fix be checked in on the mozilla trunk now?
Comment 31 Hixie (not reading bugmail) 2000-11-09 01:39:54 PST
Jeffrey, Steven: Get your facts right. The patch was attached in October. The 
reviewing was completed exactly one month ago, not five. It was triaged by PDT 
within 30 hours, and then re-examined twice. The arguments presented here did
not convince the PDT to change their mind. To be honest, most of the arguments
were either emotional, or just plain rude. The business world doesn't work with
content-free arguments like that. The PDT specifically asked for high profile
sites that this affected, and none were given.

I've written the above in this bug so that any independent third parties that
wish to record what is going on will have both viewpoints. If you wish to argue
with my statements on particular points, please do so by e-mail or in the
newsgroups, where this belongs. Thanks.


Karnaze: Since you have r= and sr=, you can check-in now. :-)
Comment 32 Stephan Jaensch 2000-11-09 03:49:59 PST
Ian: Get your facts right. The first patch was attached in June and then three
and a half months NOTHING happened. And it's nice to know that it doesn't brake
a top100 site, but that does not change the severity of the bug. Wasn't
standards compliance the top priority (and marketing message) for NS6?

What's the message now? It doesn't brake top100 sites, so it's ok? What do you
want to tell web developers? Develop according to standards: not possible with
NS6. And the big problem is not a top100 site, they surely can spare a developer
that works around browser bugs. But if we continue to see 'Use IE to view this
page correctly' on the hundreds of thousands personal/small company web pages
then yes, we will lose the browser battle. And how do you want to convince these
people to support NS6 if it's broken?
Comment 33 jsp 2000-11-09 05:53:02 PST
FWIW: I've just gotten a copy of the M18 source and manually applied the
10/05/00 patch. Maybe I've made a mistake somewhere along the way, but I don't
see any change in behavior with any of the test cases. I'm running Windows 2000.
Comment 34 Jeffrey Baker 2000-11-09 08:41:25 PST
Ian, you are the one with your facts wrong.  The patch was attached on 5 June by
karnaze@netscape.com, and updated cosmetically on October 5.  As I write this,
the fix has been known for five months, four days.  At the time the fix was
created, we still had not even branched for M16.

There have been thousands and thousands of changes made to the code sine June
5.  This fix could have been among them but wasn't.  After the fix festered for
almost a half year, PDT comes along and declares that the fix is too high risk. 
This is the history of Netscape project management.  Their standard operating
procedure seems (from this developer's perspective) to be to ignore changes as
long as they must in order to say that something is too late.  We should be
clear in placing the blame here.  The fix was not too late.  The fix arrived a
half year ago.  It is PDT who is too late, and as such their inaction has
effectively cancelled out the hard work of the developer who did their job.

On to the other topic of why Netscape 6 will deserve to be booed, rejected and
boycotted.  It is because NN6 will not help web developers one single bit.  We
will not be able to sit down with a list of W3C recommedations and produce work
which renders predictably in the target browser, NN6.  Instead, we will have to
continue the now 4-year-old run-around of writing our code to the standard, and
then spending much more time trying to find out why it doesn't work in NN6. 
This is NOT an improvement.

From my other perspective as a person who has speny hundreds of unpaid hours and
a lot of emotion working on Mozilla, NN6 is a betrayal of everything I have
worked for.  NN6 is going to be by far the most used deployment of Mozilla, or
rather Mozilla as it stood six weeks ago.  As such, it will determine the future
not only of the Netscape product, but the entire Mozilla project.  If NN6 flops,
Mozilla is sunk, and along with it the hopes that many of us had for the
standardized web.

Because of my frustration with the way Netscape throttles development, I have
stopped all of my Mozilla work.  I don't even run my automated alpha-linux
nightly builds anymore.  Be aware that in at least one case, PDT behavior has
removed a contributor from the Mozilla community.
Comment 35 Jeffrey Baker 2000-11-09 08:44:22 PST
BTW, here is my list of standards that NN6 will conform to:
Comment 36 Henri Sivonen (:hsivonen) (Not reading bugmail or doing reviews until 2016-10-03) 2000-11-09 11:15:00 PST
*** Bug 59625 has been marked as a duplicate of this bug. ***
Comment 37 Jarrod Gray 2000-11-09 11:21:38 PST
Why isn't this being checked in on the trunk?
Comment 38 dyapp 2000-11-09 14:51:14 PST
www.voodooextreme.com is one site that maybe this is a problem, when the 
browser window is smaller than the whole page. 
Comment 39 Jason Eager 2000-11-12 14:45:46 PST
This should get checked into the trunk ASAP, since it has r= and a=.
Comment 40 Blake Ross 2000-11-12 14:50:32 PST
Please stop the silly drama.  If you're quitting Mozilla because of Netscape's 
internal processes, then you were never a Mozilla contributor to begin with; 
you should have applied for a job at Netscape.
Comment 41 Owen Williams 2000-11-12 17:34:18 PST
(note: bug-submitter / non-programmer totally unreliable opinion alert)
I think this bug might apply to _all_ sites that use the UBB (ultimate bulletin
board) system.  This would apply to a large, large number of sites.
www.hometheaterforum.com and www.megatokyo.com are just two that I use daily.
Can someone confirm this?
Comment 42 Jason Eager 2000-11-13 13:43:01 PST
karnaze: Is there any reason why this couldn't go into the tree immediately?
Comment 43 karnaze (gone) 2000-11-13 13:51:01 PST
I'll run the regression tests (just in case I forgot to) and check this in today 
or tomorrow. 
Comment 44 karnaze (gone) 2000-11-15 10:04:55 PST
The patch is checked in.
Comment 45 rob cherny 2000-11-22 13:20:49 PST
awesome.  ...almost perfect!  :)

the patch seems to fix the problem of cellpadding when hiding, showing, and
animating the tables ... however the problem with changing font sizes still
exists - sorta - let me explain.

when manipulated from the view text size menu, the cellpadding now remains as
intended.  however when you change the text size via the DOM the padding still
disappears.

Is this a different problem (build 2000112004)?

http://www.cherny.com/mozilla/padding/padding2.html
Comment 46 rob cherny 2000-11-30 21:59:49 PST
this isn't fully resolved/fixed.  can this be updated? please see my test case
above.
Comment 47 Christian Mattar 2000-12-19 06:44:01 PST
I'm reopening since there still seem to be some problems
Comment 48 ekrock's old account (dead) 2001-01-18 13:19:43 PST
Nom. nsbeta1. It's bad to have JS manipulation of the DOM messing up the visual
display of the document (in this case, causing cell padding to disappear), as
this means the developer must choose between *either* accessing the document
contents from JS through the DOM *or* having fidelity with intended CSS style.
Let's see if we can nail the remaining problems and close this out for nsbeta1.
Comment 49 Bernd 2001-02-16 09:12:32 PST
WFM win98 2001021520
Comment 50 Jeffrey Baker 2001-02-16 10:33:30 PST
Yep.  I hadn't looked at this in a long time, but both testcases work for me on
Linux 2001-02-14-03.
Comment 51 Oliver Klee 2001-03-02 19:58:22 PST
Marking WORKSFORME as per user comments.
Comment 52 Amarendra Hanumanula 2001-03-22 13:20:38 PST
QA contact update
Comment 53 Amarendra Hanumanula 2001-06-04 17:14:14 PDT
 Marking verified
Build Id# 2001053104

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