[FIX]"ASSERTION: Unexpected left/right/top/bottom unit" with getComputedStyle on "position: relative; left: 3ch;" element

RESOLVED FIXED

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Jesse Ruderman, Assigned: bz)

Tracking

(Blocks: 1 bug, {assertion, testcase})

Trunk
x86
Mac OS X
assertion, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

10 years ago
Created attachment 275364 [details]
testcase

###!!! ASSERTION: Unexpected left/right/top/bottom unit: 'Error', file /Users/jruderman/trunk/mozilla/layout/style/nsComputedDOMStyle.cpp, line 2726

This reminds me of bug 379405.
Created attachment 275606 [details] [diff] [review]
Possible fix

This does fix the bug... but I wonder about the behavior here in general.  In particular, the behavior of returning 0 if the offsets are auto, or are percentages and there's no containing block, as well as the "look at the opposite side" thing.  I think it might make more sense to just use SetValueToCoord here and be done with it...
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #275606 - Flags: superreview?(dbaron)
Attachment #275606 - Flags: review?(dbaron)
Created attachment 275607 [details] [diff] [review]
The right diff
Attachment #275606 - Attachment is obsolete: true
Attachment #275607 - Flags: superreview?(dbaron)
Attachment #275607 - Flags: review?(dbaron)
Attachment #275606 - Flags: superreview?(dbaron)
Attachment #275606 - Flags: review?(dbaron)
That patch depends on bug 391221.
Depends on: 391221
Summary: "ASSERTION: Unexpected left/right/top/bottom unit" with getComputedStyle on "position: relative; left: 3ch;" element → [FIX]"ASSERTION: Unexpected left/right/top/bottom unit" with getComputedStyle on "position: relative; left: 3ch;" element
Comment on attachment 275607 [details] [diff] [review]
The right diff

>+  // XXXbz I wish there were some centralized place that had the
>+  // condition of this |if| statement.
>   if (coord.GetUnit() != eStyleUnit_Coord &&
>-      coord.GetUnit() != eStyleUnit_Percent) {
>+      coord.GetUnit() != eStyleUnit_Percent &&
>+      coord.GetUnit() != eStyleUnit_Chars) {

Can you just check coord.GetUnit() == eStyleUnit_Auto?

(Was this one of the places where there used to be some eStyleUnit_Null?)

r+sr+a=dbaron

(Didn't really look at the test, but I trust you there.  I hope you generally test that the test fails without the patch?)
Attachment #275607 - Flags: superreview?(dbaron)
Attachment #275607 - Flags: superreview+
Attachment #275607 - Flags: review?(dbaron)
Attachment #275607 - Flags: review+
Attachment #275607 - Flags: approval1.9+
> Can you just check coord.GetUnit() == eStyleUnit_Auto?

Um.... yes.  ;)

And yes, I do check that the tests fail without the patch.
Checked in with that change.  Fixed.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Reporter)

Comment 7

10 years ago
Crashtest checked in.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.