Last Comment Bug 648294 - Opening and closing Panorama fails because keys as given in the DTD are not recognized anymore when synthesizing the key event
: Opening and closing Panorama fails because keys as given in the DTD are not r...
Status: RESOLVED FIXED
[mozmill-panorama][mozmill-test-failure]
: regression
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: -- normal
: Firefox 8
Assigned To: Raymond Lee [:raymondlee]
:
Mentors:
http://mozmill-release.brasstacks.moz...
Depends on:
Blocks: 587276 603789 629050 647248 660175
  Show dependency treegraph
 
Reported: 2011-04-07 09:33 PDT by Anthony Hughes (:ashughes) [GFX][QA][Mentor]
Modified: 2016-04-12 14:00 PDT (History)
11 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
v1 (8.44 KB, patch)
2011-04-07 21:05 PDT, Raymond Lee [:raymondlee]
ttaubert: feedback+
Details | Diff | Splinter Review
v2 (10.22 KB, patch)
2011-04-12 23:01 PDT, Raymond Lee [:raymondlee]
ttaubert: feedback+
Details | Diff | Splinter Review
v3 (10.37 KB, patch)
2011-04-14 18:55 PDT, Raymond Lee [:raymondlee]
ian: review+
Details | Diff | Splinter Review
Patch for checkin (10.35 KB, patch)
2011-04-18 18:26 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review
v5 (12.34 KB, patch)
2011-06-30 00:45 PDT, Raymond Lee [:raymondlee]
dao+bmo: review+
ian: review+
ttaubert: feedback+
Details | Diff | Splinter Review

Description Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-04-07 09:33:03 PDT
ERROR:
waitFor([object Proxy],"TabView is still open.")@resource://mozmill/modules/utils.js:449 @:0 

All of our Panorama tests have been failing since 2011-04-01.  This is most likely due to a recent change in functionality in Firefox.

Since daily testruns do not happen on 4.0, I need to investigate there first.  This may be trunk-only.

This bug blocks landing of any new Panorama tests.
Comment 1 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-04-07 09:44:02 PDT
The tests do not fail on Firefox 4.0.  This is trunk-only.
Comment 2 Aaron Train [:aaronmt] 2011-04-07 10:45:05 PDT
This is the strangest thing, from digging in a bit and looking at the entity lookup for tabView.commandKey on tabView::close. This entity is retrieving the appropriate entity fine, which has the keycode value: 'e' - the same as entity value retrieved for tabView::open, but the controller::keypress call is not appropriately closing Tab View. Here comes the strange part, replace cmdKey with the keycode value 'E' (uppercase), and the tests now pass. 

Why is keypress not appropriately closing tabView with the retrieved keycode value?
Why will it act appropriately on a hardcoded uppercase keycode?
Comment 3 Henrik Skupin (:whimboo) 2011-04-07 12:37:31 PDT
Can one of you please give the changeset between those two builds when the regression has been started?
Comment 5 Ian Gilman [:iangilman] 2011-04-07 14:41:12 PDT
Bug 587276 landed during that window; I'm guessing that's the cause. It's possible the tests just need to be updated to the new invocation pattern. Raymond can hopefully shed some light.
Comment 6 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-04-07 14:57:21 PDT
Interesting to see that the Panorama development team uses hardcoded string values instead of DTD strings in their own tests.

http://hg.mozilla.org/mozilla-central/diff/565c588e3e51/browser/base/content/test/tabview/browser_tabview_launch.js

-    EventUtils.synthesizeKey("e", { accelKey: true, shiftKey: true }, win);
+    EventUtils.synthesizeKey("E", { accelKey: true, shiftKey: true }, win);
Comment 7 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-04-07 15:00:39 PDT
The DTD still refers to lowercase 'e':
http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.dtd#317

Are these key values case sensitive?
Comment 8 Henrik Skupin (:whimboo) 2011-04-07 15:47:36 PDT
This is not something our test is doing wrong. It's clearly a regression in the TabView code. The key identifier as read from the DTD file is the one which has to work. And we are synthesizing the right character:

var cmdKey = utils.getEntity(this.dtds, "tabView.commandkey");
this._controller.keypress(null, cmdKey, {accelKey: true, shiftKey: true});

(In reply to comment #6)
> Interesting to see that the Panorama development team uses hardcoded string
> values instead of DTD strings in their own tests.

That's something which happens across the different components and that's one of the reasons why we have Mozmill tests. Only our framework is currently able to get that information from the DTD files.

> http://hg.mozilla.org/mozilla-central/diff/565c588e3e51/browser/base/content/test/tabview/browser_tabview_launch.js
> 
> -    EventUtils.synthesizeKey("e", { accelKey: true, shiftKey: true }, win);
> +    EventUtils.synthesizeKey("E", { accelKey: true, shiftKey: true }, win);

This change looks broken and conflicts with the value we currently have in the DTD file as given by Anthony above.

Moving this over to Panorama. Also it would be great to get this fixed hopefully soon. Thanks.
Comment 9 Raymond Lee [:raymondlee] 2011-04-07 19:46:45 PDT
I've mentioned the reason why I changed the lower case e to upper case E in our test here bug 587276 comment c31.

" Furthermore, you would notice in some tests, I've changed to the letter "e" to
"E" when the key combination involves shiftKey=true.  When ctrl/cmd + shift + e
is pressed in the tabview UI, the charCode of "E" would be passed into the
white list function processBrowserKeys() because the shift button is also
pressed.  However, the charCode of "e" is passed to the white list function
when stimulating a key press using EventUtils.synthesizeKey().  That's why I've
changed the lower case letter to upper one if the key combination involves
shift key.  Another way to leave the tests and fix the problem is to add the
lower case as well as upper case charCodes to the white list function
processBrowserKeys() 

-    EventUtils.synthesizeKey("e", { accelKey: true, shiftKey: true });
+    EventUtils.synthesizeKey("E", { accelKey: true, shiftKey: true });
"

I am working on a patch to fix this.
Comment 10 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-04-07 19:57:36 PDT
(In reply to comment #9)
> I am working on a patch to fix this.

Will that be done on bug 587276? If not, is there another bug? Will it be done on this bug?
Comment 11 Raymond Lee [:raymondlee] 2011-04-07 20:02:35 PDT
(In reply to comment #10)
> (In reply to comment #9)
> > I am working on a patch to fix this.
> 
> Will that be done on bug 587276? If not, is there another bug? Will it be done
> on this bug?

Will upload the patch to this bug.
Comment 12 Raymond Lee [:raymondlee] 2011-04-07 21:05:15 PDT
Created attachment 524566 [details] [diff] [review]
v1

This should fix the issue.
Comment 13 Raymond Lee [:raymondlee] 2011-04-08 16:12:16 PDT
(In reply to comment #12)
> Created attachment 524566 [details] [diff] [review]
> v1
> 
> This should fix the issue.

http://tbpl.mozilla.org/?tree=MozillaTry&rev=15a131a369f0
Comment 14 Raymond Lee [:raymondlee] 2011-04-11 01:57:10 PDT
Tim: We want to land this asap.  If you have time, could you give me feedback on this patch please?
Comment 15 Henrik Skupin (:whimboo) 2011-04-11 09:46:18 PDT
Christian, this regression busted all of our Mozmill tests for Panorama and even a couple more for all localized and the en-US build because we can't exit the Panorama view anymore. We would really like to see it fixed on Aurora or the causing check-in to be backed out. We are not on buildbot yet but i think for our tests the same policy should apply. We hope that we can find the best solution for it soon. Thanks.
Comment 16 Tim Taubert [:ttaubert] 2011-04-12 01:59:49 PDT
Comment on attachment 524566 [details] [diff] [review]
v1

Sorry for the late feedback. This is kind of a solution though we really need to refactor that bit in terms of eliminating the switch statement - we should use a hash map for quick look-up.

So this patch is quite important because MozMill tests do not run at all for Panorama. I would tend to give you a feedback+ and ask you to please open a follow-up bug to refactor this.

Let's see what Ian thinks about this when reviewing :)
Comment 17 Tim Taubert [:ttaubert] 2011-04-12 07:05:03 PDT
m-c has been merged to aurora:

http://hg.mozilla.org/mozilla-central/rev/a95d42642281

Seems like another 6 weeks to write a proper patch for this :)
Comment 18 Raymond Lee [:raymondlee] 2011-04-12 23:01:44 PDT
Created attachment 525629 [details] [diff] [review]
v2

Replaced switch blocks with hash map look-up.
Comment 19 Henrik Skupin (:whimboo) 2011-04-13 00:06:12 PDT
(In reply to comment #17)
> m-c has been merged to aurora:
>
> Seems like another 6 weeks to write a proper patch for this :)

It's not that trivial. I have talked with Christian about this issue today and as told by him he would like to see a fix even on Aurora. But I will let him comment with details here.
Comment 20 Dão Gottwald [:dao] 2011-04-13 02:46:47 PDT
(In reply to comment #15)
> We would really like to see it fixed on Aurora or
> the causing check-in to be backed out. We are not on buildbot yet but i think
> for our tests the same policy should apply.

Mozmill tests aren't developer- but QA-managed, they're naturally arcane to developers. I don't think the same rules can apply. A backout doesn't seem justified, as this code isn't wrong per se, it's just bad test interaction. You can work around it by using toLocaleUpperCase in your test.
Comment 21 Henrik Skupin (:whimboo) 2011-04-13 09:49:07 PDT
(In reply to comment #20) 
> justified, as this code isn't wrong per se, it's just bad test interaction. You
> can work around it by using toLocaleUpperCase in your test.

Why do you think that this is bad test interaction? In our Mozmill tests we are doing it the right way by retrieving the appropriate entity from the DTD file, while mostly all the Mochitests are using a hard-coded value, which doesn't allow those tests to be run in localized builds. When you check the DTD entity for opening/closing Panorama you will see that a lower case 'e' is used for those two mentioned actions. CC'ing Axel for further l10n input.
Comment 22 Dão Gottwald [:dao] 2011-04-13 10:32:16 PDT
(In reply to comment #21)
> Why do you think that this is bad test interaction? In our Mozmill tests we are
> doing it the right way by retrieving the appropriate entity from the DTD file,

As far as I can tell, there is no problem for users but only for the tests, so apparently the tests don't quite match reality.
Comment 23 Henrik Skupin (:whimboo) 2011-04-13 10:42:15 PDT
(In reply to comment #22)
> As far as I can tell, there is no problem for users but only for the tests, so
> apparently the tests don't quite match reality.

If that's the case EventUtils has to be fixed then.
Comment 24 Henrik Skupin (:whimboo) 2011-04-13 12:20:38 PDT
We have landed the proposed workaround for the Mozmill tests now.
Comment 25 Tim Taubert [:ttaubert] 2011-04-14 15:47:34 PDT
Comment on attachment 525629 [details] [diff] [review]
v2

Looks good, thanks! Nits:

>     delete this._browserKeys;
>     this._browserKeys = keys;

>+    delete this._browserKeysWithShift;
>+    this._browserKeysWithShift = keys;

I think we don't need the delete statement right before the assignment, do we? They seem superfluous.
Comment 26 Raymond Lee [:raymondlee] 2011-04-14 18:55:50 PDT
Created attachment 526172 [details] [diff] [review]
v3

Removed the delete statements.
Comment 27 Raymond Lee [:raymondlee] 2011-04-14 22:33:36 PDT
Comment on attachment 526172 [details] [diff] [review]
v3

Passed Try
http://tbpl.mozilla.org/?tree=MozillaTry&rev=d1a7ab2827da
Comment 28 Dão Gottwald [:dao] 2011-04-15 03:15:55 PDT
(In reply to comment #23)
> (In reply to comment #22)
> > As far as I can tell, there is no problem for users but only for the tests, so
> > apparently the tests don't quite match reality.
> 
> If that's the case EventUtils has to be fixed then.

Maybe. It seems possible that this is the right way to proceed here.
Comment 29 Henrik Skupin (:whimboo) 2011-04-15 10:14:44 PDT
Neil, could you please have a look at this problem or CC someone who knows the details of EventUtils and especially synthesizing shortcut events? Would it be safe to assume that we could always use an uppercase letter to synthesize those shortcuts, even when the DTD file lists it as lowercase?
Comment 30 Neil Deakin 2011-04-15 11:45:20 PDT
The character supplied to synthesizeKey should be the uppercase letter when the shift key flag is set, usually, yes. I believe that isn't true if caps lock is enabled on Windows however as pressing shift creates a lowercase letter instead. So it may not be desirable to have this be converted automatically.
Comment 31 Henrik Skupin (:whimboo) 2011-04-15 14:37:05 PDT
(In reply to comment #30)
> The character supplied to synthesizeKey should be the uppercase letter when the
> shift key flag is set, usually, yes. I believe that isn't true if caps lock is
> enabled on Windows however as pressing shift creates a lowercase letter
> instead. So it may not be desirable to have this be converted automatically.

Would that mean the DTD file has to be updated and exchange 'e' with 'E' to fix it? Sounds like the wrong letter has been used in there.

http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.dtd#317
Comment 32 Ian Gilman [:iangilman] 2011-04-18 09:50:27 PDT
Comment on attachment 526172 [details] [diff] [review]
v3

Looks good.
Comment 33 Raymond Lee [:raymondlee] 2011-04-18 18:26:28 PDT
Created attachment 526885 [details] [diff] [review]
Patch for checkin
Comment 34 Dão Gottwald [:dao] 2011-04-19 04:08:17 PDT
Please don't land this in the middle of the attempt to figure out whether this is the right fix.
Comment 35 Henrik Skupin (:whimboo) 2011-04-27 06:26:55 PDT
(In reply to comment #31)
> Would that mean the DTD file has to be updated and exchange 'e' with 'E' to fix
> it? Sounds like the wrong letter has been used in there.
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.dtd#317

Neil, could you please comment on it? Would be nice to get an idea of how to solve this bug.

Tim, I re-add bug 603789 to the dependency list, because it is the reason of the regression and has to be kept on it.
Comment 36 Raymond Lee [:raymondlee] 2011-05-03 20:23:43 PDT
(In reply to comment #35)
> (In reply to comment #31)
> > Would that mean the DTD file has to be updated and exchange 'e' with 'E' to fix
> > it? Sounds like the wrong letter has been used in there.
> > 
> > http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.dtd#317
> 
> Neil, could you please comment on it? Would be nice to get an idea of how to
> solve this bug.
> 
> Tim, I re-add bug 603789 to the dependency list, because it is the reason of
> the regression and has to be kept on it.

Hi Neil, could you please comment on it.  Thanks
Comment 37 Henrik Skupin (:whimboo) 2011-05-14 03:05:32 PDT
Dao, how shall we proceed on this bug? Shall we move out the remaining question to a separate bug? Not sure how long it will take for Neil to answer.
Comment 38 Tim Taubert [:ttaubert] 2011-05-27 02:22:15 PDT
bugspam
Comment 39 Tim Taubert [:ttaubert] 2011-05-27 02:27:08 PDT
bugspam
Comment 40 Tim Taubert [:ttaubert] 2011-05-27 02:50:54 PDT
(In reply to comment #31)
> (In reply to comment #30)
> > The character supplied to synthesizeKey should be the uppercase letter when the
> > shift key flag is set, usually, yes. I believe that isn't true if caps lock is
> > enabled on Windows however as pressing shift creates a lowercase letter
> > instead. So it may not be desirable to have this be converted automatically.
> 
> Would that mean the DTD file has to be updated and exchange 'e' with 'E' to
> fix it? Sounds like the wrong letter has been used in there.
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/
> browser/browser.dtd#317

Cc'ing Gavin. Hopefully he can shed some light on how to correctly fix this.
Comment 41 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-05-30 15:08:19 PDT
The panorama key event handling code should just behave the same way the underlying XBL <key> handling code does: compare charcodes case-insensitively. 

That means lowercasing the value retrieved from the <key> elements in _setupBrowserKeys (as in nsXBLPrototypeHandler::ConstructPrototype [1], where "key" attributes are read) and the value from the event in processBrowserKeys (as in nsXBLPrototypeHandler::KeyEventMatched [2] where they're compared to an event's charCode).

[1] http://hg.mozilla.org/mozilla-central/annotate/1dfc5592f438/content/xbl/src/nsXBLPrototypeHandler.cpp#l930
[2] http://hg.mozilla.org/mozilla-central/annotate/1dfc5592f438/content/xbl/src/nsXBLPrototypeHandler.cpp#l604
Comment 42 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-05-30 15:16:34 PDT
I suppose that might involve having to also check shiftKey separately, but it looks like the code already does that. Duplicating XBL <key> matching logic is far from ideal. Are there followups to find a better solution?
Comment 43 Henrik Skupin (:whimboo) 2011-06-29 06:01:17 PDT
Raymond and Tim, is that enough information you need for an update of the patch?
Comment 44 Raymond Lee [:raymondlee] 2011-06-30 00:45:01 PDT
Created attachment 543080 [details] [diff] [review]
v5

(In reply to comment #43)
> Raymond and Tim, is that enough information you need for an update of the
> patch?

OK, updated the patch based on comment 41.
Comment 45 Raymond Lee [:raymondlee] 2011-06-30 00:46:00 PDT
Comment on attachment 543080 [details] [diff] [review]
v5

Sent it to Try and waiting for results
Comment 46 Raymond Lee [:raymondlee] 2011-06-30 11:38:00 PDT
Comment on attachment 543080 [details] [diff] [review]
v5

Passed try
http://tbpl.mozilla.org/?tree=Try&rev=7d06d9a7a3d8
Comment 47 Tim Taubert [:ttaubert] 2011-07-04 00:53:51 PDT
Comment on attachment 543080 [details] [diff] [review]
v5

Review of attachment 543080 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!
Comment 48 Tim Taubert [:ttaubert] 2011-07-04 01:14:25 PDT
Dietrich is out this week, maybe you'd want to find someone else to review in the meantime.
Comment 49 Ian Gilman [:iangilman] 2011-07-13 09:29:38 PDT
Comment on attachment 543080 [details] [diff] [review]
v5

Review of attachment 543080 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great. I'm glad we've lost those switch statements. 

I don't know that we need two reviews on this... it's up to you, Dao, if you want to weigh in.
Comment 50 Tim Taubert [:ttaubert] 2011-07-13 10:41:32 PDT
http://hg.mozilla.org/integration/fx-team/rev/cbeda84b1a5f
Comment 51 Tim Taubert [:ttaubert] 2011-07-13 15:31:22 PDT
http://hg.mozilla.org/mozilla-central/rev/cbeda84b1a5f

Note You need to log in before you can comment on or make changes to this bug.