469227 - redesign mechanism for restricting properties from ::first-line and ::first-letter pseudo elements

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

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

The current mechanism for restricting properties from applying to ::first-line and ::first-letter pseudo elements is somewhat broken; see the comments in nsHTMLCSSStyleSheet that are being removed in this patch.

We could do it much more simply with the flags field in nsCSSProps.  Patches coming in a bit.

Note that there are a bunch of recently-added properties that we should have restricted but that we did not.

The only interesting new properties are:
 -moz-border-image - just like border, :first-letter but not :first-line
 -moz-box-shadow - just like border (ditto)
 -moz-text-shadow - both :first-letter and :first-line, I think
 word-wrap - just like white-space should have been (although wasn't when we made it work on inlines) -- applies to neither


The current code implicitly blocks some properties (e.g., border-color) by blocking others (e.g., border-style), and also implicitly blocks some by the fact that they will never have any effect.  So I'll attach three patches:

 * the first patch should give us something equivalent to the current code, but without the deficiencies described in nsHTMLCSSStyleSheet
 * the second patch should apply the things that were implicit
 * the third patch should fix actual mistakes, so that our list ends up being the list in CSS 2.1 as modified by my comments about new properties above

Comment 1

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

Attachment: patch 1: redesign mechanism, don't change which properties are blocked

This contains a reftest for a correctness bug fixed by this patch.

Comment 2

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

Attachment: patch 2: also block properties that were "effectively" blocked before

Comment 3

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

Attachment: patch 3: also block properties that should have been blocked

Comment 4

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

Note that the comment added to nsCSSPropList.h in patch 1 isn't really completely true until all three patches are applied.

Full reftests pass.

Comment 5

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

... and layout/style mochitests pass.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 352647 [details] [diff] [review]
patch 1: redesign mechanism, don't change which properties are blocked

>@@ -1619,16 +1690,26 @@ nsRuleNode::WalkRuleTree(const nsStyleSt
>+  // A bit in nsCSSProps's flags that has to be present for this
>+  // property to apply.

Maybe:

  // If needed, reset the properties that don't have a flag that allows them to be set for this style context.

>+    // Recompute |detail| based on the restrictions we just applied.
>+    detail = CheckSpecifiedProperties(aSID, *aSpecificData);

Hmm.  So this is basically relying on the fact that all rules down to our most-specific rule node are rules that apply to the same pseudo-element and don't apply to anything else, right?  Otherwise using this detail to propagate dependent and none bits would be wrong.   

I wonder whether we can somehow assert this, but at the very least we should document it.

With that looks good, I think.  I have to admit that my eyes started glazing over part way through that big header.  :(

Comment 7

[Boris Zbarsky [:bzbarsky]]

And the point is, I didn't compare the final header, with all three patches applied, to the spec list.  I'm sort of assuming that's been done.  ;)  I can double-check that if you really want me to, I guess.

Comment 8

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

(In reply to comment #6)
> >+    // Recompute |detail| based on the restrictions we just applied.
> >+    detail = CheckSpecifiedProperties(aSID, *aSpecificData);
> 
> Hmm.  So this is basically relying on the fact that all rules down to our
> most-specific rule node are rules that apply to the same pseudo-element and
> don't apply to anything else, right?  Otherwise using this detail to propagate
> dependent and none bits would be wrong.   
> 
> I wonder whether we can somehow assert this, but at the very least we should
> document it.

Er, now that you point this out, I think it's wrong, and I should change back to the code I originally had, which was:

  if (detail == eRuleFullMixed)
    detail = eRulePartialMixed;
  else if (detail == eRuleFullInherited)
    detail = eRulePartialInherited;
  else if (detail == eRuleFullReset)
    detail = eRulePartialReset;

which always produces a more conservative result.

I'll see if I can write a testcase for the bug...

Comment 9

[Boris Zbarsky [:bzbarsky]]

Oh, right.  Rulenodes are per-rule, not per-selector.  So a rule which has a selector like ":first-line, .foo" would in fact have a rulenode that would appear on both paths.  If that rule specifies a border and the first-line is resolved first, we should get a bug, I'd think.

Comment 10

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

Attachment: patch 1: redesign mechanism, don't change which properties are blocked

With the change in comment 8 (although not quite as conservative, since I can compare detail to the result of CheckSpecifiedProperties to see if anything changed), and a test for that (which I wrote before seeing comment 9).

Comment 11

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

Comment on attachment 352656 [details] [diff] [review]
patch 1: redesign mechanism, don't change which properties are blocked

Er, never mind, the problem you're pointing out in comment 9 is yet another issue.  Maybe we still need a dummy rule node for properties like this.

Comment 12

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

Er, probably I'll just make a pair of dummy rules just like we used to have, except ones that don't do anything.

Comment 13

[Boris Zbarsky [:bzbarsky]]

The rules will have to have lower specificity than any other rules, right (to separate out the relevant ruletree branches)?

Comment 14

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

Oh, I had actually been thinking higher, but if I go with lower then I could keep the original adjustment to |detail|.  The trick with lower is doing it so that ProbePseudoStyleContext still works.

Comment 15

[Boris Zbarsky [:bzbarsky]]

I guess we can do higher if we disallow caching above the dummy node in the ruletree.  That would certainly make it easier to do ProbePseudoStyleContext...

Comment 16

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

Attachment: patch 1: redesign mechanism, don't change which properties are blocked

Putting the extra nodes at the lowest level was actually pretty easy.

Full reftests and layout/style mochitests pass.  I added a reftest for the additional case this fixes as well.

Comment 17

[Boris Zbarsky [:bzbarsky]]

David, does 469227-2 fail with the first iteration of this patch?  I'd expect it to work ok, but for the border to not show up if the <p> and <span> are switched...  Maybe we should test both orders?

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 352672 [details] [diff] [review]
patch 1: redesign mechanism, don't change which properties are blocked

r+sr=bzbarsky with that nit.

Comment 19

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

The bug with 469227-2 was that the border did show up on ::first-line even though it shouldn't have.

I'll test the other order as well, though.

Comment 20

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

Fixed on mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/3d372e6ff724
http://hg.mozilla.org/mozilla-central/rev/41fd48e76fb0
http://hg.mozilla.org/mozilla-central/rev/ca48a3b92dee

Comment 21

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

Fixed on mozilla-1.9.1:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2a49e0977be2
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c187dab6cb9a
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/80136fcf1d21