Closed Bug 1251797 Opened 6 years ago Closed 6 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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox47 --- affected
firefox48 --- fixed

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.
Depends on: 649142
(I'll post this for review once heycam is back.)
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)
Attachment #8730062 - Flags: review?(cam) → review+
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?
(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.
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
(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.
https://hg.mozilla.org/mozilla-central/rev/718aa12e555b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Depends on: CVE-2017-7753
You need to log in before you can comment on or make changes to this bug.