I tried to work on bug 1102038, but the existing code is such a mess that I didn't feel adding another layer of special case handling would be reasonable. There are 2 refactorings I've wanted to do for a while here: 1. split out the code handling setting/removing the 'selected' attribute. This will dramatically simplify patches like for things like bug 1107695. 2. separate the logic examining the key event from the logic moving the selection around. I didn't feel confident touching this code without test coverage, so I started by writing tests in bug 1124900.

Comment on attachment 8553368 [details] [diff] [review] Part 1 - simplify handling of the 'selected' attribute Review of attachment 8553368 [details] [diff] [review]: ----------------------------------------------------------------- I wonder if it would make more sense to have selectedButton be a property of the popup where the buttons appear rather than of the textbox. Makes more sense in my head and maybe could use some methods to selectFirst, selectLast etc. I'd be ok with this landing without changing that though.

https://hg.mozilla.org/integration/fx-team/rev/966ff0f3c21e https://hg.mozilla.org/integration/fx-team/rev/966573b47371

https://hg.mozilla.org/mozilla-central/rev/966ff0f3c21e https://hg.mozilla.org/mozilla-central/rev/966573b47371

Comment on attachment 8553368 [details] [diff] [review] Part 1 - simplify handling of the 'selected' attribute Approval Request Comment [Feature/regressing bug #]: cleanup for code landed in bug 1088660. [User impact if declined]: can't uplift bug 1102038 without this dependency. [Describe test coverage new/current, TreeHerder]: tests landed in bug 1124900. [Risks and why]: I think it's OK for aurora, but too much changes for beta. [String/UUID change made/needed]: none.

Comment on attachment 8553369 [details] [diff] [review] Part 2 - separate selection and keyboard event logics Approval Request Comment: see comment 7.

I think this is a good fix to take on Aurora. Given the size of the change, I would like to see this verified on Nightly before uplift. This should already be in Nightly. Florian - Can you please ensure that this is verified at which point I'll approve for uplift?

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #9) > I think this is a good fix to take on Aurora. Given the size of the change, > I would like to see this verified on Nightly before uplift. This should > already be in Nightly. > > Florian - Can you please ensure that this is verified at which point I'll > approve for uplift? I had marked this qe-verify- because there's no expected user-visible change here. It's all preparation work for bug 1102038, so I think anybody verifying bug 1102038 will also verify the changes here at the same time.

(In reply to Florian Quèze [:florian] [:flo] from comment #10) > I had marked this qe-verify- because there's no expected user-visible change > here. It's all preparation work for bug 1102038, so I think anybody > verifying bug 1102038 will also verify the changes here at the same time. wfm

Comment on attachment 8553368 [details] [diff] [review] Part 1 - simplify handling of the 'selected' attribute Verification noted in bug 1102038. Aurora+

https://hg.mozilla.org/releases/mozilla-aurora/rev/9ae4b7412921 https://hg.mozilla.org/releases/mozilla-aurora/rev/da3e1d683106