Closed Bug 593646 Opened 14 years ago Closed 14 years ago

rename 'Open in a New Window' to 'Move to New Window' and move it to a more sensible location

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 4.0b7

People

(Reporter: fryn, Assigned: fryn)

References

Details

(Whiteboard: [strings])

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
Current tab context menu:
...
-------------------------
  Open in a New Window
  Make into App Tab
  Move to Group        >
-------------------------
...

Should be the following for clear language and strictly increasing level of movement:
...
-----------------------
  Make into App Tab
  Move to Group      >
  Move to New Window
-----------------------
...

We should remove the article "a" for consistency, since we don't use it in items, such as "Open Link in New Window".

We ought to get this in by beta6 for string freeze!
Attachment #472218 - Flags: ui-review?(faaborg)
Attachment #472218 - Flags: review?(dao)
Comment on attachment 472218 [details] [diff] [review]
patch

Yeah, looks like we never updated strings when we got tab tear off working without a refresh, thanks for catchig this.
Attachment #472218 - Flags: ui-review?(faaborg) → ui-review+
There has never been a refresh, this feature explicitly waited for bug 113934.

"Move to a New Window" was considered in bug 482450, too.
Comment on attachment 472218 [details] [diff] [review]
patch

r- as it stands, needs more careful consideration.
Attachment #472218 - Flags: review?(dao) → review-
however, bug 559431 suggest changing it to "detach tab"
(In reply to comment #3)
> Comment on attachment 472218 [details] [diff] [review]
> patch
> 
> r- as it stands, needs more careful consideration.

If I understand correctly, "review" refers to code review, not UI review.
Review means sanity checking in various regards. There was obviously a wrong assumption in comment 1, so I could as well have canceled the ui-review+. Either way this isn't ready to land. I'll be happy to re-review this once it's sorted if there was a good reason for preferring "Open in a New Window" over "Move to a New Window" in bug 482450.
+1 for Move. It doesn't open the tab, it moves it. Not sure how much more reasoning is needed. :)
I agree with Frank's proposed changes.  "Open" implies a reload - it's the first step of going to a web destination.  "Move" implies relocation but not reload.  As opening in a new window preserves DOM state and (mostly) doesn't reload, move is a more appropriate term than open.

Removing article "a" is good too - we generally don't use articles in strings because they are not needed for clarity
Limi: not quite right; we should look through the old bugs and understand the context, looking before leaping.

Having done that, I don't think there was an explicit reason. Additionally, now that we have "Move to Group" it makes sense to also have "Move to Window" instead of "Open in New Window". Finally, I think "Open in New Window" may lead people to believe it will behave more like "duplicate" than "move".

Thanks for checking, Dao (although cancelling the ui-r wouldn't be apropos, more likely re-requesting along with your concerns!)
Comment on attachment 472218 [details] [diff] [review]
patch

Thanks for the input, beltzner, boriss, and limi.

Re-requesting review.
Attachment #472218 - Flags: review- → review?
Attachment #472218 - Flags: review? → review+
Comment on attachment 472218 [details] [diff] [review]
patch

Dao, while I may have protested at first, your sanity check resulted an improved process, and we are better for it. Thanks for the review.
Attachment #472218 - Flags: approval2.0?
Attachment #472218 - Flags: approval2.0? → approval2.0+
Keywords: checkin-needed
Whiteboard: [strings]
blocking2.0: --- → ?
patching file browser/base/content/browser.xul
Hunk #1 FAILED at 110
1 out of 1 hunks FAILED -- saving rejects to file browser/base/content/browser.xul.rej
patching file browser/locales/en-US/chrome/browser/browser.dtd
Hunk #1 FAILED at 12
1 out of 1 hunks FAILED -- saving rejects to file browser/locales/en-US/chrome/browser/browser.dtd.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh attachment.cgi?id=472218
Keywords: checkin-needed
I'll be landing tomorrow, so will make sure this gets in as a ridealong if it hasn't already.

Frank if you can update the patch (make sure it has user info + commit comment)
http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed

Also add this bug to the ride along section of the landing queue, since people are looking at it. https://wiki.mozilla.org/LandingQueue
blocking2.0: ? → ---
zpao, thanks for the link and help.
Attachment #472218 - Attachment is obsolete: true
Keywords: checkin-needed
Pushed http://hg.mozilla.org/mozilla-central/rev/16071f59d247
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b7
Verified fixed
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b7pre) Gecko/20100918 Firefox/4.0b7pre
Status: RESOLVED → VERIFIED
Litmus test has been updated:
https://litmus.mozilla.org/show_test.cgi?id=10010
Flags: in-litmus+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: