1068894 - [Context menu] Open in New Tab opens links in a new window

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System::Window Mgmt, defect)

Description

[Marcia Knous [:marcia]]

Flame while running:

Gaia   47939f4c41d0c941e5047e5d1af74a79b7d8e0d5
SourceStamp e20869e87e23
BuildID 20140917000205
Version 34.0a2
v. 123

STR:
1. Open a web page that has links to other pages
2. Long press on one of the links to open the context menu
3. The context menu comes up - Select "Open link in New Tab"

Expected: It should read "Open link in New window" as described in the Rocketbar spec on Page 33

Comment 1

[Gaurav Mittal (gauravmittal1995)]

Hey, I would like to work on this, can u assign it to me?

Comment 2

[Marcia Knous [:marcia]]

ni on rob for desired behavior here.

Comment 3

[Rob MacDonald [:robmac]]

Thanks, Marcia. NI'ing Francis as the browser ux owner.

Comment 4

[Francis Djabri [:djabber]]

The latest specs are at: https://mozilla.box.com/s/lbw2wzw3p4jvxs24k4sg

Please see Browser Chrome spec, ver 0.7, page 22. The correct command should be "Open in New Window"

Comment 5

[Gaurav Mittal (gauravmittal1995)]

@gregor .... is it posiible to solve this by changing the https://github.com/mozilla-b2g/gaia/blob/master/apps/browser/locales/browser.en-US.properties#L9 line to "Open in New Window"???
Similarly for https://github.com/mozilla-b2g/gaia/blob/master/apps/browser/locales/browser.fr.properties#L9 to "Ouvrir dans une nouvelle fenêtre" , https://github.com/mozilla-b2g/gaia/blob/master/apps/browser/locales/browser.zh-TW.properties#L9 to "在新视窗开启" and https://github.com/mozilla-b2g/gaia/blob/master/apps/browser/locales/browser.ar.properties#L9 to "فتح في إطار جديد" !! ... also change the title from "open-in-new-tab" to "open-in-new-window" and replace all instance of it from the source ????

Comment 6

[Gregor Wagner [:gwagner]]

Ben, can you help out gauravmittal?

Comment 7

[Ben Francis [:benfrancis]]

Sure, hi guaravmittal.

The .properties files you're looking at belong to the old browser app. This bug refers to the new system browser, which is part of the system app.

You can find this string you need in the system app https://github.com/mozilla-b2g/gaia/blob/master/apps/system/locales/system.en-US.properties#L560

That's the file you need to change for the English strings. To contribute to other localisations, check out this guide on MDN https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/Localizing_Firefox_OS

Comment 8

[Gaurav Mittal (gauravmittal1995)]

Attachment: PR for Bug

Comment 9

[Ben Francis [:benfrancis]]

Comment on attachment 8496924 [details] [review]
PR for Bug

Thanks for the patch! This looks right with regards to the interaction specification, but the capitalisation of the strings looks inconsistent with the others in the implementation.

I'd like to get a UI review on this. Eric, do you have the latest visuals? What should the wording and capitalisation be?

Comment 10

[Gaurav Mittal (gauravmittal1995)]

Hey,
i did it w.r.t comment 4 ( https://bugzilla.mozilla.org/show_bug.cgi?id=1068894#c4 )

Comment 11

[Eric Pang [:epang] UX]

Comment on attachment 8496924 [details] [review]
PR for Bug

Thanks for working on this Gaurav! This looks good, but I'm minus-ing based on the string.  We'll need to update to sentence case "Open in new window" to be consistent with the other options.  Please re-flag me for review when updated, thanks!

Comment 12

[Gaurav Mittal (gauravmittal1995)]

Ok, done .... can u review it again?

Comment 13

[Eric Pang [:epang] UX]

Comment on attachment 8496924 [details] [review]
PR for Bug

Looks good to me know, thanks Gaurav!

Comment 14

[Ben Francis [:benfrancis]]

Comment on attachment 8496924 [details] [review]
PR for Bug

Thanks gauravmittal!

Comment 15

[Ben Francis [:benfrancis]]

There was a failing integration test which looks unrelated so I re-triggered the job on TBPL.

Comment 16

[Ben Francis [:benfrancis]]

Comment on attachment 8496924 [details] [review]
PR for Bug

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): n/a
[User impact] if declined: Confusing UI - says "new tab" when it means "new window".
[Testing completed]: Manual
[Risk to taking this patch] (and alternatives if risky): Low risk, string change only.
[String changes made]: One string changed.

Comment 17

[Kevin Grandon :kgrandon]

Adding late-l10n here. I find it unlikely that we would break string freeze for this, but maybe we could settle for an english-only string change here? For that we would need to revert the key to be the same, and we should check with the l10n guys also.

Comment 18

[Ben Francis [:benfrancis]]

https://github.com/mozilla-b2g/gaia/commit/ab33e6b904d53e2578345e06d3be7d671f0f6bb8

Comment 19

[Axel Hecht [:Pike]]

Given this took two weeks to bubble up, I'd just go for a wontfix on for 2.1.

We should expect that this bug is just gonna ship in the languages that people use on the phone.

I'd surely not break string freeze for this. If you do a en-US non-id-changing copy fix is a different question.

Comment 20

[[:fabrice] Fabrice Desré]

Comment on attachment 8496924 [details] [review]
PR for Bug

really-too-late-l10n, sorry!

Comment 21

[Marcia Knous [:marcia]]

I understand why this got minused, but unfortunately to me it is confusing to still be showing tabs in the context menu when everywhere else in the UI we show window.