Closed
Bug 509569
Opened 15 years ago
Closed 14 years ago
"ASSERTION: canStoreInRuleTree must be false for reset structs if any properties were specified as inherit" with -moz-border-start
Categories
(Core :: CSS Parsing and Computation, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: jruderman, Assigned: bzbarsky)
References
Details
(Keywords: assertion, testcase)
Attachments
(2 files)
92 bytes,
text/html
|
Details | |
1.97 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
###!!! ASSERTION: canStoreInRuleTree must be false for reset structs if any properties were specified as inherit: '!canStoreInRuleTree || aRuleDetail == eRuleNone || aRuleDetail == eRulePartialReset || aRuleDetail == eRuleFullReset', file /Users/jruderman/central/layout/style/nsRuleNode.cpp, line 4589
Assignee | ||
Comment 1•15 years ago
|
||
Hmm. So the issue here is that the |border| shorthand doesn't reset the value for eCSSProperty_border_start_style_value (which therefore remains inherit, even though it's not used because the sources are both set to physical). So we end up in ComputeBorderData with aRuleDetail == eRulePartialMixed, but never set aCanStoreInRuleTree to false (which I think is correct in this case; the inherit values are never going to be used, since you can't set the *_source values to "logical" without also resetting the style_value. Obvious fixes: 1) Loosen up the assert for the mixed cases (possibly only for border/padding /margin structs). 2) Make parsing reset more stuff somehow. 3) Make AdjustLogicalBoxProp set aCanStoreInRuleTree to false if any of the value cssvalues are inherit, not just the ones it ends up using). Need to think a bit more about whether there's a correctness issue here, or just assert-avoidance.
Assignee | ||
Comment 2•15 years ago
|
||
> 2) Make parsing reset more stuff somehow.
This is not an option; for example this style:
"-moz-border-start: inherit; border-left: none; border-right: none"
would trigger the assert. Further, it would trigger the assert even if the declarations were scattered over multiple rules. There's no way the parser can deal with this.
Now in terms of correctness... The assert only fires if both the ltr source and rtl source for a box prop are "physical" and the relevant values are not parent-dependent while one of the logical value values is inherit. If we cache such a struct in the ruletree, I think we're ok, since it in fact doesn't contain any information that depends on the parent.
We could make the rule detail correct by using Check*Callback functions and passing the inherited/total/etc numbers from nsRuleNode::CheckSpecifiedProperties to the callbacks and then having the callback detect the case when both sources are physical and adjust the inherited number as needed. But in practice, I'd think this situation is rare; it's simpler to do item 3 from above.
Component: Style System (CSS) → Spelling checker
Target Milestone: --- → mozilla2.0
Assignee | ||
Comment 3•15 years ago
|
||
Oh, and since there's no correctness issue, I don't think we need to worry about this for 1.9.2.
Component: Spelling checker → Style System (CSS)
Target Milestone: mozilla2.0 → ---
Assignee | ||
Comment 4•15 years ago
|
||
This is the simple thing to do, but note comment 2. This is just "fixing" the assert by disallowing an otherwise valid optimization....
Comment on attachment 413128 [details] [diff] [review] Possible fix I have mixed feelings about whether we should do this at all, but I suppose it's a very rare case, so r=dbaron.
Attachment #413128 -
Flags: review?(dbaron) → review+
Assignee | ||
Comment 6•14 years ago
|
||
Pushed http://hg.mozilla.org/mozilla-central/rev/57f4d921cd32
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•