460664 - [FIX]fix for bug 395623 didn't change inheritance when all properties in struct are 'inherit'

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Bug 395623 attempted to change what we inherit from for 'property:inherit' when property is a non-inherited property.  However, testcases written during the CSS meeting (using background:inherit vs. background-color: inherit) showed that this behavior change doesn't work when all of the properties in a struct are explicitly 'inherit'.  This may be a security bug, given that bug 395623 was.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Ugh.  Right.  That's what the assert right there is all about...

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 343832 [details] [diff] [review]
Fix

r+sr=dbaron

Please also add tests for the case where we're looking at the first line of

<div><p>this paragraph</p></div>

where there is both a div:first-line and a p:first-line rule.  The spec says the pseudo-markup for that case should involve nesting <div:first-line><p:first-line>.  I don't think we do that now, but your current code will be insufficient for that when we do (since it would cause inheriting from the div::first-line).  (For both cases, the original one and the one this bug fixes.)

You might also want to add a comment at both places where we have this code pointing to the other place.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Added that test, and changed the adjustment code (in both places) to be:

  while (parentContext &&
         parentContext->GetPseudoType() == nsCSSPseudoElements::firstLine) {
    parentContext = parentContext->GetParent();
  }

to be more future-proof.

Pushed as changeset a407bdae5ea8.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch as pushed

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 343922 [details] [diff] [review]
Patch as pushed

Probably good to fix this on branch too.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Had to add a "background: green" to the <p> in the new test as well.

Comment 8

[Daniel Veditz [:dveditz]]

Comment on attachment 343922 [details] [diff] [review]
Patch as pushed

Won't have enough time for trunk testing first for this cycle, we'll catch it next time.

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 343922 [details] [diff] [review]
Patch as pushed

approved for 1.9.0.5, a=dveditz for release-drivers

(please watch tinderbox for the tree to open before landing)

Comment 10

[Daniel Veditz [:dveditz]]

Assuming that this doesn't apply to the 1.8 branch, like bug 395623

Comment 11

[Boris Zbarsky [:bzbarsky]]

That's correct.  This doesn't apply to 1.8.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Fixed on 1.9.0 branch.

Comment 13

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.9.0.5 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.5pre) Gecko/2008120105 GranParadiso/3.0.5pre using the reftest (http://mxr.mozilla.org/mozilla/source/layout/reftests/first-line/parent-style-2.html?ctype=text/html&raw=1).