Opening and closing Panorama fails because keys as given in the DTD are not recognized anymore when synthesizing the key event

RESOLVED FIXED in Firefox 8

Status

Firefox Graveyard
Panorama
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: ashughes, Assigned: raymondlee)

Tracking

({regression})

Trunk
Firefox 8
regression
Dependency tree / graph

Details

(Whiteboard: [mozmill-panorama][mozmill-test-failure], URL)

Attachments

(1 attachment, 4 obsolete attachments)

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.
The tests do not fail on Firefox 4.0.  This is trunk-only.

Updated

6 years ago
Keywords: regression
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?
Can one of you please give the changeset between those two builds when the regression has been started?
31st->1st
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2f45c30741c5&tochange=1a89509e25e4
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.
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);
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?
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.
Assignee: anthony.s.hughes → nobody
Blocks: 587276
Component: Mozmill Tests → Panorama
Product: Mozilla QA → Firefox
QA Contact: mozmill-tests → panorama
Version: unspecified → Trunk
Summary: Failure with TabView.open() & TabView.close() → Opening and closing Panorama fails because keys as given in the DTD are not recognized anymore when synthesizing the key event
(Assignee)

Comment 9

6 years ago
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.
(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?
(Assignee)

Comment 11

6 years ago
(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.
(Assignee)

Comment 12

6 years ago
Created attachment 524566 [details] [diff] [review]
v1

This should fix the issue.
Assignee: nobody → raymond
Attachment #524566 - Flags: feedback?(tim.taubert)
Blocks: 648590
(Assignee)

Comment 13

6 years ago
(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
Keywords: checkin-needed
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
(Assignee)

Updated

6 years ago
Blocks: 603789
(Assignee)

Comment 14

6 years ago
Tim: We want to land this asap.  If you have time, could you give me feedback on this patch please?
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 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 :)
Attachment #524566 - Flags: feedback?(tim.taubert) → feedback+
(Assignee)

Updated

6 years ago
Attachment #524566 - Flags: review?(ian)
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 :)
Blocks: 649405
Blocks: 647248
No longer blocks: 649405
No longer blocks: 648590
(Assignee)

Comment 18

6 years ago
Created attachment 525629 [details] [diff] [review]
v2

Replaced switch blocks with hash map look-up.
Attachment #524566 - Attachment is obsolete: true
Attachment #524566 - Flags: review?(ian)
Attachment #525629 - Flags: feedback?(tim.taubert)
(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.
(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.
(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.
(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.
(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.
We have landed the proposed workaround for the Mozmill tests now.
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.
Attachment #525629 - Flags: feedback?(tim.taubert) → feedback+
(Assignee)

Comment 26

6 years ago
Created attachment 526172 [details] [diff] [review]
v3

Removed the delete statements.
Attachment #525629 - Attachment is obsolete: true
Attachment #526172 - Flags: review?(ian)
(Assignee)

Comment 27

6 years ago
Comment on attachment 526172 [details] [diff] [review]
v3

Passed Try
http://tbpl.mozilla.org/?tree=MozillaTry&rev=d1a7ab2827da
(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.
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?
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.
(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 on attachment 526172 [details] [diff] [review]
v3

Looks good.
Attachment #526172 - Flags: review?(ian) → review+
(Assignee)

Comment 33

6 years ago
Created attachment 526885 [details] [diff] [review]
Patch for checkin
Attachment #526172 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
Please don't land this in the middle of the attempt to figure out whether this is the right fix.
Keywords: checkin-needed
No longer blocks: 603789
Blocks: 653099
(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.
Blocks: 603789
(Assignee)

Comment 36

6 years ago
(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
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.
bugspam
No longer blocks: 653099
bugspam
Blocks: 660175
(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.
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
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?
Raymond and Tim, is that enough information you need for an update of the patch?
(Assignee)

Comment 44

6 years ago
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.
Attachment #526885 - Attachment is obsolete: true
Attachment #543080 - Flags: feedback?(tim.taubert)
(Assignee)

Comment 45

6 years ago
Comment on attachment 543080 [details] [diff] [review]
v5

Sent it to Try and waiting for results
(Assignee)

Comment 46

6 years ago
Comment on attachment 543080 [details] [diff] [review]
v5

Passed try
http://tbpl.mozilla.org/?tree=Try&rev=7d06d9a7a3d8
Comment on attachment 543080 [details] [diff] [review]
v5

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

Looks good!
Attachment #543080 - Flags: feedback?(tim.taubert) → feedback+
(Assignee)

Updated

6 years ago
Attachment #543080 - Flags: review?(dietrich)
Dietrich is out this week, maybe you'd want to find someone else to review in the meantime.
(Assignee)

Updated

6 years ago
Attachment #543080 - Flags: review?(dao)
(Assignee)

Updated

6 years ago
Attachment #543080 - Flags: review?(dietrich)
(Assignee)

Updated

6 years ago
Attachment #543080 - Flags: review?(ian)
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.
Attachment #543080 - Flags: review?(ian) → review+

Updated

6 years ago
Attachment #543080 - Flags: review?(dao) → review+
http://hg.mozilla.org/integration/fx-team/rev/cbeda84b1a5f
Whiteboard: [mozmill-panorama][mozmill-test-failure] → [mozmill-panorama][mozmill-test-failure][fixed-in-fx-team]
http://hg.mozilla.org/mozilla-central/rev/cbeda84b1a5f
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [mozmill-panorama][mozmill-test-failure][fixed-in-fx-team] → [mozmill-panorama][mozmill-test-failure]
Target Milestone: --- → Firefox 8
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.