Closed Bug 297389 Opened 16 years ago Closed 16 years ago

dropdown select options chopped off

Categories

(Core :: Layout: Form Controls, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: ajschult784, Assigned: mats)

References

()

Details

(Keywords: regression, testcase)

Attachments

(4 files, 1 obsolete file)

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.
Attached file 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.
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.
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)
I can reproduce the bug using your PHP script, thanks.
Flags: blocking1.8b3?
Mats, are you looking into this or should I do it when I get back?
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

Assignee: nobody → mats.palmgren
Attached patch Patch rev. 1 (obsolete) — Splinter Review
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
Attachment #186060 - Flags: superreview?(bzbarsky)
Attachment #186060 - Flags: review?(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.
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.
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.
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...
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...
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.
Attachment #186060 - Attachment is obsolete: true
Attachment #186060 - Flags: superreview?(bzbarsky)
Attachment #186060 - Flags: review?(bzbarsky)
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?
I agree, that is a more robust solution. New patch coming up shortly...
Attached patch Patch rev. 2Splinter Review
Performance for Testcase #3 is the same as before.
Attachment #186662 - Flags: superreview?(roc)
Attachment #186662 - Flags: review?(roc)
Comment on attachment 186662 [details] [diff] [review]
Patch rev. 2

Looks great!
Attachment #186662 - Flags: superreview?(roc)
Attachment #186662 - Flags: superreview+
Attachment #186662 - Flags: review?(roc)
Attachment #186662 - Flags: review+
Attachment #186662 - Flags: approval1.8b3?
Attachment #186662 - Flags: approval1.8b3? → approval1.8b3+
Checked in to trunk at 2005-06-20 17:00 PDT

-> FIXED
Status: NEW → RESOLVED
Closed: 16 years ago
Flags: blocking1.8b3?
Resolution: --- → FIXED
Depends on: 311380
Blocks: 227683
Depends on: 313260
You need to log in before you can comment on or make changes to this bug.