Closed Bug 1052992 Opened 5 years ago Closed 5 years ago

stopPropagation during a keypress breaks some findbar actions

Categories

(Toolkit :: Find Toolbar, defect)

30 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: eevee, Assigned: yarik.sheptykin, Mentored)

Details

(Whiteboard: [lang=js] )

Attachments

(1 file, 4 obsolete files)

Focus the page.  Press /.  The quick-find toolbar appears.

Pop open the dev console and enter:

    document.addEventListener('keypress', function(e){e.stopPropagation();});

Focus the page again and press /.  Nothing happens.

Ctrl-Tab and Ctrl-Shift-Tab are hampered, but not broken; pressing either will focus the tab bar instead of switching tabs.  Ctrl-PgUp and Ctrl-PgDn do nothing.  (I'm using Tree Style Tabs, but have heard similar gripes from other Firefox users.)

Ctrl-F and Ctrl-T still work, and I suspect other global Ctrl-letter combinations do as well.  It seems to be just these couple of tab-specific shortcuts that are broken by stopPropagation.

I've encountered this on Twitter and a handful of other sites that poorly implement keyboard shortcuts; it's particularly annoying when trying to Ctrl-Tab through several tabs quickly.

I've been trying to disable content stealing of browser shortcut keys with a greasemonkey script, but ultimately I have to use stopImmediatePropagation, and that's not a very good solution if it ends up breaking the keys I'm trying to save.
Thanks for filing a bug. We're aware of the twitter thing and are looking into it separately (as far as we know this was unintentional on their end and should be fixed soon), but I'm duping this to the general bug on this matter, which is bug 380637. Yes, it's old, but yes, we're looking at it again. :-)
Status: UNCONFIRMED → RESOLVED
Closed: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 380637
The problem is not what Twitter is doing; that's just how I stumbled upon it.

Calling stopPropagation, **but not** preventDefault, on an event handler should not block its default browser behavior from firing.
Status: RESOLVED → UNCONFIRMED
Resolution: DUPLICATE → ---
(In reply to Eevee (Alex Munroe) [:eevee] from comment #2)
> The problem is not what Twitter is doing; that's just how I stumbled upon it.
> 
> Calling stopPropagation, **but not** preventDefault, on an event handler
> should not block its default browser behavior from firing.

Right, which is exactly what bug 380637 is about. Hence marking it as a duplicate. Keeping two identical bugreports alive doesn't help anyone.
Status: UNCONFIRMED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 380637
Status: RESOLVED → VERIFIED
Forgive my enduring ignorance, but the linked bug looks to be largely a policy discussion, and only mentions stopPropagation re a patch from seven years ago (and even then in a chrome context).  This is a technical problem, regardless of whatever policy decision is made: default behavior should still occur if preventDefault is never called.  Even if that's wrong, there's still a bug here, because different shortcuts behave differently.
(In reply to Eevee (Alex Munroe) [:eevee] from comment #4)
> Forgive my enduring ignorance, but the linked bug looks to be largely a
> policy discussion, and only mentions stopPropagation re a patch from seven
> years ago (and even then in a chrome context).

Purely the fact that it's an old problem doesn't change that it's the same problem.

>  This is a technical problem,

No. Chrome solves this only "partially" in that it stops *some* shortcuts from working, and they get hell from web devs for doing even that. There is definitely also a policy issue at stake here.

> regardless of whatever policy decision is made: default behavior should
> still occur if preventDefault is never called.

This depends on the interpretation of "default behaviour". But really, we're getting into the specifics of that other bug report which this is duplicated to...

>  Even if that's wrong,
> there's still a bug here, because different shortcuts behave differently.

I would not be surprised if that was caused by the add-on rather than anything else. In fact, on current nightly builds, I can't even reproduce the issue with ctrl-tab/pgup/pgdown.

The fact that quick find can be stopPropagation()'d to a halt by the page seems like a feature to me - you wouldn't want it to suddenly pop up if you were in, say, a canvas-based editor like google docs. In any case, this all goes back to the same bug that I marked this as a duplicate of, and rehashing arguments from there won't do any good here.
(In reply to :Gijs Kruitbosch (intermittently here 14-15 August; then away until 19th) from comment #5)
> No. Chrome solves this only "partially" in that it stops *some* shortcuts
> from working, and they get hell from web devs for doing even that. There is
> definitely also a policy issue at stake here.

I'm not asking that you stop /any/ page-defined shortcuts from working.

I have an event handler.  I want to prevent further event handlers on the page from firing, so I call stopPropagation.  defaultPrevented is false, yet in some (but not all) cases, the Firefox UI is reacting differently than if there were no event handlers on the page at all.  

http://www.w3.org/TR/DOM-Level-3-Events/#widl-Event-stopPropagation

"Note: This method does not prevent the default action from being invoked — use Event.preventDefault() for that effect."

If the UI's reaction to a shortcut is not a 'default action' (a spec-defined term which appears nowhere in bug 380637) then I may be wrong.  Otherwise, this is a bug and a spec violation.

I don't want to have a drawn-out argument on Bugzilla of all places, but you seem to be fixating on the fact that I mentioned Twitter, which is very frustrating.  I've used several different sites on which ctrl-tab focuses the tab bar, which is something preventDefault DOES NOT cause (it makes ctrl-tab do nothing), but stopPropagation DOES cause (this bug).  So those pages MUST be intending to stop the event bubbling internally, but NOT intending to affect the browser's behavior.  (None of them had any reason to listen to the tab key, anyway, so this makes a lot of sense.)  I was in a hurry and perhaps handwaved a little too hard, but I'd hoped the bug title and repro instructions were enough to indicate the problem.

So please forget about Twitter.  The problem lies with the implementation.  stopPropagation is broken: it is breaking browser shortcuts when the spec says it should not.

Further observations:
- ctrl-tab: preventDefault only blocks it on keydown.  stopPropagation only screws it up on keypress.
- /: preventDefault blocks it on either keydown or keypress.  stopPropagation only blocks it on keypress.

Of course, stopImmediatePropagation has the same problem, its being a superset of stopPropagation.

> I would not be surprised if that was caused by the add-on rather than
> anything else. In fact, on current nightly builds, I can't even reproduce
> the issue with ctrl-tab/pgup/pgdown.

Bug 1008772 changed tab-switching shortcuts to be un-stealable.  Looks like even preventDefault() doesn't stop them as of fx32.

/ is still broken in nightly with a fresh profile.

> The fact that quick find can be stopPropagation()'d to a halt by the page
> seems like a feature to me - you wouldn't want it to suddenly pop up if you
> were in, say, a canvas-based editor like google docs.

Yes, that concern is why there's a separate preventDefault, to prevent the default action.  Which stopPropagation explicitly does not do.  Per spec.
(In reply to Eevee (Alex Munroe) [:eevee] from comment #6)

I agree with Alex.

The file bug 380637 is a discussion about whether we allow or not Web pages to hijack Firefox UI keyboard equivalents. Even if the answer is yes (we allow, so WONTFIX), there is a separate bug, hence this file bug 1052992, which is not a duplicate.
(In reply to Eevee (Alex Munroe) [:eevee] from comment #6)
> (In reply to :Gijs Kruitbosch (intermittently here 14-15 August; then away
> until 19th) from comment #5)
> > No. Chrome solves this only "partially" in that it stops *some* shortcuts
> > from working, and they get hell from web devs for doing even that. There is
> > definitely also a policy issue at stake here.
> 
> I'm not asking that you stop /any/ page-defined shortcuts from working.

My point was, if the website can't block the standard shortcut from working (esp. ctrl-w and similar "close this page/window/thing" shortcuts), web authors get upset, cf. https://code.google.com/p/chromium/issues/detail?id=33056

> I have an event handler.  I want to prevent further event handlers on the
> page from firing, so I call stopPropagation.  defaultPrevented is false, yet
> in some (but not all) cases, the Firefox UI is reacting differently than if
> there were no event handlers on the page at all.  
> 
> http://www.w3.org/TR/DOM-Level-3-Events/#widl-Event-stopPropagation
> 
> "Note: This method does not prevent the default action from being invoked —
> use Event.preventDefault() for that effect."
> 
> If the UI's reaction to a shortcut is not a 'default action'

This text doesn't seem to exist in the whatwg spec (which is preferred over the w3c version, AIUI) here: http://dom.spec.whatwg.org/#dispatching-events . On the other hand, I can't really find any definition there of what the canceled flag does (which is what preventDefault sets). The algorithm described in the text above seems to make it reasonable that if we treat the '/' shortcut as "just another event listener" (which it is, implementation-wise), if stop propagation is called, it does not get evoked. Maybe I'm not looking in the right place? Ms2ger?

> I don't want to have a drawn-out argument on Bugzilla of all places, but you
> seem to be fixating on the fact that I mentioned Twitter, which is very
> frustrating.  I've used several different sites on which ctrl-tab focuses
> the tab bar, which is something preventDefault DOES NOT cause (it makes
> ctrl-tab do nothing), but stopPropagation DOES cause (this bug).  So those
> pages MUST be intending to stop the event bubbling internally, but NOT
> intending to affect the browser's behavior.  (None of them had any reason to
> listen to the tab key, anyway, so this makes a lot of sense.)  I was in a
> hurry and perhaps handwaved a little too hard, but I'd hoped the bug title
> and repro instructions were enough to indicate the problem.
> 
> So please forget about Twitter.  The problem lies with the implementation. 
> stopPropagation is broken: it is breaking browser shortcuts when the spec
> says it should not.

My last comment didn't mention or imply anything about Twitter, so I'm not sure why you think I'm "fixated" on it.

My main point is: whether or not web pages should be able to affect application shortcuts is, as best I can tell, a policy question, which has existed for a long time and is currently tracked in the bug this was duped to. Different browsers behave differently. The specs should adapt to the result of the policy discussion, because specs should (ideally) specify "sensible" behaviour.

> Further observations:
> - ctrl-tab: preventDefault only blocks it on keydown.  stopPropagation only
> screws it up on keypress.
> - /: preventDefault blocks it on either keydown or keypress. 
> stopPropagation only blocks it on keypress.
> 
> Of course, stopImmediatePropagation has the same problem, its being a
> superset of stopPropagation.

Can you clarify exactly, on a clean profile (and indicating which OS this is on - Linux as per the initial report, or somewhere else?) against trunk, which shortcuts suffer from this issue besides '/'?

I'm going to just move this to Core :: DOM Event handling, but I somewhat suspect that this is an issue with a specific shortcut, as everything besides '/' which I tried on OS X and Windows worked fine (but needinfo'ing to be sure!). If this is just quickfind, it might be a dupe of bug 738903 (not sure what google does these days and/or if it's the same as what it did at that point in time).
Status: VERIFIED → REOPENED
Component: Untriaged → DOM: Events
Ever confirmed: true
Flags: needinfo?(eevee.mozilla)
Flags: needinfo?(Ms2ger)
Product: Firefox → Core
Resolution: DUPLICATE → ---
(to be clear, I couldn't reproduce the ctrl-tab/pgup/pgdown issues with not working 'completely' on either Linux or OS X, on Firefox 32 and/or Nightly (34), so I'm assuming that that's either fixed by the bug you cited earlier, or due to tabmixplus)
(In reply to :Gijs Kruitbosch (intermittently here 14-15 August; then away until 19th) from comment #9)
> (to be clear, I couldn't reproduce the ctrl-tab/pgup/pgdown issues with not
> working 'completely' on either Linux or OS X

Egh, this was meant to say *Windows* or OS X. It would surprise me if we behaved differently specifically on Linux, but it is of course possible...
This is the kind of thing smaug could answer much more efficiently :)
Flags: needinfo?(Ms2ger) → needinfo?(bugs)
If stopPropagation breaks some default actions, that sounds like a bug in chrome code.
It should listen events in system event group to which calling stopPropagation in
default group doesn't affect.
bug 380637 doesn't have much to do with stopPropagation() handling, more about
preventDefault()
Flags: needinfo?(bugs)
It is also possible that platformHTMLBindings.xml should use system group, not default.
Though, looks like those are added to system group by default in nsXBLService::AttachGlobalKeyHandler
As far as I see, http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/findbar.xml?force=1#269 should add listener to system event group.
Currently one would need to use nsIEventListenerService.addSystemEventListener.

(Well add [ChromeOnly] addSystemEventListener to EventTarget soon.)
> Can you clarify exactly, on a clean profile (and indicating which OS this is
> on - Linux as per the initial report, or somewhere else?) against trunk,
> which shortcuts suffer from this issue besides '/'?
> 
> I'm going to just move this to Core :: DOM Event handling, but I somewhat
> suspect that this is an issue with a specific shortcut, as everything
> besides '/' which I tried on OS X and Windows worked fine (but needinfo'ing
> to be sure!). If this is just quickfind, it might be a dupe of bug 738903
> (not sure what google does these days and/or if it's the same as what it did
> at that point in time).

Thank you.  :)  Using a clean profile on firefox-nightly-34.0a1.20140813-1 on Arch Linux x64, and carpet bombing key events with:

window.addEventListener('keydown', function(e) { e.stopImmediatePropagation(); }, true);
window.addEventListener('keypress', function(e) { e.stopImmediatePropagation(); }, true);
window.addEventListener('keyup', function(e) { e.stopImmediatePropagation(); }, true);

...the only shortcuts I can find that don't work are / and '.  I suspected something systemic when Ctrl-Pgup/PgDn/Tab were also broken in 30, but it sounds like quick find and tab switching might just be/have been independently buggy.
Flags: needinfo?(eevee.mozilla)
Component: DOM: Events → XUL Widgets
Product: Core → Toolkit
To eevee or someone else who would like to work on this bug:
see comment 15 for a link to the event listeners to switch to using Cc["@mozilla.org/eventlistenerservice;1"].getService(Ci.nsIEventListenerService).addSystemEventListener
Mentor: MattN+bmo
Status: REOPENED → NEW
Component: XUL Widgets → Find Toolbar
OS: Linux → All
Hardware: x86_64 → All
Summary: stopPropagation during a keypress breaks some default actions → stopPropagation during a keypress breaks some findbar actions
Whiteboard: [lang=js]
Hi, everyone!

First, thanks, eevee, for a detailed explanation of the problem. I could reproduce the issue discribed in comment 16. Ctrl-Pgup/PgDn/Tab kept working despite stopImmediatePropagation().

I applied changes offered by Matthew (comment 17) to the files mentioned in comment 15. This seemed to resolve the problem. I also updated the code for removing the listeners as I though I was related. 

I am not sure how to write a unit test for this, but would be whilling to learn how to test ui if a test is desired. Would be great if someone could point me to an example, though.
Attachment #8496272 - Flags: review?(MattN+bmo)
Hi Iaroslav,

I'll review this later today.

For testing, I would start by looking at https://mxr.mozilla.org/mozilla-central/source/toolkit/content/tests/browser/browser_findbar.js . You can run this with: ./mach mochitest-browser toolkit/content/tests/browser/browser_findbar.js
You can probably make a test open a tab (see promiseTestPageLoad) with an event listener which calls stopPropagation on all keypresses and then synthesize a keyboard event when the page is focused and make sure the findbar action for that key still worked.
(In reply to Matthew N. [:MattN] from comment #19)

Hi Matthew! Thanks for your help!

> I'll review this later today.
No need. I combined the current patch with the previous. So you can review the fix along with the test for it.

> For testing, I would start by looking at
> https://mxr.mozilla.org/mozilla-central/source/toolkit/content/tests/browser/
> browser_findbar.js . You can run this with: ./mach mochitest-browser
> toolkit/content/tests/browser/browser_findbar.js
Thanks for the hint! I added a new testing task to browser_findbar.js.

> You can probably make a test open a tab (see promiseTestPageLoad) with an
> event listener which calls stopPropagation on all keypresses and then
> synthesize a keyboard event when the page is focused and make sure the
> findbar action for that key still worked.
I tried to follow you suggestions. I found a similar test in browser_keyevents_during_autoscrolling.js [1]. I based the test on your comment 19 and eevee"s comment 16. The test fails without the fix (comment 17) and passes with the fix. All browser tests pass on my setup, but report the following at the very end.

"###!!! [Parent][OnMaybeDequeueOne] Error: Channel closing: too late to send/recv, messages will be lost"

Not sure, what that could mean. 

I have just received level 1 access and would like to try to run the tests on the TryServer myself if this is ok with you. But first I want to make sure that my code is alright, so I am looking forward to receive your review!

1. http://lxr.mozilla.org/mozilla-central/source/toolkit/content/tests/browser/browser_keyevents_during_autoscrolling.js
Assignee: nobody → yarik.sheptykin
Attachment #8496272 - Attachment is obsolete: true
Attachment #8496272 - Flags: review?(MattN+bmo)
Attachment #8497756 - Flags: review?(MattN+bmo)
Comment on attachment 8497756 [details] [diff] [review]
Add quick-search event listeners to system event group.

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

::: toolkit/content/tests/browser/browser_findbar.js
@@ +15,5 @@
> +  let browser = gBrowser.getBrowserForTab(tab);
> +  let findbar = gBrowser.getFindBar();
> +
> +  // Initially the findbar should be hidden.
> +  is(findbar.hidden, true, "Findbar is hidden at start.");

This should move into the beginning of the for loop below but you will see that it will cause a failure.

@@ +18,5 @@
> +  // Initially the findbar should be hidden.
> +  is(findbar.hidden, true, "Findbar is hidden at start.");
> +
> +  // Pressing these keys open the findbar.
> +  let hotkeys = ["/", "'"];

Nit: you could use const instead of let but then you should uppercase the variable name.

@@ +21,5 @@
> +  // Pressing these keys open the findbar.
> +  let hotkeys = ["/", "'"];
> +
> +  // Checking if findbar appears when any hotkey is pressed.
> +  for(let key of hotkeys) {

Nit: missing space after |for| here and below.

@@ +24,5 @@
> +  // Checking if findbar appears when any hotkey is pressed.
> +  for(let key of hotkeys) {
> +    EventUtils.sendChar(key, browser.contentWindow);
> +    is(findbar.hidden, false, "Findbar should not be hidden.");
> +    findbar.close();

The problem with this is that the close is not synchronous since there is an animation. There is code at https://mxr.mozilla.org/mozilla-central/source/toolkit/content/tests/chrome/bug451286_window.xul#63 to handle this but that's for a different kind of test so ideally we could handle this inside findbar.xml so all tests could be notified in some consistent way. The alternative is to copy that function for now.

@@ +28,5 @@
> +    findbar.close();
> +  }
> +
> +  // Stop propagation for all keyboard events.
> +  let window = browser.contentWindow;

I think this won't work well with E10S (multi-process Firefox) although this file already has that problem.

@@ +32,5 @@
> +  let window = browser.contentWindow;
> +  let stopPropagation = function(e) { e.stopImmediatePropagation(); };
> +  window.addEventListener('keydown', stopPropagation, true);
> +  window.addEventListener('keypress', stopPropagation, true);
> +  window.addEventListener('keyup', stopPropagation, true);

mozilla-central JS style is to use double-quotes.

::: toolkit/content/widgets/findbar.xml
@@ +264,5 @@
>            if (this._browser) {
> +            els.removeSystemEventListener(
> +              this._browser, "keypress", this, false);
> +            els.removeSystemEventListener(
> +              this._browser, "mouseup", this, false);

Nit: we normally do line breaks after an argument instead of after the opening parenthesis.
Attachment #8497756 - Flags: review?(MattN+bmo) → feedback+
(In reply to Matthew N. [:MattN] from comment #21)

Hi, Matthew! Thanks for the detailed review!

> @@ +24,5 @@
> > +  // Checking if findbar appears when any hotkey is pressed.
> > +  for(let key of hotkeys) {
> > +    EventUtils.sendChar(key, browser.contentWindow);
> > +    is(findbar.hidden, false, "Findbar should not be hidden.");
> > +    findbar.close();
> 
> The problem with this is that the close is not synchronous since there is an
> animation. There is code at
> https://mxr.mozilla.org/mozilla-central/source/toolkit/content/tests/chrome/
> bug451286_window.xul#63 to handle this but that's for a different kind of
> test so ideally we could handle this inside findbar.xml so all tests could
> be notified in some consistent way. The alternative is to copy that function
> for now.

I did as you suggested and copied the helper function from the other test file. I seems to work well after a minor modification. What do you think about opening a new bug for handling closing properly in findbar.xml and updating the affected tests? I would be whiling to work on this.

> > +  // Stop propagation for all keyboard events.
> > +  let window = browser.contentWindow;
> 
> I think this won't work well with E10S (multi-process Firefox) although this
> file already has that problem.
> 

I don't really know how to address this. If you believe the whole testfile has e problem with e10s, then maybe I could create another bug to deal with this issue? I could also try to fix it within this bug. e10s seems well documented, so I hope I can figure out how to.

Anyways, I attached an updated patch. Looking forward to get your review and further hints.
Attachment #8497756 - Attachment is obsolete: true
Attachment #8499202 - Flags: review?(MattN+bmo)
Hey, Matthew! I noticed that the patch I submitted last time has some issues with testing. I fails if the tab is does not have focus. Now, I am requesting the focus with gBrowser.selectedTab and waiting for focus with waitForFocus. This seemed the make test run stable.
Attachment #8499202 - Attachment is obsolete: true
Attachment #8499202 - Flags: review?(MattN+bmo)
Attachment #8500060 - Flags: review?(MattN+bmo)
Hey Matthew!

I am not sure how to proceed with this bug. I need a suggestion.
I have tried multiple things to make the test a e10s compliant over the last week. Unfortunately I could not come up with any working solution. To be more precise, I cannot get the code that interacts with the page-content to execute if I put it into the frame script. The documentation on e10s testing is somehow scarce. None of the tests in toolkit/content/tests/browser/ have e10s support. Every test is skipped if e10s is enabled [1].

As I suggested earlier, I would try to upgrade these tests to support e10s, because it seems like an interesting and useful project, but I will definitely need some help. I this sounds reasonable I would open an extra bug for this.

1. http://mxr.mozilla.org/mozilla-central/source/toolkit/content/tests/browser/browser.ini#4
Comment on attachment 8500060 [details] [diff] [review]
Add quick-search event listeners to system event group

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

I apologize very much for the delay.

(In reply to Iaroslav Sheptykin from comment #24)
> I am not sure how to proceed with this bug. I need a suggestion.
> I have tried multiple things to make the test a e10s compliant over the last
> week. Unfortunately I could not come up with any working solution. To be
> more precise, I cannot get the code that interacts with the page-content to
> execute if I put it into the frame script. The documentation on e10s testing
> is somehow scarce. None of the tests in toolkit/content/tests/browser/ have
> e10s support. Every test is skipped if e10s is enabled [1].

Yeah, I was thinking the test should move to the chrome directory to make it work with e10s as this fix isn't really browser-specific anyways. That can be done in an e10s follow-up.

I've pushed this to try server to run automated tests in production: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=25d2edf259ef
I will vouch for you to get Level 1 Commit Access so you can do this yourself in the future. See https://www.mozilla.org/hacking/commit-access-policy/

::: toolkit/content/tests/browser/browser_findbar.js
@@ +153,5 @@
> +/**
> + * A wrapper for the findbar's method "close", which is not synchronous
> + * because of animation.
> + */
> +function closeFindbarAndWait(findbar) {

Can you move this to head.js so it can be used by other tests in this directory? It will be made available in this test's scope automatically.

::: toolkit/content/widgets/findbar.xml
@@ +264,5 @@
>            if (this._browser) {
> +            els.removeSystemEventListener(this._browser, "keypress", this,
> +              false);
> +            els.removeSystemEventListener(this._browser, "mouseup", this,
> +              false);

Nit: align the |false| up with the first argument like so:
            els.removeSystemEventListener(this._browser, "keypress", this,
                                          false);
Attachment #8500060 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8500060 [details] [diff] [review]
Add quick-search event listeners to system event group

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

::: toolkit/content/widgets/findbar.xml
@@ +258,5 @@
>          ]]></getter>
>          <setter><![CDATA[
> +          // A shortcut for the service.
> +          let els = Cc["@mozilla.org/eventlistenerservice;1"].
> +            getService(Ci.nsIEventListenerService);

There is a test failure on all platforms:
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/docshell/test/chrome/test_bug298622.xul | uncaught exception - ReferenceError: Cc is not defined at chrome://global/content/bindings/findbar.xml:251

https://mxr.mozilla.org/mozilla-central/source/docshell/test/chrome/test_bug298622.xul which opens/runs https://mxr.mozilla.org/mozilla-central/source/docshell/test/chrome/bug298622_window.xul

It looks like Cc and Ci aren't defined in that case. You can instead use Components.classes and Components.interfaces respectively.
(In reply to Matthew N. [:MattN] (behind on reviews - focused on Loop) from comment #25)
> Yeah, I was thinking the test should move to the chrome directory to make it
> work with e10s as this fix isn't really browser-specific anyways. That can
> be done in an e10s follow-up.

Okay. As I wrote previously I would be willing to work on this.

> I will vouch for you to get Level 1 Commit Access so you can do this
> yourself in the future. See

I have already got L1 access as of bug 1074148. But haven't had a chance yet to push anything to the try server. I would like to do it this time if this is ok with you?
 
(In reply to Matthew N. [:MattN] (behind on reviews - focused on Loop) from comment #26)
 
> It looks like Cc and Ci aren't defined in that case. You can instead use
> Components.classes and Components.interfaces respectively.

Thanks, for the hint. I updated the .xul file. Those tests pass locally now:

./mach mochitest-browser docshell/test/
567 INFO Passed:  269
Attachment #8500060 - Attachment is obsolete: true
Attachment #8505005 - Flags: review+
> > I will vouch for you to get Level 1 Commit Access so you can do this
> > yourself in the future. See
> 
> I have already got L1 access as of bug 1074148. But haven't had a chance yet
> to push anything to the try server. I would like to do it this time if this
> is ok with you?

Hi Matthew! I pushed the patch to the try server and here is what I got:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e90c254582be
https://tbpl.mozilla.org/?tree=Try&rev=e90c254582be

I am not sure if those 3 failures are related to our work.
If everything is fine, should I mark this bug as "checkin-needed"?
Yes, those 3 seem like unrelated intermittent failures so I pushed this:

https://hg.mozilla.org/integration/fx-team/rev/9794e8f22ac8

Thanks for the patch and your patience. I hope you find another interesting bug.
Status: NEW → ASSIGNED
Flags: in-testsuite+
Whiteboard: [lang=js] → [lang=js] [fixed-in-fx-team]
(In reply to Matthew N. [:MattN] (behind on reviews - focused on Loop) from comment #29)
> Yes, those 3 seem like unrelated intermittent failures so I pushed this:
> 
> https://hg.mozilla.org/integration/fx-team/rev/9794e8f22ac8
> 
> Thanks for the patch and your patience. I hope you find another interesting
> bug.

Thanks for helping me to get started with firefox bugfixing! If you ever decide for upgrading the browser_ test to support e10s and opening a bug for it, please let me know. I would love to work on that if you superwise. Meanwhile, I will find something to work on. Bug 971052 for ex. looks interesting.
https://hg.mozilla.org/mozilla-central/rev/9794e8f22ac8
Status: ASSIGNED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Whiteboard: [lang=js] [fixed-in-fx-team] → [lang=js]
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.