297389 - dropdown select options chopped off

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Andrew Schultz]

With linux trunk 2005060905, when I load the bug page in the URL, the "XP Apps:
GUI Features" shows as "XP Apps: G".  This has been reported before, but it was
never reproducible, I can't find the bug and I think it's resolved WFM.  And I
have a testcase.  This bug doesn't show up with 1.7.x builds, but shows up with
1.8a1.  I can poke nightlies before that if needed.

Comment 1

[Andrew Schultz]

Attachment: testcase

with linux trunk 2005060905, I see "XP Apps:" with the testcase if I serve up
the first 290 bytes and then the rest.	the first 290 bytes ends at 'Features"
selected>XP Apps: '.  Serving up the whole document at once and using another
document.body.offsetHeight instead does not cause the bug.

Comment 2

[Mats Palmgren (inactive)]

WFM, Mozilla nightly build 2005-06-10-03 Linux xft+gtk2.
I tried both with file: and http: using a FIFO and 'netcat' respectively to
introduce a delay as you said (I tried 1 sec and 10 sec delays).
Please provide more details on your test environment.

Comment 3

[Andrew Schultz]

I'm serving up the file with a php script running on my local machine.  I've
noticed that http connections (at least over a network) tend to get buffered,
which screws everything up.

Do you see part of the file initially?  With just the first 290 bytes, you
should get a select box with "XP Apps:".  When the rest of the file arrives, the
select box expands to the appropriate size, but the rest of the text is missing?

The php script I'm using is http://rheneas.eng.buffalo.edu/~andrew/slow.php
I can reproduce the bug with chunk=290 and delay=100000 (0.1 seconds)

Comment 4

[Mats Palmgren (inactive)]

I can reproduce the bug using your PHP script, thanks.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Mats, are you looking into this or should I do it when I get back?

Comment 6

[Mats Palmgren (inactive)]

nsComboboxControlFrame is probably a bit to eager to display the
selected option. OTOH, I can't find any notification for when an
option is complete to use for redisplaying the value either...

Here's an ad-hoc trace of what happens:

nsListControlFrame::AddOption 0: ""
nsHTMLSelectElement::InsertOptionsIntoList calling OnOptionSelected
nsHTMLSelectElement::OnOptionSelected calling OnOptionSelected
nsComboboxControlFrame::OnOptionSelected() calling RedisplayText(0)
>>>nsComboboxControlFrame::RedisplayText
  Posted RedisplayTextEvent ""
<<<nsComboboxControlFrame::RedisplayText
>>>nsComboboxControlFrame::HandleRedisplayTextEvent
  calling FlushPendingNotifications(Flush_ContentAndNotify)
nsComboboxControlFrame::RedisplaySelectedText() calling RedisplayText(0)
>>>nsComboboxControlFrame::RedisplayText
  Revoking earlier RedisplayTextEvent
  Posted RedisplayTextEvent "XP Apps:"
<<<nsComboboxControlFrame::RedisplayText
  done FlushPendingNotifications(Flush_ContentAndNotify)
<<<nsComboboxControlFrame::HandleRedisplayTextEvent
nsListControlFrame::DoneAddingChildren 1
>>>nsComboboxControlFrame::HandleRedisplayTextEvent
  calling FlushPendingNotifications(Flush_ContentAndNotify)
  done FlushPendingNotifications(Flush_ContentAndNotify)
<<<nsComboboxControlFrame::HandleRedisplayTextEvent

Comment 7

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

Sort of a wallpaper, but I couldn't think of a better way.
Let me know if you have any suggestions.

With this patch we get:

nsListControlFrame::AddOption 0: ""
nsHTMLSelectElement::InsertOptionsIntoList calling OnOptionSelected
nsHTMLSelectElement::OnOptionSelected calling OnOptionSelected
nsComboboxControlFrame::OnOptionSelected() calling RedisplayText(0)
>>>nsComboboxControlFrame::RedisplayText
  Posted RedisplayTextEvent ""
<<<nsComboboxControlFrame::RedisplayText
>>>nsComboboxControlFrame::HandleRedisplayTextEvent
  calling FlushPendingNotifications(Flush_ContentAndNotify)
nsComboboxControlFrame::RedisplaySelectedText() calling RedisplayText(0)
>>>nsComboboxControlFrame::RedisplayText
  Revoking earlier RedisplayTextEvent
  Posted RedisplayTextEvent "XP Apps:"
<<<nsComboboxControlFrame::RedisplayText
  done FlushPendingNotifications(Flush_ContentAndNotify)
<<<nsComboboxControlFrame::HandleRedisplayTextEvent
nsListControlFrame::DoneAddingChildren 1
nsComboboxControlFrame::RedisplaySelectedText() calling RedisplayText(0)
>>>nsComboboxControlFrame::RedisplayText
  Posted RedisplayTextEvent "XP Apps: GUI Features"
<<<nsComboboxControlFrame::RedisplayText
>>>nsComboboxControlFrame::HandleRedisplayTextEvent
  calling FlushPendingNotifications(Flush_ContentAndNotify)
  done FlushPendingNotifications(Flush_ContentAndNotify)
<<<nsComboboxControlFrame::HandleRedisplayTextEvent
>>>nsComboboxControlFrame::HandleRedisplayTextEvent
  calling FlushPendingNotifications(Flush_ContentAndNotify)
  done FlushPendingNotifications(Flush_ContentAndNotify)
<<<nsComboboxControlFrame::HandleRedisplayTextEvent

Comment 8

[Boris Zbarsky [:bzbarsky]]

> I can't find any notification for when an option is complete

The DoneAddingChildren notification is what we want here, I would think.  See
nsHTMLContentSink and nsXMLContentSink.  It's only called for certain nodes;
you'd have to add option to the list.

Comment 9

[Boris Zbarsky [:bzbarsky]]

The patch looks like it should work reasonably well; if using DoneAddingChildren
on the options is too complicated (or too slow, even), we should go with this
fix.  Let me know whether you want me to review it or whether you want to try
DoneAddingChildren on options.

Comment 10

[Mats Palmgren (inactive)]

Reading the comment at:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/base/public/nsIContent.h&rev=3.109&root=/cvsroot&mark=626-638#623

makes me think adding a DoneAddingChildren() for <option>s to solve this problem
isn't worth it and could be a inefficient on pages with many options.
I don't see any real advantages with that solution compared to the suggested one,
so please review it as is.

Comment 11

[Boris Zbarsky [:bzbarsky]]

I can try.  If I don't get to it before the 19th, though (which is looking very
likely with the connection I have here), then it won't be until July...

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

When the parser appends content to the DOM, shouldn't
nsHTMLOptionElement::NotifyTextChanged be called? Either the parser adds a new
text node child to the option, or it modifies the existing text node child. It
looks like it must be the latter since the former is already monitored by
nsHTMLOptionElement::InsertChildAt/nsHTMLOptionElement::AppendChild. It looks
like the latter is not detected anywhere (nor is someone modifying the text
node's DOM "data" attribute). Maybe that's the right way to fix this...

Comment 13

[Mats Palmgren (inactive)]

nsHTMLOptionElement::NotifyTextChanged is internal and it doesn't look like it
was intended to be used outside the class. But you're right in your analysis
of what occurs. We get a nsHTMLOptionElement::AppendChildTo with the first
part of the text and later there is a call to nsGenericDOMDataNode::AppendData
with the rest, which calls document->CharacterDataChanged(this, PR_TRUE);
and then nsTextFrame::CharacterDataChanged causes a reflow.

The same problem occurs with nsGenericDOMDataNode::SetData as the next
testcase demonstrates.

Comment 14

[Mats Palmgren (inactive)]

Attachment: Testcase #2 (SHIFT-Reload then Reload)

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It looks like all the text-changed observation in nsHTMLOptionElement and in the
frame classes funnels into nsComboboxControlFrame::RedisplaySelectedText. So how
about we stop trying to observe content changes and rely instead on any change
to the text triggering a reflow against the option frame or its child text node?
Every time we reflow the comboboxcontrolframe, we could compare the current text
of the selected option to the old text; if the text has changed, we do
RedisplaySelectedText. Thoughts?

Comment 16

[Mats Palmgren (inactive)]

I agree, that is a more robust solution. New patch coming up shortly...

Comment 17

[Mats Palmgren (inactive)]

Attachment: Testcase #3 (performance test)

Comment 18

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

Performance for Testcase #3 is the same as before.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 186662 [details] [diff] [review]
Patch rev. 2

Looks great!

Comment 20

[Mats Palmgren (inactive)]

Checked in to trunk at 2005-06-20 17:00 PDT

-> FIXED