171749 - 'display: none' ignored on generated content [GC]

Status: VERIFIED FIXED

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

Description

[Eric A. Meyer (dead account)]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC; en-US; rv:1.2b) Gecko/20020930
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC; en-US; rv:1.2b) Gecko/20020930

http://www.w3.org/TR/CSS21/generate.html#before-after-content (and the
equivalent section of CSS2) permits generated content to be given the 'display'
value 'none'.  This is not being done properly in Gecko.  I'll attach a testcase
immediately.

Reproducible: Always

Steps to Reproduce:

Comment 1

[Eric A. Meyer (dead account)]

Attachment: Testcase showing too much generated content

None of the red generated content should be visible.  I have tests which show
Gecko honoring 'display: block' for generated content, so this isn't a case of
ignoring the property, but a specific allowed value.

Comment 2

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

nsCSSFrameConstructor::CreateGeneratedContentFrame handles this fine.  However,
we have this lovely bit of code in nsRuleNode::ComputeDisplayData:

2915   if (generatedContent) {
2916     PRUint8 displayValue = display->mDisplay;
2917     if (parentDisplay->IsBlockLevel()) {
2918       // For block-level elements the only allowed 'display' values are:
2919       // 'none', 'inline', 'block', and 'marker'
2920       if ((NS_STYLE_DISPLAY_INLINE != displayValue) &&
2921           (NS_STYLE_DISPLAY_BLOCK != displayValue) &&
2922           (NS_STYLE_DISPLAY_MARKER != displayValue)) {
2923         // Pseudo-element behaves as if the value were 'block'
2924         displayValue = NS_STYLE_DISPLAY_BLOCK;
2925       }
2926
2927     } else {
2928       // For inline-level elements the only allowed 'display' values are
2929       // 'none' and 'inline'
2930       displayValue = NS_STYLE_DISPLAY_INLINE;
2931     }
2932
2933     if (display->mDisplay != displayValue) {
2934       // Reset the value
2935       display->mDisplay = displayValue;
2936     }
2937   }

Comment 3

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

Attachment: patch

Comment 4

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

Attachment: additional testcase showing reason for first change in patch

This testcase shows why the first of the changes in my patch is needed. 
Basically, the non-block enforcement for inlines wasn't working in the case
where |parentDisplay == display|, which depended on which properties in the
display struct were specified.	So, in other words, I had to change the
"standard preamble" of the Compute*Data functions (just as I told Mats not to
do in bug 93227 (and thus I cc:ed him for his curiosity, and in case he wants
to double-check that this patch makes sense)).

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 101244 [details] [diff] [review]
patch

r/sr=bzbarsky

Comment 6

[Mats Palmgren (inactive)]

Yes, the patch makes sense and looks good.
However the code could be rearranged to collect the 'generatedContent' bits
into one block if you want that.
The end of ComputeDisplayData() looks something like this:
================
  if (generatedContent) {
    // fix position and float
  }

  if (display->mDisplay != NS_STYLE_DISPLAY_NONE &&
      display->mFloats != NS_STYLE_FLOAT_NONE)
    // fix stuff for floater

  if (display->IsAbsolutelyPositioned() ...) {
    // fix stuff for abs. pos.
  }
  
  if (generatedContent) {
    // fix display
  }

================
The middle two 'if' blocks will never match generatedContent because
of the "fix position and float" block we did above. So we could write:
  if (generatedContent) {
      // fix position, float and display
  } else {
     // the two middle 'if' blocks
  }

Comment 7

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

Attachment: patch with Mats Palmgren's suggestion

This patch takes (roughly) the suggestion in the previous comment, except that
I put the 'display: none' check right in the |else| to make it an |else if|.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 101404 [details] [diff] [review]
patch with Mats Palmgren's suggestion

r/sr=bzbarsky

Comment 9

[Mats Palmgren (inactive)]

> I put the 'display: none' check right in the |else| to make it an |else if|.

Yes, that looks even better.

Comment 10

[kinmoz]

Comment on attachment 101404 [details] [diff] [review]
patch with Mats Palmgren's suggestion

sr=[e-mail]

Comment 11

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

Fix checked in to trunk, 2002-10-02 17:42 PDT.

Comment 12

[Madhur Bhatia]

checked testcase attachment 101134 [details] and attachment 101245 [details].
They look good on 
win2000 ---- 2002-10-03-08 trunk build.
linux 7.2 -- 2002-10-03-08 trunk build.
marking the bug verified.

nominating for nsbeta1

Comment 13

[Madhur Bhatia]

really nominating ....