91054 - padding/margin percents don't inherit computed value

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[fantasai]

Overview:

	Padding and margin inherit don't inherit the computed value when
	shorthand notation is used.

Steps to Reproduce:

	Open up testcase in Mozilla.
	Each test has two sets of <div>s -- one with inherited % values, and
	another with specified values that should correspond to the value 
	inherited in the first test.

Expected Results:

	Each pair of tests should render the same.

Actual Results

	The two tests in "Margin A" and "Padding A" don't render the same;
	the percentage is inherited instead of the computed value.

Tested on nightly build id=2001070708 on Windows 98

Additional Information:
	May be related to bug 45631

Comment 1

[fantasai]

Attachment: testcase

Comment 2

[Pierre Saslawsky]

I'm going to attach a simpler testcase.  The bug is not related to the shorthand 
notation, it is a regression which is probably related to hyatt's rule tree 
landing from bug 78695.  If this is the case, I hope we don't have a design 
problem that causes the specified values to be inherited, instead of the computed 
values.

I don't have builds from 05/30 and 06/01 to make sure that the regression comes 
from the rule tree landing.  I tried with a [06/07 moz0.9.1] build - it works - 
and with a [06/08 trunk] build - it doesn't work.

Comment 3

[Pierre Saslawsky]

Attachment: smaller testcase

Comment 4

[fantasai]

> The bug is not related to the shorthand notation,

Yeah, you're right. The bug seems to occur only when all margins are specified 
as 'inherit'.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Hyatt says sr=hyatt (we worked on this together).

Comment 7

[David Hyatt]

Yup, sr=hyatt

Comment 8

[Pierre Saslawsky]

-> dbaron

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Hmmm.  I suspect this actually isn't going to fix the problem for 'line-height'.
 (I'm assuming 'font-size' is working since we'd have noticed otherwise.)

Comment 10

[Pierre Saslawsky]

I'm testing similar cases too.  So far so good, I did not find anything that 
breaks but your help (and Ian's too) would be appreciated.

I'd like to lobby PDT to get that fix in, if several of us certify that it won't 
break anything else.  I think it's a fairly basic feature that got broken here 
(although it's not in any testcase apparently).

Ian, David: what do you think? Is it worth the effort to try to get that in for 
6.1?

Comment 11

[Pierre Saslawsky]

David: font-size works indeed.  I'll attach a testcase.

Comment 12

[Pierre Saslawsky]

Attachment: testcase for font-size

Comment 13

[fantasai]

Pierre: Do you want all percentage inheritance testcases here? (Which means the
summary should be changed.)

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think the real fix here (is 'line-height' broken?  fantasai said it was on
IRC, I think.) is to change the way |Check*Properties| count inherit values in
the exact cases where we store inherit values literally on the struct.

Comment 15

[fantasai]

Attachment: line-height inheritance

Comment 16

[fantasai]

Attachment: background-position

Comment 17

[fantasai]

width, max-width, min-width, height, max-height, min-height, left, right, top, 
and bottom all inherit properly. I couldn't test vertical-align due to bug 
93865.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The problem with my previous proposal is that how we store the properties on the
struct depends on the parent from which it's inheriting, and therefore it can't
be cached in the rule tree even if we determine that, in a certain case, we use
the eCSSUnit_Inherit value throughout.  So I think if we hit a case where we
might have to use an eCSSUnit_Inherit we need to both:
 * make sure we don't cache on the rule node
 * make sure not to simply defer to the parent's struct (inherit the whole thing)

I think I have a solution for this, along with some changes to make the checking
functions less tedious.

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch, first draft (haven't yet reverse-checked tables)

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

There are a bunch of comments in that patch that are out of date (since I had to
change things a bit as I went...).

Comment 21

[Chris Waterson]

This seems like a good idea to me.

Comment 22

[David Hyatt]

Yeah, i like this.  I would carefully double-check everything. :)

r/sr=hyatt

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: correct unnecessary PR_TRUE for mBorderWidth

Comment 24

[Chris Waterson]

sr=waterson

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in 2001-09-04 20:13 PDT.

Comment 26

[fantasai]

The tests for background-position and line-height fail. The line-height one is
weird, because it inherits percentages properly when 'inherit' is explicity
specified, but inherits the proportion when it's not.

2001102309, Win2K

Comment 27

[Pierre Saslawsky]

Attachment: line-height inheritance v2

Comment 28

[Pierre Saslawsky]

reopening with a new milestone

Comment 29

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The line-height bug is bug 97726 and is almost definitely a different bug from
this one.  Do you have a testcase for 'background-position'?

Comment 30

[fantasai]

Yeah--the sixth attachment from the top ("background-position").

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I split off the background-position issue into bug 117624.  Marking this bug as
fixed again.

Comment 32

[Madhur Bhatia]

the first 2 attachemnts work fine.
Marking verified