do inheritance without layout-dependent computations (CSS2.1 change) (remove eStyleUnit_Inherit and nsStyleCoord::SetInheritValue)

RESOLVED FIXED in mozilla1.6beta

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
14 years ago
10 years ago

People

(Reporter: Hixie (not reading bugmail), Assigned: dbaron)

Tracking

({perf})

Trunk
mozilla1.6beta
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Reporter)

Description

14 years ago
According to CSS2.1 (a CR for which a fix for this bug should probably wait),
inheritance is no longer layout-dependent in any case.

This should make it possible to optimise our system and remove the need for an
inheritance value in the style contexts. (I sure hope what I'm saying is making
sense. Ahem.)
Sounds great. This is a fantastic move and we should definitely take advantage
of it.

I assume it will still be true in CSS3?
(Reporter)

Comment 2

14 years ago
I sure hope so.
Hmm. What exactly has changed in CSS2.1? For example, given

<div style="position:absolute; top:0; left:0;">
  Hello Kitty
  <div style="position:absolute; top:100px; left:0; height:inherit; background:red">
    Hello Kitty
  </div>
</div>

CSS2.1 still seems to say that the red DIV gets the computed height of the outer
DIV, which requires layout.
(Reporter)

Comment 4

14 years ago
The change I mentioned has not yet been included in any published version of
CSS2.1. The intention is that the inherited value in the case you give would be
'auto' (assuming that is the specified value on the outer DIV too, which it
would be in the absence of any other stylesheets).

Updated

14 years ago
Blocks: 203448
OS: Windows 2000 → All
Hardware: PC → All
(Reporter)

Comment 5

14 years ago
This will affect bug 117887, bug 93853, and bug 45631.
Summary: Optimise inheritance to not require layout-dependent computations → Optimise inheritance to not require layout-dependent computations (change how inheriting computed values works for CSS2.1)
(Reporter)

Comment 6

14 years ago
*** Bug 226474 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 7

14 years ago
from dbaron's description of bug 226474:
> The changes in CSS 2.1 mean that we should be able to remove eStyleUnit_Inherit
> and nsStyleCoord::SetInheritValue.  This will let us simplify a good bit of 
> code...
(Assignee)

Updated

14 years ago
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.6beta
(Assignee)

Updated

14 years ago
Summary: Optimise inheritance to not require layout-dependent computations (change how inheriting computed values works for CSS2.1) → do inheritance without layout-dependent computations (CSS2.1 change) (remove eStyleUnit_Inherit and nsStyleCoord::SetInheritValue)
(Assignee)

Comment 8

14 years ago
The patch I'm going to attach will, I think, make the iscoord_ parameter in the
CSSPropList macros unused.  However, I want to leave it since I'm going to need
something similar (although not exactly the same) for bug 147777.

I think this patch is low risk because it's removing very poorly tested code,
and code that should incompatible with what any other browser that actually
supports 'inherit' does.  I'd like to get it in now because I think it might
make bug 215857 easier to understand and/or fix.
(Assignee)

Comment 9

14 years ago
Created attachment 136070 [details] [diff] [review]
patch (diff -ub)

This patch uses diff -b (ignore changes in amount of whitespace).

If you really want to review my indentation changes, I can attach that version
as well...
(Assignee)

Updated

14 years ago
Attachment #136070 - Flags: superreview?(bz-vacation)
Attachment #136070 - Flags: review?(bz-vacation)
Comment on attachment 136070 [details] [diff] [review]
patch (diff -ub)

I think I can trust you on indentation changes... ;)

r+sr=bzbarsky; I'm glad to see some of that code go....
Attachment #136070 - Flags: superreview?(bz-vacation)
Attachment #136070 - Flags: superreview+
Attachment #136070 - Flags: review?(bz-vacation)
Attachment #136070 - Flags: review+
(Assignee)

Comment 11

14 years ago
Comment on attachment 136070 [details] [diff] [review]
patch (diff -ub)

See comment 8 about risk.
Attachment #136070 - Flags: approval1.6b?

Comment 12

14 years ago
Comment on attachment 136070 [details] [diff] [review]
patch (diff -ub)

a=asa (on behalf of drivers) for checkin to 1.6beta
Attachment #136070 - Flags: approval1.6b? → approval1.6b+
(Assignee)

Comment 13

14 years ago
The regression tests that changed (ignoring the usual noise and the style data
mismatches that are expected) are:

table/bugs/bug32447.html (moved a little to the right)
table/bugs/bug34538.html (exact same test as previous)
table/bugs/bug89315.html (invisible bbox change, maybe noise)

So the only real change that I'm worried about is attachment 6700 [details].
(Assignee)

Comment 14

14 years ago
The change to attachment 6700 [details] is more compatible with IE/Windows, and I think
more correct.  (I think it's because the 'left: inherit', etc., on
::-moz-table-outer are now working as intended.)
(Assignee)

Comment 15

14 years ago
Fix checked in, 2003-11-24 11:46 -0800.
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
So... I wonder how this checkin affected bug 53663 and its dependencies.  See
bug 53663 comment 12.
Blocks: 53663
(Reporter)

Comment 17

14 years ago
Well, 'inherit' still isn't the specified value.
What do you mean, Ian?
(Reporter)

Comment 19

14 years ago
bz: The comment you linked to says we want to "inherit" the specified value. But
in CSS2.1, the 'inherit' keyword inherits the computed value, and even in CSS2.1
the computed value is not the specified value.
Actually, I think the computed and specified values are fully equivalent for the
purposes of that code.  The key here is that "inherit" takes place at style
resolution time instead of layout time, so "inherit" when a parent has "auto"
will become "auto" (which is the problem in bug 53663).
(Reporter)

Comment 21

14 years ago
Well if that comment is wrong, then nevermind. :-)
(Assignee)

Updated

14 years ago
Blocks: 208274
(Assignee)

Comment 22

13 years ago
Created attachment 167449 [details] [diff] [review]
remove iscoord (checked in to trunk, 2004-11-30 11:24 -0800)
(Assignee)

Comment 23

13 years ago
Comment on attachment 167449 [details] [diff] [review]
remove iscoord (checked in to trunk, 2004-11-30 11:24 -0800)

I don't think the bit in comment 8 about bug 147777 is true anymore.
Attachment #167449 - Flags: superreview?(bzbarsky)
Attachment #167449 - Flags: review?(bzbarsky)
Comment on attachment 167449 [details] [diff] [review]
remove iscoord (checked in to trunk, 2004-11-30 11:24 -0800)

r+sr=bzbarsky
Attachment #167449 - Flags: superreview?(bzbarsky)
Attachment #167449 - Flags: superreview+
Attachment #167449 - Flags: review?(bzbarsky)
Attachment #167449 - Flags: review+
(Assignee)

Updated

13 years ago
Attachment #167449 - Attachment description: remove iscoord → remove iscoord (checked in to trunk, 2004-11-30 11:24 -0800)
(Assignee)

Comment 25

12 years ago
Created attachment 197373 [details] [diff] [review]
fix comments in nsStyleStruct.h (checked in, 2005-09-25 18:58 -0700)

I missed removing ", inherit" from a bunch of comments in nsStyleStruct.h
Attachment #197373 - Flags: superreview?(bzbarsky)
Attachment #197373 - Flags: review?(bzbarsky)
Comment on attachment 197373 [details] [diff] [review]
fix comments in nsStyleStruct.h (checked in, 2005-09-25 18:58 -0700)

r+sr=bzbarsky
Attachment #197373 - Flags: superreview?(bzbarsky)
Attachment #197373 - Flags: superreview+
Attachment #197373 - Flags: review?(bzbarsky)
Attachment #197373 - Flags: review+
(Assignee)

Updated

12 years ago
Attachment #197373 - Attachment description: fix comments in nsStyleStruct.h → fix comments in nsStyleStruct.h (checked in, 2005-09-25 18:58 -0700)
(Assignee)

Updated

10 years ago
Blocks: 93865
You need to log in before you can comment on or make changes to this bug.