Closed
Bug 297389
Opened 20 years ago
Closed 20 years ago
dropdown select options chopped off
Categories
(Core :: Layout: Form Controls, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: ajschult784, Assigned: MatsPalmgren_bugz)
References
()
Details
(Keywords: regression, testcase)
Attachments
(4 files, 1 obsolete file)
377 bytes,
text/html
|
Details | |
527 bytes,
text/html
|
Details | |
104.58 KB,
text/html
|
Details | |
25.75 KB,
patch
|
roc
:
review+
roc
:
superreview+
asa
:
approval1.8b3+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•20 years ago
|
||
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.
Assignee | ||
Comment 2•20 years ago
|
||
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.
Reporter | ||
Comment 3•20 years ago
|
||
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)
![]() |
||
Updated•20 years ago
|
Flags: blocking1.8b3?
Assignee | ||
Comment 6•20 years ago
|
||
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
Assignee | ||
Comment 7•20 years ago
|
||
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)
![]() |
||
Comment 8•20 years ago
|
||
> 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•20 years ago
|
||
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.
Assignee | ||
Comment 10•20 years ago
|
||
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•20 years ago
|
||
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...
Assignee | ||
Comment 13•20 years ago
|
||
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.
Assignee | ||
Comment 14•20 years ago
|
||
Assignee | ||
Updated•20 years ago
|
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?
Assignee | ||
Comment 16•20 years ago
|
||
I agree, that is a more robust solution. New patch coming up shortly...
Assignee | ||
Comment 17•20 years ago
|
||
Assignee | ||
Comment 18•20 years ago
|
||
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+
Assignee | ||
Updated•20 years ago
|
Attachment #186662 -
Flags: approval1.8b3?
Updated•20 years ago
|
Attachment #186662 -
Flags: approval1.8b3? → approval1.8b3+
Assignee | ||
Comment 20•20 years ago
|
||
Checked in to trunk at 2005-06-20 17:00 PDT -> FIXED
Status: NEW → RESOLVED
Closed: 20 years ago
Flags: blocking1.8b3?
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•