Closed Bug 288821 Opened 20 years ago Closed 20 years ago

Select cleared on reload.

Categories

(Core :: Layout: Form Controls, defect)

Product:
Core
Component:
Layout: Form Controls
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: hhschwab, Assigned: MatsPalmgren_bugz)

References

(
URL
)

Details

(Keywords: regression, testcase)

Attachments

(5 files, 2 obsolete files)

Testcase #3
1.29 KB, text/html
Details
textcase #3 minimized
452 bytes, text/html
Details
Patch rev. 1
3.53 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
Patch rev. 1 (diff -w)
2.88 KB, patch
Details | Diff | Splinter Review
Patch B rev. 1 (additional patch)
3.78 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
User-Agent: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b2) Gecko/20050329 Build Identifier: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b2) Gecko/20050329 You are looking at this bugzilla page. Just do a reload, and look at the OS: Field. You shortly will see "Win98", and then it is cleared. When I change tabs, come back to a bug, sometimes more than OS is missing: I´ve got Bug 285730 in loaded in 2 tabs side by side, one is miising the contents of Component:, and to the right: OS: Version: Severity: Target Milestone: correctly shwon are Hardware: and Priority: I´m setting Severity to Major, as I don´t want to see this behaviour in the wild. Don´t mind, as long as it is seen only here. I can´t see this on a local copy of a bug page, so I can´t produce a testcase. Reproducible: Always Steps to Reproduce: 1. Scroll to the top of the bug 2. Reload, watch the contents of field OS: 3. Shift-Reload, watch the contents of field OS: Actual Results: on reload, the contents of some selects is shortly shown, then vanished. Mostly seen on OS: Expected Results: render as specified in the Select fields.
Attached file reduced testcase (obsolete) —
testcase stripped down from this page, added <base href="https://bugzilla.mozilla.org/"> to get it working locally. Should show Product: Core OS: Windows 98 On reload Windows98 and/or Core are shown shortly, then cleared. That doesn´t happen all the time, maybe multiple reloads are required. I´m seeing the bug always on the original bugpage, and often on the testcase.
Attached file minimized testcase (obsolete) —
Part of the source: <base href="https://bugzilla.mozilla.org/"> <link href="skins/standard/global.css" rel="stylesheet" type="text/css"> <link href="skins/custom/global.css" rel="stylesheet" type="text/css"> </head> <body bgcolor="#ffffff"> <table cellspacing="1" cellpadding="1" border="0"> <tr> <td align="right"><b><u>P</u>roduct:</b></td> <td> <label for="product" accesskey="p"> <select name="product" id="product"> <option value="Composer">Composer </option> <option value="Core" selected>Core </option> <option value="Directory">Directory </option> </select> </label> </td> CSS must be external, for getting a delay, I assume. Previous testcase didn´t have <label... but did have </label> I´m seeing the bug on Reload, always on the bugzilla page, often, not always on the testcase. Computer is a slow Celeron 333 128 MB RAM.
Attachment #179450 - Attachment is obsolete: true
So... both the bugzilla page and the testcase work fine here with a 2005-03-29-05 Linux trunk SeaMonkey nightly with no extensions.
Attached file Testcase #3 —
Forcing a reflow triggers it for me (on Reload).
Attachment #179454 - Attachment is obsolete: true
This is a pretty bad regression I would say, suggestions?
Assignee: nobody → mats.palmgren
Flags: blocking1.8b2?
OS: Windows 98 → All
Any idea why this is happening? It looks like either we're failing to call RedisplayText or we're failing to reflow afterward. Maybe we need to mark the select dirty too, not just the display frame?
Attached file textcase #3 minimized —
removed style and table, so only one select remains: <body bgcolor="#ffffff"> <select name="op_sys" id="op_sys"> <option value="Windows 98" selected>Windows 98</option> <script type="text/javascript" language="javascript">var x = window.scroll(0,1000);</script> <option value="Windows ME">Windows ME</option> </select> </body>
We get two calls to RedisplayText() in quick succession: [0x877d788] >>>>>>> RedisplayText: aIndex=-1 mDisplayedIndex=0 [0x877d788] RedisplayText: current="1" new="" [0x877d788] RedisplayText: shouldSetValue=1 [0x877d788] creating RedisplayTextEvent=0x8787dd8 "" posting RedisplayTextEvent on 0x877d788 [0x877d788] <<<<<<< RedisplayText: aIndex=-1 mDisplayedIndex=-1 [0x877d788] >>>>>>> RedisplayText: aIndex=0 mDisplayedIndex=-1 [0x877d788] RedisplayText: current="1" new="1" [0x877d788] RedisplayText: shouldSetValue=0 [0x877d788] <<<<<<< RedisplayText: aIndex=0 mDisplayedIndex=0 Document file:///usr/local/test/288821.html loaded successfully [0x877d788] ####### Handle event RedisplayTextEvent=0x8787dd8 "" [0x877d788] deleting RedisplayTextEvent aEvent=0x8787dd8 The problem is that the first event has not been handled when the second call is made and there is an optimization of sorts that peeks at current value and ignores the request if it's the same as the requested value. http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/forms/nsComboboxControlFrame.cpp&rev=1.311&root=/cvsroot&mark=1813-1825#1786 I think we should just remove that block. I guess we could count the number of outstanding events and ignore them in RedisplayTextEvent::HandleEvent if the number > 1 if we really care... but I don't think so - RedisplayText() does not seem like it's called that often and it doesn't seem like a heavy operation...
Attached patch Patch rev. 1 — — Splinter Review
Attachment #179487 - Flags: superreview?(bzbarsky)
Attachment #179487 - Flags: review?(bzbarsky)
Hmm... Don't we want to just cancel posted redisplay events if a redisplay comes in while they're posted? Otherwise it seems like we'll end up with the wrong index when all is said and done... That said, if this is indeed called rarely (for example, not called lots of times during page parsing as options are appended to the select), then I can see removing the optimization.
(In reply to comment #12) > Otherwise it seems like we'll end up with the wrong > index when all is said and done... mDisplayedIndex is updated directly in RedisplayText(), it's only the "refreshing" of the text frame that is delayed with the event and what text to display is carried with the event so the update is not index dependent. I don't see how we could end up with the wrong index... I tested it on a <select> with many <option>s (I took the testcase in bug 252158 and changed them to size="1"). On SHIFT-Reload I get zero calls to RedisplayText. On Reload I get 6 calls to RedisplayText (there are 5 <select>s) I also tried it on this Bugzilla page. SHIFT-Reload: zero calls. Reload: 9 calls (there are 13 <select size=1> here and 9 of them have an <option selected> as far as I can see).
Comment on attachment 179487 [details] [diff] [review] Patch rev. 1 r+sr=bzbarsky
Attachment #179487 - Flags: superreview?(bzbarsky)
Attachment #179487 - Flags: superreview+
Attachment #179487 - Flags: review?(bzbarsky)
Attachment #179487 - Flags: review+
Actually, I think we still have one problem. PLEvents are not guaranteed to be delivered in the order posted if you always post to the current event queue (because the event posted later may get posted to a nested event queue). So I think we want to revokeEvents any time we're posting an event here, so that we only have one event in flight at any given time...
(In reply to comment #15) > PLEvents are not guaranteed to be delivered in the order posted Wow, how nasty, I didn't know that. Unfortunately I've already checked in Patch rev. 1 (2005-04-03 12:58 PDT). > So I think we want to revokeEvents any time we're posting an event here Ok, I will do a followup patch for that..
Trace output with this patch: [0x8765f9c] >>>>>>> RedisplayText: aIndex=-1 mDisplayedIndex=0 [0x8765f9c] creating RedisplayTextEvent=0x8760a58 "" [0x8765f9c] posting RedisplayTextEvent 0x8760a58 "" [0x8765f9c] <<<<<<< RedisplayText: aIndex=-1 mDisplayedIndex=-1 [0x8765f9c] >>>>>>> RedisplayText: aIndex=0 mDisplayedIndex=-1 [0x8765f9c] creating RedisplayTextEvent=0x8760a98 "1" [0x8765f9c] RevokeEvents [0x8765f9c] deleting RedisplayTextEvent aEvent=0x8760a58 [0x8765f9c] posting RedisplayTextEvent 0x8760a98 "1" [0x8765f9c] <<<<<<< RedisplayText: aIndex=0 mDisplayedIndex=0 Document file:///usr/local/test/288821.html loaded successfully [0x8765f9c] ####### Handle event RedisplayTextEvent=0x8760a98 "1" [0x8765f9c] deleting RedisplayTextEvent aEvent=0x8760a98
Attachment #179512 - Flags: superreview?(bzbarsky)
Attachment #179512 - Flags: review?(bzbarsky)
Comment on attachment 179512 [details] [diff] [review] Patch B rev. 1 (additional patch) r+sr=bzbarsky. Looks great.
Attachment #179512 - Flags: superreview?(bzbarsky)
Attachment #179512 - Flags: superreview+
Attachment #179512 - Flags: review?(bzbarsky)
Attachment #179512 - Flags: review+
Hermann, thanks for the heads up, if you could verify the fix on that slow Windows box that would be great. Thanks again. Patch B rev. 1 (additional patch) checked in 2005-04-03 15:16 PDT. -> FIXED
Status: NEW → RESOLVED
Closed: 20 years ago
Flags: blocking1.8b2?
Resolution: --- → FIXED
Boris, I'm a bit curious under which circumstances a nested event queue is created?
Any time a modal dialog is posed, for example... So say you put up an alert in window A while an event is posed for a <select> in window B; then event won't fire until the alert comes down.
On Win98SE, Athlon XP1600, using testcase 3, I always can see the regression in the regressed builds, and I could see it on the normal bugpage sometimes, when I did a reload while doing a fast download via DSL. tested regressed: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b2) Gecko/20050401 tested working: tinderbox build BuildID 2005040322
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: