71717 - ComboboxControlFrame is resolving style on a text node: this should be eliminated

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Marc Attinasi]

Text Nodes should not have style applied to them. Currently there is bug 56117
in the style system that allows text nodes to be styled, but this needs to be
fixed and when it is the comboboxes will not work correctly.

If we can revisit this and determine if the styling of the text node for the
display area then maybe we can eliminate the styling of the text node.

Comment 1

[rods (gone)]

Well, I did this (see below) and the combobox seems to size ok, maybe the text 
styling isn't needed anymore:

  // create the style context for the anonymous frame
  nsCOMPtr<nsIStyleContext> styleContext;
  rv = aPresContext->ResolvePseudoStyleContextFor(mContent, 
                 nsHTMLAtoms::mozDisplayComboboxControlFrame,
                 mStyleContext,
                 PR_FALSE,
                 getter_AddRefs(styleContext));
  if (NS_FAILED(rv)) { return rv; }
  if (!styleContext) { return NS_ERROR_NULL_POINTER; }

  // create a text frame and put it inside the block frame
  rv = NS_NewTextFrame(shell, &mTextFrame);
  if (NS_FAILED(rv)) { return rv; }
  if (!mTextFrame) { return NS_ERROR_NULL_POINTER; }
#if 0
  nsCOMPtr<nsIStyleContext> textStyleContext;
  rv = aPresContext->ResolvePseudoStyleContextFor(mContent, 
                          nsHTMLAtoms::mozDisplayComboboxControlFrame,
                          styleContext,
                          PR_FALSE,
                          getter_AddRefs(textStyleContext));
  if (NS_FAILED(rv)) { return rv; }
  if (!textStyleContext) { return NS_ERROR_NULL_POINTER; }
  nsCOMPtr<nsIContent> content(do_QueryInterface(mDisplayContent));
  mTextFrame->Init(aPresContext, content, mDisplayFrame, textStyleContext, 
nsnull);
#else
  nsCOMPtr<nsIContent> content(do_QueryInterface(mDisplayContent));
#endif

Comment 2

[rods (gone)]

Well, actually its two pixels smaller than it should be, but I think that was 
there before. It seems to be a problem that comes and goes for some reason.

And this fixes the sizing problem:
Index: forms.css
===================================================================
RCS file: /cvsroot/mozilla/layout/html/document/src/forms.css,v
retrieving revision 3.8
diff -u -r3.8 forms.css
--- forms.css   2001/03/11 13:26:55     3.8
+++ forms.css   2001/03/12 22:07:10
@@ -161,6 +161,10 @@
   padding: 0 5px 0 3px;
 }

+select[size="1"] > option {
+  padding: 0 5px 0 5px;
+}
+
 option[-moz-option-selected] {
   color: HighlightText ! important;
   background-color: Highlight ! important;

Comment 3

[Marc Attinasi]

Cool! So, does it look like this will be eliminated? I'll try the #ifdef with my
changes to prevent text nodes in selector matching and see what happens! Thanks Rod.

Comment 4

[Marc Attinasi]

Hey Rod, there is a textFrame being styled in CreateFrameFor too, and that is 
the one that is causing all of the grief. If I #if 0 that out like you did for 
the DisplayFrame then the highlight is not wide enough, and the scrollbar on the 
dropdown seems whacked. Can you please take a look?

Comment 5

[Marc Attinasi]

Kevin, the fix attached is not sufficient. Please see my comment from 2001-04-03
23:33

Clearing 'fix in hand' annotation.

Comment 6

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

The selector that is matching text nodes is
":-moz-display-comboboxcontrol-frame".  See:

http://lxr.mozilla.org/seamonkey/search?string=moz-display-comboboxcontrol-
http://lxr.mozilla.org/seamonkey/search?string=mozDisplayComboboxCont

Comment 7

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

Attachment: proposed patch

Comment 8

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

The above patch stops any selectors from matching the text node and doesn't
cause any appearance regressions that I can see.  It does two things:

 * makes the anonymous block frame a pseudo within the select content rather
than a pseudo within the text node so that the style context can match selectors

 * gives the text frame's style context the text node pseudo rather than the
-moz-display-combobox-controlframe pseudo

Does this seem reasonable?

Comment 9

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

This patch is subsumed by the larger patch in bug 56117.

Comment 10

[rods (gone)]

I tested it, it seems to size and work correctly.

r=rods

Comment 11

[Marc Attinasi]

sr=attinasi

Comment 12

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

Fixed within patch for bug 56117.