306299 - EM/TM have lost focus memory after landing of bug 300780

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Przemyslaw Bialik]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b4) Gecko/20050828 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b4) Gecko/20050828 Firefox/1.0+

EM/TM have lost focus memory after landing of bug 300780.
Bug 300780 Comment #6 states that this wasn't intended (it was meant to be only
about initial focus in DM).
Read Bug 300780 Comment #5 for some focus history ;)

Reproducible: Always

Steps to Reproduce:
1. Open EM and select one of the installed extensions (other than the first one)
2. Close and reopen EM

Actual Results:  
Focus goes to the first extension.

Expected Results:  
Focus goes to the last selected extension.

Comment 1

[Robert Strong (they/them - no direct email)]

cc -> aaronleventhal

Comment 2

[Asa Dotzler [:asa]]

aaron, can you look into this? It seems like this may be a toolkit wide
regression that we don't want for 1.5.

Comment 3

[Aaron Leventhal]

(In reply to comment #2)
> aaron, can you look into this? It seems like this may be a toolkit wide
> regression that we don't want for 1.5.

It's not toolkit wide, although I'm looking at it. Only the extension manager
and theme manager implemented selection memory. In my opinion this should be
implemented by richlistbox for all of them. I also think this isn't a blocker,
although it should be fixed.

Comment 4

[Brendan Eich [:brendan]]

(In reply to comment #3)
> It's not toolkit wide, although I'm looking at it. Only the extension manager
> and theme manager implemented selection memory. In my opinion this should be
> implemented by richlistbox for all of them. I also think this isn't a blocker,
> although it should be fixed.

You are missing the point.  Your change was an incompatible widget API change. 
Period, end of story.

Maybe it's a good change otherwise.  Perhaps the incompatibility could be
tolerated (we don't have a great record here, or process for testing before
shipping to find out).  But it's way late in the game to be changing toolkit
level widget semantics.  That's what Asa was getting at.

/be

Comment 5

[Benjamin Smedberg]

On the contrary, we added richlistbox in the 1.8 cycle, so we haven't shipped
this API yet.

Comment 6

[Aaron Leventhal]

Brendan, I said I was looking at it and that it should be fixed. I also wanted
to there are problems with the implementation of selection memory, so that we
can eventually do this right. Perhaps someone who worked on it can comment why
we call init() for richlistbox only in extension and theme manager, and not in
the download manager.

Comment 7

[Brendan Eich [:brendan]]

(In reply to comment #5)
> On the contrary, we added richlistbox in the 1.8 cycle, so we haven't shipped
> this API yet.

I stand corrected.

However, here we are with a regression from what may be a fine toolkit change. 
Was now the time for this change?  If it made other cases better and regressed
only EM/TM, maybe.  But it's not obvious.

This bug is nominated for blocking1.8b4, time is short, and there's no patch. 
What is the plan?

/be

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

I don't think it's a disastrous regression for 1.5 in terms of user experience.
I can see it being annoying in some use cases surrounding managing and updating
one's extensions (ie: update, see the "will be updated when you continue"
message, reload browser and open to confirm that it updated and have to scroll
down) and others surrounding selecting the current theme (ie: it will be
somewhat odd to open the theme manager and not have the currently selected theme
be the one in focus). But I don't expect these use cases to be encountered
frequently.

If the new widget implementation is providing significant advantage in other
places, I'd say keep it, and get the behaviour fixed ASAP, but don't block
release for it.

(note: this is also the sort of thing that, IMO, can get packaged comfortably as
an incremental post-1.5 update like 1.5.1, as it simply restores the widget to
standard and expected behaviour)

Comment 9

[Brendan Eich [:brendan]]

beltzner: what is the rationale for Download Manager not remembering the
selected item, while EM and TM have heretofore done so?

I'd be happier if we answered these crisply, before just triaging and moving on:

1.  Is the new API the right one?
1a.  Is the default correct?
1b.  Should there be a declarative way to get the memoized selection behavior?
2.  Should EM and TM behave as they have, while DM differs?

I don't think we should treat widgets in toolkit as experimental and subject to
incompatible change after we release, unless we have a clear mark per widget or
API and a process for freezing.  Too many times we think we're the only user of
a widget, and we are wrong in ways that bind us forever.  Do we have such a mark
and process?

/be

Comment 10

[Doron Rosenberg (IBM)]

Note that the new widget did not cause this, rather Aaron's patch to make it
more accessible did.  I made sure the new widget behaved like the widget it was
replacing.  And as was brought up before, the selectinh the last selected item
was never part of the original widget, is is code living inside the manager's code.

The patch in 300780 was never ideal, the main widget should be setting inital
focus, but that is post-1.5 fodder.

Comment 11

[Brendan Eich [:brendan]]

(In reply to comment #10)
> Note that the new widget did not cause this, rather Aaron's patch to make it
> more accessible did.

Yeah, I know -- that's what I wrote in comment 4.

> I made sure the new widget behaved like the widget it was replacing.

Good for you!

> And as was brought up before, the selectinh the last selected item
> was never part of the original widget, is is code living inside the manager's
> code.

Yes, but now that code is broken.

> The patch in 300780 was never ideal, the main widget should be setting inital
> focus, but that is post-1.5 fodder.

Ok then.

Anyone else want to consider the design questions in comment 9?

/be

Comment 12

[Aaron Leventhal]

Attachment: Low risk patch: 1) when last-selected persistance is being utilized, don't fixup initial selection, 2) when selected item destroyed, persist selection position in list rather than clearSelection()

I realize Doron is not a peer, but for this patch he is the most appropriate
reviewer. Mconnor is away on vacation.

Comment 13

[Doron Rosenberg (IBM)]

Comment on attachment 194364 [details] [diff] [review]
Low risk patch: 1) when last-selected persistance is being utilized, don't fixup initial selection, 2) when selected item destroyed, persist selection position in list rather than clearSelection()

That is pretty evil :)

How about introducing a new boolean attribute (selectFirstItem?)?  That would
be cleaner probably.

Comment 14

[Aaron Leventhal]

(In reply to comment #13)
> (From update of attachment 194364 [details] [diff] [review] [edit])
> That is pretty evil :)
> 
> How about introducing a new boolean attribute (selectFirstItem?)?  That would
> be cleaner probably.
> 

I was looking for the smallest lowest risk change. First I tried just to see if
last-selected itself was set, but for some reason it wasn't set yet at that point.

It seems to me if last-selected is not set then you'd want to select the first
item. Since I couldn't do that I just checked for persistance.

Ultimately I think we shouldn't have a separate persist="last-selected",
"last-selected" and selectFirstItem. That's why I resist adding that new
attribute now.

Comment 15

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #9)
> beltzner: what is the rationale for Download Manager not remembering the
> selected item, while EM and TM have heretofore done so?

I really can't think of many, but wasn't around when that widget was designed.
Why don't I describe what I think the ideal interaction would be for any & all
managers. In order of selection-preference ..:

1st choice = the active entry (ie: current theme or download)
2nd choice = the most-recently added entry (ie: new theme, extension, download)
3rd choice = the most-recently selected entry
4th choice = the first entry in the list

Selection should only persist within a selection, I'd think. This design is
based on what is most likely interesting to a user who's opening the various
managers.

Comment 16

[Brendan Eich [:brendan]]

(In reply to comment #14)
> It seems to me if last-selected is not set then you'd want to select the first
> item. Since I couldn't do that I just checked for persistance.

Clever!
 
> Ultimately I think we shouldn't have a separate persist="last-selected",
> "last-selected" and selectFirstItem. That's why I resist adding that new
> attribute now.

I think I agree.  Just give me a clue -- why does setting listBox.selectedIndex
= itself do anything magic?  I'da thunk the widget would keep "get" and "set"
state for selectedIndex (including side effects) coherent.

/be

Comment 17

[Aaron Leventhal]

> why does setting listBox.selectedIndex = itself do anything magic?

I don't know how templates work, but it seems that whenever there is a change
all the current items get removed from the DOM tree and a bunch of new ones get
created. Setting it to itself makes sure that selectedItem points to one of the
new guys.

Comment 18

[Brendan Eich [:brendan]]

(In reply to comment #17)
> > why does setting listBox.selectedIndex = itself do anything magic?
> 
> I don't know how templates work, but it seems that whenever there is a change
> all the current items get removed from the DOM tree and a bunch of new ones get
> created. Setting it to itself makes sure that selectedItem points to one of the
> new guys.

Comment that odd fact and unsolicited sr=me.

/be

Comment 19

[Asaf Romano (gone)]

Comment on attachment 194364 [details] [diff] [review]
Low risk patch: 1) when last-selected persistance is being utilized, don't fixup initial selection, 2) when selected item destroyed, persist selection position in list rather than clearSelection()

>Index: toolkit/content/widgets/richlistbox.xml
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/content/widgets/richlistbox.xml,v
>retrieving revision 1.17
>diff -p -u -5 -r1.17 richlistbox.xml
>--- toolkit/content/widgets/richlistbox.xml	30 Aug 2005 15:45:39 -0000	1.17
>+++ toolkit/content/widgets/richlistbox.xml	30 Aug 2005 20:55:29 -0000
>@@ -320,12 +320,14 @@
> 
>     <implementation implements="nsIAccessibleProvider, nsIDOMXULSelectControlItemElement">
>       <constructor>
>         <![CDATA[
>           var listBox = this.control;
>-          if (!listBox.selectedItem)
>-            listBox.goDown();  // Fix up initial focus as items are created
>+          if (!listBox.selectedItem) {
>+            if (listBox.getAttribute("persist").indexOf("last-selected") == -1)
>+              listBox.goDown();  // Fix up initial focus as items are created
>+          }

So, if i'm reading this right, the first time you open a richlistbox, no item
is selected.

Comment 20

[Asaf Romano (gone)]

So, it seems EM calls <richlistbox>.init() in order to get the last-selected
stuff done, that (and the whole last-selected code inside the EM) should be
moveed into the widget itself. We shouldn't requier clients of this widget to
ever call init; even worse when it's only necessary sometimes.

If persisting |last-selected| hasn't been done before the ctor, let's move the
focus stuff from the ctor to the init method and call it from the ctor by
setTimeout... :-/

(In other news, why isn't attributes-persisting already done at this point?)

Comment 21

[Doron Rosenberg (IBM)]

(In reply to comment #20)
> So, it seems EM calls <richlistbox>.init() in order to get the last-selected
> stuff done, that (and the whole last-selected code inside the EM) should be
> moveed into the widget itself. We shouldn't requier clients of this widget to
> ever call init; even worse when it's only necessary sometimes.
> 
> If persisting |last-selected| hasn't been done before the ctor, let's move the
> focus stuff from the ctor to the init method and call it from the ctor by
> setTimeout... :-/
> 
> (In other news, why isn't attributes-persisting already done at this point?)

The reason is two fold:

JavaScript in the EM actually sets the RDF datasource on the dialog onload, so
before the dialog onload, the richlistbox is empty.  Hence the need for init(),
since I couldn't figure out a good way to listen when new content is generated
and we were short in time.  So I made the EM call init() when it set the
template datasource.  I posted a patch to another bug (somewhere, I believe it
was backed out by aaron, can't find the bug) that actually listened to template
rebuilds and reset selection as required.  It is the right approach, but needs
some more testing.

And as Aaron pointed out, we have another issue where the RDF template is
rebuilt when it shouldn't be, though I believe rob_strong was working on
removing those cases.

If we want to fix it correctly, I could take the cycles, but that would require
bigger code changes and I am unsure if we are willing to take bigger changes at
this point.

Comment 22

[Aaron Leventhal]

> beltzner: what is the rationale for Download Manager not remembering the
> selected item, while EM and TM have heretofore done so?

I think the problem is that the download manager takes a lot longer for the
initial items to be constructed from the template, and there currently is no
event or setTimeout trick that can be used after construction to easily set
initial selection. We'd have to hack that into <constructor> for the actual
items, which is icky.

Comment 23

[Aaron Leventhal]

It seems like Mano and Doron and possibly others want to find a correct fix even
for beta (see bug 306522 for using template listeners). I also want a clean fix,
but I don't really have a lot of time to do that right now.

I'll try to get to a cleaner fix Thursday or Friday. Apologies, but at the
moment I have a couple of more urgent blockers. If someone else wants to tackle
this that would be great, otherwise I'll get to it as soon as I can.

Comment 24

[Aaron Leventhal]

Doron has posted a cleaner fix for this bug and bug 305677 in bug 306522 --
thanks Doron. I'm all for doing it right. We should be careful though, because
it is not a small change.

Comment 25

[Aaron Leventhal]

Doron is working on the fix in bug 306522.

Comment 26

[Asa Dotzler [:asa]]

when bug 306552 lands for this we'll have our solution I believe so not a blocker.

Comment 27

[Brendan Eich [:brendan]]

(In reply to comment #26)
> when bug 306552 lands for this we'll have our solution I believe so not a blocker.

That can't be the bug you mean, but I think this is a blocker because we need to
get the API for richlistbox right before shipping.  Welcome to the world of API
compatibility and toolkit/platform contracts.

/be

Comment 28

[Asaf Romano (gone)]

Fixed on trunk (by bug 306522).