Closed
Bug 1251797
Opened 9 years ago
Closed 9 years ago
only make style data conditional on writing mode when a logical declaration *could* win the cascade
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla48
People
(Reporter: dbaron, Assigned: dbaron)
References
Details
Attachments
(1 file)
In bug 649142 we made logical declarations fault a style struct out of the rule tree when they're present in the cascade for the computation of a value (except when all values in the struct are specified higher in the cascade -- then we don't, nsRuleNode::WalkRuleTree will optimize away the call that does this). We've since made this a writing mode dependency rather than faulting.
However, shortly after bug 649142 I wrote a followup patch to change this so that we only do this if the logical declaration could potentially win the cascade (i.e., there's an unspecified value for one of the relevant physical properties). I'm inclined to think we should do this.
I've had this patch in my tree for a while.
Assignee | ||
Comment 1•9 years ago
|
||
Assignee | ||
Comment 2•9 years ago
|
||
(I'll post this for review once heycam is back.)
Assignee | ||
Comment 3•9 years ago
|
||
I believe this is useful for cases like having logical properties in the
UA style sheet that are commonly overridden (e.g., margins on lists).
Review commit: https://reviewboard.mozilla.org/r/37019/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37019/
Attachment #8730062 -
Flags: review?(cam)
Updated•9 years ago
|
Attachment #8730062 -
Flags: review?(cam) → review+
Comment 4•9 years ago
|
||
Comment on attachment 8730062 [details]
MozReview Request: Bug 1251797 - Don't fault struct out of rule tree if all of the potential physical property destinations already have a winning value in the cascade. r?heycam
https://reviewboard.mozilla.org/r/37019/#review36305
::: layout/style/nsCSSDataBlock.cpp:301
(Diff revision 1)
> -#ifdef DEBUG
> - {
> - // Table-length is 1 for single prop, 2 for axis prop, 4 for block prop.
> + // Table-length is 1 for single prop, 2 for axis prop, 4 for block prop.
> - size_t len = isSingleProperty ? 1 : (isAxisProperty ? 2 : 4);
> + size_t len = isSingleProperty ? 1 : (isAxisProperty ? 2 : 4);
> +#ifdef DEBUG
> + {
These braces can go now that we have moved |len| out of the #ifdef.
::: layout/style/nsCSSDataBlock.cpp:314
(Diff revision 1)
> #endif
> +
> + for (size_t i = 0; i < len; i++) {
> + if (aRuleData->ValueFor(props[i])->GetUnit() == eCSSUnit_Null) {
> + // A declaration of one of the logical properties in this logical
> + // group would be the winning declaration in the cascade. This
Maybe s/would be/is/?
::: layout/style/nsCSSDataBlock.cpp:318
(Diff revision 1)
> + // mean that for sure, though. For example, if this is a
> + // block-start logical property, and all but the bottom physical
> + // property were set. But the common case we want to hit here is
In this "For example" maybe mention that the writing mode would need to be horizontal too?
Assignee | ||
Comment 5•9 years ago
|
||
(In reply to Cameron McCormack (:heycam) (busy Mar 14–18) from comment #4)
> ::: layout/style/nsCSSDataBlock.cpp:314
> (Diff revision 1)
> > #endif
> > +
> > + for (size_t i = 0; i < len; i++) {
> > + if (aRuleData->ValueFor(props[i])->GetUnit() == eCSSUnit_Null) {
> > + // A declaration of one of the logical properties in this logical
> > + // group would be the winning declaration in the cascade. This
>
> Maybe s/would be/is/?
I think instead I'll add "(but maybe not aProperty)", since that's why I didn't use "is".
> ::: layout/style/nsCSSDataBlock.cpp:318
> (Diff revision 1)
> > + // mean that for sure, though. For example, if this is a
> > + // block-start logical property, and all but the bottom physical
> > + // property were set. But the common case we want to hit here is
>
> In this "For example" maybe mention that the writing mode would need to be
> horizontal too?
I don't see why -- the block-start side can never be the bottom; it can be top (for horizontal writing mode), right, or left.
Assignee | ||
Comment 6•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/718aa12e555b11e5980ea59929bf37c7416fd1db
Bug 1251797 - Don't fault struct out of rule tree if all of the potential physical property destinations already have a winning value in the cascade. r=heycam
Comment 7•9 years ago
|
||
(In reply to David Baron [:dbaron] ⌚️UTC-7 from comment #5)
> > In this "For example" maybe mention that the writing mode would need to be
> > horizontal too?
>
> I don't see why -- the block-start side can never be the bottom; it can be
> top (for horizontal writing mode), right, or left.
I understand now.
Comment 8•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•8 years ago
|
Depends on: CVE-2017-7753
You need to log in
before you can comment on or make changes to this bug.
Description
•