When using Alt+Enter in the location bar in popup windows, the tab should open in a full browser window

RESOLVED FIXED in Firefox 10

Status

()

Firefox
General
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Gavin, Assigned: Gavin)

Tracking

(Blocks: 1 bug, {regression})

Trunk
Firefox 10
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 .x+)

Details

Attachments

(2 attachments, 8 obsolete attachments)

Comment hidden (empty)
Blocks: 579874
Depends on: 586234
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.
Attachment #488742 - Flags: review?(dao)
Assignee: nobody → gavin.sharp
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?
I didn't really want to extend the signature change to openUILinkIn, particularly since it uses explicit arguments...
(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?
(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.
(In reply to comment #4)
> Because the tab would otherwise open in the background with
> browser.tabs.loadBookmarksInBackground = true?

Right.
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.
(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...
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

7 years ago
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

7 years ago
Make the logic consistent between gURLBar.handleCommand and searchbar.handleSearchCommand.

Comment 12

7 years ago
(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?
(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.
blocking2.0: --- → betaN+

Comment 14

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

Updated

7 years ago
Blocks: 598587
Attachment #488742 - Flags: review?(dao)
Created attachment 497597 [details] [diff] [review]
updated patch

I think I probably should write a test for this.
Attachment #488742 - Attachment is obsolete: true
Keywords: regression
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).
Attachment #497597 - Attachment is obsolete: true
Whiteboard: [softblocker]
Created attachment 505186 [details] [diff] [review]
unbitrotted
Attachment #498713 - Attachment is obsolete: true
Is there a good reason that this blocks betaN?  If not, it should be moved over to final+.
Whiteboard: [softblocker] → [softblocker][final?]
blocking2.0: betaN+ → final+
Whiteboard: [softblocker][final?] → [softblocker]

Updated

6 years ago
Blocks: 634347
If this gets reviewed before we generate RC, it has implicit a=beltzner
Whiteboard: [softblocker] → [softblocker][pre-approved by beltzner]
Is this just waiting for a test?
Yes.
** 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
blocking2.0: final+ → .x+
Created attachment 521264 [details] [diff] [review]
patch
Attachment #505186 - Attachment is obsolete: true

Comment 24

6 years ago
So what's up with this bug? Is there a reason it's on hold or was it simply forgotten.

Comment 25

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

6 years ago
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.
Attachment #561382 - Flags: feedback?(gavin.sharp)
Attachment #561382 - Flags: feedback?(dao)

Comment 27

6 years ago
Comment on attachment 561382 [details] [diff] [review]
wip tests

I'm making this better - ignore this patch.
Attachment #561382 - Attachment is obsolete: true
Attachment #561382 - Flags: feedback?(gavin.sharp)
Attachment #561382 - Flags: feedback?(dao)

Comment 28

6 years ago
Created attachment 563594 [details] [diff] [review]
location bar tests
Attachment #563594 - Flags: review?(gavin.sharp)

Comment 29

6 years ago
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.
Attachment #563595 - Flags: feedback?(gavin.sharp)

Updated

6 years ago
Attachment #521264 - Attachment is obsolete: true

Updated

6 years ago
Blocks: 690640

Updated

6 years ago
No longer blocks: 690640
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.
Attachment #563594 - Flags: review?(gavin.sharp) → feedback+
Thanks a lot for writing those tests, btw :)
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).
Attachment #563595 - Attachment is obsolete: true
Attachment #564923 - Flags: review?(dao)
Attachment #563595 - Flags: feedback?(gavin.sharp)
Blocks: 674161
No longer blocks: 598587

Updated

6 years ago
Attachment #564923 - Flags: review?(dao) → review+

Comment 33

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

6 years ago
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.
Attachment #563594 - Attachment is obsolete: true
Attachment #565268 - Flags: review?(gavin.sharp)
Attachment #565268 - Flags: review?(dao)
Attachment #565268 - Flags: review?(gavin.sharp)
Attachment #565268 - Flags: review?(dao)
Attachment #565268 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/d161a15f9dec
https://hg.mozilla.org/integration/mozilla-inbound/rev/4fce25f18e7a
Flags: in-testsuite+
Whiteboard: [softblocker][pre-approved by beltzner]
Target Milestone: --- → Firefox 10
https://hg.mozilla.org/mozilla-central/rev/4fce25f18e7a
https://hg.mozilla.org/mozilla-central/rev/d161a15f9dec
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.