Closed Bug 551434 Opened 10 years ago Closed 9 years ago

Toolbar search (search bar / keywords) work not in selected engine but in active page

Categories

(Core :: XUL, defect, P1)

1.9.2 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla2.0b5
Tracking Status
blocking2.0 --- beta5+
blocking1.9.2 --- .17+
status1.9.2 --- .17-fixed
status1.9.1 --- unaffected

People

(Reporter: woodroof, Assigned: enndeakin)

References

()

Details

(Keywords: regression, testcase, Whiteboard: [sg:low spoof])

Attachments

(3 files, 4 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; ru; rv:1.9.2) Gecko/20100115 Firefox/3.6 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; ru; rv:1.9.2) Gecko/20100115 Firefox/3.6 (.NET CLR 3.5.30729)

Active page with suggested search plugins prevent search from search toolbar.

Reproducible: Always

Steps to Reproduce:
1. Go to http://slovari.yandex.ru/
2. Select "Google" in search toolbar.
3. Type "hello".
4. Press Enter.
Actual Results:  
Loading page: http://slovari.yandex.ru/search.xml?text=&st_translate=on

Expected Results:  
Loaded page: http://www.google.com/search?q=hello&ie=utf-8&oe=utf-8&aq=t&rls=org.mozilla:ru:official&client=firefox

Alt+Enter works good
Good catch. This is a regression from Firefox 3.5. Confirmed with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2.2pre) Gecko/20100308 Namoroka/3.6.2pre ID:20100308035343
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: in-testsuite?
OS: Windows XP → All
Hardware: x86 → All
Version: unspecified → 3.6 Branch
on trunk i get as actual behavior http://slovari.yandex.ru/search.xml?text=&st_translate=0 loaded + a new tab opened with the google search for the search term. weird.
Even with a fresh profile? When I run a trunk build no new tabs will be opened.
Looks like the page just steals focus on Enter. A regression range would be helpful...
Component: Search → General
Product: Firefox → Core
QA Contact: search → general
Version: 3.6 Branch → Trunk
regression window:
works:
http://hg.mozilla.org/mozilla-central/rev/90d3e6d2cbb9
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2a1pre) Gecko/20090610 Namoroka/3.6a1pre ID:20090610042525

fails:
http://hg.mozilla.org/mozilla-central/rev/4430cae50dad
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2a1pre) Gecko/20090611 Namoroka/3.6a1pre ID:20090611044033

pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=90d3e6d2cbb9&tochange=4430cae50dad
WORKAROUND:

diff -b -U 20 org/utilityOverlay.js new/utilityOverlay.js
--- org/utilityOverlay.js	2010-03-06 15:52:16.569609600 +0900
+++ new/utilityOverlay.js	2010-03-20 03:07:30.956487600 +0900
@@ -235,41 +235,41 @@
   case "tabshifted":
     loadInBackground = !loadInBackground;
     // fall through
   case "tab":
     let browser = w.gBrowser;
     browser.loadOneTab(url, {
                        referrerURI: aReferrerURI,
                        postData: aPostData,
                        inBackground: loadInBackground,
                        allowThirdPartyFixup: aAllowThirdPartyFixup,
                        relatedToCurrent: aRelatedToCurrent});
     break;
   }
 
   // If this window is active, focus the target window. Otherwise, focus the
   // content but don't raise the window, since the URI we just loaded may have
   // resulted in a new frontmost window (e.g. "javascript:window.open('');").
   var fm = Components.classes["@mozilla.org/focus-manager;1"].
              getService(Components.interfaces.nsIFocusManager);
   if (window == fm.activeWindow)
-    w.content.focus();
+    setTimeout(function(w){w.content.focus();},0);
   else
     w.gBrowser.selectedBrowser.focus();
 }
 
 // Used as an onclick handler for UI elements with link-like behavior.
 // e.g. onclick="checkForMiddleClick(this, event);"
 function checkForMiddleClick(node, event) {
   // We should be using the disabled property here instead of the attribute,
   // but some elements that this function is used with don't support it (e.g.
   // menuitem).
   if (node.getAttribute("disabled") == "true")
     return; // Do nothing
 
   if (event.button == 1) {
     /* Execute the node's oncommand or command.
      *
      * XXX: we should use node.oncommand(event) once bug 246720 is fixed.
      */
     var target = node.hasAttribute("oncommand") ? node :
                  node.ownerDocument.getElementById(node.getAttribute("command"));
Blocks: 178324
In addition to the Comment #0,
Keyword search on location bar also broken.

Steps to Reproduce:
1. Go to http://slovari.yandex.ru/
2. Wait complete loading the page
3. Type "Mozilla Firefox" in Locationbar (keyword.URL should be default value, thus google search)
4. Press Enter.

Actual Results:  
Loading page: http://slovari.yandex.ru/search.xml?text=&st_translate=0

Expected Results:  
Loaded page:
http://www.mozilla.com/en-US/firefox/firefox.html
OR
http://www.google.com/search?ie=UTF-8&oe=UTF-8&sourceid=navclient&gfns=1&q=Mozilla+Firefox

This also something wrong  :
gBrowser.selectedBrowser.focus(); in gURLBar.handleCommand .

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a4pre) Gecko/20100319 Minefield/3.7a4pre ID:20100319040017
Component: General → XUL
QA Contact: general → xptoolkit.widgets
Version: Trunk → 1.9.2 Branch
Duplicate of this bug: 559010
Neil, would you have time to check this bug?
status1.9.2: --- → ?
I can not reproduce the original bug with the URL http://slovari.yandex.ru/ with version 3.6.3, it seems that it works fine (also the location bar). Fails with the address from the duplicated bug http://www.terra.es
(In reply to comment #10)
> I can not reproduce the original bug with the URL http://slovari.yandex.ru/
> with version 3.6.3, it seems that it works fine (also the location bar). Fails
> with the address from the duplicated bug http://www.terra.es

I can not reproduce the URL http://slovari.yandex.ru/ on  Namoroka/3.6a1pre ID:20090611044033 too.
It means the site changed something .
Attached file testcase (obsolete) —
[STR1]
1. Start Fiirefox with new profile
2. Open testcase
3. Input text in the SearchBar and Press Enter key

[STR2]
1. Start Fiirefox with new profile
2. Open testcase
3. Input text in the LocationBar and Press Enter key

[Actual resalt]
Loading  https://bugzilla.mozilla.org/

[Expected]
Search results are displayed
Keywords: testcase
blocking2.0: --- → ?
Summary: Toolbar search work not in selected engine but in active page → Toolbar search (search bar / keywords) work not in selected engine but in active page
blocking2.0: ? → beta1+
Priority: -- → P1
Duplicate of this bug: 552285
Neil, can you take a look at this?  Also, bumping this to beta2 per beta re-triage with dbaron.  If this should indeed block beta1, please re-nom.
Assignee: nobody → enndeakin
blocking2.0: beta1+ → beta2+
This testcase loads a url whenever it receives focus.

openUILinkIn loads the search url and then focuses the content window. Should it be waiting for the new url to start loading before it does this?
(In reply to comment #17)
> This testcase loads a url whenever it receives focus.
Yes.
> 
> openUILinkIn loads the search url and then focuses the content window. Should
> it be waiting for the new url to start loading before it does this?
 Yes that is problem, and this behavior was brought by fixing bug 178324.

I think that the focus command should be performed after finish new page loading.
The testcase seems unrealistic and not really relevant for real-world pages.

http://www.terra.es/ listens for the keyup event, which we clearly don't want it to get when you press enter in the search bar...
(In reply to comment #19)
> http://www.terra.es/ listens for the keyup event, which we clearly don't want
> it to get when you press enter in the search bar...

That is bug 552285. Is this one supposed to be a the same issue?
Attached file testcase
more realistic testcase based on http://www.terra.es/
Attachment #438761 - Attachment is obsolete: true
(In reply to comment #20)
> (In reply to comment #19)
> > http://www.terra.es/ listens for the keyup event, which we clearly don't want
> > it to get when you press enter in the search bar...
> 
> That is bug 552285. Is this one supposed to be a the same issue?

Appears to be the same.
There are two problems here,
1. focus event fire to content before complete loading page
2. focus event fire to content before keyup event to serchbar/urlbar,
aren't it?
(In reply to comment #23)
> There are two problems here,
> 1. focus event fire to content before complete loading page

Sure, but focusing web pages isn't generally a problem. We do this when switching tabs, for instance, which causes your test case to navigate away as well. This test case is just quite weird.

> 2. focus event fire to content before keyup event to serchbar/urlbar,
> aren't it?

Yes, that's the real problem. We don't want content to get this keyup event.
(In reply to comment #24)
> (In reply to comment #23)
> > There are two problems here,
> > 1. focus event fire to content before complete loading page
> 
> Sure, but focusing web pages isn't generally a problem. We do this when
> switching tabs, for instance, which causes your test case to navigate away as
> well. This test case is just quite weird.

Yes, the testcase is not real.
However, in the release build,
1.surplus focus event fired to existing content page. -- something bad behavior
2.not fire focus event  to  new loaded page.-- should be fixed

I think that both should be fixed.
> 2.not fire focus event  to  new loaded page.-- should be fixed

You get the same behavior when clicking a link which replaces the current page with a new page.
(In reply to comment #26)
> > 2.not fire focus event  to  new loaded page.-- should be fixed
> 
> You get the same behavior when clicking a link which replaces the current page
> with a new page.

I see.
Note that this version of the patch still leaks the stored node.
Attachment #455744 - Flags: feedback?(Olli.Pettay)
Duplicate of this bug: 552285
This needs to be fixed ASAP. Visible regression on branch.
blocking1.9.2: --- → .8+
blocking2.0: beta2+ → beta3+
Olli? You working on this? This is a hard beta 3 blocker, and I think we should try to get it into 1.9.2 ASAP despite only being marked as "wanted", it's a bad and visible regression.
Somehow I don't think this is going to make it into Beta 3. I'd like to know what to do to get more motion/traction on this bug. Damon, any ideas?
How do we ensure this doesn't slip another beta?
blocking2.0: beta3+ → beta4+
Surely this has security implications as well? eg.

1. Go to dodgy website
2. Search for something using toolbar search
3. Search query instead gets passed to dodgy website
4. Dodgy website then returns a spoof search page, eg. a page that looks like Google, with malicious links.
Sorry, I wasn't CC'ed to this bug so I didn't get any bugmail, except the
feedback email long ago. (And since it is not in my review queue, I somehow
managed to not notice it)
Comment on attachment 455744 [details] [diff] [review]
One possibility is to store the target of the keydown and fire keypress and keyup at the same target

This looks pretty regression risky, and has problems.
What happens if the target of the keydown is removed from document
before keypress? Or what if the element is moved to another document?
Focusmanager thinks that the focus is still in the old window, but
key events would go to some other window.

What is wrong with the approach in comment 6?
Attachment #455744 - Flags: feedback?(Olli.Pettay) → feedback-
(In reply to comment #36)

> What is wrong with the approach in comment 6?

Because it doesn't work. For the workaround given in comment 6, if you'll notice closely, 'w' will be null, so an error just occurs, focusing nothing.

-    w.content.focus();
+    setTimeout(function(w){w.content.focus();},0);

A related workaround may be to focus after the document has started loaded and appeared onscreen, as I described in comment 17. But Dao or Gavin might be better at knowing whether that was viable.
(In reply to comment #37)
> A related workaround may be to focus after the document has started loaded and
> appeared onscreen, as I described in comment 17. But Dao or Gavin might be
> better at knowing whether that was viable.

This sounds fragile, as the user could do other stuff in the meantime. If somehow viable I still think making content not get the keyup event for key strokes started in chrome would be the ideal solution, rather than hacking around it every time this issue might pop up in chrome.
How do other browsers handle this issue? Surely that would be the way forward?
Other browsers don't have DOM in chrome and content.

I think I could accept approach where the key event target can't change
between keydown and keyup if the original target is in chrome and the possible
new target in content. That should be less-regression-risky approach.
That would be some rather small tweaks to the patch.
Changing the keyboard target handling in content pages is very regression risky.
I believe Google uses all sort of hack for IME etc. to move the focus in
keydown.
Attached patch patch (obsolete) — Splinter Review
This patch stores the keydown target and only retargets the keypress/keyup events when focus changes from chrome to content.

However, this patch leaks if shutdown with Ctrl/Cmd+Q and I'm not sure why. gKeyDownTarget is set to null at shutdown, yet it doesn't leak if gKeyDown is never set.
Attachment #455744 - Attachment is obsolete: true
What all does the patch leak? Only the element or "the world"?
Not just the element. Everything leaks.
Neil's still chasing the leak, and says it's unlikely to be resolved in time for the freeze for b4. Not going to change the flag yet, but making a note so drivers are aware of the risk of slippage here.
Attachment #463640 - Attachment is obsolete: true
Attachment #465363 - Flags: review?(Olli.Pettay)
I'm not sure why this was made a 1.9.2 release blocker if it's been affecting us the series. A fix is nice, sure, but why not just take it whenever it's done for trunk?
blocking1.9.2: .9+ → ---
Removing the blocking flag for 1.9.2.9, as it wouldn't bock the release (we've lived with it this long in 3.6.x). We'll pick this up in the next one, assuming the review goes through and it gets nominated again.
FWIW, we added the 1.9.2.9 flag because recent changes in the code on the websites (google and linkedin) which exercised the bug.
Attachment #465363 - Flags: review?(Olli.Pettay) → review+
Looks like we got a patch, should it be checked in? Are we waiting on a talos run from tryserver or something?
Keywords: checkin-needed
I've asked Gavin to run this through tryserver so we can make a call tomorrow morning on how safe it is to take at the last moment.
--> beta5, we'll get it early next beta cycle, like right after the tree re-opens.
blocking2.0: beta4+ → beta5+
http://hg.mozilla.org/mozilla-central/rev/f43f9b764efb

Target milestone is mozilla2.0b5.
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite? → in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b5
Status: RESOLVED → UNCONFIRMED
blocking2.0: beta5+ → beta4+
Ever confirmed: false
Flags: in-testsuite+ → in-testsuite?
Resolution: FIXED → ---
Status: UNCONFIRMED → RESOLVED
blocking2.0: beta4+ → beta5+
Closed: 9 years ago9 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b5pre) Gecko/20100824 Minefield/4.0b5pre
Status: RESOLVED → VERIFIED
Is it planned to include this fix in the 3.6 series?  

I understand that most people don't use websites that are affected by this problem, but for those of us who visit such sites several times a day (e.g. http://nciku.com/), this bug is highly annoying.
Duplicate of this bug: 604646
It would be great to get a 1.9.2 patch for this bug.  We have had traffic to the security@m.o list regarding seeing this bug in the wild. Neil, any chance you could make a branch patch for us?
Whiteboard: [sg:low spoof]
blocking1.9.2: ? → .14+
This patch doesn't apply cleanly, but the backporting looks fairly trivial. Attaching an untested 1.9.2 version.
Attached patch Possible 1.9.2 patch (obsolete) — Splinter Review
Attachment #505548 - Flags: feedback?(enndeakin)
I'm wondering if we should be retargeting to the right presshell on trunk as well.
Attachment #505548 - Attachment is obsolete: true
Attachment #505809 - Flags: review?(Olli.Pettay)
Attachment #505548 - Flags: feedback?(enndeakin)
Moving to 1.9.2.15 as this missed 1.9.2.14 and it is sg:low
blocking1.9.2: .14+ → .15+
Attachment #505809 - Flags: review?(Olli.Pettay) → review+
Comment on attachment 505809 [details] [diff] [review]
Working 1.9.2 patch which runs the test

Approved for 1.9.2.17, a=dveditz for release-drivers
Attachment #505809 - Flags: approval1.9.2.17+
You need to log in before you can comment on or make changes to this bug.