Last Comment Bug 610203 - When using Alt+Enter in the location bar in popup windows, the tab should open in a full browser window
: When using Alt+Enter in the location bar in popup windows, the tab should ope...
Status: RESOLVED FIXED
: regression
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: Firefox 10
Assigned To: :Gavin Sharp [email: gavin@gavinsharp.com]
:
:
Mentors:
Depends on: 586234
Blocks: 579874 634347 674161
  Show dependency treegraph
 
Reported: 2010-11-07 04:50 PST by :Gavin Sharp [email: gavin@gavinsharp.com]
Modified: 2011-10-07 04:09 PDT (History)
12 users (show)
gavin.sharp: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.x+


Attachments
patch (4.96 KB, patch)
2010-11-07 05:32 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Splinter Review
updated patch (5.00 KB, patch)
2010-12-14 13:25 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Splinter Review
updated patch (5.37 KB, patch)
2010-12-20 04:17 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Splinter Review
unbitrotted (5.29 KB, patch)
2011-01-19 14:01 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Splinter Review
patch (4.90 KB, patch)
2011-03-23 11:46 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Splinter Review
wip tests (7.22 KB, patch)
2011-09-20 21:51 PDT, :Margaret Leibovic
no flags Details | Diff | Splinter Review
location bar tests (7.40 KB, patch)
2011-09-29 16:37 PDT, :Margaret Leibovic
gavin.sharp: feedback+
Details | Diff | Splinter Review
unbitrotted patch (4.85 KB, patch)
2011-09-29 16:43 PDT, :Margaret Leibovic
no flags Details | Diff | Splinter Review
patch (4.69 KB, patch)
2011-10-05 10:54 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
dao+bmo: review+
Details | Diff | Splinter Review
updated location bar tests (8.64 KB, patch)
2011-10-06 09:58 PDT, :Margaret Leibovic
gavin.sharp: review+
Details | Diff | Splinter Review

Description :Gavin Sharp [email: gavin@gavinsharp.com] 2010-11-07 04:50:44 PST

    
Comment 1 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-11-07 05:32:21 PST
Created attachment 488742 [details] [diff] [review]
patch

Adding inBackground parameter to openLinkIn feels kind of wrong, but I don't really see an alternative that doesn't involve duplicating the chromehidden check, which also doesn't seem great.
Comment 2 Dão Gottwald [:dao] 2010-11-07 05:48:58 PST
Comment on attachment 488742 [details] [diff] [review]
patch

>-            openUILinkIn(url, where,
>-                        { allowThirdPartyFixup: true, postData: postData });
>+
>+            openLinkIn(url, where,
>+                       { allowThirdPartyFixup: true,
>+                         postData: postData,
>+                         fromContent: false,

Use openUILinkIn and omit fromContent?
Comment 3 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-11-07 05:50:50 PST
I didn't really want to extend the signature change to openUILinkIn, particularly since it uses explicit arguments...
Comment 4 Dão Gottwald [:dao] 2010-11-07 05:52:52 PST
(In reply to comment #1)
> Created attachment 488742 [details] [diff] [review]
> patch
> 
> Adding inBackground parameter to openLinkIn feels kind of wrong, but I don't
> really see an alternative that doesn't involve duplicating the chromehidden
> check, which also doesn't seem great.

Because the tab would otherwise open in the background with browser.tabs.loadBookmarksInBackground = true?
Comment 5 Dão Gottwald [:dao] 2010-11-07 05:54:35 PST
(In reply to comment #3)
> I didn't really want to extend the signature change to openUILinkIn,
> particularly since it uses explicit arguments...

The third argument is just passed through, it's not bound to the explicit arguments.
Comment 6 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-11-07 05:58:46 PST
(In reply to comment #4)
> Because the tab would otherwise open in the background with
> browser.tabs.loadBookmarksInBackground = true?

Right.
Comment 7 Dão Gottwald [:dao] 2010-11-07 06:11:55 PST
I'm not sure that matters... browser.tabs.loadBookmarksInBackground is already overloaded.

If we add inBackground, it seems that it would be more consistent to assign it directly to loadInBackground.
Comment 8 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-11-07 07:38:59 PST
(In reply to comment #7)
> I'm not sure that matters... browser.tabs.loadBookmarksInBackground is already
> overloaded.

I don't think it ever makes sense to have Alt+Enter load a tab in the background, and I don't really think we should introduce a behavior change just because the implementation is slightly bothersome.

> If we add inBackground, it seems that it would be more consistent to assign it
> directly to loadInBackground.

I thought of that, but it felt wrong that we'd end up reversing the passed-in value in the tabshifted case...
Comment 9 Dão Gottwald [:dao] 2010-11-07 08:19:18 PST
But reversing the behavior when the user hits Shift is what "tabshifted" exists for: 'same as "tab" but in background if default is to select new tabs, and vice versa'.

Explicitly passing "tabshifted" and setting inBackground would be confusing, but it wouldn't make sense to call the function that way in the first place. "tabshifted" would usually come from whereToOpenLink and be transparent to the caller.

Maybe the parameter name should indicate that it specifies default behavior. I wouldn't mind it being lengthy since it should probably be omitted in favor of the prefs in most cases.
Comment 10 ithinc 2010-11-07 20:05:41 PST
Comment on attachment 488742 [details] [diff] [review]
patch

Normal Enter had better call openUILinkIn(url, "current") instead of loadURI. This will fix the overriding current pinned tab problem.
Comment 11 ithinc 2010-11-07 20:10:29 PST
Make the logic consistent between gURLBar.handleCommand and searchbar.handleSearchCommand.
Comment 12 ithinc 2010-11-08 00:07:44 PST
(In reply to comment #1)
> Adding inBackground parameter to openLinkIn feels kind of wrong, but I don't
> really see an alternative that doesn't involve duplicating the chromehidden
> check, which also doesn't seem great.

Add 'tabbackground' and 'tabforeground' for where?
Comment 13 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-11-08 01:05:38 PST
(In reply to comment #10)
> Normal Enter had better call openUILinkIn(url, "current") instead of loadURI.
> This will fix the overriding current pinned tab problem.

That's a separate issue, and one that I don't want to get into in this bug.
Comment 14 ithinc 2010-11-10 22:42:21 PST
(In reply to comment #6)
> (In reply to comment #4)
> > Because the tab would otherwise open in the background with
> > browser.tabs.loadBookmarksInBackground = true?
> 
> Right.

Alt+Enter in the search bar involves browser.tabs.loadBookmarksInBackground preference, so it may be not a problem.
Comment 15 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-12-14 13:25:27 PST
Created attachment 497597 [details] [diff] [review]
updated patch

I think I probably should write a test for this.
Comment 16 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-12-20 04:17:44 PST
Created attachment 498713 [details] [diff] [review]
updated patch

We really need better test coverage here, but they are a pain to write... I found a few other edge cases that my previous patch hadn't taken into consideration. I think the only remaining behavior change introduced by this patch is unconditionally calling preventDefault/stopPropagation for Alt+Enter events (seems like we should do that even if the current tab is empty and we end up taking the "current" path).
Comment 17 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-01-19 14:01:41 PST
Created attachment 505186 [details] [diff] [review]
unbitrotted
Comment 18 :Ehsan Akhgari 2011-02-09 21:57:12 PST
Is there a good reason that this blocks betaN?  If not, it should be moved over to final+.
Comment 19 Mike Beltzner [:beltzner, not reading bugmail] 2011-02-15 20:14:56 PST
If this gets reviewed before we generate RC, it has implicit a=beltzner
Comment 20 Dão Gottwald [:dao] 2011-02-25 09:00:45 PST
Is this just waiting for a test?
Comment 21 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-02-25 10:54:29 PST
Yes.
Comment 22 Mike Beltzner [:beltzner, not reading bugmail] 2011-03-03 07:22:01 PST
** PRODUCT DRIVERS PLEASE NOTE **

This bug is one of 7 automatically changed from blocking2.0:final+ to blocking2.0:.x during the endgame of Firefox 4 for the following reasons:

 - it was marked as a soft blocking issue without a requirement for beta coverage
Comment 23 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-03-23 11:46:34 PDT
Created attachment 521264 [details] [diff] [review]
patch
Comment 24 khagaroth 2011-09-10 09:24:31 PDT
So what's up with this bug? Is there a reason it's on hold or was it simply forgotten.
Comment 25 :Margaret Leibovic 2011-09-11 19:20:41 PDT
(In reply to khagaroth from comment #24)
> So what's up with this bug? Is there a reason it's on hold or was it simply
> forgotten.

This bug was held up because we wanted to write tests before landing this patch. Unfortunately, there aren't any existing tests for these code paths, which makes this task more time consuming.

I actually started working on some tests for this last week because this bug blocks bug 598587, which we really want for the new home tab feature.
Comment 26 :Margaret Leibovic 2011-09-20 21:51:58 PDT
Created attachment 561382 [details] [diff] [review]
wip tests

Here are some tests I've been working on to test the existing location bar behavior (Iwithout Gavin's patch applied). I still need to flesh out the tests for save and new window behavior, but I wanted to get feedback to make sure these are on the right track.

I figure once we get test coverage for the existing behavior, it will be much easier to add new tests for new behavior.
Comment 27 :Margaret Leibovic 2011-09-29 12:01:14 PDT
Comment on attachment 561382 [details] [diff] [review]
wip tests

I'm making this better - ignore this patch.
Comment 28 :Margaret Leibovic 2011-09-29 16:37:13 PDT
Created attachment 563594 [details] [diff] [review]
location bar tests
Comment 29 :Margaret Leibovic 2011-09-29 16:43:22 PDT
Created attachment 563595 [details] [diff] [review]
unbitrotted patch

I had to change the patch quite a bit because of all the logic changes from bug 658383 and friends. Gavin, maybe you want to change your patch at this point, since most of the code around it has changed?

If you do want to change the patch, perhaps you could be super awesome and also account for bug 598587 in the rewrite.
Comment 30 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-10-05 10:49:09 PDT
Comment on attachment 563594 [details] [diff] [review]
location bar tests

>diff --git a/browser/base/content/test/browser_locationBarCommand.js b/browser/base/content/test/browser_locationBarCommand.js

>+  { desc: "Ctrl/Cmd+Return keypress",
>+    event: { meta: true },
>+    check: checkCurrent

This looks wrong - shouldn't that be accelKey, and checkNewTab?

Looks good otherwise, though there is that issue with some other test interfering with this that needs to be sorted out.
Comment 31 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-10-05 10:49:55 PDT
Thanks a lot for writing those tests, btw :)
Comment 32 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-10-05 10:54:07 PDT
Created attachment 564923 [details] [diff] [review]
patch

I'd like to remove the preventDefault/stopPropagation stuff marked by the XXX comment, but I don't have easy access to windows/linux machines to test at the moment.

I have a patch on top of this for bug 674161 which will simplify the urlbarBindings logic (and should fix bug 598587).
Comment 33 :Margaret Leibovic 2011-10-06 09:54:09 PDT
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #30)
> >+  { desc: "Ctrl/Cmd+Return keypress",
> >+    event: { meta: true },
> >+    check: checkCurrent
> 
> This looks wrong - shouldn't that be accelKey, and checkNewTab?

Yeah, should be accelKey, but no, doing cmd+return opens the URI in the current tab. If that's not what you think should happen, we'll have to file a bug on that.

> Looks good otherwise, though there is that issue with some other test
> interfering with this that needs to be sorted out.

New patch with that coming shortly.
Comment 34 :Margaret Leibovic 2011-10-06 09:58:51 PDT
Created attachment 565268 [details] [diff] [review]
updated location bar tests

I found that browser_bug647886.js ended with a page loaded, so that was breaking my tests, since they expect the urlbar to be empty when the test starts. Making browser_bug647886.js do it's stuff in a new tab fixed this problem.

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