If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

dropdown select options chopped off

RESOLVED FIXED

Status

()

Core
Layout: Form Controls
RESOLVED FIXED
13 years ago
12 years ago

People

(Reporter: Andrew Schultz, Assigned: mats)

Tracking

({regression, testcase})

Trunk
x86
Linux
regression, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

13 years ago
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

13 years ago
Created attachment 185918 [details]
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.
(Assignee)

Comment 2

13 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

13 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)
(Assignee)

Comment 4

13 years ago
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?
(Assignee)

Comment 6

13 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

13 years ago
Created attachment 186060 [details] [diff] [review]
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
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.
(Assignee)

Comment 10

13 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.
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

12 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

12 years ago
Created attachment 186423 [details]
Testcase #2 (SHIFT-Reload then Reload)
(Assignee)

Updated

12 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

12 years ago
I agree, that is a more robust solution. New patch coming up shortly...
(Assignee)

Comment 17

12 years ago
Created attachment 186660 [details]
Testcase #3 (performance test)
(Assignee)

Comment 18

12 years ago
Created attachment 186662 [details] [diff] [review]
Patch rev. 2

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

12 years ago
Attachment #186662 - Flags: approval1.8b3?

Updated

12 years ago
Attachment #186662 - Flags: approval1.8b3? → approval1.8b3+
(Assignee)

Comment 20

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

-> FIXED
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Flags: blocking1.8b3?
Resolution: --- → FIXED
Depends on: 311380
Blocks: 227683
(Assignee)

Updated

12 years ago
Depends on: 313260
You need to log in before you can comment on or make changes to this bug.