Closed Bug 591619 Opened 9 years ago Closed 9 years ago

combo box displays text not inside option tag

Categories

(Core :: Layout: Form Controls, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla2.0b8
Tracking Status
blocking2.0 --- final+

People

(Reporter: tspiteri, Assigned: bzbarsky)

References

()

Details

(Keywords: html5, regression)

Attachments

(1 file, 3 obsolete files)

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b4) Gecko/20100818 Firefox/4.0b4
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b4) Gecko/20100818 Firefox/4.0b4

When there is text outside of option tags, it is still displayed in the combo box, unlike Firefox 3.6 and Internet Explorer 8. The page linked in the URL field has this snippet:
<input type="hidden" name="scaleValue" value="0" />
            <select id="selectedWeek" name="selectedWeek" onChange="submit()">
            <option value ="0" SELECTED>This Week</option>
            if( $weekcount > 1 ) {
            <option value ="1" >Next week</option>

            }
                  <option value =2 >2 Weeks Out</option>
                  <option value =3 >3 Weeks Out</option>
                  <option value =4 >4 Weeks Out</option>
                  <option value =5 >5 Weeks Out</option>
                  <option value =6 >6 Weeks Out</option>
            </select>

In Firefox 4.0b4, the combo box displays "if( $weekcount > 1 ) {" and "}" as seprate unclickable lines.

Reproducible: Always
Doing a quick Check, this regressed within Alpha 4 <-> 5.
Component: General → Layout: Form Controls
Product: Firefox → Core
QA Contact: general → layout.form-controls
Version: unspecified → Trunk
Regression from HTML5 parser (turning it off fixes the bug).

Henri, with the old parser the text seems to be gone from the DOM.  What do other browsers do?  At first glance webkit has the text in the DOM; Opera does not.  What about IE?
Status: UNCONFIRMED → NEW
blocking2.0: --- → ?
Component: Layout: Form Controls → HTML: Parser
Ever confirmed: true
QA Contact: layout.form-controls → parser
Keywords: html5
OS: Windows 7 → All
Hardware: x86 → All
Looks like maybe IE leaves the text in.

If this is a wontfix on the parser end, we'll need to deal with it on the layout side somehow...
Also, looks like the old parser dropped all tags that were kids of <select>
completely, while the new one drops the tags but lets their text kids be kids
of the <select>?

If we do address this on the layout side, I'll need to know exactly what I can
expect the new content model of <select> to look like.
(In reply to comment #4)
> If we do address this on the layout side, I'll need to know exactly what I can
> expect the new content model of <select> to look like.

Regardless of what the HTML parser does, this is "anything" through XML and the DOM, no?

It looks like we should display the contents of select>option and select>optgroup>option, see <http://www.whatwg.org/html/#the-select-element-0>.
> Regardless of what the HTML parser does, this is "anything" through XML and the
> DOM, no?

Yes, but I have no issue with showing the text in that case, frankly.

I suppose we can suppress non-option-non-optgroup kids of <select> and non-option kids of <optgroup>, though...  The latter will be somewhat of a pain, since right now <optgroup> is just a normal block.
And even if I suppress them in frame construction, it's quite possible that there's various code in the listbox and combobox frames that would still do things with the DOM nodes.  Someone needs to audit.
Henri, can you have a look?
Assignee: nobody → hsivonen
blocking2.0: ? → final+
Since the parser behavior matches IE8, IE9 PP and old WebKit parser (and, of course, the new WebKit parser), my first choice would be to treat this as a layout bug in Gecko and not as a spec or parser bug--in particular because there now isn't an "in option" insertion mode.

Boris, is that OK with you?
Yes, if I get an answer to my question from comment 4.
(In reply to comment #4)
> Also, looks like the old parser dropped all tags that were kids of <select>
> completely, while the new one drops the tags but lets their text kids be kids
> of the <select>?
> 
> If we do address this on the layout side, I'll need to know exactly what I can
> expect the new content model of <select> to look like.

The HTML5 parsing algorithm can put the following as a child of <select>:
 * Text nodes
 * Script element nodes
 * Comment nodes
 * Option element nodes
 * Optgroup element nodes

The HTML5 parsing algorithm can put the following as a child of <optgroup>:
 * Text nodes
 * Script element nodes
 * Comment nodes
 * Option element nodes

The HTML5 parsing algorithm can put the following as a child of <option>:
 * Text nodes
 * Script element nodes
 * Comment nodes
Assignee: hsivonen → nobody
Component: HTML: Parser → Layout: Form Controls
QA Contact: parser → layout.form-controls
Duplicate of this bug: 605076
Assignee: nobody → bzbarsky
Priority: -- → P1
There seems to be neither spec nor interop for how this should behave outside a <select>, so I'll limit the magic to inside <select>s.
Attachment #484624 - Attachment is obsolete: true
Comment on attachment 484778 [details] [diff] [review]
Don't create frames for non-option kids of <optgroup> or non-option and non-optgroup kids of <select>.

I decided to do this instead of using CSS, because I can't match text nodes in CSS and because I wanted to avoid |select > :not(:any(option, optgroup))| rules hanging out in forms.css.
Attachment #484778 - Flags: review?(dbaron)
Whiteboard: [need review]
Comment on attachment 484778 [details] [diff] [review]
Don't create frames for non-option kids of <optgroup> or non-option and non-optgroup kids of <select>.

This seems like it isn't going to behave correctly when children are dynamically inserted or removed inside a select or an optgroup; we'll get frames for them in the dynamic case in cases where we wouldn't statically.

Otherwise it looks fine, though.
Attachment #484778 - Flags: review?(dbaron) → review-
Ah, because I need to set mInHTMLSelect.  Gah.  I'll add a test for that.

Are you OK with me just removing mInHTMLSelect and the check for it?  It was meant to be an optimization, but recovering that state on dynamic changes doesn't seem worth it.
[2010-11-29 21:58:33] <bz> so for that option/select thing
[2010-11-29 21:58:46] <bz> any objections to me just dropping the "is inside select element" check?
[2010-11-29 21:59:08] <bz> (and slightly modifying the rest of the condition to deal with null parent)
[2010-11-29 21:59:22] <dbaron> the potential issue there is perf?
[2010-11-29 21:59:32] <dbaron> sounds fine, assuming it performs reasonably
[2010-11-29 21:59:38] <bz> yeah
[2010-11-29 21:59:44] <dbaron> maybe there's a way to reorder the checks so it's faster for the common cases?
[2010-11-29 21:59:50] <bz> even perf-wise, it's basically an extra Tag() compare 
[2010-11-29 21:59:58] <bz> yeah, I'll look into it
[2010-11-29 22:00:03] <dbaron> sounds fine, though
[2010-11-29 22:00:05] <bz> ok
[2010-11-29 22:00:14] <bz> I'll post a patch tomorrow
[2010-11-29 22:00:16] <dbaron> k
Attachment #484778 - Attachment is obsolete: true
Comment on attachment 494644 [details] [diff] [review]
Don't create frames for non-option kids of <optgroup> or non-option and non-optgroup kids of <select>.

This also adds the dynamic test the previous patch failed.
Attachment #494644 - Flags: review?(dbaron)
Attachment #494644 - Attachment is obsolete: true
Attachment #494644 - Flags: review?(dbaron)
Attachment #494723 - Flags: review?(dbaron)
Comment on attachment 494723 [details] [diff] [review]
And even without crashing on null parent frame

r=dbaron
Attachment #494723 - Flags: review?(dbaron) → review+
Whiteboard: [need review] → [need landing]
Pushed http://hg.mozilla.org/mozilla-central/rev/e0285b393e11 and then http://hg.mozilla.org/mozilla-central/rev/e19fb998583e to restore the check for anonymous kids that got lost.
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [need landing]
Target Milestone: --- → mozilla2.0b8
You need to log in before you can comment on or make changes to this bug.