Closed Bug 954683 Opened 10 years ago Closed 10 years ago

First listitem in log viewer never shows that it is selected

Categories

(Instantbird Graveyard :: Other, defect)

x86
All
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: aleth)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

*** Original post on bio 1252 at 2012-01-30 23:19:00 UTC ***

*** Due to BzAPI limitations, the initial description is in comment 1 ***
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1252 as attmnt 1144 at 2012-01-30 23:19:00 UTC ***

Due to what I suspect is an unknown mozilla bug, the first listitem in the log viewer never appears to be selected (even when it is). DOM Inspector shows this listitem never gets the 'selected' and 'current' attributes when it is selected (though the onselect event is fired, which is why things still worked).

As I was looking at this part of the code anyway, I added this workaround, which only actually selects the listitem after everything else has been initialized already. I didn't manage to narrow down exactly what in the sequencing was causing the problem, but it works now, and I'm not sure the issue warrants more time spent on it.
Attachment #8352889 - Flags: review?(florian)
*** Original post on bio 1252 at 2012-01-30 23:59:25 UTC ***

Clokep pointed out this is a dup of bug 953799 (bio 356).
*** Original post on bio 1252 at 2012-02-02 20:59:54 UTC ***

As per bug 953799 (bio 356), this bug affects Windows too, so the patch needs testing there also. (If it really has a timing component as per comments in that bug, if necessary one could remove line 80 (listbox.selectedIndex = 0) from the patch to be safe).
OS: Linux → All
Attached patch Patch with proper paths (obsolete) — Splinter Review
*** Original post on bio 1252 as attmnt 1162 at 2012-02-07 01:29:00 UTC ***

This looks OK to me (and works on Windows), I'm attaching a new patch with fixed paths for ease of flo to apply.
Attachment #8352907 - Flags: review?(florian)
Comment on attachment 8352889 [details] [diff] [review]
Patch

*** Original change on bio 1252 attmnt 1144 at 2012-02-07 01:29:02 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352889 - Attachment is obsolete: true
Attachment #8352889 - Flags: review?(florian)
Assignee: nobody → aleth
Status: NEW → ASSIGNED
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1252 as attmnt 1184 at 2012-02-21 14:30:00 UTC ***

I took another look at finding the cause of this and consequently improved the patch a little. As mentioned above, I presume it's a timing issue - one can make the first listitem bind using GetBoundingClientRect or ensureItemIsVisible, but if the listitem is selected immediately afterwards, there is a problem, which does not appear on OSX (though the underlying mozilla code there is the same as far as I can tell). There is no event that is fired when an XBL has finished initializing, which would otherwise be something to try. 

Obviously this is an ugly hack, especially as the delay chosen is somewhat arbitrary (even using 0 is enough for me, but may not be enough on slow machines?).

Needs testing on Windows again!
Attachment #8352934 - Flags: review?(florian)
Comment on attachment 8352907 [details] [diff] [review]
Patch with proper paths

*** Original change on bio 1252 attmnt 1162 at 2012-02-21 14:30:05 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352907 - Attachment is obsolete: true
Attachment #8352907 - Flags: review?(florian)
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1252 as attmnt 1185 at 2012-02-21 14:52:00 UTC ***

Use executeSoon, longer comment.
Attachment #8352935 - Flags: review?(florian)
Comment on attachment 8352934 [details] [diff] [review]
Patch

*** Original change on bio 1252 attmnt 1184 at 2012-02-21 14:52:04 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352934 - Attachment is obsolete: true
Attachment #8352934 - Flags: review?(florian)
*** Original post on bio 1252 at 2012-02-21 15:24:42 UTC ***

Comment on attachment 8352935 [details] [diff] [review] (bio-attmnt 1185)
Patch

The code looks OK, the indentation is strange though; and you have a double space before Ci.nsIEventTarget.
Attached patch PatchSplinter Review
*** Original post on bio 1252 as attmnt 1186 at 2012-02-21 16:45:00 UTC ***

Fixed indentation & spaces. (I thought the indentation was technically correct, as the function there appears as an argument, but it did look weird.)
Attachment #8352936 - Flags: review?(florian)
Comment on attachment 8352935 [details] [diff] [review]
Patch

*** Original change on bio 1252 attmnt 1185 at 2012-02-21 16:45:43 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352935 - Attachment is obsolete: true
Attachment #8352935 - Flags: review?(florian)
Comment on attachment 8352936 [details] [diff] [review]
Patch

*** Original change on bio 1252 attmnt 1186 at 2012-02-21 16:57:32 UTC ***

Ok, let's take this! Sorry for the delayed and confusing review comments!
Attachment #8352936 - Flags: review?(florian) → review+
*** Original post on bio 1252 at 2012-02-24 15:35:41 UTC ***

https://hg.instantbird.org/instantbird/rev/20c735606a4c
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.2
*** Original post on bio 1252 at 2012-06-12 10:46:41 UTC ***

(In reply to comment #0)

> Due to what I suspect is an unknown mozilla bug,

This is no longer an unknown bug, I filed https://bugzilla.mozilla.org/show_bug.cgi?id=763872 on it, with a patch.
Depends on: 763872
You need to log in before you can comment on or make changes to this bug.