120189 - Make sure nsOutlinerContentView::AttributeChanged() won't die in a recursive loop

Status: RESOLVED FIXED

(Core :: XUL, defect, P3)

Description

[Jan Varga [:janv]]

<bryner> Jan: um, so, i think removing mIgnoreOptionSelected will cause problems
<Jan> bryner: I thought that there are used 2 different attributes
<bryner> oh, right
<bryner> nevermind
<bryner> :)
<bryner> er
<bryner> no, i think it will cause a problem
<bryner> here's why
<bryner> if an option is selected via script, the optionSelectedPseudo attribute
gets set
<bryner> that causes us to call into the outliner selection to toggle the
selected state...
<bryner> which then calls back into the content view to update state on the
SelectElement
<bryner> which then resets the optionSelectedPseudo attribute
<bryner> and around and around we go
<Jan> bryner: I'll file a new bug on me to investigate this case, I can't test
it again with all outliners :(
<bryner> it will only affect xbl select widgets
<Jan> yeah I know
<bryner> Jan: i sort of need those to work :)
<bryner> this would be solved by moving the select element update into the xbl
event handlers like one of your patches did
<bryner> it seems like there was a problem with that, and i can't remember what
it was
<Jan> yes!
<Jan> move it, move it

so maybe we just eliminate all callback notifications in C++
by moving them to XBL

Comment 1

[John Morrison]

*** Bug 120190 has been marked as a duplicate of this bug. ***

Comment 2

[Brian Ryner (not reading)]

Attachment: patch

Jan, can you please r= this?  I moved all of the content node state setting
into the event handlers and reverted the changes to OutlinerContentView and
OutlinerSelection to do that.

Comment 3

[Jan Varga [:janv]]

-> bryner

Comment 4

[Jan Varga [:janv]]

Comment on attachment 65529 [details] [diff] [review]
patch

this is so cool, nice work bryner
r=varga

Comment 5

[jag (Peter Annema)]

Comment on attachment 65529 [details] [diff] [review]
patch

sr=jag

I like it, ship it!

Comment 6

[Brian Ryner (not reading)]

Comment on attachment 65529 [details] [diff] [review]
patch

Hm, this doesn't quite work because of the attribute changed notification. 
Working on it.

Comment 7

[Brian Ryner (not reading)]

Attachment: patch v2.0

Here's a new patch.  Basically the same as the old one, but I added a notify
parameter to SetOptionsSelectedByIndex, which we set to false when we call this
from the event handler.  This prevents the duplicate notification.

Comment 8

[Jan Varga [:janv]]

Comment on attachment 65590 [details] [diff] [review]
patch v2.0

r=varga

Comment 9

[jag (Peter Annema)]

Comment on attachment 65590 [details] [diff] [review]
patch v2.0

sr=jag

Comment 10

[Brian Ryner (not reading)]

checked in.