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



Firefox OS
Gaia::System::Window Mgmt
3 years ago
3 years ago


(Reporter: marcia, Assigned: Gaurav Mittal (gauravmittal1995))



Firefox Tracking Flags

(Not tracked)


(Whiteboard: [systemsfe])


(1 attachment)



3 years ago
Flame while running:

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

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

3 years ago
Hey, I would like to work on this, can u assign it to me?
Flags: needinfo?(mozillamarcia.knous)

Comment 2

3 years ago
ni on rob for desired behavior here.
Flags: needinfo?(mozillamarcia.knous) → needinfo?(rmacdonald)
Thanks, Marcia. NI'ing Francis as the browser ux owner.
Flags: needinfo?(rmacdonald) → needinfo?(fdjabri)
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"
Flags: needinfo?(fdjabri)
Whiteboard: [systemfe] → [systemsfe]
Assignee: nobody → gauravmittal1995

Comment 5

3 years ago
@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 ????
Flags: needinfo?(anygregor)
Ben, can you help out gauravmittal?
Flags: needinfo?(anygregor) → needinfo?(bfrancis)
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
Flags: needinfo?(bfrancis)

Comment 8

3 years ago
Created attachment 8496924 [details] [review]
PR for Bug
Attachment #8496924 - Flags: review?(bfrancis)
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?
Attachment #8496924 - Flags: ui-review?(epang)

Comment 10

3 years ago
i did it w.r.t comment 4 ( https://bugzilla.mozilla.org/show_bug.cgi?id=1068894#c4 )
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!
Attachment #8496924 - Flags: ui-review?(epang) → ui-review-

Comment 12

3 years ago
Ok, done .... can u review it again?


3 years ago
Attachment #8496924 - Flags: ui-review- → ui-review?(epang)
Comment on attachment 8496924 [details] [review]
PR for Bug

Looks good to me know, thanks Gaurav!
Attachment #8496924 - Flags: ui-review?(epang) → ui-review+
Comment on attachment 8496924 [details] [review]
PR for Bug

Thanks gauravmittal!
Attachment #8496924 - Flags: review?(bfrancis) → review+
There was a failing integration test which looks unrelated so I re-triggered the job on TBPL.
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.
Attachment #8496924 - Flags: approval-gaia-v2.1?
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.
Keywords: late-l10n
Last Resolved: 3 years ago
Resolution: --- → FIXED

Comment 19

3 years ago
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 on attachment 8496924 [details] [review]
PR for Bug

really-too-late-l10n, sorry!
Attachment #8496924 - Flags: approval-gaia-v2.1? → approval-gaia-v2.1-

Comment 21

3 years ago
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.
You need to log in before you can comment on or make changes to this bug.